Feature gate initializers field #51436
Conversation
k8s-ci-robot
added
the
size/S
k8s-ci-robot
added
the
cncf-cla: yes
liggitt
added
release-note
liggitt
force-pushed
the
initializer-feature
branch
from
fd1589b to
f0dfa7c
Compare
k8s-ci-robot
added
size/M
liggitt
force-pushed
the
initializer-feature
branch
from
f0dfa7c to
50abe14
Compare
|
|
||
| if !utilfeature.DefaultFeatureGate.Enabled(features.Initializers) { | ||
| if err := utilfeature.DefaultFeatureGate.Set(string(features.Initializers) + "=true"); err != nil { | ||
| glog.Errorf("error enabling Initializers feature as part of admission plugin setup: %v", err) |
There was a problem hiding this comment.
utilruntime.HandleError
|
/approve Setting feature based on presence of Admission controller is very reasonable - the opposite does not make sense. Although it may be novel to some people, so we should make sure we communicate it. |
k8s-github-robot
added
the
approved
liggitt
force-pushed
the
initializer-feature
branch
from
50abe14 to
658956f
Compare
|
cc @kubernetes/sig-api-machinery-api-reviews |
k8s-ci-robot
added
sig/api-machinery
| @@ -99,6 +101,12 @@ func BeforeUpdate(strategy RESTUpdateStrategy, ctx genericapirequest.Context, ob | |||
| } | |||
| objectMeta.SetGeneration(oldMeta.GetGeneration()) | |||
|
|
|||
| // Ensure Initializers are not set unless the feature is enabled | |||
| if !utilfeature.DefaultFeatureGate.Enabled(features.Initializers) { | |||
| oldMeta.SetInitializers(nil) | |||
There was a problem hiding this comment.
if the feature is disabled, the server should behave as if it doesn't know about the field at all. ideally that would be done in decoding of the existing object from etcd and the sent object from the request based on field gates, but until then, we mimic that by dropping it from all objects here.
Could you add this to the release note? Then lgtm. |
|
/retest |
Added |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: caesarxuchao, liggitt, smarterclayton Associated issue: 51429 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/retest Review the full test history for this PR. |
1 similar comment
|
/retest Review the full test history for this PR. |
|
Automatic merge from submit-queue (batch tested with PRs 51471, 50561, 50435, 51473, 51436) |
Automatic merge from submit-queue (batch tested with PRs 51707, 51662, 51723, 50163, 51633) Make feature gate threadsafe Fixes kubernetes#51548 caused by kubernetes#51436
| } | ||
|
|
||
| if !utilfeature.DefaultFeatureGate.Enabled(features.Initializers) { | ||
| if err := utilfeature.DefaultFeatureGate.Set(string(features.Initializers) + "=true"); err != nil { |
There was a problem hiding this comment.
Should adding the admission controller really override the feature flag?
The metadata.initializers field should be feature gated and disabled by default while in alpha, especially since enforcement of initializer permission that keeps users from submitting objects with their own initializers specified is done via an admission plugin most clusters do not enable yet.
Not gating the field and tests caused tests added in #51429 to fail on clusters that don't enable the admission plugin.
This PR:
Initializersfeature gate, auto-enables the feature gate if the admission plugin is enabledmetadata.initializersfield of objects on create/update if the feature gate is not set