refactor: use gcloud module for scripts, closes #401 #404

marshallford · 2020-01-16T03:11:58Z

Uses gcloud module for wait-for-cluster and delete-default-resource scripts.

Fixes #401

In other words: Removes dependency on locally installed glcloud, kubectl, and jq binaries.

modules/beta-private-cluster-update-variant/dns.tf

morgante

Unfortunately it looks like we're still seeing errors with the dependency:

Step #23 - "converge simple-regional-with-networking-local":        Error: Invalid value for module argument
Step #23 - "converge simple-regional-with-networking-local":        
Step #23 - "converge simple-regional-with-networking-local":          on ../../../cluster.tf line 236, in module "gcloud_wait_for_cluster":
Step #23 - "converge simple-regional-with-networking-local":         236:   module_depends_on = [
Step #23 - "converge simple-regional-with-networking-local":         237:     google_container_cluster.primary,
Step #23 - "converge simple-regional-with-networking-local":         238:     google_container_node_pool.pools,
Step #23 - "converge simple-regional-with-networking-local":         239:   ]
Step #23 - "converge simple-regional-with-networking-local":        
Step #23 - "converge simple-regional-with-networking-local":        The given value is not suitable for child module variable "module_depends_on"
Step #23 - "converge simple-regional-with-networking-local":        defined at
Step #23 - "converge simple-regional-with-networking-local":        .terraform/modules/example.gke.gcloud_wait_for_cluster/terraform-google-modules-terraform-google-gcloud-55b927a/variables.tf:29,1-29:
Step #23 - "converge simple-regional-with-networking-local":        all list elements must have the same type.

marshallford · 2020-01-27T18:19:03Z

I'm new to the module_depends_on concept but both elements in that list are of type "resource". Is that not specific enough? Should I instead depend on two string values returned by the resources?

morgante · 2020-01-27T18:56:59Z

I'm new to the module_depends_on concept but both elements in that list are of type "resource". Is that not specific enough? Should I instead depend on two string values returned by the resources?

Yeah I think they need to be the exact same type. Let's try making them two string values.

marshallford · 2020-01-27T20:39:08Z

How does the usage of for_each in the resource google_container_node_pool.pools impact how I reference a returned value?

  module_depends_on = [
    google_container_cluster.primary.master_version,
    google_container_node_pool.pools.name, // not just one pool name?
  ]

morgante · 2020-01-27T20:48:08Z

How does the usage of for_each in the resource google_container_node_pool.pools impact how I reference a returned value?

You'll probably need to use splat syntax:

  module_depends_on = concat([google_container_cluster.primary.master_version], google_container_node_pool.pools.*.name)

morgante · 2020-01-27T22:41:25Z

I'm getting this error now:

Step #35 - "converge stub-domains-local":        Error: Unsupported attribute
Step #35 - "converge stub-domains-local":        
Step #35 - "converge stub-domains-local":          on ../../../cluster.tf line 238, in module "gcloud_wait_for_cluster":
Step #35 - "converge stub-domains-local":         238:     join("-", google_container_node_pool.pools[*].name),
Step #35 - "converge stub-domains-local":        
Step #35 - "converge stub-domains-local":        This object does not have an attribute named "name".

marshallford · 2020-01-28T22:46:30Z

google_container_node_pool does not export any values to depend on.

marshallford · 2020-02-06T16:04:42Z

I'd like to keep this PR moving. @morgante, what's the current error? I'd love to wrap this up.

morgante · 2020-02-06T16:07:03Z

Rerunning tests. @marshallford Could you resolve conflicts?

morgante · 2020-02-06T22:48:57Z

Here's the error I'm seeing in CI, which makes me thing the dependency order isn't working properly:

Step #43 - "converge stub-domains-upstream-nameservers-local":        Error: configmaps "kube-dns" already exists
Step #43 - "converge stub-domains-upstream-nameservers-local":        
Step #43 - "converge stub-domains-upstream-nameservers-local":          on ../../../dns.tf line 88, in resource "kubernetes_config_map" "kube-dns-upstream-nameservers-and-stub-domains":
Step #43 - "converge stub-domains-upstream-nameservers-local":          88: resource "kubernetes_config_map" "kube-dns-upstream-nameservers-and-stub-domains" {

