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

backend: Add service account impersonation to GCS Backend and update the docs #26700

Merged
merged 3 commits into from
Nov 6, 2020
Merged

backend: Add service account impersonation to GCS Backend and update the docs #26700

merged 3 commits into from
Nov 6, 2020

Conversation

upodroid
Copy link
Contributor

@upodroid upodroid commented Oct 25, 2020

I recently implemented Service Account Impersonation in the Google Terraform Provider. This PR offers similar functionality in Terraform Core.

hashicorp/terraform-provider-google#7542

I reviewed all the issues labelled as backend/gcs and many of them can be closed.

Fixes: #24736 by clarifying User ADCs expire.
Fixes: #23034 by clarifying how to run terraform on Google Cloud. Most users forget to set the correct scopes on their service accounts.
Fixes: #24392 Access tokens are used to Impersonate Service Accounts. This feature removes that need.
Closes: #22038 Same as 24392
Closes: #21680 This already exists
Closes: #20785 This already exists
Closes: #18955 This already exists
Fixes: #17222 Docs were recently updated to clarify that a bucket needs to be created beforehand and my new changes about authentication. I also added a note about eventually consistency. IAM grants take a few minutes to take effect on GCS and it is the cause of some of the 403s in that issue.
Closes: #21562 I repro'd that issue and it has been fixed. Left a comment on it.
Closes: #18933 I repro'd that issue and it has been fixed. Left a comment on it.
Closes: #24716 I answered the issue the OP linked from the TPG repo.
Closes: #25040 I answered the issue.
Closes: #25050 This PR is not needed. Explained to OP how to use User ADCs.

@codecov
Copy link

codecov bot commented Oct 25, 2020

Codecov Report

Merging #26700 into master will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Impacted Files Coverage Δ
backend/remote-state/gcs/backend.go 0.00% <0.00%> (ø)
dag/marshal.go 53.42% <0.00%> (-1.37%) ⬇️
terraform/eval_apply.go 72.86% <0.00%> (-0.59%) ⬇️
states/statefile/version4.go 58.19% <0.00%> (+0.23%) ⬆️

@upodroid upodroid changed the title Add service account impersonation to GCS Backend Add service account impersonation to GCS Backend and update the docs Oct 25, 2020
@pkolyvas pkolyvas changed the title Add service account impersonation to GCS Backend and update the docs backend: Add service account impersonation to GCS Backend and update the docs Oct 26, 2020
@upodroid
Copy link
Contributor Author

@rileykarson @megan07 FYI This needs to merged to complete the service account impersonation work.

Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

Hi @upodroid, thanks for your work on this, and for working through those GCS backend issues!

This PR seems to reintroduce module vendoring, which we removed in #26358. Can you please undo that change?

Until you're able to remove vendoring, it's difficult to review the PR, but from glancing at the go.mod changes I'm not sure what has necessitated so many dependency upgrades. If would be helpful if you could list the go mod commands you used to get here.

Thanks again!

@upodroid
Copy link
Contributor Author

Hi

I'm bumping google.golang.org/api to v0.34 which breaks the etcdv3 backend because of grpc dependencies. Going by this issue etcd-io/etcd#12124 (comment) the client implementation hasn't been written well.

I pushed a commit without the vendor.

@upodroid
Copy link
Contributor Author

 REDACTED  MCW0CDP3YY  ~  Desktop  Git  terraform   gcs-impersonation  ERROR  $   go get -u google.golang.org/api 
go: google.golang.org/api upgrade => v0.34.0
go: downloading golang.org/x/tools v0.0.0-20200904185747-39188db58858
go: golang.org/x/mod upgrade => v0.3.0
go: golang.org/x/lint upgrade => v0.0.0-20200302205851-738671d3881b
go: golang.org/x/tools upgrade => v0.0.0-20201028111035-eafbe7b904eb
go: golang.org/x/xerrors upgrade => v0.0.0-20200804184101-5ec99f83aff1
go: downloading golang.org/x/tools v0.0.0-20201028111035-eafbe7b904eb
 REDACTED  MCW0CDP3YY  ~  Desktop  Git  terraform   gcs-impersonation  2✎  $   go test
# github.com/coreos/etcd/clientv3
../../../go/pkg/mod/github.com/coreos/etcd@v3.3.10+incompatible/clientv3/health_balancer.go:54:10: undefined: grpc.Address
../../../go/pkg/mod/github.com/coreos/etcd@v3.3.10+incompatible/clientv3/health_balancer.go:60:18: undefined: grpc.Address
../../../go/pkg/mod/github.com/coreos/etcd@v3.3.10+incompatible/clientv3/health_balancer.go:137:54: undefined: grpc.BalancerConfig
../../../go/pkg/mod/github.com/coreos/etcd@v3.3.10+incompatible/clientv3/health_balancer.go:208:41: undefined: grpc.Address
../../../go/pkg/mod/github.com/coreos/etcd@v3.3.10+incompatible/clientv3/health_balancer.go:383:34: undefined: grpc.Address
../../../go/pkg/mod/github.com/coreos/etcd@v3.3.10+incompatible/clientv3/health_balancer.go:434:38: undefined: grpc.Address
../../../go/pkg/mod/github.com/coreos/etcd@v3.3.10+incompatible/clientv3/health_balancer.go:470:56: undefined: grpc.BalancerGetOptions
../../../go/pkg/mod/github.com/coreos/etcd@v3.3.10+incompatible/clientv3/health_balancer.go:470:82: undefined: grpc.Address
../../../go/pkg/mod/github.com/coreos/etcd@v3.3.10+incompatible/clientv3/health_balancer.go:518:44: undefined: grpc.Address
../../../go/pkg/mod/github.com/coreos/etcd@v3.3.10+incompatible/clientv3/health_balancer.go:577:22: undefined: grpc.Address
../../../go/pkg/mod/github.com/coreos/etcd@v3.3.10+incompatible/clientv3/health_balancer.go:577:22: too many errors
FAIL    github.com/hashicorp/terraform [build failed]
 REDACTED  MCW0CDP3YY  ~  Desktop  Git  terraform   gcs-impersonation  2✎  USAGE  $   go mod edit -replace google.golang.org/grpc@v1.31.1=google.golang.org/grpc@v1.26.0
 REDACTED  MCW0CDP3YY  ~  Desktop  Git  terraform   gcs-impersonation  2✎  $   go test# google.golang.org/api/internal
../../../go/pkg/mod/google.golang.org/api@v0.34.0/internal/conn_pool.go:29:2: undefined: grpc.ClientConnInterface
# google.golang.org/genproto/googleapis/iam/v1
../../../go/pkg/mod/google.golang.org/genproto@v0.0.0-20200904004341-0bd0a958aa1d/googleapis/iam/v1/iam_policy.pb.go:480:7: undefined: grpc.ClientConnInterface
../../../go/pkg/mod/google.golang.org/genproto@v0.0.0-20200904004341-0bd0a958aa1d/googleapis/iam/v1/iam_policy.pb.go:484:11: undefined: grpc.SupportPackageIsVersion6
../../../go/pkg/mod/google.golang.org/genproto@v0.0.0-20200904004341-0bd0a958aa1d/googleapis/iam/v1/iam_policy.pb.go:508:5: undefined: grpc.ClientConnInterface
../../../go/pkg/mod/google.golang.org/genproto@v0.0.0-20200904004341-0bd0a958aa1d/googleapis/iam/v1/iam_policy.pb.go:511:28: undefined: grpc.ClientConnInterface
FAIL    github.com/hashicorp/terraform [build failed]
 REDACTED  MCW0CDP3YY  ~  Desktop  Git  terraform   gcs-impersonation  2✎  USAGE  $   

@upodroid
Copy link
Contributor Author

I have a working go.mod that isn't breaking anything yet.

@alisdair alisdair requested review from alisdair and a team October 28, 2020 20:56
@upodroid
Copy link
Contributor Author

upodroid commented Nov 3, 2020

Hey, can I get this reviewed? I would like to see this merged before 0.14 release and cherry picked to 0.13.X

@pkolyvas
Copy link
Contributor

pkolyvas commented Nov 4, 2020

Hey @upodroid! Thanks for taking the time to submit this PR. I wanted to add that we, Terraform Core, aren't domain experts on the GCS backend and so we've invited some external expertise to help us complete the review.

I should note that his will not be backported to 0.13. Our main (master) branch is currently 0.15, but if this is reviewed, approved and merged before Terraform 0.14 RC1 ships (in over a week's time) we will target it for a backport to include in the 0.14.0 release.

Thanks for your contribution and your patience.

Copy link
Contributor

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Heya folks! @pkolyvas asked me to take a look at this.

This looks good from a GCP perspective- it's largely bringing the GCS backend up to par with some new authentication configuration @upodroid added in the google provider.

* `access_token` - (Optional) A temporary [OAuth 2.0 access token] obtained
from the Google Authorization server, i.e. the `Authorization: Bearer` token
used to authenticate HTTP requests to GCP APIs. This is an alternative to
`credentials`. If both are specified, `access_token` will be used over the
`credentials` field.
* `prefix` - (Optional) GCS prefix inside the bucket. Named states for
workspaces are stored in an object called `<prefix>/<name>.tfstate`.
* `path` - (Deprecated) GCS path to the state file of the default state. For
Copy link
Contributor

Choose a reason for hiding this comment

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

Deprecated values should probably still be documented until their removal, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was deprecated more than 3 years ago.

c00e929#diff-6665bbab4d1bce80874ff6467cd7bb85a08c9faaeaf8d2da121411b3a1ddbd55

My bad, the field is still lurking around in the codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pkolyvas Do you want me to target that field for removal in 0.15?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed a commit adding that line back in.

@pkolyvas pkolyvas added the 0.14-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Nov 5, 2020
Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

Thanks again for all this work, @upodroid! 🎉

@ghost
Copy link

ghost commented Dec 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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

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