CORS 2040: [gcp] Adding Public and Private Managed zones by barbacbd · Pull Request #6288 · openshift/installer

barbacbd · 2022-08-31T16:06:32Z

**Install config added the public and private managed zones under platform/gcp
**Install config validate the public and private managed zones (if provided) - when
empty or "None" the validation does not occur.
** Added tests for install config

https://issues.redhat.com/browse/CORS-2040

jstuever · 2022-09-01T20:00:18Z

/assign

jstuever

This is a great starting point. I think we should go ahead and pass the privateZone and publicZone string into the manifest "as is" while relying on the install-config for the validation. I'm also concerned about the Terraform records not knowing which project the managed_zone is in. And, I've recommended a slightly different parseZoneName() function. Other than that, some minor tweaks.

jstuever

Two minor tweaks.... 1) I think we need to be very prescribed with which projects the terraform dns records exist, and 2) I think we should be more prescribed with the parsing of the relative path managedZone names.

jstuever

There are some functional changes needed here.... and a couple of nit pick items.

jstuever

Looking great! Just a few more minor changes.

jstuever

/approve

jstuever · 2022-09-09T02:37:49Z

/cc @AnnaZivkovic
/cc @patrickdillon

jstuever · 2022-09-19T22:27:25Z

/hold cancel

jstuever

This appears to function as desired. I'll leave it to the rest of the team to review and approve.

**Install config added the public and private managed zones under platform/gcp **Install config validate the public and private managed zones (if provided) - when empty or "None" the validation does not occur. ** Added tests for install config ** config.Spec.PublicZone and config.Spec.PrivateZone in the cluster dns manifest are not set when "None", Default (empty) values for Public and Private managed zones will not change the current behavior (zones are created and managed in the ProjectID) ** Added the terraform values to skip the creation of public and private zones and their records according to the data provided in public/privateManagedZones: "" (default): normal behavior "None": skip private and public zone and record creation Data Provided: Skip zone creation but still create the records for the zone(s). ** Terraform will use the project or network project ID when creating zones and records.

openshift-ci · 2022-09-22T21:24:28Z

@barbacbd: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-ibmcloud	4c713b737580737d56b59fb7cebf723701b5a1a7	link	false	`/test e2e-ibmcloud`
ci/prow/e2e-gcp-shared-vpc	4c713b737580737d56b59fb7cebf723701b5a1a7	link	false	`/test e2e-gcp-shared-vpc`
ci/prow/e2e-nutanix	4f519f6bda58fe7dc90e19cda78113f10557bf90	link	false	`/test e2e-nutanix`
ci/prow/okd-e2e-aws-upgrade	`d10801b`	link	false	`/test okd-e2e-aws-upgrade`
ci/prow/e2e-gcp-ovn-shared-vpc	`d10801b`	link	false	`/test e2e-gcp-ovn-shared-vpc`
ci/prow/e2e-metal-ipi	`d10801b`	link	false	`/test e2e-metal-ipi`
ci/prow/e2e-openstack	`d10801b`	link	false	`/test e2e-openstack`
ci/prow/okd-e2e-aws-ovn	`d10801b`	link	false	`/test okd-e2e-aws-ovn`
ci/prow/e2e-libvirt	`d10801b`	link	false	`/test e2e-libvirt`
ci/prow/okd-e2e-gcp-ovn-upgrade	`d10801b`	link	false	`/test okd-e2e-gcp-ovn-upgrade`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

patrickdillon · 2022-09-28T14:27:03Z

/approve

openshift-ci · 2022-09-28T14:28:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [patrickdillon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sadasu · 2022-09-28T16:21:38Z

+                        description: ID Technology Preview. ID or name of the zone.
+                        type: string
+                      project:
+                        description: ProjectID Technology Preview When the ProjectID


Nit. Missing a period here Preview "." When.

well its tech preview does it matter = )

sadasu · 2022-09-28T16:22:30Z

+                        description: ID Technology Preview. ID or name of the zone.
+                        type: string
+                      project:
+                        description: ProjectID Technology Preview When the ProjectID


Ditto as https://github.com/openshift/installer/pull/6288/files#r982613655

sadasu · 2022-09-28T16:36:00Z

+		privateZoneName := ""
+		privateZoneProject := ""
+		if installConfig.Config.GCP.PrivateDNSZone != nil && installConfig.Config.GCP.PrivateDNSZone.ID != "" {
+			createPrivateZone = false


This seems counter intuitive to me. When PrivateDNSZone != nil and the ID is provided, we don't create private zones?

no that means that one exists already we DO NOT want to create one. We are assuming it is already created and provided by the user. This option of adding the privateDNSZone.ID is not to name a new private zone but to use an existing one.

Ok. Got it. I think I was equating createPrivateZone with what createPrivateZoneRecords is doing.

sadasu · 2022-09-28T17:41:01Z

/lgtm

openshift-ci-robot · 2022-09-28T18:03:25Z

/retest-required

Remaining retests: 0 against base HEAD 9eb0224 and 2 for PR HEAD d10801b in total

barbacbd · 2022-09-28T18:28:38Z

/skip

barbacbd · 2022-09-28T18:36:28Z

/override ci/prow/e2e-azure-ovn

openshift-ci · 2022-09-28T18:37:07Z

@barbacbd: Overrode contexts on behalf of barbacbd: ci/prow/e2e-azure-ovn

Details

In response to this:

/override ci/prow/e2e-azure-ovn

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2022

openshift-ci Bot requested review from jstuever and patrickdillon August 31, 2022 16:13

barbacbd force-pushed the cors2040 branch from f0ec80d to 469bb46 Compare August 31, 2022 18:58

jstuever reviewed Sep 1, 2022

View reviewed changes

Comment thread pkg/asset/installconfig/gcp/dns.go Outdated

jstuever self-requested a review September 1, 2022 06:38

barbacbd force-pushed the cors2040 branch 3 times, most recently from be08b33 to 7c4b87a Compare September 1, 2022 19:32

openshift-ci Bot assigned jstuever Sep 1, 2022

jstuever requested changes Sep 2, 2022

View reviewed changes

barbacbd force-pushed the cors2040 branch 2 times, most recently from 70afd37 to 4b75a6e Compare September 2, 2022 17:26

jstuever requested changes Sep 2, 2022

View reviewed changes

Comment thread data/data/gcp/cluster/dns/base.tf Outdated

Comment thread data/data/gcp/cluster/dns/base.tf Outdated

Comment thread pkg/asset/installconfig/gcp/dns.go Outdated

barbacbd force-pushed the cors2040 branch from 4b75a6e to fd517dc Compare September 6, 2022 18:16

barbacbd changed the title ~~WIP CORS 2040: Adding Public and Private Managed zones~~ CORS 2040: Adding Public and Private Managed zones Sep 6, 2022

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 6, 2022

barbacbd requested review from jstuever and removed request for patrickdillon September 6, 2022 19:40

jstuever requested changes Sep 6, 2022

View reviewed changes

barbacbd force-pushed the cors2040 branch from fd517dc to 79a8e4e Compare September 7, 2022 14:59

jstuever requested changes Sep 8, 2022

View reviewed changes

Comment thread data/data/gcp/cluster/dns/variables.tf Outdated

Comment thread data/data/gcp/cluster/main.tf Outdated

Comment thread pkg/asset/installconfig/gcp/client.go Outdated

Comment thread pkg/asset/installconfig/gcp/dns.go Outdated

barbacbd force-pushed the cors2040 branch from 79a8e4e to 01e2b72 Compare September 8, 2022 12:47

barbacbd requested a review from jstuever September 8, 2022 12:49

barbacbd force-pushed the cors2040 branch 2 times, most recently from 7857bb8 to 4c713b7 Compare September 8, 2022 17:53

jstuever approved these changes Sep 9, 2022

View reviewed changes

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2022

openshift-ci Bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 19, 2022

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 19, 2022

jstuever requested a review from patrickdillon September 19, 2022 22:30

barbacbd force-pushed the cors2040 branch from daf843c to 87c84a4 Compare September 20, 2022 23:30

barbacbd requested review from jstuever and removed request for patrickdillon September 20, 2022 23:30

barbacbd force-pushed the cors2040 branch from 87c84a4 to 7778789 Compare September 21, 2022 12:06

barbacbd removed the request for review from jstuever September 21, 2022 15:22