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

graduate CSIServiceAccountToken to beta #99298

Merged
merged 1 commit into from Mar 12, 2021
Merged

Conversation

zshihang
Copy link
Contributor

@zshihang zshihang commented Feb 22, 2021

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?

CSIServiceAccountToken is Beta now

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/1855-csi-driver-service-account-token

@k8s-ci-robot k8s-ci-robot added release-note kind/api-change size/XS cncf-cla: yes do-not-merge/needs-sig needs-triage needs-priority labels Feb 22, 2021
@zshihang
Copy link
Contributor Author

@zshihang zshihang commented Feb 22, 2021

/cc @msau42

@zshihang
Copy link
Contributor Author

@zshihang zshihang commented Feb 22, 2021

/sig storage
/sig auth
/priority important-soon
/triage accepted

@k8s-ci-robot k8s-ci-robot added sig/storage sig/auth priority/important-soon triage/accepted and removed do-not-merge/needs-sig needs-priority needs-triage labels Feb 22, 2021
@fejta-bot
Copy link

@fejta-bot fejta-bot commented Feb 22, 2021

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 k8s-ci-robot added size/M area/test sig/testing and removed size/XS labels Feb 24, 2021
@msau42
Copy link
Member

@msau42 msau42 commented Feb 24, 2021

/lgtm
/approve
/label api-review

@k8s-ci-robot k8s-ci-robot added the api-review label Feb 24, 2021
@zshihang
Copy link
Contributor Author

@zshihang zshihang commented Mar 5, 2021

/retest

not sure why pull-kubernetes-verify is falling due to openspec but ./hack/verify-openapi-spec.sh passe locally

@annajung
Copy link
Member

@annajung annajung commented Mar 9, 2021

Hi @zshihang, a friendly reminder that the code freeze for 1.21 is today.
Please make sure to rebase and get the necessary reviews for this to merge by EOD PST.

@liggitt liggitt added this to the v1.20 milestone Mar 9, 2021
@liggitt liggitt removed this from the v1.20 milestone Mar 9, 2021
@liggitt liggitt added this to the v1.21 milestone Mar 9, 2021
@msau42
Copy link
Member

@msau42 msau42 commented Mar 9, 2021

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?

@zshihang
Copy link
Contributor Author

@zshihang zshihang commented Mar 9, 2021

/retest

// CSIServiceAccountToken feature is enabled.
//
// +optional
// +listType=atomic
// +patchMergeKey=audience
Copy link
Member

@liggitt liggitt Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@liggitt liggitt Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: ""

Copy link
Contributor Author

@zshihang zshihang Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member

@liggitt liggitt Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use apiequality.Semantic.DeepEqual, tokenrequests null and [] are equivalent, but reflect.DeepEqual considers them different

Copy link
Contributor Author

@zshihang zshihang Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pkg/registry/storage/csidriver/strategy.go Show resolved Hide resolved
pkg/registry/storage/csidriver/strategy.go Show resolved Hide resolved
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) {
Copy link
Member

@liggitt liggitt Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"))...)

Copy link
Contributor Author

@zshihang zshihang Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point

pkg/apis/storage/validation/validation.go Show resolved Hide resolved
@liggitt liggitt moved this from In progress to Changes requested in API Reviews Mar 9, 2021
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"))...)
Copy link
Member

@msau42 msau42 Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From #99298 (comment), the recommendation is that we only validate the mutable fields instead of the whole spec.

Copy link
Contributor Author

@zshihang zshihang Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@msau42
Copy link
Member

@msau42 msau42 commented Mar 10, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 10, 2021
@annajung
Copy link
Member

@annajung annajung commented Mar 10, 2021

kubernetes/enhancements#2047 (comment)

/milestone clear

@k8s-ci-robot k8s-ci-robot removed this from the v1.21 milestone Mar 10, 2021
@zshihang
Copy link
Contributor Author

@zshihang zshihang commented Mar 10, 2021

@annajung exception is filed.

@liggitt
Copy link
Member

@liggitt liggitt commented Mar 11, 2021

/approve

API changes lgtm

@liggitt liggitt moved this from Changes requested to API review completed, 1.21 in API Reviews Mar 11, 2021
@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Mar 11, 2021

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved label Mar 11, 2021
@palnabarun
Copy link
Member

@palnabarun palnabarun commented Mar 11, 2021

/milestone v1.21

since the exception request was APPROVED. ref: https://groups.google.com/g/kubernetes-sig-release/c/xnR5k3nI6Dk/m/VXbsxE7nAgAJ

@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 11, 2021
@zshihang
Copy link
Contributor Author

@zshihang zshihang commented Mar 11, 2021

/retest

1 similar comment
@zshihang
Copy link
Contributor Author

@zshihang zshihang commented Mar 11, 2021

/retest

@k8s-ci-robot k8s-ci-robot merged commit d43ffff into kubernetes:master Mar 12, 2021
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review approved area/test cncf-cla: yes kind/api-change lgtm priority/important-soon release-note sig/auth sig/storage sig/testing size/L triage/accepted
Projects
API Reviews
API review completed, 1.21
Development

Successfully merging this pull request may close these issues.

None yet

9 participants