WIP: Application load balancer controller#3
Conversation
|
@fischerman either mark the PR as "Draft" or use "WIP" as the title. The PR will then automatically marked as WIP/Draft |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
dergeberl
left a comment
There was a problem hiding this comment.
This is my first batch of review comments. Only had a look into the /cmd/*, /pkg/controller/* (except the tests) and the *.md files, yet. I will continue with my review for the rest.
Maybe some comments are already resolved as you also pushed during my review (I did not crosscheck again, before submitting the comments now).
| ### Limits | ||
|
|
||
| The following limitations are imposed directly by the STACKIT ALB API (not the controller itself): | ||
| - Maximum targets per pool: An individual target pool can contain a maximum of 250 targets. |
There was a problem hiding this comment.
Limit is still 250. SKE will look into limiting the auto scaler to 250 for clusters the have the ALB extension enabled.
dergeberl
left a comment
There was a problem hiding this comment.
Comments for the stackit package and testutil
| CertificateIds: []string{}, | ||
| }, | ||
| } | ||
| for fingerprint, cert := range t.certificates { |
There was a problem hiding this comment.
Map interactions are not deterministic. If I remember correctly the order in the ALB matters, as always the first match will be used. This could lead in a flapping which cert is used if multiple are fitting.
There was a problem hiding this comment.
Fixed in 5c25864
The order is stable but cannot be influenced by the customer.
There was a problem hiding this comment.
TODO: Document behavior when two certificates collide.
| @@ -0,0 +1,302 @@ | |||
| package ingress | |||
There was a problem hiding this comment.
In the update.go are multiple functions which also should be tested, like getServicesForIngresses, getTLSSecretsFromIngresses, getCertificatesForIngressClass. But also the logic for duplicateCerts should get tested.
| @@ -0,0 +1,658 @@ | |||
| package spec | |||
There was a problem hiding this comment.
We should also add tests for:
theAnnotationHTTPSOnlyand that it not serves HTTP.the sorting based onAnnotationPriorityand creation timestamp.AnnotationPlanID- the handling when setting not allowed values in annotation (like no valid int/port, not valid cidr in source range)
GetUnusedCertificates
| @@ -0,0 +1,85 @@ | |||
| apiVersion: apps/v1 | |||
There was a problem hiding this comment.
We should align this with the deploy of CSI and CCM in cloud-provider-stackit (which are also not perfect, see stackitcloud/cloud-provider-stackit#1162).
For example:
- we can use also the kube-system namespace as this is also a system component like CSI and CCM
- the cloud config don't need to be a secret
- the mount path of the service account and cloud config
- the
apiEndpointsdon't need to be in the example (this is only internally needed) - lets also create a
/deployfolder instead if the/samples/deploy/ - we need to find a image tag which is working and don't need to be updated like
latest(not sure if this will be tagged on release)
There was a problem hiding this comment.
With #6 we get a latest tag for tagged versions. Lets use that in the deployment.
There was a problem hiding this comment.
I vote for latest tag here. We can leave it like it is for now and solve this directly before merging. After merge I will create a release which then also creates the latest tag
There was a problem hiding this comment.
Will keep the thread unresolved as a reminder.
Append ingress class name to ALB name
Log event when ALB cannot be reconciled due to invalid configuration (it will not be requeued) Use the already retrieved, created or updated LB to update the status of the ingresses. Don't update the status on invalid configurations.
How to categorize this PR?
/kind enhancement
What this PR does / why we need it:
Implements the first alpha version of the STACKIT ALB controller.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Breaking changes: