fix: new ca-filter causing expontentially more api-calls

[the-technat]

Issue

Sort of related to #3565

Description

The recently merged #3591 contained a bug where on large clusters the initial reconcile loop after startup would take hours instead of minutes (on clusters with a lot of ingresses/certificates). This is due to the missing domains-cache of certificates that were filtered by this newly introduced flag.

This PR fixes this behaviour by setting an empty cache value for these filtered certificates. At least in our (@swisspost) testing environment this reduced duration of the reconcile loop significantly.

Checklist

• Added tests that cover your change (if possible)
• Added/modified documentation as required (such as the README.md, or the docs directory)
• Manually tested
• Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

• Backfilled missing tests for code in same general area 🎉
• Refactored something and made the world a better place 🌟

[k8s-ci-robot]

Hi @the-technat. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: the-technat
Once this PR has been reviewed and has the lgtm label, please assign oliviassss for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dims]

/ok-to-test
/assign @shraddhabang @oliviassss @M00nF1sh

[mkilchhofer]

Real resp. resulting diff for this feature (#3565) can be viewed here:
v2.7.1...the-technat:aws-load-balancer-controller:main

Or on this screenshot:

[M00nF1sh]

taking a look, wondering how the original PR would cause such regression when the new flag is not used given it had the len(d.allowedCAARNs) == 0 check..

[M00nF1sh]

just for my understanding, the originally PR didn't introduce any regression right?
And you faced this issue due to you used the new "allowedCAARNs" feature?
If so, this fix looks good to me. However, i'd like to change this code to be like

domains := sets.NewString(aws.StringValueSlice(certDetail.SubjectAlternativeNames)...)
switch aws.StringValue(certDetail.Type) {
case acm.CertificateTypeImported:
	d.certDomainsCache.Set(certARN, domains, d.importedCertDomainsCacheTTL)
case acm.CertificateTypeAmazonIssued, acm.CertificateTypePrivate:
	d.certDomainsCache.Set(certARN, domains, d.privateCertDomainsCacheTTL)
}
if len(d.allowedCAARNs) == 0 || slices.Contains(d.allowedCAARNs, awssdk.StringValue(certDetail.CertificateAuthorityArn)) {
   return domains, nil
}
return sets.String{}, nil

technically there is no functional difference since allowedCAARNs is a controller-level flag which is immutable given the controller's lifetime. However, from coding perspective, the cache shall be for the "domains" before the "CA filter logic" and this make the code more robust(e.g. works even allowedCAARNs can be dynamically updated somehow).

[mkilchhofer]

Our PR introduce this behavior only if both conditions are met:

1. use the --allowed-certificate-authority-arns=... parameter
2. there exists certificates inside AWS ACM which are not issued from a authority in the allowed list.

I/we can test your proposal tomorrow in our staging environment. But I assume that your proposed code change does not work properly as we then also cache the domain(s) of a certificate which is not issued from an allowed CA.

And the function first tries to load the domains from the cache before it reaches this filtering code block:

func (d *acmCertDiscovery) loadDomainsForCertificate(ctx context.Context, certARN string) (sets.String, error) {
	if rawCacheItem, ok := d.certDomainsCache.Get(certARN); ok {
		return rawCacheItem.(sets.String), nil
	}
	// only continues in case we didn't find it inside the cache

[M00nF1sh]

@mkilchhofer
You are right, the cache shall be the domains after the filter.
ideally we should refactor the code such that the cache stores certificate details from AWS. (but for now it only caches domains and your filter logic is on cert CA), thus filter logic has to be run before the cache.

I'll approve this, and maybe refactor this in the future if we ever need to make the ca list mutable.

[M00nF1sh]

/ok-to-test

[the-technat]

/retest

[oliviassss]

govulncheck failing is unrelated, will merge it.