-
Notifications
You must be signed in to change notification settings - Fork 591
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
feature gates #1970
feature gates #1970
Conversation
f80414c
to
db87c92
Compare
db87c92
to
b565419
Compare
b565419
to
2623122
Compare
2623122
to
12db4c1
Compare
12db4c1
to
c59265e
Compare
c59265e
to
48f1a8d
Compare
48f1a8d
to
bebd433
Compare
I was actually thinking about this already this morning before I read this, 080719a includes reporting for all feature gate options.
d3013a4 rips it all out as you suggest. In truth I was aware of upstream and I had intentionally gone slightly against the grain here because I was thinking about a potentially soon future where we would actually deprecate APIs like
I've since removed that one. |
749e57f
to
f5b6dcd
Compare
43fcaf8
to
b57d0e0
Compare
b57d0e0
to
4b0f436
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with changes. I think the implication is that any other controller feature flag will have the condition be &&
to require both the gate and controller flag, and there's just no example because all we have now is Knative and Knative is weird.
We're probably stuck with TCP/UDPIngress for quite some time, since upstream isn't the fastest-moving thing--we're probably quite a ways out from being able to deprecate them, since we'd want Gateway APIs in at least beta (the APIs themselves rather than our implementation) and available on major providers--even once APIs reach beta providers may not actually enable them by default (or allow you to enable them at all).
c280210
to
af00242
Compare
### Feature gates for graduated or deprecated features | ||
|
||
{{< table caption="Feature Gates for Graduated or Deprecated Features" >}} | ||
|
||
| Feature | Default | Stage | Since | Until | | ||
|----------------------------|---------|------------|-------|-------| | ||
|
||
{{< /table >}} | ||
|
||
Features that reach GA and over time become stable will be removed from this table, they can be found in the main [KIC CRD Documentation][specs] and [Guides][guides]. | ||
|
||
[specs]:https://docs.konghq.com/kubernetes-ingress-controller/latest/references/custom-resources/ | ||
[guides]:https://docs.konghq.com/kubernetes-ingress-controller/latest/guides/overview/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YAGNI - suggestion: delete
// knative is a special case because it existed before we added feature gates functionality | ||
// for this controller (only) the existing --enable-controller-knativeingress flag overrides | ||
// any feature gate configuration. See FEATURE_GATES.md for more information. | ||
Enabled: featureGates["Knative"] || c.KnativeIngressEnabled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that feature gate names deserve a dedicated enum type, and they definitely deserve const
s rather than literal values
const featureGatesDocsURL = "https:/Kong/kubernetes-ingress-controller/blob/main/FEATURE_GATES.md" | ||
|
||
// setupFeatureGates converts feature gates to controller enablement | ||
func setupFeatureGates(setupLog logr.Logger, c *Config) (map[string]bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function could have a simpler and cohesive mental model: take two map[string]bool
s (rather than c
) and have the semantics of "merging two maps key-wise but using the first in case of a conflict".
What this PR does / why we need it:
This patch documents and enables feature gates for KIC to add tooling which helps to manage the lifecycle of new and/or experimental features for the KIC.
This was meant to solve two problems:
PR Readiness Checklist:
CHANGELOG.md
release notes have been updated