marshallford · 2020-02-12T03:30:14Z

I recant my earlier statement, this recent PR clarifies that google_container_node_pool does export attributes: hashicorp/terraform-provider-google#5477

morgante · 2020-05-14T14:20:10Z

@marshallford FYI this is still failing. This is the error:

       Error: Invalid value for module argument
       
         on ../../../cluster.tf line 237, in module "gcloud_wait_for_cluster":
        237:   module_depends_on = [
        238:     google_container_cluster.primary.master_version,
        239:     google_container_node_pool.pools[*].name
        240:   ]
       
       The given value is not suitable for child module variable "module_depends_on"
       defined at
       .terraform/modules/example.gke.gcloud_wait_for_cluster/terraform-google-modules-terraform-google-gcloud-84e31b8/variables.tf:35,1-29:
       all list elements must have the same type.
       
       
       Error: Unsupported attribute
       
         on ../../../cluster.tf line 239, in module "gcloud_wait_for_cluster":
        239:     google_container_node_pool.pools[*].name

bharathkkb · 2020-06-12T02:11:01Z

@marshallford it would be great to get this in. Anything we can do to help complete this?

marshallford · 2020-06-12T03:39:28Z

@bharathkkb As you can see, I fought quite a bit with TF trying to establish a dependency between gcloud_wait_for_cluster and google_container_node_pool. If you have any suggestions on using the helper input module_depends_on please let me know.

bharathkkb · 2020-06-12T04:59:48Z

@marshallford I have a couple of ideas. Let me know if you have additional questions
i) I think the way to do the implicit dependency is

module_depends_on = concat(
    [google_container_cluster.primary.master_version],
    [for pool in google_container_node_pool.pools: pool.name]
  )

This similar to what @morgante suggested but I don't believe you can use splat with for_each resources as it will output a map. So I have used the usual for expansion here.

ii) A few other things are we should expose skip_download and possibly upgrade from the gcloud module so that users have that additional flexibility and imho skip_download should default to true. @morgante any thoughts on this?

bharathkkb · 2020-06-13T05:34:18Z

@marshallford it looks like a bunch of unrelated verify steps are failing. I see a couple of converge steps passed, so it looks promising. Could you try a rebase so we have the latest tests from master?

bharathkkb · 2020-06-13T05:36:17Z

@marshallford this is the test that failed and it was fixed here: #454

×  Command: `gcloud beta --project=ci-gke-10f1 container clusters --zone=us-central1 describe simple-regional-beta-cluster-1hne --format=json` cluster has the expected nodeMetadata conseal config
     
     expected: {"nodeMetadata"=>"EXPOSE"}
          got: {"mode"=>"GCE_METADATA", "nodeMetadata"=>"EXPOSE"}
     
     (compared using ==)
     
     Diff:
     @@ -1,2 +1,3 @@
     +"mode" => "GCE_METADATA",
      "nodeMetadata" => "EXPOSE",

     ✔  Command: `gcloud beta --project=ci-gke-10f1 container clusters --zone=us-central1 describe simple-regional-beta-cluster-1hne --format=json` cluster has the expected podSecurityPolicyConfig config

…lt-resource scripts Signed-off-by: Marshall Ford <[email protected]>

Signed-off-by: Marshall Ford <[email protected]>

marshallford · 2020-06-13T17:03:43Z

I hope I rebased correctly, dealing with the generated files wasn't fun. Had to step through a couple of manual fixes and git rebase --continue.

bharathkkb · 2020-06-13T17:36:32Z

@marshallford I am seeing some submodule generation errors, could you run make build and commit? That should take care of that error.

Checking submodule's files generation
diff -r '--exclude=.terraform' '--exclude=.kitchen' '--exclude=.git' /workspace/modules/beta-public-cluster-update-variant/cluster.tf /tmp/tmp.HIfNPcb4Hd/workspace/modules/beta-public-cluster-update-variant/cluster.tf
471,472c471,474
< resource "null_resource" "wait_for_cluster" {
<   count = var.skip_provisioners ? 0 : 1
---
> module "gcloud_wait_for_cluster" {
>   source  = "terraform-google-modules/gcloud/google"
>   version = "~> 1.0.1"
>   enabled = var.skip_provisioners
474,477c476,479
<   triggers = {
<     project_id = var.project_id
<     name       = var.name
<   }
---
>   create_cmd_entrypoint  = "${path.module}/scripts/wait-for-cluster.sh"
>   create_cmd_body        = "${var.project_id} ${var.name}"
>   destroy_cmd_entrypoint = "${path.module}/scripts/wait-for-cluster.sh"
>   destroy_cmd_body       = "${var.project_id} ${var.name}"
.
.

autogen/main/dns.tf.tmpl

bharathkkb · 2020-06-13T21:36:03Z

@marshallford looks like tests are green 😄

A few other things are we should expose skip_download and possibly upgrade from the gcloud module so that users have that additional flexibility and imho skip_download should default to true.

I would still like to provide this functionality if possible in this PR

marshallford · 2020-06-13T21:47:39Z

@bharathkkb Agreed. Given that I've been at odds with the module dependency issue I figured I'd focus on that first.

Can you expand on what you mean by

possibly upgrade from the gcloud module

bharathkkb · 2020-06-13T22:06:17Z

Can you expand on what you mean by

possibly upgrade from the gcloud module

@marshallford sure. From the gcloud module I think we should surface the following upgrade, gcloud_sdk_version, skip_download as optional variables for the GKE modules.

imho the recommended defaults should be upgrade:false, gcloud_sdk_version:296.0.0 and skip_download:true. I believe these defaults will keep parity with the way we currently do it with local-exec. @morgante any thoughts on these defaults?

…load

morgante · 2020-06-14T01:07:36Z

imho the recommended defaults should be upgrade:false, gcloud_sdk_version:296.0.0 and skip_download:true. I believe these defaults will keep parity with the way we currently do it with local-exec. @morgante any thoughts on these defaults?

I don't think we need to expose sdk_version, but otherwise agreed.

bharathkkb · 2020-06-14T03:19:52Z

@marshallford the lint tests are failing due to a timeout issue, should be safe to ignore.

bharathkkb

LGTM

stale

bharathkkb · 2020-06-18T01:31:49Z

@morgante do we need anything else before we can merge this?

… (terraform-google-modules#404)

marshallford requested review from adrienthebo, Jberlinsky and morgante as code owners January 16, 2020 03:11

morgante suggested changes Jan 21, 2020

View reviewed changes

modules/beta-private-cluster-update-variant/dns.tf Outdated Show resolved Hide resolved

morgante previously requested changes Jan 27, 2020

View reviewed changes

marshallford changed the title ~~closes #401: uses gcloud module for scripts~~ refactor: use gcloud module for scripts, closes #401 Jan 28, 2020

bharathkkb mentioned this pull request Jun 12, 2020

Enhance WI module usability with existing KSA #557

Merged

bharathkkb mentioned this pull request Jun 12, 2020

code run on TFE (Terraform Enterprise) shows error "Permission denied" on .sh scripts #556

Closed

marshallford requested review from bharathkkb and a team as code owners June 13, 2020 03:05

marshallford added 4 commits June 13, 2020 11:55

closes #401: uses gcloud module for wait-for-cluster and delete-defau…

07bc06a

…lt-resource scripts Signed-off-by: Marshall Ford <[email protected]>

swaps jq call in delete-default-resource script for kubectl jsonpath

e221106

Signed-off-by: Marshall Ford <[email protected]>

fixes tf dependency graph for dns: depend on gcloud module outputs.wait

6de9948

Signed-off-by: Marshall Ford <[email protected]>

fixes dependency graph for both dns and cluster

96f8add

marshallford added 5 commits June 13, 2020 12:00

wait_for_cluster: depend on cluster master version and pool names

83470c1

google_container_node_pool does not export any values to depend on

c50b105

wait_for_cluster: depend on node pools

218432c

gcloud module v0.5.0

254c76a

gcloud module version bump, try a new pattern for implicit dependency

01994a3

bharathkkb reviewed Jun 13, 2020

View reviewed changes

autogen/main/dns.tf.tmpl Show resolved Hide resolved

gcloud_delete_default_kube_dns_configmap module_depends_on

844766c

bharathkkb requested a review from morgante June 13, 2020 21:36

surface gcloud module options: upgrade, gcloud_sdk_version, skip_down…

35c6183

…load

removes gcloud_sdk_version var

5d34f39

bharathkkb approved these changes Jun 15, 2020

View reviewed changes

morgante merged commit 65172de into terraform-google-modules:master Jun 18, 2020

marshallford deleted the gcloud-module branch June 18, 2020 03:43

jakubpawlowski mentioned this pull request Aug 13, 2020

not a valid Win32 application error on a Windows machine #630

Closed

bharathkkb mentioned this pull request Sep 14, 2020

fix: Fix skip_provisioners enabled flag for wait_for_cluster #669

Merged

CPL-markus pushed a commit to WALTER-GROUP/terraform-google-kubernetes-engine that referenced this pull request Jul 15, 2024

fix: Use gcloud module for scripts, closes terraform-google-modules#401…

54ee9a1

… (terraform-google-modules#404)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use gcloud module for scripts, closes #401 #404

refactor: use gcloud module for scripts, closes #401 #404

marshallford commented Jan 16, 2020 •

edited by morgante

Loading

morgante left a comment •

edited

Loading

marshallford commented Jan 27, 2020 •

edited

Loading

morgante commented Jan 27, 2020

marshallford commented Jan 27, 2020

morgante commented Jan 27, 2020

morgante commented Jan 27, 2020

marshallford commented Jan 28, 2020

marshallford commented Feb 6, 2020

morgante commented Feb 6, 2020 •

edited

Loading

morgante commented Feb 6, 2020

marshallford commented Feb 12, 2020

morgante commented May 14, 2020

bharathkkb commented Jun 12, 2020

marshallford commented Jun 12, 2020 •

edited

Loading

bharathkkb commented Jun 12, 2020 •

edited

Loading

bharathkkb commented Jun 13, 2020

bharathkkb commented Jun 13, 2020

marshallford commented Jun 13, 2020

bharathkkb commented Jun 13, 2020

bharathkkb commented Jun 13, 2020

marshallford commented Jun 13, 2020

bharathkkb commented Jun 13, 2020

morgante commented Jun 14, 2020

bharathkkb commented Jun 14, 2020

bharathkkb left a comment

bharathkkb commented Jun 18, 2020

refactor: use gcloud module for scripts, closes #401 #404

refactor: use gcloud module for scripts, closes #401 #404

Conversation

marshallford commented Jan 16, 2020 • edited by morgante Loading

morgante left a comment • edited Loading

Choose a reason for hiding this comment

marshallford commented Jan 27, 2020 • edited Loading

morgante commented Jan 27, 2020

marshallford commented Jan 27, 2020

morgante commented Jan 27, 2020

morgante commented Jan 27, 2020

marshallford commented Jan 28, 2020

marshallford commented Feb 6, 2020

morgante commented Feb 6, 2020 • edited Loading

morgante commented Feb 6, 2020

marshallford commented Feb 12, 2020

morgante commented May 14, 2020

bharathkkb commented Jun 12, 2020

marshallford commented Jun 12, 2020 • edited Loading

bharathkkb commented Jun 12, 2020 • edited Loading

bharathkkb commented Jun 13, 2020

bharathkkb commented Jun 13, 2020

marshallford commented Jun 13, 2020

bharathkkb commented Jun 13, 2020

bharathkkb commented Jun 13, 2020

marshallford commented Jun 13, 2020

bharathkkb commented Jun 13, 2020

morgante commented Jun 14, 2020

bharathkkb commented Jun 14, 2020

bharathkkb left a comment

Choose a reason for hiding this comment

bharathkkb commented Jun 18, 2020

marshallford commented Jan 16, 2020 •

edited by morgante

Loading

morgante left a comment •

edited

Loading

marshallford commented Jan 27, 2020 •

edited

Loading

morgante commented Feb 6, 2020 •

edited

Loading

marshallford commented Jun 12, 2020 •

edited

Loading

bharathkkb commented Jun 12, 2020 •

edited

Loading