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

fix: grant membership to bootstrap terraform SA to access required groups #1301

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

lpezet
Copy link
Contributor

@lpezet lpezet commented Jul 27, 2024

No description provided.

@lpezet lpezet changed the title fix: grant membership to bootstrap terraform SA account to access required groups fix: grant membership to bootstrap terraform SA to access required groups Jul 27, 2024
@daniel-cit
Copy link
Contributor

/gcbrun

@eeaton
Copy link
Collaborator

eeaton commented Jul 30, 2024

@daniel-cit looks like all the CI tests are failing again, due to a breaking issue in the upstream provider.

I found another repo that identified the perma-diff introduced by Cloud Functions API changing it's behavior to add LOG_EXECUTION_ID to the env variables if not explicitly specified. I expect the same fix should work for us.

Incidentally, this issue might be avoided by the PR #1304 that is hiding this module behind an enablement flag, so will avoid deploying the Function, but we should still find a way to address it for those who explicitly enable the module. I'll do a quick PR to add the explicit LOG_EXECUTION_ID and see if that helps.

@daniel-cit
Copy link
Contributor

@daniel-cit looks like all the CI tests are failing again, due to a breaking issue in the upstream provider.

I found another repo that identified the perma-diff introduced by Cloud Functions API changing it's behavior to add LOG_EXECUTION_ID to the env variables if not explicitly specified. I expect the same fix should work for us.

Incidentally, this issue might be avoided by the PR #1304 that is hiding this module behind an enablement flag, so will avoid deploying the Function, but we should still find a way to address it for those who explicitly enable the module. I'll do a quick PR to add the explicit LOG_EXECUTION_ID and see if that helps.

I think that the flag option is a good one as a temporary fix.

there are two sets of environment variables in the cloud function, build time and run time.

For the run time we already have code that sets the LOG_EXECUTION_ID here.
The fabric example you linked is adding the LOG_EXECUTION_ID to both the runtime and build env vars.

the strange thing is that the error is on the runtime part that has the configuration just presented, like if something was messing up with the env vars:

When expanding the plan for module.cai_monitoring.module.cloud_function.google_cloudfunctions2_function.function to include new values learned so far during apply, 
provider "registry.terraform.io/hashicorp/google" produced an invalid new value for .service_config[0].environment_variables:

was null, 

but now

cty.MapVal(map[string]cty.Value{
  "LOG_EXECUTION_ID":cty.StringVal("true"),
"ROLES":cty.StringVal("roles/owner,roles/editor,roles/resourcemanager.organizationAdmin,roles/compute.networkAdmin,roles/compute.orgFirewallPolicyAdmin"),
  "SOURCE_ID":cty.StringVal("organizations/REDACTED/sources/17068193223143848275")
}).

@eeaton
Copy link
Collaborator

eeaton commented Aug 6, 2024

The unrelated issues from Functions provider in the last few comments have been addressed by #1311 , I'll resume running tests on this PR.
/gcbrun

@eeaton
Copy link
Collaborator

eeaton commented Aug 6, 2024

CI tests are green, but I don't think our tests were accurately checking for the scenario of creating groups through terraform config because the automation never encountered the reported issues. I'll try to reproduce locally to verify the changes.

# works only with google-beta
provider = google-beta
depends_on = [module.seed_bootstrap, google_service_account.terraform-env-sa, module.required_group]
for_each = local.required_groups_to_create
Copy link
Collaborator

Choose a reason for hiding this comment

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

I ran through this in my local setup, seems to address the permission problem identified in 1206, because we explicitly add the bootstrap SA as group owner.

However, this code is just addressing the required groups block, can you extend the logic to also address the optional groups block?

@lpezet
Copy link
Contributor Author

lpezet commented Aug 10, 2024

Getting that error when running make docker_test_lint:

...
44      :       1723318373.673       3.278      0       55      0       0       terraform_validate ./4-projects/modules/infra_pipelines
45      :       1723318373.743       3.404      0       55      0       0       terraform_validate ./4-projects/modules/single_project
46      :       1723318374.022       3.067      0       55      0       0       terraform_validate ./5-app-infra/business_unit_1/development
47      :       1723318374.281       3.052      0       55      0       0       terraform_validate ./5-app-infra/business_unit_1/nonproduction
48      :       1723318374.422       3.049      0       55      0       0       terraform_validate ./5-app-infra/business_unit_1/production
49      :       1723318374.887       2.751      0       55      0       0       terraform_validate ./5-app-infra/modules/env_base
50      :       1723318375.013       2.905      0       55      0       0       terraform_validate ./test/setup
ENABLE_BPMETADATA not set to 1. Skipping metadata validation.
Error: The following tests have failed: check_headers
make: *** [Makefile:28: docker_test_lint] Error 1

Where can I find this check_headers test?

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