Skip to content

Conversation

@mdzraf
Copy link
Member

@mdzraf mdzraf commented Dec 11, 2025

What type of PR is this?

Testing CI with Minimal Policy & review new proposed minimal policy.

Manual Tests Completed:

* Create Volume (With Tags)
* Modify Volume (Changed volume type, iops, and deleted tag)
* Attach Volume
* Detach Volume
* Delete Volume 
* Resize Volume
* Create Snapshot From Volume (snapshot with tags, and FSR enabled)
* Restore Volume From Snapshot
* Delete snapshot
* Copy Volume (with tags)
* Create Snapshot with Lock
* Cannot Create/consume PV from pre-existing volume
* Can create/consume PV from pre-existing volume that had tags manually added

@k8s-ci-robot
Copy link
Contributor

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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 the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Dec 11, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jsafrane for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 11, 2025
@github-actions
Copy link

Code Coverage Diff

This PR does not change the code coverage

@mdzraf mdzraf force-pushed the minimalPolicyChanges branch from 7275322 to 84e80d8 Compare December 11, 2025 16:59
"ec2:DescribeAvailabilityZones",
"ec2:DescribeInstances",
"ec2:DescribeSnapshots",
"ec2:DescribeTags",
Copy link
Member Author

Choose a reason for hiding this comment

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

We do not actually call this anywhere in the driver besides in a testing function that does not actually need it:

mockEC2.EXPECT().DescribeTags(gomock.Any(), gomock.Any()).Return(&ec2.DescribeTagsOutput{}, nil).AnyTimes()

So I have removed it from the policy.

Comment on lines 7 to 12
"ec2:DescribeAvailabilityZones",
"ec2:DescribeInstances",
"ec2:DescribeSnapshots",
"ec2:DescribeTags",
"ec2:DescribeVolumes",
"ec2:DescribeVolumesModifications",
"ec2:DescribeVolumeStatus"
Copy link
Member Author

Choose a reason for hiding this comment

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

These calls have * permissions because they only have ec2:Region as an allowed condition key, therefore we cannot pass in any condition key that would clearly identify a resource managed by the driver (I.e. tags on resource) see: https://docs.aws.amazon.com/service-authorization/latest/reference/list_amazonec2.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Only made changes here to test CI, feel free to skip review.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only made changes here to test CI, feel free to skip review.

Comment on lines 55 to 65
"Effect" : "Allow",
"Action" : [
"ec2:AttachVolume",
"ec2:DetachVolume"
],
"Resource" : "arn:aws:ec2:*:*:instance/*"
},
Copy link
Member Author

Choose a reason for hiding this comment

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

Necessary for the EBS CSI Driver to perform the privileged task of attaching/detaching volumes on a host on behalf of the user.

@mdzraf
Copy link
Member Author

mdzraf commented Dec 12, 2025

/retest

Trying to see if it was a flake, the pre-provisioned tests should all pass since we actually do add the required tags.

@mdzraf mdzraf force-pushed the minimalPolicyChanges branch from 84e80d8 to fc23e74 Compare December 12, 2025 16:28
@mdzraf mdzraf force-pushed the minimalPolicyChanges branch from fc23e74 to 6240ca8 Compare December 12, 2025 20:35
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants