feat(creds) remove kongCredType field support

[rainest]

What this PR does / why we need it:

Remove support for the kongCredType field in credential Secrets. Only honor the konghq.com/credential label.

Add a konghq.com/plugin-config label. This label indicates that a Secret contains plugin configuration and should be validated against its referrers when updated.

Add objectSelectors to the Secret blocks in the admission webhook definition. Admission will skip any Secrets without a label indicating they should be used by KIC.

Which issue this PR fixes:

Fix #4853

Special notes for your reviewer:

Found during this that we were kinda abusing the blanket Secret ingest for updates to plugin config also. Those Secrets did not have any label to filter them in.

Per chat discussion the approach I've taken here is to leave the existing blanket code in place while changing the webhook configuration to filter on either the credential or new konghq.com/validate label. The latter is a simple exists check in the webhook configuration, and code uses the value (currently only plugin) to select the validation path.

Users that do not wish to update plugin Secrets will need to remove the filter from their webhook manifest. We will need to add a Helm chart toggle for this.

Although the validate key only has the one plugin value at present I felt it best to leave it open-ended because it's difficult (breaking change that requires users update resources) to transition from a plugin-specific label key to something else in the future if we add additional validated Secret types that don't need further hint info (conversely, credential is a dedicated label because we can't determine which validation to apply without the value to indicate it's specifically key-auth or whatever).

Tests check only the fully filtered variant, as we only have the one copy of the webhook manifest that they use.
My manual check for the no-filter case was that envtest still passes with no-label.diff.txt (and fails that doesn't have the webhook manifest diff). Do we think we'd want to set up a separate envtest run for TestAdmissionWebhook_KongPlugins only with a special no-filter copy of the manifest?

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

• the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

[codecov]

Codecov Report

Attention: Patch coverage is 65.62500% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 68.0%. Comparing base (04dead8) to head (6e8f685).
Report is 1 commits behind head on main.

❗ Current head 6e8f685 differs from pull request most recent head 06ed355. Consider uploading reports for the commit 06ed355 to get more accurate results

Files	Patch %	Lines
internal/admission/handler.go	68.4%	5 Missing and 1 partial ⚠️
...ion/validation/consumers/credentials/validation.go	60.0%	1 Missing and 1 partial ⚠️
internal/dataplane/kongstate/kongstate.go	33.3%	1 Missing and 1 partial ⚠️
internal/admission/validator.go	0.0%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #5856     +/-   ##
=======================================
- Coverage   68.9%   68.0%   -1.0%     
=======================================
  Files        179     179             
  Lines      18308   18309      +1     
=======================================
- Hits       12627   12451    -176     
- Misses      4693    4885    +192     
+ Partials     988     973     -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rainest]

Tentative TODOs pending approval of the design for plugin Secret configuration validation:

• Add a chart toggle to revert to the blanket ingest.
• Add documentation similar to https://docs.konghq.com/kubernetes-ingress-controller/latest/guides/migrate/credential-kongcredtype-label/ for the validate label. We can probably go ahead and add this to 3.1 even though it's not technically applicable to it, as adding the label while using the older versions is fine. Update the changelog to include the additional page.
• [Maybe] Remove support for the blanket ingest in a future version. I think we probably just keep it around indefinitely, since keeping the code for it doesn't really hurt anything. The burden is having a small amount of duplicate code and running the special envtest case if we do add that.

Please weigh in on whether you think the design as proposed is good and whether you think we need the no filter test and future support removal. I'll create issues for the above if we decide to move forward with the current approach in the PR.

[rainest]

This now depends on Kong/kubernetes-testing-framework#1036 and a new KTF release. Done.

Unfortunately, kubebuilder cannot handle webhook rules with objectSelector clauses, so you can't generate rules that filter ingested objects. See https://book.kubebuilder.io/reference/markers/webhook and kubernetes-sigs/controller-tools#553.

The accepted workaround for this is to roll your own rules and patch them in. This requires changing the envtest runner so it can invoke kustomize.

We'll need to note that the chart hook should be generated from kustomize build <upstream>/config/webhook.

[czeslavo]

Do we think we'd want to set up a separate envtest run for TestAdmissionWebhook_KongPlugins only with a special no-filter copy of the manifest?

Yeah, I think this would make sense to have this covered properly given we're going to support this mode of operation for some time still. I suggest making this a separate PR though.

---

I think that as a part of #5856 we should also make sure that the Helm chart generates the webhook config with label filters and also that docs.konghq.com articles for KIC 3.2 that create Secrets already use the labels.

I've created a KIC 4.0 issue to track making the labels required for reconciliation as the next step in improving performance: #5876

[mheap]

If there is a breaking change that was not deprecated in the previous minor version (which it wasn't, as it didn't exist) I don't think we can make it the default in the chart.

My proposal is to keep the old blanket ingest in the chart as the default, and if anyone raises questions we can let me know how to change it. We can also encourage the preferred way via docs

[rainest]

@mheap I'd still advocate for doing it in one go, since this one is a bit unusual. Main questions are:

• What'd be the timeline on changing the default if we leave it on the blanket one?
• Would it make any difference if we can, with the filter on, alert users to their use of un-labeled Secrets? This case is weird because it only disables validation, so you could turn on filters without labels and be none the wiser: your configuration will be unaffected, and you'd only notice if you managed to add broken configuration later.

---

I wouldn't expect most users to go looking for the filter toggle, as it's not something you'd expect to exist even if you do hit the performance/UX problems with unrelated Secrets, and I think adoption would be low as such.

We're not strictly deprecating anything yet, more changing a default (with the caveat that there wasn't previously an option to change). I'm not even sure we'd ever fully remove the blanket handling, since AFAIK the cost of keeping it is low.

If we don't expect to have it as the default in 3.2 or 3.3 (it's not clear what the timeline would be after/if we can have one version where the label exists but isn't required by default), I'd want something like 97185d3 to call it out, and that'd be annoying if you are intentionally leaving the filter off. There's unfortunately no way to distinguish between intentional omissions and new installs that just neglected to include the label.

We do still load referenced plugins without labels in the reconciler and parser--unlike the webhook, their Secret ingest is triggered by the reference. We can check if a referenced Secret lacks a label there and log a warning for it.

[czeslavo]

I wouldn't expect most users to go looking for the filter toggle, as it's not something you'd expect to exist even if you do hit the performance/UX problems with unrelated Secrets, and I think adoption would be low as such.

I agree we can expect low adoption if we just provide a default-off option in the Helm chart to filter Secrets on labels. But I think that's the best we can do:

