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

Document best practice for service accounts, scopes, and IAM roles #3239

Conversation

conorgil
Copy link
Contributor

After further research, I realized that what I wrote in #3116 was incorrect. The scopes field cannot be left blank.

This PR corrects that error and adds fully functional examples to comput_instance and compute_instance_template explaining the best practice for assigning permissions to instances using service accounts and IAM roles.

I make sure to link to the supporting documentation where I got my information from.

- Update and unify documentation for both
  compute_instance and compute_instance_template
  resources
- Add example demonatrating the best practice for
  using service accounts and IAM roles to define
  permissions for instances. Specifically, explain
  why scopes are still required and how they interact
  with the IAM roles defined for the service account.
- Link to documentation which explains the best practices
  demonstrated in the examples.
@conorgil
Copy link
Contributor Author

@rileykarson ping since you merged #3116 and likely have the best context for this.

@chrisst chrisst self-assigned this Mar 14, 2019
Copy link
Contributor

@chrisst chrisst left a comment

Choose a reason for hiding this comment

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

The content here is super valuable but the resource documentation isn't the right place for documenting GCP best practices. The goal for these pages is to document the structure of the resource and give a handful of examples of configuration as a jump start. Documenting application of GCP best practices or trying to add examples for every use case is a quickly moving target that would result in a significant amount of churn in our documentation. This will be especially true for complex resources such as compute_instance or container_cluster.

Our goal is that these docs can follow a few rules:

  • No examples or comments should violate best practices
  • Best practices for using a resource should be minimal and link to GCP documentation
  • References to how the GCP products work should do the same
  • There should be at least 1 example for the most minimal configuration
  • Other examples should be for very common use patterns or mutually exclusive configurations.

I think the Best Practices blocks and examples in this PR are more appropriate in a blog post that could be referenced. You could try posting it to https://cloud.google.com/community/tutorials/ if you don't have an existing publishing medium. If this was pared down to just the links that you have found and the clarifications to the fields I think it would be more appropriate for this medium.

@conorgil
Copy link
Contributor Author

@chrisst thanks for commenting!

At a minimum, the Terraform docs need to be updated to correct the error that was merged in #3116; the scopes field cannot be an empty array when using IAM roles to specify permissions on a service account. Whether that happens in this PR or a new one is TBD.

The content here is super valuable but the resource documentation isn't the right place for documenting GCP best practices.

I agree that the resource documentation is likely not the right place for detailed information about many best practices, but I think this might be an exception because IAM permissions are intrinsically linked to creating compute instances.

No examples or comments should violate best practices

The examples in both compute_instance and compute_instance_template violate best practice because they only show use of the scopes field, which GCP docs clearly state is a legacy approach. The examples should be updated to highlight use of a service account and IAM roles.

Best practices for using a resource should be minimal and link to GCP documentation

I tried my best to strive for this goal in this PR. Is there something specific that jumps out at you as non-minimal in the example that potentially could be removed? I'm happy to update.

If the title of the example is contentious because it says "best practice", then I'd be happy to change the title to something like "Using with service accounts and IAM roles".

References to how the GCP products work should do the same

Perhaps my example adds too much pre-text explanation and this is what you think can/should be pared down? I was motivated by the incredibly useful examples in the compute_instance_template docs for Using with Instance Group Manager and Deploying the Latest Image, which contain a similar amount of background explanation.

There should be at least 1 example for the most minimal configuration

The example in my PR is a very minimal example for launching a compute instance and controlling its permissions following the best practice of using a Service Account and IAM roles. I could make it smaller by eliminating use of locals and inline the IAM role in the google_project_iam_member resource.

Do you mean that the example should be the most minimal example possible to convey the concept of how the resource works, but does not necessarily need to be fully functional?

Other examples should be for very common use patterns or mutually exclusive configurations.

The main reason I think this example should be included here instead of an external blog post or community article is that this is the common use pattern; anyone who launches a compute instance in almost any scenario will want to assign it permissions. Figuring out how to utilize multiple Terraform resources to achieve this basic use-case was incredibly frustrating for me, so I expect others have felt the same.

Has any of the above convinced you that the example should be included in these docs?

@chrisst
Copy link
Contributor

chrisst commented Mar 19, 2019

I think the intent of the examples in instance_template was to document how to use resources together when they require extra information to use together without errors. Information like create_before_destroy shows how to use the resources when the most straight forward approach would throw errors. The example of deploying the latest image is a bit blurrier but it's there to document some choices that were made in the provider that aren't part of the upstream api.

In your case this isn't an explanation of nuances within the provider code but a good pattern of using GCP. It's a more compelling argument to me for documenting a common usage pattern of compute_instance but I'm not sure how often people are attaching service_accounts to their compute instances rather than managing communication between services in their application logic. I'll see if my team mates have more insight on this and get back to you.

I do think that correcting the examples and the scopes documentation is a good move though. If you'd like to add that in another PR while this is being resolved that would get them in much more quickly.

// web server instances and will be granted
// IAM roles that define the minimum set of
// required permissions.
account_id = "web-server"
Copy link
Contributor

Choose a reason for hiding this comment

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

I already encounter soft delete in GCP with service when playing with terraform apply/destroy

Since this time I use random provider to get new name on apply/destroy

@rileykarson
Copy link
Collaborator

I'm going to close this as won't fix. Along the lines that @chrisst was saying, we want to hit a lot closer to reference documentation than best practices documentation (unless those best practices are specific to Terraform).

Regardless, thanks for your suggestions!

@rileykarson rileykarson closed this Oct 7, 2020
@ghost
Copy link

ghost commented Nov 7, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants