project: check for billing account permissions as pre-requisite #5671

umairidris · 2020-02-13T16:06:53Z

Currently, the provider will create the project but then fail the billing account link step leaving the resource in a 'tainted' state. This pre-emptively checks whether the caller will have access to the billing account before the project is created.

We now get an error like:

Error: failed pre-requisites: missing permissions on "billingAccounts/abc": [billing.resourceAssociations.create]

terraform-google-modules/terraform-google-project-factory#373

rileykarson

very drive-by and very not necessary: I would love to see this as a CustomizeDiff instead of an apply-time check. I think we could fail out at plan time for an even better user experience.

chrisst

Thank you for this! I have a couple quick things.

chrisst · 2020-02-13T17:47:20Z

google/resource_google_project.go

+	}
+	resp, err := config.clientBilling.BillingAccounts.TestIamPermissions(ba, req).Do()
+	if err != nil {
+		return fmt.Errorf("failed to check permissions on billing account %q: %v", ba, err)


I think it's super edge case but I can see a possibility that a user will fail this call but still be able to successfully create the project and update the billing account. For example a transient network error could cause this to fail but since we wrap the call to create in retries it will survive these such errors.

You could wrap this in retries, but IMO we can just log this at WARN and return nil as this is good enough for the 99% case.

So one of the problems the project factory preconditions script tries to solve is when a service account is running but the account's project hasn't enabled certain APIs. So for example if the project didn't enable cloudbilling api then this test iam permissions call would fail I imagine. Thus, I think we should leave this check in. I can make it a retry if you prefer.

WDYT?

@chrisst, if this check failed as just a warning in the logs, how would a typical user notice?

@danawillow It will just revert to existing behavior:
a) if the permission isn't valid, it would taint the resource
b) if the permission is valid, the resource would create correctly.

Since this is just a precondition check it doesn't seem legitimate that a precondition should fail the resource creation for a transient error.

Relying on the failure to fetch permissions as a signal that api's aren't correctly enabled seems like it could just be another explicit check. We have a similar tainting problem when trying to delete the default network but compute.googleapis.com isn't enabled.

@umairidris I'd be happy with either explicit checks for api enablement + silent failures or retries added. My primary concern is not adding a new category of failing to create a project

I misunderstood what @chrisst was saying at first, but I get it now. Basically, if the call to retrieve permissions fails, then we should ignore that because it was likely transient. but if the call succeeds and the permission isn't there, then fail before trying to create the project. Checking for API enablement is something we also can do first to make sure that the call to retrieve permissions fails for only transient or new reasons, not ones we already understand, and retries on the testIamPermissions call wouldn't fix that because they would just fail every time. I'm all caught up now :)

Do you have a way to check if an API is enabled on the caller? I haven't been able to find a good way apart from trying to just use the API client to do something...

That also makes checking for compute api tricky as you need a project to be able to call the API on, but you don't have the project in the pre-requisites check...

The preconditions script tries to work around this by calling list APIs on the project set in gcloud config but this isn't always a valid check. For example if I am running from my local workstation with my own credentials then it doesn't matter what project I set in gcloud, I believe it uses the default gcloud project for my API calls.

friendly ping @chrisst

Yeah I didn't think about the chicken/egg problem with enabling the compute API. I did some digging and I'm not sure if billing accounts are a parent type that can have services enabled/disabled on it. I think this change is definitely an improvement as is, so I'm happy accepting that there's a small chance somebody will fail due to a transitive error. If we see people filing issues about it we can reassess.

@chrisst I was thinking, if we create a compute API client and make a call on a non-existent project, would the error be the project doesn't exist or that the API is not enabled on the caller? I am guessing it would be the latter. If so, would this be an appropriate "hack" to work around this? i.e. call the API and check if the error is API not enabled OR check the error is 'resource not found' which implies the API must be enabled?

I can do some testing around this idea...

chrisst · 2020-02-13T17:51:48Z

google/resource_google_project.go

