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

feat(controllers) handle default IngressClass #2313

Merged
merged 3 commits into from
Mar 21, 2022
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Mar 9, 2022

What this PR does / why we need it:
When 'ingressclass.kubernetes.io/is-default-class: "true"' is present on the ingress controller's configured IngressClass, ingest class-aware resources that have no IngressClass set.

Add a new watch to reconcilers, which requeues objects if the default IngressClass changes. This ensures that existing classless objects are ingested if our class becomes the default class during operation.

Rework utility functions related to class matching to make default class handling work.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): part of #2177

Special notes for your reviewer:
This originally only applied to Ingress objects, as their link to IngressClasses is the only link actually in the spec. In retrospect during development it seemed odd that you'd have a default class that ingested classless Ingresses while you still needed to apply the class to everything else.

We did have heterogenous classless handling (i.e. classless Ingresses supported, but not classless KongClusterPlugins) with the expectation that the de facto classless handling behavior would go away entirely with Ingress v1 and the dedicated ingressClassName field. As that didn't happen, and default/classless handling is now a de jure part of the spec, I've applied it across the board.

FWIW #853 has been exacerbated by the addition of the controller-level filters, as the duplication of effort here demonstrates.

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

@rainest rainest requested a review from a team as a code owner March 9, 2022 18:54
@rainest rainest temporarily deployed to Configure ci March 9, 2022 18:54 Inactive
@rainest rainest force-pushed the feat/ingress-class-default branch from 7b1e1a3 to a943922 Compare March 9, 2022 19:04
@rainest rainest temporarily deployed to Configure ci March 9, 2022 19:04 Inactive
@rainest rainest temporarily deployed to Configure ci March 9, 2022 19:04 Inactive
@rainest rainest temporarily deployed to Configure ci March 9, 2022 19:40 Inactive
@rainest rainest temporarily deployed to Configure ci March 9, 2022 19:40 Inactive
@rainest rainest force-pushed the feat/ingress-class-default branch from a943922 to ec83f1f Compare March 9, 2022 20:19
@rainest rainest temporarily deployed to Configure ci March 9, 2022 20:19 Inactive
@rainest rainest temporarily deployed to Configure ci March 9, 2022 20:20 Inactive
@rainest rainest temporarily deployed to Configure ci March 9, 2022 20:36 Inactive
@shaneutt shaneutt enabled auto-merge (squash) March 9, 2022 20:36
@shaneutt shaneutt disabled auto-merge March 9, 2022 20:36
@rainest rainest temporarily deployed to Configure ci March 9, 2022 21:44 Inactive
@rainest rainest temporarily deployed to Configure ci March 10, 2022 01:06 Inactive
@rainest rainest marked this pull request as draft March 10, 2022 01:06
@rainest rainest temporarily deployed to Configure ci March 15, 2022 21:59 Inactive
@rainest rainest temporarily deployed to Configure ci March 15, 2022 22:02 Inactive
@rainest rainest temporarily deployed to Configure ci March 15, 2022 23:45 Inactive
@rainest rainest temporarily deployed to Configure ci March 18, 2022 00:57 Inactive
@rainest
Copy link
Contributor Author

rainest commented Mar 18, 2022

Lint and unit currently broken due to a function name change; test updates are pending me figuring out a good way to organize the additional cases. The below should still be reviewable as-is.

409e959 reworks the utility class matcher(s) to try and address the comments related to it and consolidate some code.

We originally had predicate generators allow toggling whether the resource allows the netv1 Ingress spec field and/or the annotation to manage it, though I don't think that was strictly necessary--the try everything approach should work fine. The spec field is present only on a single type that we have to cast to when checking anyway, and everything accepts the annotation.

We mention that the annotation is deprecated (and I think this may be why we had those options) but it's kinda weird to deprecate an annotation since you can't remove specific annotations from a spec, and thus implementation is entirely on the client side. There's probably no great reason to actually remove support unless we want to make a clean cut away from the old annotation, since you can always simply not use it (assuming we eventually add dedicated fields to our CRDs in the future).

Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, once the commits that resolve #2313 (comment) are in I'll do a final pass.

When 'ingressclass.kubernetes.io/is-default-class: "true"' is present on
the ingress controller's configured IngressClass, ingest Ingresses that
have no IngressClass set.

Split the v1 Ingress controller out of the generated controllers, to
handle re-queueing reconciles if an IngressClass is changed to be the
default IngressClass.
@rainest rainest temporarily deployed to Configure ci March 18, 2022 21:12 Inactive
@rainest rainest temporarily deployed to Configure ci March 18, 2022 21:17 Inactive
@rainest rainest temporarily deployed to Configure ci March 18, 2022 21:17 Inactive
@rainest rainest temporarily deployed to Configure ci March 18, 2022 21:22 Inactive
@rainest rainest temporarily deployed to Configure ci March 18, 2022 21:22 Inactive
@rainest rainest temporarily deployed to Configure ci March 18, 2022 21:38 Inactive
@rainest rainest temporarily deployed to Configure ci March 18, 2022 21:52 Inactive
@rainest rainest temporarily deployed to Configure ci March 18, 2022 21:52 Inactive
@rainest rainest temporarily deployed to Configure ci March 18, 2022 22:08 Inactive
@rainest rainest requested a review from shaneutt March 18, 2022 22:09
@rainest
Copy link
Contributor Author

rainest commented Mar 18, 2022

No luck trying generics so whatever, the number of types we need to check isn't that huge (the Ingress annotation check covers almost everything since it's the same annotation throughout; netv1 spec and Knative are the exceptions), so I went ahead and just duplicated the loops for each.

Also rolled the separate commit showing the changes to MatchesIngressClass and friends into the previous one, since it's been reviewed and was only separate to highlight the change.

rainest and others added 2 commits March 18, 2022 15:15
Use default IngressClass behavior for any resource that supports ingress
class. This re-integrates the Ingress V1 controller into the generated
controllers, as all controllers that support class now include the
additional IngressClass watch.

Condense the store class annotation validation functions into a single
function for either Ingress or Knative annotations.

Add a logger to the fake store for tests.

Co-authored-by: Shane Utt <[email protected]>
Co-authored-by: Michał Flendrich <[email protected]>
@rainest rainest temporarily deployed to Configure ci March 18, 2022 22:15 Inactive
@rainest rainest temporarily deployed to Configure ci March 18, 2022 22:15 Inactive
@rainest rainest temporarily deployed to Configure ci March 18, 2022 22:31 Inactive
@rainest rainest merged commit 1c05a64 into main Mar 21, 2022
@rainest rainest deleted the feat/ingress-class-default branch March 21, 2022 17:20
@rainest rainest mentioned this pull request Mar 21, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants