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

Add master_global_access_config #601

Merged

Conversation

ilkelma
Copy link
Contributor

@ilkelma ilkelma commented Jul 13, 2020

Fixes #579 and implements https://www.terraform.io/docs/providers/google/r/container_cluster.html#master_global_access_config

I ran the integration tests and got errors because the examples pin the google provider versions to ~> 3.16.0 which does not include the new master_global_access_config block.

I have a stashed commit I can push that bumps all those example versions to the latest 3.29.0 but I didn't know whether that should be part of this commit or there are other things that need to fall in line to bump all those versions.

@ilkelma ilkelma requested review from bharathkkb, Jberlinsky and a team as code owners July 13, 2020 23:48
Copy link
Member

@bharathkkb bharathkkb 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 PR! I think it makes sense to follow this approach #585 (comment) to just use a boolean flag for this. Bumping dependencies as necessary also sgtm.

@ilkelma
Copy link
Contributor Author

ilkelma commented Jul 14, 2020

Thanks for the PR! I think it makes sense to follow this approach #585 (comment) to just use a boolean flag for this. Bumping dependencies as necessary also sgtm.

Done - I've bumped all versions in test files + examples - some may not be strictly necessary and I can pare back to only the ones that involve a beta private cluster (and thus would need this change) if needed.

@morgante
Copy link
Contributor

@ilkelma I don't think we need to upgrade the test version—the beta examples are already in 3.29. Do you mind reverting that?

@ilkelma
Copy link
Contributor Author

ilkelma commented Jul 14, 2020

looks like the integration tests succeed if I upgrade everything but fail otherwise so something needs upgrading it would seem but i'm not sure exactly where... is it possible y'all could let me know? Likely would be faster than me trying to run the tests on my end

@morgante
Copy link
Contributor

@ilkelma The error is on the simple-regional-with-networking example:

       Error: Invalid template interpolation value
       
         on ../../../cluster.tf line 261, in module "gcloud_wait_for_cluster":
        261:   create_cmd_body        = "${var.project_id} ${var.name}"
           |----------------
           | var.project_id is null
       
       The expression result is null. Cannot include a null value in a string
       template.

@ilkelma
Copy link
Contributor Author

ilkelma commented Jul 14, 2020

@ilkelma The error is on the simple-regional-with-networking example:

       Error: Invalid template interpolation value
       
         on ../../../cluster.tf line 261, in module "gcloud_wait_for_cluster":
        261:   create_cmd_body        = "${var.project_id} ${var.name}"
           |----------------
           | var.project_id is null
       
       The expression result is null. Cannot include a null value in a string
       template.

I'm not sure how this relates to my changes 🤔 i'll see if I can reproduce in local integration tests for some clues...

@morgante
Copy link
Contributor

morgante commented Jul 14, 2020

I'm not sure how this relates to my changes 🤔 i'll see if I can reproduce in local integration tests for some clues...

Yeah I'm not sure either but we can see that it's only happening on your branch. I'm especially confused why the provider version upgrade would affect it.

If you can try to troubleshoot locally, that would be helpful.

@bharathkkb
Copy link
Member

@ilkelma @morgante its on the prepare step actually. The projects are not getting created. Seems like API activations failure. It's not related to this PR as I am seeing it in other modules as well.

Error: Error enabling the Compute Engine API required to delete the default network: Error waiting for Enable Project "ci-gke-b8681adb-e462" Services: [compute.googleapis.com]: timeout while waiting for state to become 'done: true' (last state: 'done: false', timeout: 4m0s) 

  on .terraform/modules/gke-project-2/terraform-google-project-factory-8.0.1/modules/core_project_factory/main.tf line 96, in resource "google_project" "main":
  96: resource "google_project" "main" {



Error: Error enabling the Compute Engine API required to delete the default network: Error waiting for Enable Project "ci-gke-asm-b8681adb-03ca" Services: [compute.googleapis.com]: timeout while waiting for state to become 'done: true' (last state: 'done: false', timeout: 4m0s) 

  on .terraform/modules/gke-project-asm/terraform-google-project-factory-8.0.1/modules/core_project_factory/main.tf line 96, in resource "google_project" "main":
  96: resource "google_project" "main" {

@ilkelma
Copy link
Contributor Author

ilkelma commented Jul 15, 2020

@bharathkkb @morgante I bumped the version from 3.25 -> 3.29 in the prep step (the versions.tf file in test/setup) on a hunch but i can't see the results. It got about 20 more minutes in I think so possibly got past that point?

@bharathkkb
Copy link
Member

I think f6096e8 was flaky 1695094 has passed.

bharathkkb
bharathkkb previously approved these changes Jul 15, 2020
Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

Overall LGTM, a few suggestion 😄

modules/beta-private-cluster/variables.tf Show resolved Hide resolved
modules/beta-private-cluster/variables.tf Show resolved Hide resolved
@morgante morgante merged commit 8a9f904 into terraform-google-modules:master Jul 16, 2020
@bharathkkb bharathkkb mentioned this pull request Nov 9, 2020
CPL-markus pushed a commit to WALTER-GROUP/terraform-google-kubernetes-engine that referenced this pull request Jul 15, 2024
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.

How to enable master global access
3 participants