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

Remove ClusterRole permissions for extensions API group #2216

Merged
merged 1 commit into from
Jan 24, 2023

Conversation

jdn5126
Copy link
Contributor

@jdn5126 jdn5126 commented Jan 13, 2023

What type of PR is this?
cleanup

Which issue does this PR fix:
#1232

What does this PR do / Why do we need it:
This PR removes ClusterRole permissions for extensions API group. From what I can tell, the aws-node pod does not need list or watch permissions for any extensions.

Looking at the history, these permissions were originally added for daemonsets, but aws-node does not need to list or watch any other daemonsets. It does not need to list or watch any deployments or replicasets either.

If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
N/A

Testing done on this change:
Manually ran all IPv4 and IPv6 integration and e2e tests and did not run into any issues.

Automation added to e2e:
N/A

Will this PR introduce any new dependencies?:
No

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
This will not break upgrades or downgrades. A running cluster has been tested.

Does this change require updates to the CNI daemonset config files to work?:
Yes

Does this PR introduce any user-facing change?:
Yes

Remove ClusterRole permissions for extensions API group

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jdn5126 jdn5126 requested review from jayanthvn and achevuru January 13, 2023 23:43
@jdn5126 jdn5126 requested a review from a team as a code owner January 13, 2023 23:43
@jdn5126
Copy link
Contributor Author

jdn5126 commented Jan 13, 2023

The last associated PR I found was #1082

@jdn5126 jdn5126 force-pushed the extension_scope branch 4 times, most recently from ca659bf to 20383c1 Compare January 17, 2023 17:37
@jayanthvn
Copy link
Contributor

Did you test this with custom networking? We were not able to get ENIconfigs CRD without the extensions RBAC, #1082 description has the logs. It would be good to confirm.

@jdn5126
Copy link
Contributor Author

jdn5126 commented Jan 17, 2023

Did you test this with custom networking? We were not able to get ENIconfigs CRD without the extensions RBAC, #1082 description has the logs. It would be good to confirm.

Oh, I don't think I ran custom networking tests, let me try that now. I started this assuming that I would need to change * to just ENIConfig, but I wanted to test with nothing first to see what errors were raised. Haven't seen any errors so far, but will report back once custom networking is run.

@jdn5126
Copy link
Contributor Author

jdn5126 commented Jan 20, 2023

Did you test this with custom networking? We were not able to get ENIconfigs CRD without the extensions RBAC, #1082 description has the logs. It would be good to confirm.

I ran the custom_networking e2e tests without the extensions RBAC and the tests still passed. Are there any specific cases you think I should cover with custom networking? The test cases do cover ENIConfig

@jayanthvn
Copy link
Contributor

I am wondering it is not an issue now because we are fetching from node cache, if we are able to fetch eniconfigs and allocate ENIs then we should be good.

@jdn5126 jdn5126 force-pushed the extension_scope branch 2 times, most recently from adbd034 to 0ae68d7 Compare January 23, 2023 23:24
jayanthvn
jayanthvn previously approved these changes Jan 23, 2023
Copy link
Contributor

@jayanthvn jayanthvn left a comment

Choose a reason for hiding this comment

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

lgtm

@jayanthvn
Copy link
Contributor

We need to track MAO and default manifest changes for this.

@jdn5126 jdn5126 merged commit faa849e into aws:master Jan 24, 2023
@jdn5126 jdn5126 deleted the extension_scope branch January 24, 2023 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants