Add option to filter certificates by tag before adding it to LB #658
Conversation
Signed-off-by: Lucas Thiesen <[email protected]>
|
👍 |
Signed-off-by: Lucas Thiesen <[email protected]>
|
lgtm |
Signed-off-by: Lucas Thiesen <[email protected]>
|
👍 |
| err := api.ListCertificatesPages(params, func(page *acm.ListCertificatesOutput, lastPage bool) bool { | ||
| acmSummaries = append(acmSummaries, page.CertificateSummaryList...) | ||
| return true | ||
| }) | ||
|
|
||
| if tag := strings.Split(filterTag, "="); filterTag != "=" && len(tag) == 2 { |
There was a problem hiding this comment.
minor: I think for readability it's better to create two if's with the same return here.
The first if will check filterTag, the second will split and check len.
With such two if's, the filterTag != "=" won't be lost somewhere in the middle of the line and would be much easier to notice.
There was a problem hiding this comment.
I don't see how filterTag != "=" can help in the condition. I think len(tag)==2 is enough, because if filterTag == "=" then it will have 3 parts after split.
There was a problem hiding this comment.
Ah https://go.dev/play/p/oGRkFVn_zu9 now I got it :D
So I am fine with the condition.
aws/acm_test.go
Outdated
| @@ -69,9 +71,64 @@ func TestACM(t *testing.T) { | |||
| Error: nil, | |||
| }, | |||
| }, | |||
| { | |||
| msg: "Found ACM Cert with correct filter tag", | |||
There was a problem hiding this comment.
Would you like to add tests with several certificates in the list.
For example, "2 certs with corresponding tag" or "1 cert with corresponding tag + 1 cert with not corresponding tag"?
| @@ -115,6 +116,9 @@ func loadSettings() error { | |||
| StringMapVar(&additionalStackTags) | |||
| kingpin.Flag("cert-ttl-timeout", "sets the timeout of how long a certificate is kept on an old ALB to be decommissioned."). | |||
| Default(defaultCertTTL).DurationVar(&certTTL) | |||
|
|
|||
| kingpin.Flag("cert-filter-tag", "sets a tag so the ingress controller only consider ACM or IAM certificates that have this tag set when adding a certificate to a load balancer."). | |||
There was a problem hiding this comment.
I don't quite understand, what value for this flag should be provided, if user of the controller would like to have all certificates, without any filters?
There was a problem hiding this comment.
You don't set it -> "" -> no tag filter is used.
| err := api.ListCertificatesPages(params, func(page *acm.ListCertificatesOutput, lastPage bool) bool { | ||
| acmSummaries = append(acmSummaries, page.CertificateSummaryList...) | ||
| return true | ||
| }) | ||
|
|
There was a problem hiding this comment.
maybe we should add a
if filterTag = "" {
return acmSummaries
}
There was a problem hiding this comment.
We could but I believe this case is covered in the if in line 52, because if filterTag == "" then len(tag) != 2, and in this case we will just return acmSummaries in line 56, which is just after the if.
Signed-off-by: Lucas Thiesen <[email protected]>
|
👍 |
1 similar comment
|
👍 |
`getACMCertificateSummaries` did not check `ListCertificatesPages` error and returned empty `CertificateSummary` slice and no error via `filterCertificatesByTag` if `filterTag` is configured. Empty `CertificateSummary` results in empty `existingStackCertificateARNs` for each `loadBalancer` model which triggered stack update: ``` level=info msg="Updating \"internet-facing\" stack for 0 certificates / 0 ingresses" ``` and then stack deletion on the next cycle: ``` level=info msg="Deleted orphaned stack \"kube-ingress-aws-controller-aws-XXX\"" ``` due to empty certificate tags after previous update. Follow up on #658 Signed-off-by: Alexander Yastrebov <[email protected]>
* aws: refactor ACM fake client to support returning errors Signed-off-by: Alexander Yastrebov <[email protected]> * aws: fail on ListCertificatesPages error `getACMCertificateSummaries` did not check `ListCertificatesPages` error and returned empty `CertificateSummary` slice and no error via `filterCertificatesByTag` if `filterTag` is configured. Empty `CertificateSummary` results in empty `existingStackCertificateARNs` for each `loadBalancer` model which triggered stack update: ``` level=info msg="Updating \"internet-facing\" stack for 0 certificates / 0 ingresses" ``` and then stack deletion on the next cycle: ``` level=info msg="Deleted orphaned stack \"kube-ingress-aws-controller-aws-XXX\"" ``` due to empty certificate tags after previous update. Follow up on #658 Signed-off-by: Alexander Yastrebov <[email protected]> --------- Signed-off-by: Alexander Yastrebov <[email protected]>
This feature aims to enable
kube-ingress-aws-controllerto ignore certificates based on tags. Effectively what this means is, you can specify a tag through the command line and the ingress controller will only use IAM/ACM certificates that have that specific tag when matching the host to define which certificate is more appropriate to add to the LB.If a tag is not specified the ingress controller shall pick all available certificates for matching.
The main idea behind this feature is to make it very specific which certificates should be considered, this feature does not avoid incidents, but make it clearer to operators which certificates are being considered in the matching phase.
This PR addresses the issue: #652