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

the project and regions are not parsed properly, both entries end up … #6

Merged
merged 4 commits into from
Oct 31, 2018

Conversation

kenthua
Copy link
Contributor

@kenthua kenthua commented Oct 25, 2018

ex:

   + module.subnet_iam_binding.google_compute_subnetwork_iam_member.subnet_iam_additive
      id:         <computed>
      etag:       <computed>
      member:     "serviceAccount:service-158105332103@container-engine-robot.iam.gserviceaccount.com"
      project:    "gke-subnet"
      region:     "gke-subnet"
      role:       "roles/compute.networkUser"
      subnetwork: "gke-subnet"

Running the example listed, requires the provider to specify project and region

Does that mean the main.tf in the module should handle region?

@morgante morgante self-requested a review October 25, 2018 23:41
@morgante
Copy link
Contributor

Can you clarify which example you saw this issue with?

@kenthua
Copy link
Contributor Author

kenthua commented Oct 25, 2018

The subnet iam example

@adrienthebo
Copy link

@kenthua could you provide the variables that you're passing to the subnets example? I'm setting up a reproduction case locally, but in the mean time I'd like to see the subnet resources that you're providing.

@kenthua
Copy link
Contributor Author

kenthua commented Oct 26, 2018

A simple reference based off the example. If I did specify project, it would also try to create a project iam binding.

This is in addition to a provider reference with project and region.

module "subnet_iam_binding" {
  source = "../../"

  subnets = ["gke-subnet"]

  mode = "additive"

  bindings = {
    "roles/compute.networkUser" = [
      "serviceAccount:service-158105332103@container-engine-robot.iam.gserviceaccount.com"
    ]
  }
}

@adrienthebo
Copy link

adrienthebo commented Oct 26, 2018

@kenthua I was able to reproduce your scenario with the following:

# terraform.tfvars
credentials_file_path="credentials.json"
group_email="test_sa_group@example.com"
user_email="thebo@example.com"
sa_email="bare-service-account@thebo-pfhost1-ed64.iam.gserviceaccount.com"
#subnet_one="projects/thebo-host-e503/regions/us-west1/subnetworks/default-us-west1-01"
#subnet_two="projects/thebo-host-e503/regions/us-west1/subnetworks/us-west1-01"
subnet_one="default-us-west1-01"
subnet_two="us-west1-01"

When specifying subnets, are you providing the full subnet path (eg the commented out subnet_one and subnet_two values) or just the subnet name?

edit - whoops, you provided an example - sorry about that! Instead of specifying subnets = ["gke-subnet"] could you try using the fully qualified subnet name?

@kenthua
Copy link
Contributor Author

kenthua commented Oct 26, 2018

@adrienthebo Apologies, that was it, thanks! Should have read the docs that said full path, but maybe a more clear example in the readme?

subnet = "projects/<project_id>/regions/<region>/subnetworks/<subnet_name>"

@morgante
Copy link
Contributor

Agreed this could be made clearer.

Specifically, can we:

  1. Add a clarification to README
  2. For the subnet example, instead of taking in a big "subnet" variable let's pull in a project ID, region, and subnet name as separate variables. and construct the URL within the example.

…quirement and construct it manually in the example
@kenthua
Copy link
Contributor Author

kenthua commented Oct 26, 2018

Fixed them up, @adrienthebo @morgante let me know your thoughts.

Copy link

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

There are a couple of whitespace fixups I'd like to see before we merge this. I've verified these changes and everything runs as expected; 👍 for merge after the fixups lands.

examples/subnet/main.tf Outdated Show resolved Hide resolved
examples/subnet/main.tf Outdated Show resolved Hide resolved
Copy link

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

Thanks for the fixups, LGTM!

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@morgante morgante merged commit 19fbf6a into terraform-google-modules:master Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants