-
Notifications
You must be signed in to change notification settings - Fork 286
feat: Add sample for custom node service account #869
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
Conversation
|
Here is the summary of changes. You are about to add 4 region tags.
This comment is generated by snippet-bot.
|
|
/gcbrun |
apeabody
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @shannonxtreme!
glasnt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some issues with your PR, comments below.
Also: if you're going to rebase, reword your commit to start with feat: to solve the coventional commit error
|
/gcbrun |
1 similar comment
|
/gcbrun |
|
/gcbrun |
|
Oh I should probably rebase and squash |
182b76f to
19b95dd
Compare
|
/gcbrun |
apeabody
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @shannonxtreme! All the tests are now passing!
During final review I believe there is an opportunity to consolidate/deduplicate by adding the required region tags to gke/autopilot/custom_service_account/main.tf and dropping the gke/node_service_account/main.tf file?
|
@apeabody the reason I made it a separate file is because creating the least privilege service account is a standalone task that's applicable both Standard and Autopilot clusters (and to standard node pools). It'd feel less correct to include this region in the autopilot cluster create file in, for example, the gke hardening guide. Wdyt? |
Thanks @shannonxtreme! I know we've done it in the past, but lets check with @glasnt on any best practice? |
The way region tags work is that you can have one file with multiple different sections marked within a region tag for inclusion in the documentation. So in this case, you can wrap the service account creation section separately from the cluster declaration, so you can have this code embedded in separate steps of your documentation. If you have the service account in both the standard and autopilot cluster files, you can wrap it in region tags (like Andrew's suggest changes), and embed each part for each cluster type in context. Since that code is already defined, there's no reason to repeat the code in an isolated sample, when you can just re-use the region tag from another sample. Now I can see where if you wanted to show how to show the creation in a page not specific to a cluster type you may want a separate example, but given the code is identical I think it's more correct to reuse the code from one of the cluster types, if it can apply to both. Note that region tags must be unique per repo, so some of Andrew's suggested comments will only be valid if the duplicate file is deleted, but reusing code through region tags is best practice 👍 |
|
Makes sense @glasnt, I'll make those changes soon. Out of scope, but curious: would a standalone service account sample have made more sense in the |
Yup! You're welcome to reuse create_service_agent that already exists (currently used in docs), but it looks like it's a bit context-specific. You could also link to this docs page showing this code. We could also create a simpler example explicitly for re-use like this. |
5c9c3e9 to
6cf0db6
Compare
|
Removed the standalone directory and added a region tag to the sample in |
|
/gcbrun |
Description
Fixed #754
Add docs samples for creating a custom IAM service account that has the least-privilege role for GKE nodes.
Checklist
Readiness
Style
guide
Testing
I have performed tests described in the Contributing guide:
terraform applyterraform fmtcheckIntended location
Yes, this sample will be (or already is) included on cloud.google.com
Location(s):
As well as in docs that show how to create a least-privilege IAM service account for GKE.
API enablement
Review