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.

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

fix!: Revert create least privilege default service account (#1757) #1827

Merged
merged 4 commits into from
Dec 26, 2023
Merged

fix!: Revert create least privilege default service account (#1757) #1827

merged 4 commits into from
Dec 26, 2023
+237 −20

Conversation

apeabody
Copy link
Contributor

This reverts commit 350faa7.

@apeabody apeabody requested review from ericyz and a team as code owners December 21, 2023 18:03
@apeabody apeabody mentioned this pull request Dec 21, 2023
Merged
@apeabody apeabody changed the title Revert "feat!: Create least privilege default service account (#1757)" fix!: Revert create least privilege default service account (#1757) Dec 21, 2023
@apeabody apeabody merged commit 0d7f638 into master Dec 26, 2023
8 checks passed
@apeabody apeabody deleted the ap-fds3 branch December 26, 2023 16:21
@release-please release-please bot mentioned this pull request Dec 26, 2023
Merged
@gorge511
Copy link
Contributor

Hi @apeabody, why this is reverted to old solution and not to the new generally available role roles/container.nodeServiceAgent? Is there any reasoning I'm not aware of?

@apeabody
Copy link
Contributor Author

roles/container.nodeServiceAgent

Hi @gorge511 - I believe at the time of the revert that roles/container.nodeServiceAgent wasn't yet generally available. Would be happy to review a PR to switch it to now. We can include in the upcoming release.

@apeabody
Copy link
Contributor Author

roles/container.nodeServiceAgent

Hi @gorge511 - I believe at the time of the revert that roles/container.nodeServiceAgent wasn't yet generally available. Would be happy to review a PR to switch it to now. We can include in the upcoming release.

I haven't had a chance to dig into the details, but looks like use of an Agent role isn't recommended for this propose: #1757 (comment)

@gorge511
Copy link
Contributor

I haven't had a chance to dig into the details, but looks like use of an Agent role isn't recommended for this propose: #1757 (comment)

Thanks for the quick response. Yes, it seems that the Agent role is not the right one, even if it looks really good for this use case 🙂
I guess we should continue with the discussion in #1757, there are more people in the discussion.

gorge511 added a commit to gorge511/terraform-google-kubernetes-engine that referenced this pull request Jan 13, 2024
@gorge511
feat: Update least privilege default service account
46ae290 
This update follows changes from terraform-google-modules#1757 and reverts terraform-google-modules#1827.

The role `roles/container.nodeServiceAccount` is deprecated now and it is replaced with new `roles/container.defaultNodeServiceAccount` role.
Unfortunately this is not yet documented in Google docs.

As the scope of the new role is smaller than the old one, this should be considered breaking change.
@gorge511 gorge511 mentioned this pull request Jan 13, 2024
Merged
@yorik yorik mentioned this pull request May 14, 2024
Closed
CPL-markus pushed a commit to WALTER-GROUP/terraform-google-kubernetes-engine that referenced this pull request Jul 15, 2024
@apeabody
fix!: Revert create least privilege default service account (terrafor…
dd53d92 
…m-google-modules#1757) (terraform-google-modules#1827)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bharathkkb bharathkkb bharathkkb approved these changes

@ericyz ericyz Awaiting requested review from ericyz ericyz is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@apeabody @gorge511 @bharathkkb