CORS-4044, CORS-4045, CORS-4046, CORS-4047, CORS-4048, CORS-4049, CORS-4050, CORS-4051: Add private dns zone section to GCP install config #9792
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@barbacbd: This pull request references CORS-4044 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/cc @patrickdillon |
|
/label platform/gcp |
|
@barbacbd: The label(s) DetailsIn response to this:
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-sigs/prow repository. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/label platform/google |
barbacbd
force-pushed
the
CORS-4044
branch
4 times, most recently
from
df91db7 to
4c883d9
Compare
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
barbacbd
changed the title
|
/jira refresh |
|
@barbacbd: This pull request references CORS-4044 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
barbacbd
changed the title
|
@barbacbd: This pull request references CORS-4044 which is a valid jira issue. This pull request references CORS-4045 which is a valid jira issue. This pull request references CORS-4046 which is a valid jira issue. This pull request references CORS-4047 which is a valid jira issue. This pull request references CORS-4048 which is a valid jira issue. This pull request references CORS-4049 which is a valid jira issue. This pull request references CORS-4050 which is a valid jira issue. This pull request references CORS-4051 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@barbacbd: This pull request references CORS-4044 which is a valid jira issue. This pull request references CORS-4045 which is a valid jira issue. This pull request references CORS-4046 which is a valid jira issue. This pull request references CORS-4047 which is a valid jira issue. This pull request references CORS-4048 which is a valid jira issue. This pull request references CORS-4049 which is a valid jira issue. This pull request references CORS-4050 which is a valid jira issue. This pull request references CORS-4051 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
| // It is possible to create a managed zone without a description using the GCP web console. | ||
| // If the description is missing the managed zone modification will fail. | ||
| // The description will differ slightly from a zone that the installer created. | ||
| zone.Description = "Used by OpenShift Installer" |
There was a problem hiding this comment.
Nit: "Used by OpenShift" would have been sufficient.
|
/hold wait for 4.21 branching |
openshift-ci
bot
added
the
do-not-merge/hold
|
/test e2e-gcp-xpn-dedicated-dns-project |
| if zone != nil { | ||
| if icdns != nil && icdns.PrivateZone != nil && icdns.PrivateZone.Name != zone.Name { | ||
| allErrs = append(allErrs, field.Invalid( | ||
| field.NewPath("platform").Child("gcp").Child("dns").Child("privateZone").Child("name"), | ||
| zone.Name, | ||
| fmt.Sprintf("expected private zone name %s or empty string", icdns.PrivateZone.Name), | ||
| )) | ||
| } else if err := checkRecordSets(client, ic, project, zone, []string{apiRecordType(ic), apiIntRecordName(ic)}); err != nil { | ||
| allErrs = append(allErrs, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
| if zone != nil { | |
| if icdns != nil && icdns.PrivateZone != nil && icdns.PrivateZone.Name != zone.Name { | |
| allErrs = append(allErrs, field.Invalid( | |
| field.NewPath("platform").Child("gcp").Child("dns").Child("privateZone").Child("name"), | |
| zone.Name, | |
| fmt.Sprintf("expected private zone name %s or empty string", icdns.PrivateZone.Name), | |
| )) | |
| } else if err := checkRecordSets(client, ic, project, zone, []string{apiRecordType(ic), apiIntRecordName(ic)}); err != nil { | |
| allErrs = append(allErrs, err) | |
| } | |
| } | |
| if zone != nil { | |
| if zoneName != "" && zoneName != zone.Name { | |
| allErrs = append(allErrs, field.Invalid( | |
| field.NewPath("platform").Child("gcp").Child("dns").Child("privateZone").Child("name"), | |
| zoneName, | |
| fmt.Sprintf("expected private zone name %s or empty string", zone.Name), | |
| )) | |
| } else if err := checkRecordSets(client, ic, project, zone, []string{apiRecordType(ic), apiIntRecordName(ic)}); err != nil { | |
| allErrs = append(allErrs, err) | |
| } | |
| } |
I think we can reference the zoneName directly.
Also, we need to use the "expected" name from the retrieved zone metadata to suggest the user (i.e. instead of the what they already enterred), right?
| project := ic.GCP.ProjectID | ||
| zoneName := "" | ||
| icdns := ic.GCP.DNS | ||
| if ic.GCP.NetworkProjectID != "" && icdns != nil && icdns.PrivateZone != nil { |
There was a problem hiding this comment.
| if ic.GCP.NetworkProjectID != "" && icdns != nil && icdns.PrivateZone != nil { | |
| if icdns != nil && icdns.PrivateZone != nil { |
ic.GCP.NetworkProjectID != "" is always true here since we have a check above, right?
| if IsNotFound(err) { | ||
| return append(allErrs, field.NotFound(field.NewPath("baseDomain"), fmt.Sprintf("Private DNS Zone (%s/%s)", project, ic.BaseDomain))) | ||
| } | ||
| return append(allErrs, field.InternalError(field.NewPath("baseDomain"), err)) |
There was a problem hiding this comment.
Should we ignore the "not-found" error since the installer will create one?
Other errors such as mismatched dns name is a custom error that is caught anyways. If so, we should mark those errors as invalid instead of internal?
install.openshift.io_installconfigs.yaml: ** Updated fields from the types/installconfig/gcp CORS-4047: Add private Zone Validation pkg/types/gcp/platform.go: ** Add the user specified private dns zone ** Add static validation pkg/asset/installconfig/gcp/validation.go: ** When private dns zone information is provided, ensure that the project and zone are used for validation. CORS-4045: Update Clsuter Metadata ** Add the GCP private zone information to the cluster metadata CORS-4048: Update TFVars to include private zone info CORS-4049: Find the correct project for the dns zones ** Update the DNS Manifest to take the correct private zone project when specified. ** Note: Need to update DNS Spec to take in a project. CORS-4046: Delete Private Zones pkg/destroy/gcp: ** Use the cluster metadata to update the gcp cluster uninstaller. ** Find DNS zones in the correct project. Delete the zones that can and should be deleted. ** Delete the DNS records in the private and public zones. pkg/destroy/gcp: ** Destroy DNS zones if they have the "owned" label. installconfig/gcp: ** Generate a new Client function to find private DNS zones where the base domain and zone name are both provided. manifests/dns: ** Use the new client function to ensure that we find the correct private zone when private zone information is provided in the install config file. clusterapi/dns: ** Use the new client function to ensure that we find the correct private zone when private zone information is provided in the install config file. Adding the "shared" tag when the installer does not create the private managed zone. ** On Destroy, search the private dns zone for the labels. If the shared label with a key matching the cluster ID exists, remove the label.
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
/lgtm
Code looks good to me! Let's see if we can run a pre-submit...
| logrus.Debug("No private DNS Zone found") | ||
| if IsNotFound(err) { | ||
| // Ignore the not found error, because the zone will be created in this instance. | ||
| continue | ||
| } |
There was a problem hiding this comment.
nit: We should move the log inside the check? Or we can just remove it :D
There was a problem hiding this comment.
I guess we never really hit IsNotFound error previously while testing because it is handled elsewherein the call (otherwiser it should fail):
client.GetDNSZoneFromParams(context.TODO(), paramSet)If so, we can remove this check and logs all together :D
|
/test e2e-gcp-xpn-dedicated-dns-project Let's see if the presubmit works now... 👀 |
|
/retest-required |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/test e2e-gcp-ovn |
|
@barbacbd: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
/test e2e-gcp-ovn The install succeeded though... |
|
/test gcp-private e2e-gcp-ovn-byo-vpc |
| allErrs = append(allErrs, field.Invalid( | ||
| field.NewPath("platform").Child("gcp").Child("dns").Child("privateZone").Child("name"), | ||
| zoneName, | ||
| fmt.Sprintf("found existing private zone %s in project %s with base domain %s", zone.Name, project, ic.BaseDomain), |
openshift-merge-bot
bot
merged commit 5f4e6cd
into
openshift:main
7 participants
install.openshift.io_installconfigs.yaml:
** Updated fields from the types/installconfig/gcp
pkg/types/gcp/platform.go:
** Add the user specified private dns zone
** Add static validation
pkg/asset/installconfig/gcp/validation.go:
** When private dns zone information is provided, ensure that the project and zone are used for validation.
** The public zone should be in the same project as the private zone. When the private zone is provided, assume the same project for public zone validation.