Skip to content

Fix service account node audience restriction for in-tree pv to csi migration #129993

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 6, 2025
Merged

Fix service account node audience restriction for in-tree pv to csi migration #129993

merged 2 commits into from
Feb 6, 2025

Conversation

aramase
Copy link
Member

@aramase aramase commented Feb 5, 2025 •
edited
Loading

Translate in-tree inline volume or in-tree persistent volumes to CSI using the CSI translator while verifying whether the kubelet is allowed to request a token for an audience when enforcing service account node audience restrictions.

fixes #129935

Fixes a regression with the ServiceAccountNodeAudienceRestriction feature where `azureFile` volumes encounter "failed to get service accoount token attributes" errors

[KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-auth/4412-projected-service-account-tokens-for-kubelet-image-credential-providers/README.md

/kind bug
/sig auth
/triage accepted
/milestone v1.33
/priority important-soon

@k8s-ci-robot
Copy link
Contributor

@aramase: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Milestone Maintainers Team and have them propose you as an additional delegate for this responsibility.

In response to this:

Translate in-tree PV to CSI using the CSI translator while verifying whether the kubelet is allowed to request a token for an audience when enforcing service account node audience restrictions.

fixes #129935

TODO

[KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-auth/4412-projected-service-account-tokens-for-kubelet-image-credential-providers/README.md

/kind bug
/sig auth
/triage accepted
/milestone v1.33
/priority important-soon

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/auth Categorizes an issue or PR as relevant to SIG Auth. triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 5, 2025
@aramase aramase changed the title Aramase/i/fix 129935 Fix service account node audience restriction for in-tree pv to csi migration Feb 5, 2025
@aramase
Copy link
Member Author

aramase commented Feb 5, 2025

/assign @dobsonj @jsafrane

Could you review this PR? previous review: #128077 (comment).

@aramase aramase force-pushed the aramase/i/fix_129935 branch from 8281e13 to 8ff8f8b Compare February 5, 2025 20:46
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 5, 2025
@aramase
Copy link
Member Author

aramase commented Feb 5, 2025 •
edited
Loading

Added a unit test in d6c50c3 that shows the failure. The test passes with the fix and I've added integration test as well.

=== RUN   Test_nodePlugin_Admit/intree_pv_to_csi,_allow_create_of_token_when_audience_in_pod_-->_csi_-->_driver_-->_tokenrequest_with_audience,_ServiceAccountNodeAudienceRestriction=true
    admission_test.go:246: nodePlugin.Admit() error = serviceaccounts "mysa" is forbidden: audience "foo" not found in pod spec volume, expected 
--- FAIL: Test_nodePlugin_Admit/intree_pv_to_csi,_allow_create_of_token_when_audience_in_pod_-->_csi_-->_driver_-->_tokenrequest_with_audience,_ServiceAccountNodeAudienceRestriction=true (0.00s)


=== RUN   Test_nodePlugin_Admit/intree_inline_vol_to_csi,_allow_create_of_token_when_audience_in_pod_-->_csi_-->_driver_-->_tokenrequest_with_audience,_ServiceAccountNodeAudienceRestriction=true
    admission_test.go:246: nodePlugin.Admit() error = serviceaccounts "mysa" is forbidden: audience "foo" not found in pod spec volume, expected 
--- FAIL: Test_nodePlugin_Admit/intree_inline_vol_to_csi,_allow_create_of_token_when_audience_in_pod_-->_csi_-->_driver_-->_tokenrequest_with_audience,_ServiceAccountNodeAudienceRestriction=true (0.00s)

@aramase
Copy link
Member Author

aramase commented Feb 5, 2025

/assign enj

carlory
carlory reviewed Feb 6, 2025
View reviewed changes
@aramase
node audience restriction: add unit tests to simulate intree inline_v…
d6c50c3 
…ol/pv to csi failure

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
@aramase aramase force-pushed the aramase/i/fix_129935 branch from 8ff8f8b to 042a344 Compare February 6, 2025 18:51
@aramase
Copy link
Member Author

aramase commented Feb 6, 2025

/assign liggitt

liggitt
liggitt reviewed Feb 6, 2025
View reviewed changes
liggitt
liggitt reviewed Feb 6, 2025
View reviewed changes
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

generally looks reasonable, a couple questions / nits

What are we planning to do to recover 1.32 from this regression? our options are to backport this fix or to toggle ServiceAccountNodeAudienceRestriction to false in 1.32. I'd be inclined to do the latter.

@aramase
node audience restriction: use csi translator to convert intree inlin…
62809dd 
…e_vol/pv to csi

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
@aramase aramase force-pushed the aramase/i/fix_129935 branch from 042a344 to 62809dd Compare February 6, 2025 19:17
@liggitt
Copy link
Member

liggitt commented Feb 6, 2025

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 988023942ea3b779d12eaf31096d4515060c8adc

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aramase, liggitt

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 Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 6, 2025
@aramase
Copy link
Member Author

aramase commented Feb 6, 2025

What are we planning to do to recover 1.32 from this regression? our options are to backport this fix or to toggle ServiceAccountNodeAudienceRestriction to false in 1.32. I'd be inclined to do the latter.

I was thinking we do the backport. Were you thinking we disable it only in v1.32 and keep it as-is (enabled) in v1.33+?

@liggitt
Copy link
Member

liggitt commented Feb 6, 2025

Were you thinking we disable it only in v1.32 and keep it as-is (enabled) in v1.33+?

Yes

@liggitt liggitt mentioned this pull request Feb 6, 2025
Merged
@aramase
Copy link
Member Author

aramase commented Feb 6, 2025

Were you thinking we disable it only in v1.32 and keep it as-is (enabled) in v1.33+?

Yes

#130015 to disable it in 1.32.

This was referenced Feb 6, 2025
Closed
Open
@k8s-ci-robot k8s-ci-robot merged commit 7b38ff4 into kubernetes:master Feb 6, 2025
13 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.33 milestone Feb 6, 2025
@aramase aramase deleted the aramase/i/fix_129935 branch February 6, 2025 22:40
@aramase aramase mentioned this pull request Feb 27, 2025
Open
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@carlory carlory carlory left review comments

@mikedanese mikedanese Awaiting requested review from mikedanese

Assignees

@liggitt liggitt

@jsafrane jsafrane

@enj enj

@dobsonj dobsonj

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Auth
Archived in project
Milestone
v1.33
Development

Successfully merging this pull request may close these issues.

CSI migration with in-tree azure-file provider fails to mount PVC to Pod
7 participants
@aramase @k8s-ci-robot @liggitt @carlory @jsafrane @enj @dobsonj