chore: disabling SCC notifications for now #1304

lpezet · 2024-07-28T04:40:54Z

No description provided.

eeaton · 2024-07-29T09:43:10Z

@lpezet the results of the Lint test show specific errors in your code changes:
https://github.com/terraform-google-modules/terraform-example-foundation/actions/runs/10129013421/job/28008555085?pr=1304

│ Error: Unsupported attribute
│ 
│   on iam.tf line 173, in resource "google_project_iam_member" "project_scc_admin":
│  173:   project = module.scc_notifications.project_id
│     ├────────────────
│     │ module.scc_notifications is a list of object
│ 
│ Can't access attributes on a list of objects. Did you mean to access
│ attribute "project_id" for a specific element of the list, or across all
│ elements of the list?
╵
╷
│ Error: Unsupported attribute
│ 
│   on sa.tf line 18, in resource "google_service_account" "cai_monitoring_builder":
│   18:   project                      = module.scc_notifications.project_id
│     ├────────────────
│     │ module.scc_notifications is a list of object
│ 
│ Can't access attributes on a list of objects. Did you mean to access
│ attribute "project_id" for a specific element of the list, or across all
│ elements of the list?

Once you get the Lint tests passing I'll review the specific changes

eeaton · 2024-07-29T15:06:02Z

1-org/envs/shared/iam.tf

  role    = "roles/securitycenter.adminEditor"
  member  = "group:${var.gcp_groups.scc_admin}"
 }

 resource "google_project_iam_member" "global_secrets_admin" {
-  count   = var.gcp_groups.global_secrets_admin != null ? 1 : 0
+  count   = var.gcp_groups.global_secrets_admin && var.enable_scc_notifications != null ? 1 : 0


This resource is unrelated SCC notificaitons, please revert this change.

eeaton · 2024-07-29T15:12:21Z

1-org/envs/shared/iam.tf

@@ -162,21 +162,21 @@ resource "google_project_iam_member" "audit_bq_data_viewer" {
 }

 resource "google_organization_iam_member" "org_scc_admin" {
-  count  = var.gcp_groups.scc_admin != null && local.parent_folder == "" ? 1 : 0
+  count  = var.gcp_groups.scc_admin != null && local.parent_folder == "" && var.enable_scc_notifications ? 1 : 0


This role is still required even if we remove the SCC notification config (somebody still has to use and configure SCC, not all of which can be automated through Terraform). Please revert this change.

eeaton · 2024-07-29T15:48:22Z

1-org/envs/shared/variables.tf

@@ -20,6 +20,12 @@ variable "enable_hub_and_spoke" {
  default     = false
 }

+variable "enable_scc_notifications" {
+  description = "Enable Security Command Center Notifications."


This is one variable that handles two distinct sets of functionality and use cases. (SCC notification is to export findings, the cai_monitoring module is to generate custom findings from an asset feed that are then fed into SCC and subsequently through the notification config). Both use cases are affected by the same bug, but logically I would want to control enablement of them separately, and the description/name are misleading.

To make it clear that this is a tech workaround and not about enabling features, how about rewriting it like the following:

variable "enable_scc_resources_in_terraform" { description = "Create Security Command Center resources in Terraform. If your organization has newly enabled any preview features for SCC and get an error related to the v2 API, you must set this variable to false because the v2 API does not yet support Terraform resources. See [issue 1189](https://github.com/terraform-google-modules/terraform-example-foundation/issues/1189) for context."

eeaton · 2024-07-29T15:49:19Z

1-org/envs/shared/iam.tf

-    "roles/storage.objectViewer",
-    "roles/artifactregistry.writer",
-  ])
+  project = module.scc_notifications[0].project_id


Is there a reason to set the boolean logic inside the for_each block, instead of using a count argument like other resources?

You might have overlooked this comment, bump :)

eeaton · 2024-07-29T15:56:37Z

1-org/envs/shared/projects.tf

-
+  source                   = "terraform-google-modules/project-factory/google"
+  version                  = "~> 15.0"
+  count                    = var.enable_scc_notifications ? 1 : 0


For consistent conventions, I suggest to leave this module (which just creates a placeholder project) untouched. Here's my reasoning:

Even if the blueprint user does not configure notification configs as part of the automated blueprint, there are still various use cases in the written guide that assume this placeholder project exists. Removing it completely goes against the written recommendations.

This is also cleaner because it reduces a number of places you've modified "module.scc_notifications[0].project_id" to accommodate the changed array structure when using count arguments.

The convention used in the centralized logging module is to use the project created in this file even though several modules there are turned on or off based on variables.

eeaton · 2024-07-29T16:06:47Z

/gcbrun

lpezet · 2024-07-30T03:37:18Z

@eeaton I believe I addressed everything you reported. On my own setup, I'm still getting that converting TF resource to CAI/exit code 33 error. Maybe I need to teardown 1-org and re-apply everything.

eeaton · 2024-07-30T10:58:05Z

/gcbrun

eeaton · 2024-07-30T11:03:13Z

1-org/envs/shared/iam.tf

-    "roles/storage.objectViewer",
-    "roles/artifactregistry.writer",
-  ])
+  project = module.scc_notifications[0].project_id


You might have overlooked this comment, bump :)

eeaton · 2024-07-30T11:05:18Z

1-org/envs/shared/iam.tf

  org_id = local.org_id
  role   = "roles/securitycenter.adminEditor"
  member = "group:${var.gcp_groups.scc_admin}"
 }

 resource "google_project_iam_member" "project_scc_admin" {
-  count   = var.gcp_groups.scc_admin != null && var.enable_scc_notifications ? 1 : 0
+  count   = var.gcp_groups.scc_admin != null && var.enable_scc_resources_in_terraform ? 1 : 0
  project = module.scc_notifications[0].project_id


There are still 3 references to module.scc_notifications[0].project_id that will throw an error now that we removed the count argument from the module scc_notifications in projects.tf. Revert these 3 references back to module.scc_notifications.project_id.

Somehow this passed Lint tests (can't explain why, though? This definitely throws an invalid index error when running Terraform plan in my local environment). I've kicked off the full CI build, takes some hours but I expect it will fail with the same invalid index error.

daniel-cit · 2024-07-31T14:07:31Z

/gcbrun

eeaton · 2024-07-31T15:40:28Z

Code changes look good, but the unit tests in CI are failing with the error below.

TestOrg 2024-07-31T15:03:30Z command.go:185: ERROR: (gcloud.pubsub.topics.describe) NOT_FOUND: Resource not found (resource=top-scc-notification). This command is authenticated as [email protected] using the credentials in /tmp/3595579577, specified by the [auth/credential_file_override] property.
Step #8 - "verify-org":     gcloud.go:84: error while running command: exit status 1; ERROR: (gcloud.pubsub.topics.describe) NOT_FOUND: Resource not found (resource=top-scc-notification).

We have an internal environment to automatically build and test all the resources before. The logs and errors from this are internal-only as a control against leaked secrets, so unfortunately there's a bit of back-and-forth required to convey the error and fix it.

To recreate these tests in your local environment, see contributing. The relevant test for the scope of changes in this PR is just test/integration/org/org_test.go.

In this case, I think the simplest fix is to comment out the lines that assert the resources related to SCC and CAI-monitoring have been created. Since we're now treating them as optional, it makes sense to disable the tests that require these resources. Add a commit that comments out these lines from org_test.go:

everything related to the SCC notification config and associated pubsub topics. L186-200
everything related to the CAI monitoring. L298-322

I haven't yet run this myself, I'm just comparing the test file against the resources I know we've changed, give that a try. If you're having a hard time with the tests locally I'll try to recreate it in my environment to confirm what's missing in the tests.

lpezet · 2024-07-31T23:13:01Z

@eeaton I'm trying to go through the tests myself, but the make docker_test_prepare fails at some point with:

│ Error: Request `Enable Project Service "sourcerepo.googleapis.com" for project "ci-foundation-77uj5a-q58j"` returned error: Batch request and retried single request "Enable Project Service \"sourcerepo.googleapis.com\" for project \"ci-foundation-77uj5a-q58j\"" both failed. Final error: failed to enable services: failed on request preconditions: googleapi: Error 403: Permission denied to enable service [sourcerepo.googleapis.com]
│ Help Token: ARZIt87RX7leTepHHK1jRqfguQNIdBkkfU0LPxizvbwU-FQaOJgrL4izifLpQnwnjrHVPMIIMFyp9P7haV0pXuO1pxhXNFbfJfwpvQAkHM8jW2KB
│ Details:
│ [
│   {
│     "@type": "type.googleapis.com/google.rpc.PreconditionFailure",
│     "violations": [
│       {
│         "subject": "?error_code=110002\u0026service=servicemanagement.googleapis.com\u0026permission=servicemanagement.services.bind\u0026resource=ci-foundation-77uj5a-q58j",
│         "type": "googleapis.com"
│       }
│     ]
│   },
│   {
│     "@type": "type.googleapis.com/google.rpc.ErrorInfo",
│     "domain": "serviceusage.googleapis.com",
│     "metadata": {
│       "permission": "servicemanagement.services.bind",
│       "resource": "ci-foundation-77uj5a-q58j",
│       "service": "servicemanagement.googleapis.com"
│     },
│     "reason": "AUTH_PERMISSION_DENIED"
│   }
│ ]
│ , forbidden
│ 
│   with module.project.module.project-factory.module.project_services.google_project_service.project_services["sourcerepo.googleapis.com"],
│   on .terraform/modules/project/modules/project_services/main.tf line 31, in resource "google_project_service" "project_services":
│   31: resource "google_project_service" "project_services" {
│ 
╵
make: *** [Makefile:66: docker_test_prepare] Error 1

My SA has the roles mentioned in the CONTRIBUTING file. I tried adding more roles (Service Usage Admin and Service Management Admin) but no luck.
I'll keep trying but what am I missing?

eeaton · 2024-08-01T10:58:33Z

The issue you're encountering is about the CSR deprecation. It looks like the docker tests also have a dependency on CSR that needs to be unpicked. Docker tests don't meaningfully use CSR, but there are places where the CSR API is enabled, which will now fail.

CSR is still available to orgs that used it prior to deprecation, but not to new orgs who hadn't used it. So unfortunately that means your environment and mine have different behavior so it hasn't been caught by our automated tests. I expect you can get around this by removing / commenting out any references to "sourcerepo.googleapis.com". The following files are likely culprits (there are a few more in later stages, but those aren't blockers for tests in the Bootstrap and Org stages.

0-bootstrap/cb.tf
test/integration/bootstrap/bootstrap_test.go
terraform-example-foundation/test/setup/main.tf

I recreated the tests with changes from your branch in my local environment, confirmed that the suggested lines to comment out in org_test.go do indeed allow the CI tests to pass. So if you're getting stuck recreating the CI in your local environment, just add a commit with those changes to org_test.go and the remote automated CI should be able to run and validate the changes.

eeaton · 2024-08-01T13:28:08Z

/gcbrun

eeaton

All tests have passed and code changes are LGTM

Disabling Security Command Center notifications.

2ffb8cd

lpezet requested review from rjerrems, gtsorbo, eeaton, sleighton2022 and a team as code owners July 28, 2024 04:40

lpezet mentioned this pull request Jul 28, 2024

1-org: Apply fails for scc resources with API version error. #1189

Closed

Linting fixes.

93c3197

eeaton requested changes Jul 29, 2024

View reviewed changes

Changes as per @eeaton feedback.

c417022

eeaton requested changes Jul 30, 2024

View reviewed changes

eeaton mentioned this pull request Jul 30, 2024

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

Closed

lpezet added 2 commits July 30, 2024 19:52

Removing index from module.scc_notifications references.

4fc04cc

Fixed optional cai-monitoring artifact registry name output.

fad31c3

eeaton mentioned this pull request Aug 1, 2024

Docker tests fail due to CSR dependency #1309

Closed

As per eeaton feedback, commented out SCC and CAI related tests.

f949003

eeaton approved these changes Aug 1, 2024

View reviewed changes

eeaton merged commit 320e9d5 into terraform-google-modules:master Aug 1, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: disabling SCC notifications for now #1304

chore: disabling SCC notifications for now #1304

lpezet commented Jul 28, 2024

eeaton commented Jul 29, 2024

eeaton Jul 29, 2024

eeaton Jul 29, 2024

eeaton Jul 29, 2024 •

edited

Loading

eeaton Jul 29, 2024

eeaton Jul 30, 2024

eeaton Jul 29, 2024

eeaton commented Jul 29, 2024

lpezet commented Jul 30, 2024 •

edited

Loading

eeaton commented Jul 30, 2024

eeaton Jul 30, 2024

eeaton Jul 30, 2024

daniel-cit commented Jul 31, 2024

eeaton commented Jul 31, 2024 •

edited

Loading

lpezet commented Jul 31, 2024 •

edited

Loading

eeaton commented Aug 1, 2024

eeaton commented Aug 1, 2024

eeaton left a comment

chore: disabling SCC notifications for now #1304

chore: disabling SCC notifications for now #1304

Conversation

lpezet commented Jul 28, 2024

eeaton commented Jul 29, 2024

eeaton Jul 29, 2024

Choose a reason for hiding this comment

eeaton Jul 29, 2024

Choose a reason for hiding this comment

eeaton Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

eeaton Jul 29, 2024

Choose a reason for hiding this comment

eeaton Jul 30, 2024

Choose a reason for hiding this comment

eeaton Jul 29, 2024

Choose a reason for hiding this comment

eeaton commented Jul 29, 2024

lpezet commented Jul 30, 2024 • edited Loading

eeaton commented Jul 30, 2024

eeaton Jul 30, 2024

Choose a reason for hiding this comment

eeaton Jul 30, 2024

Choose a reason for hiding this comment

daniel-cit commented Jul 31, 2024

eeaton commented Jul 31, 2024 • edited Loading

lpezet commented Jul 31, 2024 • edited Loading

eeaton commented Aug 1, 2024

eeaton commented Aug 1, 2024

eeaton left a comment

Choose a reason for hiding this comment

eeaton Jul 29, 2024 •

edited

Loading

lpezet commented Jul 30, 2024 •

edited

Loading

eeaton commented Jul 31, 2024 •

edited

Loading

lpezet commented Jul 31, 2024 •

edited

Loading