+	if err != nil {
+		return fmt.Errorf("failed to check permissions on billing account %q: %v", ba, err)
+	}
+	if diff := diffStringSlices(resp.Permissions, req.Permissions); len(diff) > 0 {


This is a bit overly complicated. Can you just loop through resp.Permissions and make sure that billing.resourceAssociations.create is present instead of doing this slice comparison?

chrisst · 2020-02-13T17:59:19Z

@rileykarson that's an interesting thought. It would have to be restricted to only call/fail if the resource was just about to be created (ie not on update). Also in theory a user could be generating a plan output with lower credentials to be delegated to an account with higher privileges and failing at plan time could break that work flow.
For now I think the approach in this PR is a good conservative option.

danawillow · 2020-02-13T23:28:48Z

google/resource_google_project.go

+	if err != nil {
+		return fmt.Errorf("failed to check permissions on billing account %q: %v", ba, err)
+	}
+	if has := sliceContains(perm, resp.Permissions); !has {


nit: we already have a function in this package that does this, so you can use stringInSlice(resp.Permissions, perm). You also don't use the value of has anywhere so you could just do:

if !stringInSlice(resp.Permissions, perm) { ... }

ghost · 2020-03-21T13:51:54Z

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!

umairidris added 2 commits February 13, 2020 16:02

project: check for billing account perms as pre-requisite

c203469

outdent

2800470

ghost added the size/s label Feb 13, 2020

ghost requested review from megan07 February 13, 2020 16:06

umairidris requested review from chrisst and danawillow February 13, 2020 16:07

umairidris added 2 commits February 13, 2020 16:10

typo

e7ea963

rm type from error message

78241e9

umairidris changed the title ~~project: check for billing account perms as pre-requisite~~ project: check for billing account permissions as pre-requisite Feb 13, 2020

rileykarson reviewed Feb 13, 2020

View reviewed changes

chrisst suggested changes Feb 13, 2020

View reviewed changes

chrisst self-assigned this Feb 13, 2020

simplify perms check

12f84fa

danawillow reviewed Feb 13, 2020

View reviewed changes

umairidris added 2 commits February 14, 2020 23:42

simplify bool check

dedc0ee

simplify bool check

e1c6070

ghost added size/xs and removed size/s labels Feb 14, 2020

umairidris requested review from rileykarson, chrisst and danawillow February 14, 2020 23:43

chrisst approved these changes Feb 18, 2020

View reviewed changes

chrisst mentioned this pull request Feb 18, 2020

Google project precheck GoogleCloudPlatform/magic-modules#3145

Merged

chrisst closed this in GoogleCloudPlatform/magic-modules#3145 Feb 19, 2020

This was referenced Feb 19, 2020

Google project precheck #5719

Merged

Google project precheck hashicorp/terraform-provider-google-beta#1777

Merged

ghost locked and limited conversation to collaborators Mar 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

project: check for billing account permissions as pre-requisite #5671

project: check for billing account permissions as pre-requisite #5671

umairidris commented Feb 13, 2020 •

edited

Loading

rileykarson left a comment

chrisst left a comment

chrisst Feb 13, 2020

umairidris Feb 13, 2020

danawillow Feb 13, 2020

chrisst Feb 13, 2020

chrisst Feb 13, 2020

danawillow Feb 14, 2020

umairidris Feb 14, 2020

umairidris Feb 18, 2020

chrisst Feb 18, 2020 •

edited

Loading

umairidris Feb 19, 2020 •

edited

Loading

chrisst Feb 13, 2020

umairidris Feb 13, 2020

chrisst commented Feb 13, 2020 •

edited

Loading

danawillow Feb 13, 2020

umairidris Feb 14, 2020

ghost commented Mar 21, 2020

project: check for billing account permissions as pre-requisite #5671

project: check for billing account permissions as pre-requisite #5671

Conversation

umairidris commented Feb 13, 2020 • edited Loading

rileykarson left a comment

Choose a reason for hiding this comment

chrisst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisst Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

umairidris Feb 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisst commented Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 21, 2020

umairidris commented Feb 13, 2020 •

edited

Loading

chrisst Feb 18, 2020 •

edited

Loading

umairidris Feb 19, 2020 •

edited

Loading

chrisst commented Feb 13, 2020 •

edited

Loading