Injection: enable support for automatic detection of native sidecars #49570
Conversation
|
Skipping CI for Draft Pull Request. |
istio-testing
added
do-not-merge/work-in-progress
howardjohn
force-pushed
the
injection/safe-native-sidecars
branch
from
75d38ed
to
d311b2c
Compare
istio-testing
added
the
needs-rebase
howardjohn
force-pushed
the
injection/safe-native-sidecars
branch
from
d311b2c
to
156fe23
Compare
istio-testing
removed
the
needs-rebase
istio-testing
removed
the
do-not-merge/work-in-progress
| return NativeSidecarModeNever | ||
| case "always", "true": | ||
| return NativeSidecarModeAlways | ||
| case "auto-beta": |
There was a problem hiding this comment.
Can you articulate why we need both auto-beta and auto-stable versus just auto?
That feels like a lot of knobs for a single feature (admittedly it's a critical one), and also I don't think it makes a lot of sense to have feature-level logic for opting-in depending on stability level - that should/will be defined elsewhere and can be implicit in auto.
(Yes I realize this means people who are using pure stable will not use this. That's probably fine?)
There was a problem hiding this comment.
Eventually I think the default should be auto-stable. We should not default to beta k8s features, only stable ones IMO. So the possible one to remove is auto-beta IMO.
The benefit of auto-beta is that a lot of users do want to use beta k8s features, and its IMO pretty reasonable to. So what they get is a bit of safety in doing that, rather than just on.
Most of the complexity is in the detection anyways, beta vs stable is pretty simple
There was a problem hiding this comment.
Can we just do
never, always and auto for now?
Whether auto should ignore k8s beta features or not probably should be controlled globally by e.g. @whitneygriffith's experimental/stable profiles and implicit in that (if you are in istio stable you do not get to use k8s experimental stuff, and if you are in istio experimental you do, it is not per-feature), and not at the feature level.
howardjohn
force-pushed
the
injection/safe-native-sidecars
branch
from
156fe23
to
947ac7e
Compare
howardjohn
force-pushed
the
injection/safe-native-sidecars
branch
from
947ac7e
to
145935a
Compare
|
@howardjohn: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| switch v { | ||
| case "never", "false": | ||
| return NativeSidecarModeNever | ||
| case "always", "true": | ||
| return NativeSidecarModeAlways | ||
| case "auto-beta": |
There was a problem hiding this comment.
From my perspective this is not improve UX, on the opposite it is increasing complexity
There was a problem hiding this comment.
Is it the auto-beta vs auto-stable? Or even auto would be too complex?
There was a problem hiding this comment.
I mean auto-beta auto-stable, rare people can distinguish them
There was a problem hiding this comment.
I would rather it just be never/always/auto for this PR - whether we use k8s beta features or not should not be a per-feature decision anyway, and should be controlled elsewhere globally for all features (by stable profiles, etc etc).
istio-policy-bot
added
the
lifecycle/stale
|
🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2024-03-25. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions. Created by the issue and PR lifecycle manager. |
istio-policy-bot
added
the
lifecycle/automatically-closed
Fixes #48794