Replaces gcloud commands by terraform resource #491

thiagonache · 2020-11-03T14:09:34Z

No description provided.

bharathkkb

Thanks for the PR and the work in the provider @thiagonache

I assume we can also get rid of the associated vars skip_gcloud_download use_tf_google_credentials_env_var etc and make this a breaking release. We would also need an upgrade guide to go along with it.

thiagonache · 2020-11-03T16:38:40Z

Thanks for the PR and the work in the provider @thiagonache

I assume we can also get rid of the associated vars skip_gcloud_download use_tf_google_credentials_env_var etc and make this a breaking release. We would also need an upgrade guide to go along with it.

Great! I'll work on it soon.

morgante · 2020-11-03T16:45:28Z

+1, this will need to be a breaking change w/ an upgrade guide which explains how to delete the gcloud local-exec resources entirely.

…_default_service_accounts

thiagonache · 2020-11-05T16:42:26Z

I think we'll need to have another migrate.py script because just terraform state rm won't work. I may be missing something, but the state will change relatively as far as I can see. Can someone just give a second thought to make sure I'm not missing anything, please? @morgante @bharathkkb

thiagonache · 2020-11-05T17:02:31Z

Please, hold on. I was missing one item (data)

bharathkkb · 2020-11-05T17:33:14Z

@thiagonache if you have an initial version of the upgrade guide, I am happy to test it.

thiagonache · 2020-11-05T17:45:15Z

@bharathkkb you read my mind... just pushed it. still need to polish it btw

thiagonache · 2020-11-05T22:08:27Z

did you get a chance to test it out? just wondering if you have any doubts. I've assumed that the new resource will be released on the next release (3.47).

morgante · 2020-11-05T23:00:18Z

@thiagonache I have some concerns about this deleting all SAs which have the role on existing projects. IMO we should actually filter which SAs are deleted (as we do in the script).

thiagonache · 2020-11-05T23:36:39Z

@thiagonache I have some concerns about this deleting all SAs which have the role on existing projects. IMO we should actually filter which SAs are deleted (as we do in the script).

@morgante I'm not sure if I'm following. Are you talking about the upgrade guide?

bharathkkb · 2020-11-06T02:24:35Z

@thiagonache I will test it out soon, but looking at the instructions do we have to remove it from state manually? I don't see any gcloud destroy_cmd so it will be no-op. Without the manual terraform state rm commands did it give an error when you tried to upgrade?

thiagonache · 2020-11-06T02:37:32Z

Yes. Terraform crashes on the transform step trying to access the state

…

On 5 Nov 2020, at 23:24, Bharath KKB ***@***.***> wrote: @thiagonache I will test it out soon, but looking at the instructions do we have to remove it from state manually? I don't see any gcloud destroy_cmd so it will be no-op. Without the manual terraform state rm commands did it give an error when you tried to upgrade? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

thiagonache · 2020-11-06T02:41:54Z

@thiagonache I have some concerns about this deleting all SAs which have the role on existing projects. IMO we should actually filter which SAs are deleted (as we do in the script).

@morgante I'm not sure if I'm following. Are you talking about the upgrade guide?

On the provider we are just touching the default SAs.

bharathkkb · 2020-11-06T02:55:41Z

@thiagonache I tested out a simple upgrade and I did not need to directly make any changes to the statefile.
My diff with initial skip_gcloud=true is below and it also worked false. What error did you observe?

  # module.project-factory.module.project-factory.google_project_default_service_accounts.default_service_accounts will be created
  + resource "google_project_default_service_accounts" "default_service_accounts" {
      + action           = "DISABLE"
      + id               = (known after apply)
      + project          = "project-id"
      + restore_policy   = "REVERT"
      + service_accounts = (known after apply)
    }

  # module.project-factory.module.project-factory.module.gcloud_disable.null_resource.run_command[0] will be destroyed
  - resource "null_resource" "run_command" {
      - id       = "5504134643976332785" -> null
      - triggers = {
          - "activated_apis"          = "compute.googleapis.com,container.googleapis.com"
          - "arguments"               = "5cba1c8ef7cb6cc82509abf4392b5896"
          - "create_cmd_body"         = <<~EOT
                --project_id='project-id' \
                --sa_id='num-[email protected]' \
                --credentials_path='' \
                --impersonate-service-account='' \
                --action='disable'
            EOT
          - "create_cmd_entrypoint"   = ".terraform/modules/project-factory/modules/core_project_factory/scripts/modify-service-account.sh"
          - "default_service_account" = "[email protected]"
          - "gcloud_bin_abs_path"     = "/google-cloud-sdk/bin"
          - "md5"                     = "8724d44955a417594c942e0101e4fe82"
          - "project_services"        = "project-id"
        } -> null
    }

  # module.project-factory.module.project-factory.module.gcloud_disable.null_resource.run_destroy_command[0] will be destroyed
  - resource "null_resource" "run_destroy_command" {
      - id       = "1499773560248397990" -> null
      - triggers = {
          - "activated_apis"          = "compute.googleapis.com,container.googleapis.com"
          - "default_service_account" = "[email protected]"
          - "destroy_cmd_body"        = "info"
          - "destroy_cmd_entrypoint"  = "gcloud"
          - "gcloud_bin_abs_path"     = "/google-cloud-sdk/bin"
          - "project_services"        = "project-id"
        } -> null
    }

thiagonache · 2020-11-06T03:36:14Z

It works, indeed.
After investigating I've found that the symlink I've created for the beta provider was pointing to the non beta provider 🤦
So I think we should add a note to upgrade the default_service_account to upper case if it is set somewhere.
Thank you for your support.

thiagonache · 2020-11-06T03:57:24Z

@thiagonache I have some concerns about this deleting all SAs which have the role on existing projects. IMO we should actually filter which SAs are deleted (as we do in the script).

@morgante I'm not sure if I'm following. Are you talking about the upgrade guide?

On the provider we are just touching the default SAs.

Maybe you're talking about passing the service account id (email). My understanding is since the documentation says SA is created only for two services (compute engine and app engine) and the e-mail may change, the best way to handle it is directly in the provider by service account name. There's a disclaimer in the resource documentation stating that. What I did not understand is why you said all SAs are being deleted...
Also, to include a new service account is just about three lines of code in the provider. I'm not anticipating that changing multiple times, are you?

thiagonache · 2020-11-06T16:31:13Z

@morgante Please, let me know if we are in agreement or if you want to change this.

thiagonache · 2020-11-06T22:05:24Z

@bharathkkb do you have any consideration/opinion about the discussion above?

…erraform-google-project-factory into features/gcloud_dependency

thiagonache · 2020-11-09T19:53:19Z

@morgante sir, I've fixed my issue but we still have two errors:

Checking for trailing whitespace
./helpers/setup-sa.sh:226:  
Error: Trailing whitespace found in the lines above.

It was added by you recently, wasn't?

and

terraform_validate ./test/fixtures/full 

Error: Unsupported argument

  on .terraform/modules/vpc/main.tf line 58, in resource "google_compute_subnetwork" "subnetwork":
  58:   enable_flow_logs         = lookup(var.subnets[count.index], "subnet_flow_logs", "false")

morgante · 2020-11-09T19:55:28Z

Yes, the whitespace issue was my mistake and I can fix.

The test fixture issue is likely due to your upgrading the provider version, so you'll possibly need to update the network module version to remove the enable_flow_logs variable.

bharathkkb · 2020-11-09T19:58:02Z

I believe network module was already fixed in the 2.0 release. @thiagonache can you try bumping it to the latest?

thiagonache · 2020-11-09T19:59:55Z

I believe network module was already fixed in the 2.0 release. @thiagonache can you try bumping it to the latest?

you the man!

thiagonache · 2020-11-09T20:10:56Z

@morgante @bharathkkb everything looks good on my side now. One thing I'm still not sure is if you guys agree with the approach of finding default service accounts.

morgante · 2020-11-09T20:11:58Z

README.md

@@ -122,7 +122,8 @@ determining that location is as follows:
 | budget\_amount | The amount to use for a budget alert | `number` | `null` | no |
 | budget\_monitoring\_notification\_channels | A list of monitoring notification channels in the form `[projects/{project_id}/notificationChannels/{channel_id}]`. A maximum of 5 channels are allowed. | `list(string)` | `[]` | no |
 | credentials\_path | Path to a service account credentials file with rights to run the Project Factory. If this file is absent Terraform will fall back to Application Default Credentials. | `string` | `""` | no |
-| default\_service\_account | Project default service account setting: can be one of `delete`, `deprivilege`, `disable`, or `keep`. | `string` | `"disable"` | no |
+| default\_sa\_restore\_policy | The action to be performed in the default service accounts on the resource destroy. Valid values are NONE and REVERT. If set to REVERT it will attempt to restore all default SAs but in the DEPRIVILEGE action. | `string` | `"REVERT"` | no |


I don't think we need to make this configurable.

it must be... if we DELETE the service account, the resource won't be able to revert after 30 days. So, the user must change to NONE.
I proposed to keep track of when it was deleted and just try to undelete if less than 30 days, but Riley preferred to make configurable and change the value if error happens.

It's fine for the behavior to be customizable on the provider, with the caveat that if set to "REVERT" I think the failure should not be an error (just a warning). /cc @rileykarson

I disagree, I think. Terraform failing to do what you told it to (eg undeleting the deleted SAs) should result in an error. We could consider an additional option like "REVERT_AND_IGNORE_FAILURE" though.

I think we can add another ENUM on_failure IGNORE or FAIL, but it would be an enhancement. I can work on it, but we should implement as-is for now.

I'm assuming you can work on a provider fix fairly quickly? We could also hold back this release until that's in.

which I think is feasible unless something really bad happens.

I'll be able to work on it next week and should take no more than one day. So, yeah, it's not a biggy to merge like that... we will have the new version before 30 days for sure

Sounds good!

hashicorp/terraform-provider-google#7768 I still need to work on tests but we can start to discuss it

thiagonache · 2020-11-09T20:26:54Z

@morgante I've requested a review again since we cannot apply the last change requested.

thiagonache · 2020-11-09T22:04:23Z

so, can we merge it?

morgante

Almost there.

docs/upgrading_to_project_factory_v10.0.md

examples/simple_project/main.tf

docs/upgrading_to_project_factory_v10.0.md

Replaces gcloud commands by terraform resource

b2ae6a6

thiagonache mentioned this pull request Nov 3, 2020

Remove gcloud dependency #408

Closed

2 tasks

bharathkkb reviewed Nov 3, 2020

View reviewed changes

thiagonache added 6 commits November 4, 2020 20:57

Updates module variables in README.md

4dfccc7

Updates variables across all modules

94aa559

Updates main.tf across all modules

8301f25

PIN new terraform provider version due to new resource google_project…

028fa06

…_default_service_accounts

Updates README

32734e2

All modules should require the same versions from now

97a2b8f

thiagonache added 2 commits November 5, 2020 14:44

Deletes unnecessary script

ff3633a

Adds how to upgrade to version 10.0

8b2db90

Fixes how to upgrade to version 10

96e4521

Merge branch 'master' into features/gcloud_dependency

7cf9231

thiagonache added 2 commits November 9, 2020 16:34

Changes require providers for fixtures full

12b2dc9

Merge branch 'features/gcloud_dependency' of github.com:thiagonache/t…

4f24fa2

…erraform-google-project-factory into features/gcloud_dependency

Upgrades google network module version to 2.0

8f22b82

Merge branch 'master' into features/gcloud_dependency

7968a6a

morgante suggested changes Nov 9, 2020

View reviewed changes

thiagonache requested a review from morgante November 9, 2020 20:22

Removes default_sa_restore_policy variable

79a86d4

morgante suggested changes Nov 10, 2020

View reviewed changes

docs/upgrading_to_project_factory_v10.0.md Outdated Show resolved Hide resolved

docs/upgrading_to_project_factory_v10.0.md Show resolved Hide resolved

examples/simple_project/main.tf Outdated Show resolved Hide resolved

thiagonache added 3 commits November 9, 2020 22:49

Updates documentation to upgrade to v10.0

0893a5f

Updates Migration Instructions to v10.0

9f5879d

Adds upper function in the default_service_account variable

dcd194c

thiagonache requested a review from morgante November 10, 2020 02:27

morgante previously approved these changes Nov 10, 2020

View reviewed changes

docs/upgrading_to_project_factory_v10.0.md Outdated Show resolved Hide resolved

Update docs/upgrading_to_project_factory_v10.0.md

4b2b153

morgante dismissed their stale review via 4b2b153 November 10, 2020 06:47

morgante merged commit 5886a4e into terraform-google-modules:master Nov 10, 2020

This was referenced Aug 18, 2022

chore(master): release 14.0.0 #741

Closed

chore(master): release 13.1.0 #713

Closed

release-please bot mentioned this pull request Nov 17, 2022

chore(master): release 15.0.0 #769

Closed

Replaces gcloud commands by terraform resource #491

Replaces gcloud commands by terraform resource #491

Conversation

thiagonache commented Nov 3, 2020

bharathkkb left a comment

Choose a reason for hiding this comment

thiagonache commented Nov 3, 2020

morgante commented Nov 3, 2020

thiagonache commented Nov 5, 2020 • edited Loading

thiagonache commented Nov 5, 2020

bharathkkb commented Nov 5, 2020

thiagonache commented Nov 5, 2020 • edited Loading

thiagonache commented Nov 5, 2020

morgante commented Nov 5, 2020

thiagonache commented Nov 5, 2020

bharathkkb commented Nov 6, 2020

thiagonache commented Nov 6, 2020 via email • edited Loading

thiagonache commented Nov 6, 2020

bharathkkb commented Nov 6, 2020

thiagonache commented Nov 6, 2020 • edited Loading

thiagonache commented Nov 6, 2020 • edited Loading

thiagonache commented Nov 6, 2020

thiagonache commented Nov 6, 2020

thiagonache commented Nov 9, 2020

morgante commented Nov 9, 2020

bharathkkb commented Nov 9, 2020

thiagonache commented Nov 9, 2020

thiagonache commented Nov 9, 2020

morgante Nov 9, 2020

Choose a reason for hiding this comment

thiagonache Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

morgante Nov 9, 2020

Choose a reason for hiding this comment

rileykarson Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

thiagonache Nov 9, 2020

Choose a reason for hiding this comment

morgante Nov 10, 2020

Choose a reason for hiding this comment

thiagonache Nov 10, 2020

Choose a reason for hiding this comment

thiagonache Nov 10, 2020

Choose a reason for hiding this comment

morgante Nov 10, 2020

Choose a reason for hiding this comment

thiagonache Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

thiagonache commented Nov 9, 2020

thiagonache commented Nov 9, 2020

morgante left a comment

Choose a reason for hiding this comment

thiagonache commented Nov 5, 2020 •

edited

Loading

thiagonache commented Nov 5, 2020 •

edited

Loading

thiagonache commented Nov 6, 2020 via email •

edited

Loading

thiagonache commented Nov 6, 2020 •

edited

Loading

thiagonache commented Nov 6, 2020 •

edited

Loading

thiagonache Nov 9, 2020 •

edited

Loading

rileykarson Nov 9, 2020 •

edited

Loading

thiagonache Nov 10, 2020 •

edited

Loading