Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove kongCredType support entirely #4853

Closed
3 tasks
rainest opened this issue Oct 13, 2023 · 1 comment · Fixed by #5856
Closed
3 tasks

Remove kongCredType support entirely #4853

rainest opened this issue Oct 13, 2023 · 1 comment · Fixed by #5856

Comments

@rainest
Copy link
Contributor

rainest commented Oct 13, 2023

Problem description

#4825 adds support for a credential label alongside the kongCredType field. The kongCredType field is now deprecated and needs to be removed in a later version.

In addition to removing code for handling the field, we will need a webhook definition filtered on credentials with the label only, versus the current webhook that ingests all secrets before skipping those without credential configuration: https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#matching-requests-objectselector

The Secret controller can similarly filter on labels, but currently handles both credentials and certificate Secrets referenced by an Ingress or Gateway. We can consider creating a separate controller dedicated to credentials and filter its watches by label.

Note that there is existing code (AFAIK in each of the webhook, controller, and parser) for filtering out credentials that are not referenced by a KongConsumer whose class matches the controller configuration. This shouldn't directly affect the type filter, but it's something to be aware of--we don't rely on the label filter only.

Ancillary to this, the webhook credential validation path has become a bit of a mess between admission.RequestHandler.handleSecret(), admission.KongHTTPValidator.ValidateCredentials(), credentials.ValidateCredentials(), and kongstate.FillConsumersAndCredentials() with some duplicated logic and no clear separation of tasks between them. We should consider a broader refactor of that.

Acceptance criteria

  • Admission webhook no longer supports Secrets based on their kongCredType key value
  • ValidatingWebhookConfiguration's rule for Secrets has objectSelector set to accept only Kong credentials
  • Downstream chart resource generation logic either uses version conditions or a toggle to control whether the admission rule includes the label filter.
@rainest
Copy link
Contributor Author

rainest commented Mar 1, 2024

Some discussion today highlighted that this entails both dropping the kongCredType field code and adding the webhook filter to truly make use of the label and stop ingesting random unrelated Secrets.

That chart change can be either absolute, a toggle, or a version check that determines whether or not the webhook configuration includes the label filter.

  • An absolute change breaks compatibility with (or at least disables webhook credential checks for) all KIC versions that don't support the label. We probably don't want that unless we do want it as a stick to get people to migrate.
  • A toggle allows users on KIC versions that support both the label and field to opt into limited Secret scanning if they don't want to update to 3.2 just yet.
  • Image version checks do not require additional configuration fields, but don't let you filter Secrets on 3.1 without hacks (manually patching your hook config).

Version checks are probably the best option unless this gets kicked much further down the road. If this instead goes out in, say, 3.6, and we have multiple versions where the label is supported but not mandatory, the toggle becomes more attractive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants