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

fix bug with data access iam member #3741

Merged

Conversation

megan07
Copy link
Contributor

@megan07 megan07 commented Jul 14, 2020

Fixes hashicorp/terraform-provider-google#6754

The issue is the iam_member field will translate into user_by_email, group_by_email, domain or special_group. The difference with these fields is that iam_member accepts a string formatted as prefix:value, whereas the other fields don't accept a string with the prefix. However, the backend will use the prefix of iam_member to translate the value into one of those fields and send that back in the response.

I change the request based on what I believe the response will be anyway, it's an assumption that the backend logic is the same. I would prefer to analyze the response instead, but couldn't figure out a good way to do that. Please let me know if there's a better way to address this issue.

Release Note Template for Downstream PRs (will be copied)

bigquery: fixed bug where `dataset_access.iam_member` would produce inconsistent results after apply.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 83 insertions(+), 18 deletions(-))
Terraform Beta: Diff ( 2 files changed, 83 insertions(+), 18 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=133077"

@megan07 megan07 requested a review from danawillow July 14, 2020 17:57
Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

Can you try adding a test (or testing manually) with iam_member = "allUsers"? I'm curious whether that gets returned as a special group or if it gets returned with iamMember.

@megan07
Copy link
Contributor Author

megan07 commented Jul 14, 2020

@danawillow - it is returned as an iamMember, so I'll go back to the drawing board. do you know of any other values i should be aware of? is there a list somewhere i can see?

@danawillow
Copy link
Contributor

I'm not sure! @rambleraptor, how did you know that allUsers would be in iamMember when you added it?

@rambleraptor
Copy link
Contributor

rambleraptor commented Jul 14, 2020

This was a while back, but I think I looked at https://cloud.google.com/iam/docs/overview#allusers

I usually veer towards very wide-ranging values so that examples/tests will run on as many accounts as possible without errors.

edit: feels like this might've side-stepped your question. did it answer it?

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 119 insertions(+))
Terraform Beta: Diff ( 2 files changed, 119 insertions(+))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=133112"

@megan07
Copy link
Contributor Author

megan07 commented Jul 14, 2020

@danawillow - looking over the doc from @rambleraptor , and after running a few tests, i have a hunch that allUsers is just special in and of itself? i hate to hard-code it that way (adding it to the map), but i just tried to generally write "if it doesn't have a prefix, use iam_member" and we're back in square one, because the API accepts allAuthenticatedUsers, but that is returned as a specialGroup still.

@danawillow
Copy link
Contributor

There might be a way to do some post read logic that can move things out of categories and into iamMember if the user's current state representation has it that way. I'm also going to see if I can reach any internal teams that know more about this to see if they can clarify what options iamMember has.

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccBigQueryDatasetAccess_allUsers|TestAccServiceAccountIamMember_withAndWithoutCondition|TestAccServiceAccountIamBinding_withAndWithoutCondition You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=133113"

@megan07 megan07 force-pushed the megan_dataset_access_iam_member branch from 63b563b to cd2fc5d Compare August 14, 2020 16:13
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 253 insertions(+), 17 deletions(-))
Terraform Beta: Diff ( 2 files changed, 253 insertions(+), 17 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=139923"

@megan07 megan07 requested a review from danawillow August 14, 2020 16:23
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 253 insertions(+), 17 deletions(-))
Terraform Beta: Diff ( 2 files changed, 253 insertions(+), 17 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=139924"

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 254 insertions(+), 17 deletions(-))
Terraform Beta: Diff ( 2 files changed, 254 insertions(+), 17 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=139925"

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccBigQueryDatasetAccess_iamMember|TestAccBigQueryDatasetAccess_allAuthenticatedUsers|TestAccBigQueryDatasetAccess_allUsers|TestAccContainerCluster_withInvalidReleaseChannel You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=139929"

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccBigQueryDatasetAccess_iamMember|TestAccBigQueryDatasetAccess_allAuthenticatedUsers|TestAccBigQueryDatasetAccess_allUsers|TestAccContainerCluster_withInvalidReleaseChannel You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=139931"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants