Fix shared VPC IAM bindings #164
Conversation
glarizza
requested review from
aaron-lane,
adrienthebo and
morgante
as code owners
|
Closes #98. |
There was a problem hiding this comment.
We need at least 3 suites to ensure that all of the cases in #97 are tested. This should include additional examples and test fixtures.
The branch also needs to be rebased against master.
Agreed, but it would be good if we can merge this sooner rather than later. (ie. I don't think new tests should be blocking, readme documentation should be though.) |
glarizza
force-pushed
the
gl/fixed_shared_vpc
branch
from
1213fd3 to
d01910a
Compare
There was a problem hiding this comment.
We are getting CI failures, and I think it's because we have overlapping checks between the test suites. My current thinking is that the full project assigns access to subnets and doesn't add roles/compute.networkUser to the project, but the shared_vpc_no_subnets test does add this binding. When we converge all of the tests at once, the configuration from one test fixture causes failures in another test suite.
Since there's some urgency for getting this turned around I think we should either disable the tests in this PR by commenting out the .kitchen.yml entry and fixing that in a followup PR, or extracting the shared_vpc_no_subnets tests into a separate PR.
glarizza
force-pushed
the
gl/fixed_shared_vpc
branch
from
d01910a to
5d25fe4
Compare
|
Okay, I've reverted the commit back to the fix + updated existing test, and the README updates that came out of running |
glarizza
force-pushed
the
gl/fixed_shared_vpc
branch
from
5d25fe4 to
aea11d3
Compare
This commit addresses issue terraform-google-modules#97 (terraform-google-modules#97) and updates the logic around IAM bindings with regard to shared VPC subnets. The logic is as follows: 1. If `var.shared_vpc` and `var.shared_vpc_subnets` are empty no bindings are mad 2. If `var.shared_vpc` is set but no subnets are provided with `var.shared_vpc_subnets` then the IAM bindings are set at the Host Project 3. If `var.shared_vpc` is set and `var.shared_vpc_subnets` contains subnets then the IAM bindings are granted on the subnetworks themselve This commit updates the logic used to calculate the Host Project bindings based on scenario 3 above. The tests have also been modified to ensure that those bindings AREN'T set.
This commit contains all the changes borne out of running `make` with an empty target (i.e. terraform-docs updates)
glarizza
force-pushed
the
gl/fixed_shared_vpc
branch
from
aea11d3 to
b69927e
Compare
|
@aaron-lane That makes sense why it was recommending those changes - updated |
This commit addresses issue #97
(#97)
and updates the logic around IAM bindings with regard to shared VPC
subnets. The logic is as follows:
var.shared_vpcandvar.shared_vpc_subnetsare empty nobindings are mad
var.shared_vpcis set but no subnets are provided withvar.shared_vpc_subnetsthen the IAM bindings are set at the HostProject
var.shared_vpcis set andvar.shared_vpc_subnetscontainssubnets then the IAM bindings are granted on the subnetworks
themselve
This commit updates the logic used to calculate the Host Project
bindings based on scenario 3 above. The tests have also been modified to
ensure that those bindings AREN'T set.