OCPBUGS-59430: Fail install config validation when base domain is invalid #9840

barbacbd · 2025-07-17T14:27:39Z

installconfig/gcp:

** Add a check during Validate() for the base domain of the public zone.
** Validation tests updated.

openshift-ci-robot · 2025-07-17T14:27:47Z

@barbacbd: This pull request references Jira Issue OCPBUGS-59430, which is invalid:

expected the bug to target the "4.20.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

installconfig/gcp:

** Add a check during Validate() for the base domain of the public zone.
** Validation tests updated.

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.

barbacbd · 2025-07-17T14:41:40Z

/jira refresh

openshift-ci-robot · 2025-07-17T14:41:49Z

@barbacbd: This pull request references Jira Issue OCPBUGS-59430, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.20.0) matches configured target version for branch (4.20.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jianli-wei

Details

In response to this:

/jira refresh

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.

tthvo

Looks good when I tested with an invalid baseDomain: The installer fails with an error.

$ ./openshift-install create manifests --dir=. --log-level=debug
DEBUG OpenShift Installer unreleased-master-12069-gd54eceed15eab76361351f681b6975a65926c507-dirty 
DEBUG Built from commit d54eceed15eab76361351f681b6975a65926c507 
DEBUG Fetching Master Machines...                  
DEBUG Loading Master Machines...                   
DEBUG   Loading Cluster ID...                      
DEBUG     Loading Install Config...                
DEBUG       Loading SSH Key...                     
DEBUG       Loading Base Domain...                 
DEBUG         Loading Platform...                  
DEBUG       Loading Cluster Name...                
DEBUG         Loading Base Domain...               
DEBUG         Loading Platform...                  
DEBUG       Loading Pull Secret...                 
DEBUG       Loading Platform...                    
WARNING Release Image Architecture not detected. Release Image Architecture is unknown 
INFO Credentials loaded from file "/home/thvo/.gcp/osServiceAccount.json" 
ERROR failed to fetch Master Machines: failed to load asset "Install Config": failed to create install config: baseDomain: Invalid value: "gcp.devcluster.openshift.com": baseDomain: Internal error: no matching public DNS Zone found

I just have some small questions below :D

tthvo · 2025-07-17T18:11:20Z

pkg/asset/installconfig/gcp/validation.go

 	allErrs = append(allErrs, ValidateCredentialMode(client, ic)...)
 	allErrs = append(allErrs, validatePreexistingServiceAccount(client, ic)...)
+	if err := ValidatePreExistingPublicDNS(client, ic); err != nil {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("baseDomain"), ic.BaseDomain, err.Error()))


I noticed this block was never reached if I used a non-existing domain.

installer/pkg/asset/installconfig/gcp/validation.go

Lines 419 to 421 in 97030df

if IsNotFound(err) {

return field.NotFound(field.NewPath("baseDomain"), fmt.Sprintf("Private DNS Zone (%s/%s)", ic.Platform.GCP.ProjectID, ic.BaseDomain))

}

I guess the returned error is a custom error instead of a gcpError.

installer/pkg/asset/installconfig/gcp/client.go

Line 260 in 97030df

return nil, errors.New("no matching public DNS Zone found")

Should we fix the error check too? Or since the installer fails with error anyway, it's no big deal? 🤔

I see that the error check in ValidatePreExistingPublicDNS() is similar to the one done in ValidatePrivateDNSZone().
Within ValidatePreExistingPublicDNS(), should

if IsNotFound(err) {

at https://github.com/openshift/installer/blob/main/pkg/asset/installconfig/gcp/validation.go#L401 be

if zone == nil {

Also, in the same method, should checkRecordSets() be called when zone != nil ? I concluded that after reading https://github.com/openshift/installer/blob/main/pkg/asset/installconfig/gcp/validation.go#L389-L392

I also lean towards reporting the error as "not-found" instead of "internalError" 🤔

ERROR failed to fetch Master Machines: failed to load asset "Install Config": failed to create install config:baseDomain: Invalid value: "gcp.devcluster.openshift.com": baseDomain: Internal error: no matching public DNS Zone found

Though, the final message seems clear enough to know the basedomain doesn't exist...I am OK either way :D

@sadasu If you look at GetDNSZone there is a reason that the public and private are similar but not the same. If we go searching for a public zone we must find it other wise it is an error (this is only run during external installs). If we go looking for a private zone and it does not exist then no harm no foul. That means we are safe to create one. So it is subtle but also very different.

@tthvo I switched to a NotFound error. The reason I used InternalError is that it allows you to add an error message where the NotFound error type does not.

tthvo · 2025-07-17T18:17:57Z

pkg/asset/installconfig/gcp/validation_test.go

 	gcpClient.EXPECT().GetKeyRing(gomock.Any(), &validKeyRing).Return(validKeyRingRet, nil).AnyTimes()
 	gcpClient.EXPECT().GetKeyRing(gomock.Any(), &invalidKeyRing).Return(nil, fmt.Errorf("failed to find key ring invalidKeyRingName: data")).AnyTimes()

+	gcpClient.EXPECT().GetDNSZone(gomock.Any(), validProjectName, validBaseDomain, true).Return(&dns.ManagedZone{Name: "zone-name"}, nil).AnyTimes()


nit: maybe we can use the variable validPublicDNSZone (i.e zone name is defined in validPublicZone) already defined above for these mocks?

tthvo · 2025-07-17T18:20:00Z

pkg/asset/installconfig/gcp/validation_test.go

 	gcpClient.EXPECT().GetKeyRing(gomock.Any(), &invalidKeyRing).Return(nil, fmt.Errorf("failed to find key ring invalidKeyRingName: data")).AnyTimes()

+	gcpClient.EXPECT().GetDNSZone(gomock.Any(), validProjectName, validBaseDomain, true).Return(&dns.ManagedZone{Name: "zone-name"}, nil).AnyTimes()
+	gcpClient.EXPECT().GetDNSZone(gomock.Any(), invalidProjectName, validBaseDomain, true).Return(&dns.ManagedZone{Name: "zone-name"}, nil).AnyTimes()


Since the project is invalid (i.e. using invalidProjectName), this should mock return error, right?

no that isnt the point of that test. The reason I had to throw that in there was because of another test failing for other reasons. We could change it but I didnt want to change what that test was running.

tthvo

/lgtm

patrickdillon · 2025-07-18T16:39:39Z

This validation should probably be part of the platform provisioning check rather than the installconfig asset validation. If it’s part of the asset check that requires the DNS zone to exist when validating the installconfig, which may be overly strict for UPI. With UPI you could create the domain after the installconfig.

jianli-wei · 2025-07-21T03:00:24Z

/verified-by

jianli-wei · 2025-07-21T03:01:02Z

/label qe-approved

openshift-ci-robot · 2025-07-21T03:01:09Z

@barbacbd: This pull request references Jira Issue OCPBUGS-59430, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.20.0) matches configured target version for branch (4.20.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jianli-wei

Details

In response to this:

installconfig/gcp:

** Add a check during Validate() for the base domain of the public zone.
** Validation tests updated.

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.

jianli-wei · 2025-07-21T03:14:02Z

/verified by jiwei

openshift-ci-robot · 2025-07-21T03:14:16Z

@jianli-wei: This PR has been marked as verified by jiwei. Jira issue(s) in the title of this PR will be moved to the VERIFIED state on merge.

Details

In response to this:

/verified by jiwei

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.

patrickdillon

This validation should probably be part of the platform provisioning check rather than the installconfig asset validation. If it’s part of the asset check that requires the DNS zone to exist when validating the installconfig, which may be overly strict for UPI. With UPI you could create the domain after the installconfig.

No, nevermind, I was wrong. To correctly generate the DNS manifests we need to make API calls, so even for UPI the public basedomain needs to exist when create manifests is called. Adding the validation as is done here is the correct approach.

Just one small change requested and this looks good to me.

pkg/asset/installconfig/gcp/validation.go

…alid installconfig/gcp: ** Add a check during Validate() for the base domain of the public zone. ** Validation tests updated.

barbacbd · 2025-07-24T01:40:02Z

With the latest, the NotFound Error check will make sense. The following example output was observed when the base domain was purposefully bad in the install-config.

ERROR failed to fetch Metadata: failed to load asset "Install Config": failed to create install config: baseDomain: Not found: "Public DNS Zone (openshift-dev-installer/badinstaller.gcp.devcluster.openshift.com)"

openshift-ci · 2025-07-24T06:16:55Z

@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-vsphere-ovn-multi-network	`1c9589a`	link	false	`/test e2e-vsphere-ovn-multi-network`
ci/prow/e2e-vsphere-externallb-ovn	`1c9589a`	link	false	`/test e2e-vsphere-externallb-ovn`
ci/prow/e2e-vsphere-host-groups-ovn-custom-no-upgrade	`1c9589a`	link	false	`/test e2e-vsphere-host-groups-ovn-custom-no-upgrade`
ci/prow/e2e-vsphere-ovn-multi-network-techpreview	`1c9589a`	link	false	`/test e2e-vsphere-ovn-multi-network-techpreview`
ci/prow/e2e-gcp-secureboot	`1c9589a`	link	false	`/test e2e-gcp-secureboot`

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-sigs/prow repository. I understand the commands that are listed here.

patrickdillon · 2025-07-24T12:58:40Z

/approve

openshift-ci · 2025-07-24T12:59:26Z

[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:

~~pkg/asset/installconfig/gcp/OWNERS~~ [patrickdillon]

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

tthvo

/lgtm

I can reproduce the outcome too :D

ERROR failed to fetch Metadata: failed to load asset "Install Config": failed to create install config: baseDomain: Not found: "Public DNS Zone (openshift-dev-installer/idk.this.domain)"

openshift-ci-robot · 2025-07-24T17:06:05Z

@barbacbd: Jira Issue OCPBUGS-59430: All pull requests linked via external trackers have merged:

openshift/installer#9840

Jira Issue OCPBUGS-59430 has been moved to the MODIFIED state.

Details

In response to this:

installconfig/gcp:

** Add a check during Validate() for the base domain of the public zone.
** Validation tests updated.

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-bot · 2025-07-24T19:38:51Z

[ART PR BUILD NOTIFIER]

Distgit: ose-installer
This PR has been included in build ose-installer-container-v4.20.0-202507241814.p0.gfc9c0c6.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2025-07-24T19:38:52Z

[ART PR BUILD NOTIFIER]

Distgit: ose-baremetal-installer
This PR has been included in build ose-baremetal-installer-container-v4.20.0-202507241814.p0.gfc9c0c6.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2025-07-24T20:50:56Z

[ART PR BUILD NOTIFIER]

Distgit: ose-installer-artifacts
This PR has been included in build ose-installer-artifacts-container-v4.20.0-202507241814.p0.gfc9c0c6.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci bot requested review from patrickdillon and tthvo July 17, 2025 14:29

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jul 17, 2025

openshift-ci bot requested a review from jianli-wei July 17, 2025 14:41

tthvo reviewed Jul 17, 2025

View reviewed changes

barbacbd force-pushed the OCPBUGS-59430 branch from 89b41c5 to 51ca506 Compare July 17, 2025 20:11

tthvo reviewed Jul 17, 2025

View reviewed changes

openshift-ci bot assigned tthvo Jul 17, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2025

barbacbd force-pushed the OCPBUGS-59430 branch from 51ca506 to 5b759fc Compare July 18, 2025 11:02

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2025

barbacbd mentioned this pull request Jul 18, 2025

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

Merged

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jul 21, 2025

openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Jul 21, 2025

patrickdillon requested changes Jul 21, 2025

View reviewed changes

pkg/asset/installconfig/gcp/validation.go Outdated Show resolved Hide resolved

barbacbd force-pushed the OCPBUGS-59430 branch from 5b759fc to d5323fc Compare July 22, 2025 19:38

openshift-ci-robot removed the verified Signifies that the PR passed pre-merge verification criteria label Jul 22, 2025

OCPBUGS-59430: Fail install config validation when base domain is inv…

1c9589a

…alid installconfig/gcp: ** Add a check during Validate() for the base domain of the public zone. ** Validation tests updated.

barbacbd force-pushed the OCPBUGS-59430 branch from d5323fc to 1c9589a Compare July 24, 2025 01:38

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 24, 2025

tthvo reviewed Jul 24, 2025

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2025

openshift-merge-bot bot merged commit fc9c0c6 into openshift:main Jul 24, 2025
21 of 26 checks passed

jianlinliu mentioned this pull request Jul 31, 2025

CORS-4027: Updating feature gate name to GCPClusterHostedDNSInstall openshift/release#67624

Merged

	if IsNotFound(err) {
	return field.NotFound(field.NewPath("baseDomain"), fmt.Sprintf("Private DNS Zone (%s/%s)", ic.Platform.GCP.ProjectID, ic.BaseDomain))
	}

OCPBUGS-59430: Fail install config validation when base domain is invalid #9840

OCPBUGS-59430: Fail install config validation when base domain is invalid #9840

Uh oh!

Conversation

barbacbd commented Jul 17, 2025

Uh oh!

openshift-ci-robot commented Jul 17, 2025

Uh oh!

barbacbd commented Jul 17, 2025

Uh oh!

openshift-ci-robot commented Jul 17, 2025

Uh oh!

tthvo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadasu Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tthvo left a comment

Choose a reason for hiding this comment

Uh oh!

patrickdillon commented Jul 18, 2025

Uh oh!

jianli-wei commented Jul 21, 2025

Uh oh!

jianli-wei commented Jul 21, 2025

Uh oh!

openshift-ci-robot commented Jul 21, 2025

Uh oh!

jianli-wei commented Jul 21, 2025

Uh oh!

openshift-ci-robot commented Jul 21, 2025

Uh oh!

patrickdillon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

barbacbd commented Jul 24, 2025

Uh oh!

openshift-ci bot commented Jul 24, 2025

Uh oh!

patrickdillon commented Jul 24, 2025

Uh oh!

openshift-ci bot commented Jul 24, 2025

Uh oh!

tthvo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openshift-ci-robot commented Jul 24, 2025

Uh oh!

openshift-bot commented Jul 24, 2025

Uh oh!

openshift-bot commented Jul 24, 2025

Uh oh!

openshift-bot commented Jul 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

sadasu Jul 17, 2025 •

edited

Loading