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

Service-upstream default controlled by IngressClass parameters #1131

Closed
4 tasks
mflendrich opened this issue Mar 24, 2021 · 5 comments · Fixed by #2535
Closed
4 tasks

Service-upstream default controlled by IngressClass parameters #1131

mflendrich opened this issue Mar 24, 2021 · 5 comments · Fixed by #2535
Assignees
Labels

Comments

@mflendrich
Copy link
Contributor

mflendrich commented Mar 24, 2021

First, some context: Feature requests such as #774 have shown that KIC needs a way to accept input parameters that will configure how KIC handles an ingress class as a whole. The parameters field of IngressClass that was added in k8s 1.18 seems like a good fit.

This issue is about fixing #774 using the IngressClass resource by making the default value of the service-upstream annotation configurable.

The scope of this issue is to:

  • make KIC watch the IngressClass resource whose name is equal to the configured ingress class (if such resource is present in the cluster)
  • define a parameters CRD (to be referenced by the aforementioned IngressClass's .spec.parameters). That CRD is to be a container for ingress-class-level settings.
  • using the mechanism above, implement a setting that satisfies Enable service-upstream for an entire ingress class #774 by controlling the default value for the ingress.kubernetes.io/service-upstream annotation on referenced Services. Before this change, the default value for this annotation is false. After this change, the default value should be controlled by the setting added to the parameters CRD.

Acceptance criteria:

  • today's behavior of service-upstream unchanged
  • if the IngressClass resource is present, the newly added parameters field controls the default value of the service-upstream annotation for Services referenced by that ingress class
  • integration test implemented
  • documentation updated
@mflendrich
Copy link
Contributor Author

Note kubernetes/kubernetes#97396 - the parameters object can now be namespaced.

@seh
Copy link
Contributor

seh commented Jan 21, 2022

make KIC watch the IngressClass resource whose name is equal to the configured ingress class (if such resource is present in the cluster)

I don't think this is the approach that the Ingress API intended controllers to adopt. Instead, Kong should accept a controller name via configuration, with a sensible default like you've chosen in the Helm chart, and it should only pay attention to IngressClass objects that use that same value in their "spec.controller" field.

If Kong discovers an IngressClass that matches its controller name, it should honor the related parameters and serve Ingress objects using that IngressClass name, applying the related parameters when it does so. I don't get the impression that operators should start an ingress controller specifying the name of an IngressClass. What would you do if were told to watch an IngressClass named "kong" that specified something like Contour in its "spec.controller" field?

@mflendrich
Copy link
Contributor Author

I don't think this is the approach that the Ingress API intended controllers to adopt. Instead, Kong should accept a controller name via configuration

Agreed, but that would be a breaking change. For historical reasons, KIC refers to the supported ingress class directly (by name), rather than having it linked by controller name.

My intention when writing up this proposal was to leave the door open for the change you're recommending (i.e. use the controller name to link an ingress class with a controller instance), but not making it just yet.

Another bit of complexity resulting from that change is that the controller name concept would naturally make the relationship between ingress classes and KIC instances a one-to-many relationship. Today KIC's implementation is limited to supporting only one ingress class at a time. Improving that is possible, but far from trivial.

@seh
Copy link
Contributor

seh commented Mar 23, 2022

I understand the challenge, but I don't see how you can claim to support IngressClass if you treat their names specially. Object names are not supposed to matter semantically.

If you require that operators specify the name of a specific IngressClass for a given Kong instance to use, then the "spec.controller" field is inconsequential. At best it happens to nominate Kong categorically, but in other cases you'd have to ignore it or fail to serve as soon as you read the offending value. On the upside, the field is immutable, so it can't change after you first read it.

Given your position, it would be clearer if the Kong project just declared that it doesn't read IngressClass objects at all.

@stale
Copy link

stale bot commented Mar 31, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Will be closed unless advocated for within 7 days label Mar 31, 2022
@mflendrich mflendrich added onboarding nice-to-have and removed pending author feedback stale Will be closed unless advocated for within 7 days labels Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment