-
Notifications
You must be signed in to change notification settings - Fork 55
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
Yoppworks PBMM Secure LandingZone contribution #291
Yoppworks PBMM Secure LandingZone contribution #291
Conversation
Reviewing over the next 48+h including a clean org deploy of this patch and arch review. |
Hi @fmichaelobrien , Here are some architecture diagrams of the changes from the contribution for architecture review: Centralized Logging and Monitoring We are suggesting updating these diagrams to the current Readme.md or documents. Please let us know if that is ok. |
Sounds good, the more diagrams the better, projected (ngfw perimeter based) and current (armor only) for example are all good. You can split the PR's by GitHub issue id as well if that granular approach is easier |
In general I will need to fully deploy this branch in a new clean org. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue for all the img files - rebase your PR branch
this file was already added in oct 2022
https://github.com/GoogleCloudPlatform/pbmm-on-gcp-onboarding/commits/main/docs/img/_01_guardrails_mfa_on_admin_wide_org_2.png
Jackson the TF PR needs to be rebased - half the changes in your patch are changes I already added since Oct 2022 - which looks like the version of main your PR is based on.for example
|
provider = google-beta | ||
for_each = local.filtered_monitored_projects | ||
metrics_scope = "locations/global/metricsScopes/${local.scoping_project_id}" | ||
name = "locations/global/metricsScopes/${local.scoping_project_id}/projects/${each.value.project_id}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this monitored_projects resource needs to be modified to use a non beta module or commented out in entirety for now. The issue is that the for loop that gathers project id's will also gather recently deleted project id's in the 30 day window - breaking common and sometimes prod
This is an intermittent issue with deleted projects - the for loop in prod is intermittent and can be retried but the one in common has issues with the 30 day list in combination with the state file. Yesterday I worked around it but today depending on the changes (2 new bq projects) i needed to comment out/remove the monitoring section - partially disabling some of the logging/monitoring additions. Now to be fair that module uses a beta group api that may have the same issues I have seen with the automated in-beta groups creation module (workaround there is to fully delete the group before doing user add/removes - tool gets confused on authoratative vs additive modes. I have workarounds for the non-public code that gets in the way of something that should take 20 min to merge/deploy into 1 hour of triage - with a solution. https://github.com/GoogleCloudPlatform/pbmm-on-gcp-onboarding/issues/279
scoping_project_id = (var.project == null || var.project == "") ? try(module.monitoring_center_project[0].project_id, null) : var.project | ||
scoping_project_parent_id = try(split("/", var.parent)[1], null) | ||
matched_monitored_projects = local.scoping_project_id == null || local.scoping_project_parent_id == null ? [] : try(var.monitored_projects, []) | ||
filtered_monitored_projects = { for prj in local.matched_monitored_projects : prj.project_id => prj if prj.parent.id != local.scoping_project_parent_id && prj.project_id != local.scoping_project_id } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See commented issue with intermittently bringing in deleted project ids - requiring modules/monitoring-center/main.tf to be commented out to allow the build to proceed
6e66106
to
e88583e
Compare
Hi @fmichaelobrien , the latest main is merged into the Yoppworks contribution PR as per the above steps. |
Looks good, some verification/testing examples are in the following issue where a clean org installs the build (add/spawn issues as they occur for side-PRs) |
e88583e
to
51a4f0f
Compare
Jackson, looks good, can we flip the PR from draft and fix the merge conflicts - then we can get it merged and work out adjustments as we go on the main branch after it is merged. |
51a4f0f
to
6bb02e1
Compare
@fmichaelobrien , Rebased/Merged latest main again and all look good. Since I have no write access permission to this repository, can you please click the merge button to trigger the merging? @obriensystems |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick check on the merge - there are HEAD sections left over in the docs
It is fixed by replacing content with current main's. Please continue with the merging. @fmichaelobrien |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fully reviewed
Once the 314 changes are merged and the conflicts of my earlier 284 PRs are rebased - I'll approve and merge is patch.
I have some changes to do for monitoring/alerting that needs to be done on top of your 291 push to main - so ideally we merge this before Thursday.
Any comment or proposed changes can be done post PR merge in the tracking issue
#315
@@ -78,6 +77,11 @@ resource "google_artifact_registry_repository_iam_member" "terraform-image-iam" | |||
|
|||
} | |||
|
|||
data "google_projects" "active_projects" { | |||
provider = google-beta | |||
filter = "lifecycleState:active" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll test this later with deleted projects
@@ -214,3 +267,9 @@ module "net-public-perimeter-firewall" { #net-perimeter-prj-firewall | |||
module.net-private-perimeter | |||
] | |||
} | |||
|
|||
data "google_projects" "monitored_projects" { | |||
for_each = local.monitored_project_search_filter_map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test later with deleted projects
@@ -42,7 +42,7 @@ module "partner-interconnect-primary" { | |||
}*/ | |||
|
|||
# Module used to deploy the VPC Service control defined in nonp-vpc-svc-ctl.auto.tfvars | |||
module "vpc-svc-ctl" { | |||
/*module "vpc-svc-ctl" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree - we will revisit VPC and integration with the new PSC later
REPLACE_ORGANIZATION_ROOT_NODE='folders\/167626420317' | ||
REPLACE_WITH_BOOTSTRAP_UDS=clt29test | ||
REPLACE_BOOTSTRAP_PARENT='folders\/167626420317' | ||
REPLACE_BOOTSTRAP_EMAIL=user:uday.kakkar@yoppworks.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can generisize a couple of these later
# } | ||
} | ||
|
||
resource "google_kms_crypto_key" "default_global_customer_managed_key" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice
@@ -0,0 +1,197 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can reuse this for kubeflow
@@ -23,7 +23,7 @@ set -e | |||
modify() | |||
{ | |||
|
|||
array=( environments/bootstrap/bootstrap.auto.tfvars environments/bootstrap/organization-config.auto.tfvars environments/common/common.auto.tfvars environments/nonprod/nonp-network.auto.tfvars environments/common/perimeter-network.auto.tfvars environments/prod/prod-network.auto.tfvars ) | |||
array=$( find environments/** -type f -name "*.auto.tfvars" ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice - thanks for removing the hardcoding
Jackson don't worry about extensive testing - I will be running a clean install in ob.....eering anyway as I need to make some adjustments to the bootstrap (pre-your patch) and alerting (post your patch) anyway this week.If you put in 314 I'll merge tomorrow the 19th and we can continue on the main branch with smaller changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice
LGTM
Jackson, looks good, after a rebase for expected conflicts - I will reapprove and merge |
0318ad5
to
efdc514
Compare
@fmichaelobrien Fixed some issues found from deployments and validated that this PR basically works. Please continue with the review and merging. |
Very nice Jackson. Excellent test results - thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
efdc514
to
dce21fc
Compare
Rebased again and solved all merge conflicts. @fmichaelobrien |
Thanks for the work Jackson. |
commit d28dfcf
Author: StanImprover 131805071+StanImprover@users.noreply.github.com
Date: Tue Aug 15 09:28:39 2023 -0600
commit 5af991c
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Mon May 8 11:48:02 2023 -0400
commit a23fe6c
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Wed May 3 14:39:40 2023 -0400
commit 5e2b41c
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Tue Apr 25 12:38:16 2023 -0400
commit 0b0c3a5
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Tue Apr 25 11:11:22 2023 -0400
commit c13855d
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Thu Apr 13 16:10:02 2023 -0400
commit 9fb968c
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Tue Apr 11 11:45:04 2023 -0400
commit 64bce38
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Mon Apr 3 13:40:01 2023 -0400
commit 414ca0b
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Tue Mar 28 14:36:31 2023 -0400
commit 88ba3dd
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Thu Mar 23 15:14:37 2023 -0400
commit 98aae24
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Thu Mar 23 15:14:01 2023 -0400
commit 970c16b
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Thu Mar 23 15:13:37 2023 -0400
commit e98a815
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Thu Mar 23 15:13:15 2023 -0400
commit 5e518a4
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Mon Mar 20 14:34:01 2023 -0400
commit 1fccfd5
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Thu Mar 16 13:34:29 2023 -0400
commit ea0a288
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Fri Mar 3 14:02:12 2023 -0500
commit f12ff25
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Wed Mar 1 15:09:38 2023 -0500
commit bcca300
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Tue Feb 28 13:15:29 2023 -0500
commit cf715a7
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Mon Feb 27 15:30:44 2023 -0500
commit aaf08d2
Author: uday kakkar udaykakkar@gmail.com
Date: Wed Feb 22 10:01:43 2023 -0500
commit 7dc51e9
Author: uday kakkar udaykakkar@gmail.com
Date: Tue Feb 21 13:46:18 2023 -0500
commit 43f772e
Author: root uday.kakkar@yoppworks.com
Date: Fri Feb 17 19:27:44 2023 +0000
commit 14c1380
Author: Jackson Yang jackson.yang@yoppworks.com
Date: Thu Feb 16 16:29:15 2023 -0500
commit 171fe14
Author: Jackson Yang jackson.yang@yoppworks.com
Date: Thu Feb 16 11:52:26 2023 -0500
commit 7a09624
Author: JacksonYang jackson.yang@yoopworks.com
Date: Thu Feb 16 06:39:23 2023 +0000
commit c0ea79b
Author: JacksonYang jackson.yang@yoopworks.com
Date: Wed Feb 15 21:25:38 2023 +0000
commit 2583469
Author: uday kakkar udaykakkar@gmail.com
Date: Wed Feb 15 00:49:05 2023 -0500
commit ca981f3
Author: uday kakkar udaykakkar@gmail.com
Date: Wed Feb 15 00:48:40 2023 -0500
commit 533f8cf
Author: uday kakkar udaykakkar@gmail.com
Date: Wed Feb 15 00:43:46 2023 -0500
commit 0a3bc15
Author: uday kakkar udaykakkar@gmail.com
Date: Wed Feb 15 00:42:59 2023 -0500
commit a593a21
Author: uday kakkar udaykakkar@gmail.com
Date: Wed Feb 15 00:42:11 2023 -0500
commit 9966dcb
Author: uday kakkar udaykakkar@gmail.com
Date: Wed Feb 15 00:41:40 2023 -0500
commit c764438
Author: uday kakkar udaykakkar@gmail.com
Date: Wed Feb 15 00:30:24 2023 -0500
commit 729b499
Author: uday kakkar udaykakkar@gmail.com
Date: Tue Feb 14 23:56:05 2023 -0500
commit f499580
Author: uday kakkar udaykakkar@gmail.com
Date: Tue Feb 14 23:55:35 2023 -0500
commit 77933f1
Author: uday kakkar udaykakkar@gmail.com
Date: Tue Feb 14 23:54:58 2023 -0500
commit cd2fa85
Author: Jackson Yang jackson.yang@yoppworks.com
Date: Mon Feb 13 22:53:22 2023 -0500
commit b1f8aa0
Merge: f33a6cc f10657c
Author: uday kakkar udaykakkar@gmail.com
Date: Mon Feb 13 16:29:18 2023 -0500
commit f10657c
Author: root uday.kakkar@yoppworks.com
Date: Mon Feb 13 21:28:12 2023 +0000
commit f33a6cc
Author: Jackson Yang jackson.yang@yoppworks.com
Date: Mon Feb 13 16:27:57 2023 -0500
commit 980261e
Author: root uday.kakkar@yoppworks.com
Date: Mon Feb 13 21:24:40 2023 +0000
commit 43aef4c
Author: root uday.kakkar@yoppworks.com
Date: Mon Feb 13 21:17:49 2023 +0000
commit 8863533
Author: Jackson Yang jackson.yang@yoppworks.com
Date: Mon Feb 13 14:21:31 2023 -0500
commit 88f9cb4
Author: Jackson Yang jackson.yang@yoppworks.com
Date: Mon Feb 13 10:57:51 2023 -0500
commit 0320ab6
Merge: e0ac5af ef57045
Author: uday kakkar udaykakkar@gmail.com
Date: Sat Feb 11 00:13:03 2023 -0500
commit ef57045
Author: root uday.kakkar@yoppworks.com
Date: Sat Feb 11 05:11:20 2023 +0000
commit e0ac5af
Merge: c416c6d c0048c7
Author: uday kakkar udaykakkar@gmail.com
Date: Sat Feb 11 00:00:31 2023 -0500
commit c416c6d
Author: Jackson Yang 95398233+jacyang2010@users.noreply.github.com
Date: Wed Feb 8 17:02:40 2023 -0500
commit c0048c7
Author: udaykakkar udaykakkar@gmail.com
Date: Wed Feb 8 16:39:55 2023 -0500
commit 53f69cc
Author: JacksonYang jackson.yang@yoppworks.com
Date: Wed Feb 8 10:02:04 2023 -0500
commit 2791bbc
Author: JacksonYang jackson.yang@yoppworks.com
Date: Wed Feb 8 00:06:13 2023 -0500