1. Make KIC 3.2 aware of the new konghq.com/validate label - give users a heads up in logs about the deprecation of validating non-labeled Secrets as @rainest already suggested. Also, make the deprecation timeline announcement in the release notes.
2. Add a default-off toggle in the Helm chart that would enable the webhook filtering based on labels.
3. After releasing KIC 3.3 (or any 3.x we agree for the timeline), swap the default value of the toggle in the Helm chart to true, still keeping the code in KIC unchanged (capable of handling any Secrets, labeled or not, still warning about the deprecation). If users prefer to just flip the toggle back to false they'll be able to do so, but labeling their Secrets will be advised.
4. In KIC 4.0 stop validating non-labeled Secrets (also stop reconciling them as defined in Drop support for non-labeled Secrets (Plugins' config, credentials) #5876).

[pmalek]

@czeslavo's plan makes a lot of sense to me. I'm not afraid of spamming in the logs about users not using the label as that's our means of communicating to the users relevant information (which, this one is IMHO).

[mheap]

I'm comfortable with the steps that @czeslavo outlined too.

I have a small preference to not change defaults in the existing charts (but we can change the default in the next-gen charts) but don't feel strongly enough to say that we can't change the default in future once we've had a release or two with warning logs attached

[rainest]

Test gymnastics required to run again without the filter are pretty messy, but whatever. Only added it to the KongPlugin test because there's no difference in Secret ingest for the webhook; KongClusterPlugin doesn't change anything about that.

[czeslavo]

I started doubting if my advocating for using konghq.com/validate instead of konghq.com/plugin-config was right. Rationale: if we want to make labels on Secrets a requirement for their reconciliation in KIC 4.0 (#5876), I think it would be a bit weird to make the reconciliation depend on konghq.com/validate label, wouldn't it? Introducing another one solely for reconciliation purposes doesn't make sense either. @rainest what do you think about backing off to your initial proposal in terms of the label naming? It would be more in line with the existing konghq.com/credentials and we can use it in the webhook filters without any problem.

[rainest]

I started doubting if my advocating for using konghq.com/validate instead of konghq.com/plugin-config was right. Rationale: if we want to make labels on Secrets a requirement for their reconciliation in KIC 4.0 (https://github.com/Kong/kubernetes-ingress-controller/issues/5876), I think it would be a bit weird to make the reconciliation depend on konghq.com/validate` label, wouldn't it? Introducing another one solely for reconciliation purposes doesn't make sense either. @rainest what do you think about backing off to your initial proposal in terms of the label naming? It would be more in line with the existing konghq.com/credentials and we can use it in the webhook filters without any problem.

Should we actually need this for reconciliation? I think that was unintended scope creep in the #5876 writeup. AFAIK the reference exists predicate there is already fine for that. It is, however, impossible to do for the webhook, which isn't smart enough to understand references.

We should only need label-based filters there for Secrets that are fully independent, such as CA certificates. Credentials using the label instead is something of a lazy hack because something was broken and we didn't want to debug it.

I'd still say keep the single validate. If need be we can abuse it as a reconciler filter internally also, and on the outside:

• It's still easier to avoid future webhook manifest updates.
• A single key is easier to remember for end users than similarly-themed ones, like how ingress.class is easy to remember as the universal "ingest this" annotation even for things that aren't Ingresses. It's not as great due to the additional non-obvious value, but it's at least one less thing you need to familiarize with.

In retrospect, perhaps we should have left kongCredType around and required validate along with it to achieve the filter goal there (or validate: key-auth alone), but we unfortunately didn't consider the possibility of needing to validate other types of Secrets back when we decided how to change those.

[czeslavo]

Should we actually need this for reconciliation? I think that was unintended scope creep in the #5876 writeup. AFAIK the reference exists predicate there is already fine for that. It is, however, impossible to do for the webhook, which isn't smart enough to understand references.

My main goal here was to make sure KIC can perform well in environments that have a huge volume of Secrets and not all of them should be reconciled by KIC. We had reports where users were complaining about KIC crash looping because of cache synchronization timeouts in such cases (slack thread). We could solve that by caching only labeled Secrets (using SelectorsByObject in controller-runtime). That can also be kinda worked around by making KIC watch only a single namespace (where only KIC-related secrets and resources would live), but that may be a constraint someone wouldn't like to have (e.g. if they use Gateway API and use different namespaces per team or something else).

[rainest]

We could probably use ingress.class for that. We've had previous requests to do so (#4578) to address problems with other resources in multi-instance environments, and we haven't made progress on the fancier reference-based approach. We may as well not cache other instances' resources for performance benefits as well.

It's good in that it's already set on a bunch of resources and can share the same key and value across everything, but bad in that it doesn't make sense on GWAPI resources. We could use some new key that's functionally equivalent (i.e. instances ignore anything that doesn't have whatever value they're configured to recognize) and explicitly call it out as a low-level optimization below the usual rules. While it would filter out, for example, an HTTPRoute without the label, we would not parse the HTTPRoute on the basis of the label alone, and would still require it to match a Gateway of the appropriate class.

[czeslavo]

We could probably use ingress.class for that. We've had previous requests to do so (#4578) to address problems with other resources in multi-instance environments, and we haven't made progress on the fancier reference-based approach. We may as well not cache other instances' resources for performance benefits as well.

Do you mind creating an issue for KIC 4.0 that would track this? We can discuss later whether it will be ingress.class or something new, but I agree it would be better to have one common label key for that for any object kind.