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) add IngressClass v1 #2292

Merged
merged 2 commits into from
Mar 1, 2022
Merged

feat(controllers) add IngressClass v1 #2292

merged 2 commits into from
Mar 1, 2022

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Feb 24, 2022

What this PR does / why we need it:
Adds an IngressClass controller and store to ingest and cache IngressClasses. Does not actually do anything with IngressClasses after adding them; parser functionality will be added separately.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): This handles the first bullet from #2177

Special notes for your reviewer:

IMO there are two things we should consider here:

  • Do we support IngressClass on networking.k8s.io/v1beta1? As proposed this only handles networking.k8s.io/v1, though partly to get feedback on anything we need to change before writing v1beta1 if we implement it (the code will end up being essentially identical). We could reasonably say that IngressClass functionality is only available for v1 and leave v1beta1 support as-is (i.e. it's no different than the old annotation and we only look at ingressClassName in the Ingress). Given the development history I wouldn't expect that there are many users that can use v1beta1 but not v1--they were released within a short timeframe and are identical AFAIK. On our end it's less copy-pasta code to deal with.
  • Do we support more than one controller string? As proposed this accepts only ingress-controllers.konghq.com/kong IngressClasses (note that we have a difference between the docs and chart--the latter currently uses konghq.com/ingress-controller). Whether this is configurable or not varies in other controllers based on brief review. The use case would be a single controller ingesting multiple Ingress classes. We could do this, but adding support for that would require refactoring, so it seems like it makes sense to enforce a single IngressClass controller for now to reflect the reality that we still honor --ingress-class and do not allow you to specify multiple or ingest all classes with a given controller string.

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

This only mentions the manifests for now because the code doesn't result in user-facing changes. The controller args are there, but you won't usually use those, and IMO it makes sense to defer mentioning anything other than the manifest to a PR that adds actual IngressClass-based functionality.

@rainest rainest requested a review from a team as a code owner February 24, 2022 19:32
@rainest rainest temporarily deployed to Configure ci February 24, 2022 19:33 Inactive
@rainest rainest temporarily deployed to Configure ci February 24, 2022 19:33 Inactive
@rainest rainest temporarily deployed to Configure ci February 24, 2022 19:35 Inactive
@rainest rainest temporarily deployed to Configure ci February 24, 2022 19:39 Inactive
@rainest rainest temporarily deployed to Configure ci February 24, 2022 19:39 Inactive
@rainest rainest temporarily deployed to Configure ci February 24, 2022 19:55 Inactive
Add an IngressClass controller.

Add IngressClass functions to the stores.

Add IngressClass permissions and a default IngressClass resource to the
manifests.
@rainest rainest temporarily deployed to Configure ci February 24, 2022 21:36 Inactive
@rainest rainest temporarily deployed to Configure ci February 24, 2022 21:36 Inactive
@rainest rainest temporarily deployed to Configure ci February 24, 2022 21:53 Inactive
@shaneutt shaneutt self-requested a review February 25, 2022 23:03
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.

Overall LGTM, one change requested. I see what you mean about this iteration not really doing anything by itself (just caches the objects but the dataplane code doesn't do anything with it yet) my only thought on that is we shouldn't release this change until we've got the complete support ready, but maybe you were already thinking that.

Do we support IngressClass on networking.k8s.io/v1beta1?

I think no. Version 1 has been out for a long time now so our expectation should be that almost everyone using ingress has upgraded by now. If someone puts in an issue to get support for older versions we can still look into it then but I have my doubts anyone will.

Do we support more than one controller string?

Again I think we should just use the single controller string for the time being, and if someone has a use case that needs something different they can put in an issue for that and we can take a look. Just keep it simple for this iteration.

config/rbac/role.yaml Outdated Show resolved Hide resolved
Add a new NeedsStatusPermissions field to the controller generator
configuration. When true, the controller will include status permission
annotations.

Set NeedsStatusPermissions to false for IngressClass.
@rainest rainest temporarily deployed to Configure ci March 1, 2022 19:35 Inactive
@rainest rainest requested a review from shaneutt March 1, 2022 19:35
@rainest rainest temporarily deployed to Configure ci March 1, 2022 19:35 Inactive
@rainest
Copy link
Contributor Author

rainest commented Mar 1, 2022

It's "unreleased" insofar as the changelog doesn't mention any new features other than the new resource in the manifest (which we do want to release soon since some providers, namely AWS, have started rejecting v1 Ingresses that reference a non-existent IngressClass). Code's there for us to build on, but it's essentially inaccessible (the flag to disable the controller is in place but I wouldn't expect anyone to use it). We may or may not implement actual functionality before the next release, but it's not a release blocker if we don't.

@rainest rainest temporarily deployed to Configure ci March 1, 2022 19:51 Inactive
@rainest rainest merged commit 4ac1871 into main Mar 1, 2022
@rainest rainest deleted the feat/ingress-class branch March 1, 2022 20:09
@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.

2 participants