graduate CSIServiceAccountToken to beta #99298
Conversation
k8s-ci-robot
added
release-note
|
/cc @msau42 |
|
/sig storage |
k8s-ci-robot
added
sig/storage
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
k8s-ci-robot
added
size/M
|
/lgtm |
k8s-ci-robot
added
the
api-review
|
/retest not sure why pull-kubernetes-verify is falling due to openspec but |
|
Hi @zshihang, a friendly reminder that the code freeze for 1.21 is today. |
|
This lgtm, there is one open question on whether we need to increment the generation number in the strategy. @liggitt can you do another pass? |
|
/retest |
pkg/apis/storage/types.go
Outdated
| // CSIServiceAccountToken feature is enabled. | ||
| // | ||
| // +optional | ||
| // +listType=atomic | ||
| // +patchMergeKey=audience |
There was a problem hiding this comment.
I don't think a mergeable list is what we want here (and it actually doesn't work consistently in my experiments).
The audience field is not actually required by the API server (you can kubectl apply -f ... --validate=false a manifest that is missing it, or create one via a controller without explicitly sending an audience field in a tokenrequest item)
If you do that, you end up with an existing persisted object kubectl apply and kubectl apply --server-side don't know how to merge updates to properly.
They either complain and fail:
kubectl apply -f ~/snippets/csidriver/driver2.yaml
warning: error calculating patch from openapi spec: map: map[] does not contain declared merge key: audience
error: error when applying patch:
to:
Resource: "storage.k8s.io/v1, Resource=csidrivers", GroupVersionKind: "storage.k8s.io/v1, Kind=CSIDriver"
Name: "tokens", Namespace: ""
for: "/Users/liggitt/snippets/csidriver/driver2.yaml": error when creating patch with:
original:
{"apiVersion":"storage.k8s.io/v1","kind":"CSIDriver","metadata":{"annotations":{},"name":"tokens"},"spec":{"tokenRequests":[{}],"volumeLifecycleModes":["Persistent","Ephemeral"]}}
modified:
{"apiVersion":"storage.k8s.io/v1","kind":"CSIDriver","metadata":{"annotations":{"kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"storage.k8s.io/v1\",\"kind\":\"CSIDriver\",\"metadata\":{\"annotations\":{},\"name\":\"tokens\"},\"spec\":{\"tokenRequests\":[{\"audience\":\"another\"}],\"volumeLifecycleModes\":[\"Persistent\",\"Ephemeral\"]}}\n"},"name":"tokens"},"spec":{"tokenRequests":[{"audience":"another"}],"volumeLifecycleModes":["Persistent","Ephemeral"]}}
current:
{"apiVersion":"storage.k8s.io/v1","kind":"CSIDriver","metadata":{"annotations":{"kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"storage.k8s.io/v1\",\"kind\":\"CSIDriver\",\"metadata\":{\"annotations\":{},\"name\":\"tokens\"},\"spec\":{\"tokenRequests\":[{}],\"volumeLifecycleModes\":[\"Persistent\",\"Ephemeral\"]}}\n"},"creationTimestamp":"2021-03-09T20:57:24Z","managedFields":[{"apiVersion":"storage.k8s.io/v1","fieldsType":"FieldsV1","fieldsV1":{"f:metadata":{"f:annotations":{".":{},"f:kubectl.kubernetes.io/last-applied-configuration":{}}},"f:spec":{"f:attachRequired":{},"f:fsGroupPolicy":{},"f:podInfoOnMount":{},"f:requiresRepublish":{},"f:storageCapacity":{},"f:tokenRequests":{".":{},"k:{\"audience\":\"\"}":{".":{},"f:audience":{}}},"f:volumeLifecycleModes":{".":{},"v:\"Ephemeral\"":{},"v:\"Persistent\"":{}}}},"manager":"kubectl-client-side-apply","operation":"Update","time":"2021-03-09T20:57:24Z"}],"name":"tokens","resourceVersion":"330","uid":"98c2d9e1-817f-40a0-b2ba-fae511f4adb1"},"spec":{"attachRequired":true,"fsGroupPolicy":"ReadWriteOnceWithFSType","podInfoOnMount":false,"requiresRepublish":false,"storageCapacity":false,"tokenRequests":[{"audience":""}],"volumeLifecycleModes":["Persistent","Ephemeral"]}}
for: "/Users/liggitt/snippets/csidriver/driver2.yaml": map: map[] does not contain declared merge key: audience
or silently keep the existing audience:"" entry and add another item to the list.
I would expect a single actor to be defining the CSIDriver spec and dictating the entire content of this list, which would mean it should stay atomic.
There was a problem hiding this comment.
kubectl apply --validate=false with this:
apiVersion: storage.k8s.io/v1
kind: CSIDriver
metadata:
name: tokens
spec:
volumeLifecycleModes:
- Persistent
- Ephemeral
tokenRequests: [{}]
then try to kubectl apply with this:
apiVersion: storage.k8s.io/v1
kind: CSIDriver
metadata:
name: tokens
spec:
volumeLifecycleModes:
- Persistent
- Ephemeral
tokenRequests: [{"audience":"another"}]
kubectl apply --server-side submits the request, but produces a CSIDriver containing the old audience as well, rather than dropping it properly:
tokenRequests:
- audience: another
- audience: ""There was a problem hiding this comment.
didn't know there is a --validate=false to ignore the validation. how to mark a field as required if--validate=false? then i think above case would be prevented.
i was thinking there might be a case where a controller is reconciling the CSIDriver object but yea, single actor is most common.
changed back to atomic.
| } | ||
|
|
||
| // Any changes to the mutable fields increment the generation number. | ||
| if !reflect.DeepEqual(oldCSIDriver.Spec.TokenRequests, newCSIDriver.Spec.TokenRequests) || !reflect.DeepEqual(oldCSIDriver.Spec.RequiresRepublish, newCSIDriver.Spec.RequiresRepublish) { |
There was a problem hiding this comment.
use apiequality.Semantic.DeepEqual, tokenrequests null and [] are equivalent, but reflect.DeepEqual considers them different
| if !apiequality.Semantic.DeepEqual(old.Spec, new.Spec) { | ||
| allErrs = append(allErrs, field.Invalid(field.NewPath("spec"), new.Spec, "field is immutable")) | ||
| // immutable fields should not be mutated. | ||
| if !apiequality.Semantic.DeepEqual(old.Spec.AttachRequired, new.Spec.AttachRequired) { |
There was a problem hiding this comment.
can do all of these as a one-liner like this:
allErrs = append(allErrs, apimachineryvalidation.ValidateImmutableField(new.Spec.AttachRequired, old.Spec.AttachRequired, field.NewPath("spec", "attachedRequired"))...)
| if !apiequality.Semantic.DeepEqual(old.Spec, new.Spec) { | ||
| allErrs = append(allErrs, field.Invalid(field.NewPath("spec"), new.Spec, "field is immutable")) | ||
| } | ||
| allErrs = append(allErrs, validateCSIDriverSpec(&new.Spec, field.NewPath("spec"))...) |
There was a problem hiding this comment.
From #99298 (comment), the recommendation is that we only validate the mutable fields instead of the whole spec.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
kubernetes/enhancements#2047 (comment) /milestone clear |
|
@annajung exception is filed. |
|
/approve API changes lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, msau42, zshihang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/milestone v1.21 since the exception request was APPROVED. ref: https://groups.google.com/g/kubernetes-sig-release/c/xnR5k3nI6Dk/m/VXbsxE7nAgAJ |
|
/retest |
1 similar comment
|
/retest |
What type of PR is this?
/kind api-change
What this PR does / why we need it:
graduate CSIServiceAccountToken to beta
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: