OCPBUGS-51299: Add default values for validation to kms keys #9781

barbacbd · 2025-06-12T14:10:41Z

installconfig/gcp:

** Add default location and project to the kms key validation. The installer does not require these fields to be entered, so the defaults will be selected from the projectID and region fields of the GCP Platform.

openshift-ci-robot · 2025-06-12T14:10:49Z

@barbacbd: This pull request references Jira Issue OCPBUGS-51299, 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

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

In response to this:

installconfig/gcp:

** Add default location and project to the kms key validation. The installer does not require these fields to be entered, so the defaults will be selected from the projectID and region fields of the GCP Platform.

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 · 2025-06-12T14:12:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign patrickdillon for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/asset/installconfig/gcp/OWNERS

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

tthvo

Looks good! I have a small question :D And I think unit tests needs fixing (plus fmt/lint issues).

tthvo · 2025-06-12T19:38:57Z

pkg/asset/installconfig/gcp/client.go

+	if kmsKeyRef.ProjectID == "" {
+		logrus.Debugf("kms key project ID missing, using default project: %s", projectID)
+		projectID = project
+	}
+
+	location := kmsKeyRef.Location
+	if location == "" {
+		logrus.Debugf("kms key location missing, using default region: %s", region)
+		location = region
+	}


Looking at this PR and #9767, I noticed we have other places that need defaulting too.

This needs project and location defaulting:

installer/pkg/asset/machines/gcp/machines.go

Lines 145 to 148 in 7c5b649

Name: mpool.OSDisk.EncryptionKey.KMSKey.Name,

KeyRing: mpool.OSDisk.EncryptionKey.KMSKey.KeyRing,

ProjectID: mpool.OSDisk.EncryptionKey.KMSKey.ProjectID,

Location: mpool.OSDisk.EncryptionKey.KMSKey.Location,

This needs location defaulting:

installer/pkg/asset/machines/gcp/gcpmachines.go

Lines 29 to 35 in 7c5b649

func generateDiskEncryptionKeyLink(kmsKey *gcptypes.KMSKeyReference, projectID string) string {

if kmsKey.ProjectID != "" {

projectID = kmsKey.ProjectID

}

return fmt.Sprintf(kmsKeyNameFmt, projectID, kmsKey.Location, kmsKey.KeyRing, kmsKey.Name)

}

Should we set the defaults somewhere early for this kmsKey field? So that, it is handled at one place.

I think the default is supposed to be stored in the defaultMachinePlatform. The reason we have these other validations is that the defaultMachinePlatform can also be empty.

This makes sense (good catch @tthvo) but it is strange that the associated bug has been successfully tested.

Because the values (project & region) are already contained in the install config, it should be possible to extend the default function in pkg/types:

installer/pkg/asset/installconfig/installconfig.go

Line 103 in e6e1537

defaults.SetInstallConfigDefaults(a.Config)

That would set the value in the install config, so it would also supersede the changes introduced in #9767. One thing I don't recall is that if the location is empty whether we default to the cluster region or global, but either case we should be able handle fine in pkg/types.

Alternatively if you wanted to handle this only for machines you could extend the defaultGCPMachinePoolPlatform function to populate the default values:

installer/pkg/asset/machines/worker.go

Lines 132 to 140 in e6e1537

func defaultGCPMachinePoolPlatform(arch types.Architecture) gcptypes.MachinePool {

return gcptypes.MachinePool{

InstanceType: icgcp.DefaultInstanceTypeForArch(arch),

OSDisk: gcptypes.OSDisk{

DiskSizeGB: powerOfTwoRootVolumeSize,

DiskType: "pd-ssd",

},

}

}

I think it is more extensible/future proof to handle this in the first way that was suggested. In which case I think you would ultimately end up extending the SetMachinePoolDefaults function

installer/pkg/types/defaults/machinepools.go

Line 9 in e6e1537

func SetMachinePoolDefaults(p *types.MachinePool, platform string) {

The function would need to be revised to pass in the platform struct that contains the project. You would also need to make sure the defaultMachinePoolPlatform is populated. It seems like the existing code does not handle that.

tthvo · 2025-06-12T21:22:29Z

pkg/asset/installconfig/gcp/client.go

+	if kmsKeyRef.ProjectID == "" {
+		logrus.Debugf("kms key project ID missing, using default project: %s", projectID)


Suggested change

if kmsKeyRef.ProjectID == "" {

logrus.Debugf("kms key project ID missing, using default project: %s", projectID)

if projectID == "" {

logrus.Debugf("kms key project ID missing, using default project: %s", project)

nit: I think we need to use project variable here. Otherwise, the log will be empty?

DEBUG kms key project ID missing, using default project: DEBUG kms key location missing, using default region: us-east1

jianli-wei · 2025-06-13T08:12:53Z

/label qe-approved

openshift-ci-robot · 2025-06-13T08:12:59Z

@barbacbd: This pull request references Jira Issue OCPBUGS-51299, 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

In response to this:

installconfig/gcp:

** Add default location and project to the kms key validation. The installer does not require these fields to be entered, so the defaults will be selected from the projectID and region fields of the GCP Platform.

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.

installconfig/gcp: ** Add default location and project to the kms key validation. The installer does not require these fields to be entered, so the defaults will be selected from the projectID and region fields of the GCP Platform.

tthvo · 2025-07-01T14:40:37Z

/retest-required

openshift-ci · 2025-07-01T18:20:49Z

@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-gcp-ovn-byo-vpc	`2365860`	link	false	`/test e2e-gcp-ovn-byo-vpc`
ci/prow/e2e-gcp-ovn-xpn	`2365860`	link	false	`/test e2e-gcp-ovn-xpn`
ci/prow/e2e-vsphere-ovn-multi-network	`2365860`	link	false	`/test e2e-vsphere-ovn-multi-network`
ci/prow/gcp-private	`2365860`	link	false	`/test gcp-private`
ci/prow/e2e-azure-ovn-resourcegroup	`2365860`	link	false	`/test e2e-azure-ovn-resourcegroup`
ci/prow/e2e-vsphere-static-ovn	`2365860`	link	false	`/test e2e-vsphere-static-ovn`
ci/prow/okd-scos-e2e-aws-ovn	`2365860`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-gcp-default-config	`2365860`	link	false	`/test e2e-gcp-default-config`
ci/prow/e2e-vsphere-externallb-ovn	`2365860`	link	false	`/test e2e-vsphere-externallb-ovn`
ci/prow/e2e-gcp-secureboot	`2365860`	link	false	`/test e2e-gcp-secureboot`

Full PR test history. Your PR dashboard.

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.

openshift-ci bot requested review from jianli-wei, patrickdillon and tthvo June 12, 2025 14:10

tthvo reviewed Jun 12, 2025

View reviewed changes

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

barbacbd requested a review from tthvo June 30, 2025 13:24

barbacbd force-pushed the OCPBUGS-51299-validation branch from e26ab80 to bb64b42 Compare June 30, 2025 14:08

barbacbd force-pushed the OCPBUGS-51299-validation branch from bb64b42 to 2365860 Compare June 30, 2025 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

OCPBUGS-51299: Add default values for validation to kms keys #9781

OCPBUGS-51299: Add default values for validation to kms keys #9781

Uh oh!

barbacbd commented Jun 12, 2025

Uh oh!

openshift-ci-robot commented Jun 12, 2025

Uh oh!

openshift-ci bot commented Jun 12, 2025

Uh oh!

tthvo left a comment

Uh oh!

tthvo Jun 12, 2025

Uh oh!

barbacbd Jun 30, 2025

Uh oh!

patrickdillon Jun 30, 2025

Uh oh!

tthvo Jun 12, 2025

Uh oh!

jianli-wei commented Jun 13, 2025

Uh oh!

openshift-ci-robot commented Jun 13, 2025

Uh oh!

tthvo commented Jul 1, 2025

Uh oh!

openshift-ci bot commented Jul 1, 2025

Uh oh!

Uh oh!

	Name: mpool.OSDisk.EncryptionKey.KMSKey.Name,
	KeyRing: mpool.OSDisk.EncryptionKey.KMSKey.KeyRing,
	ProjectID: mpool.OSDisk.EncryptionKey.KMSKey.ProjectID,
	Location: mpool.OSDisk.EncryptionKey.KMSKey.Location,

	func generateDiskEncryptionKeyLink(kmsKey *gcptypes.KMSKeyReference, projectID string) string {
	if kmsKey.ProjectID != "" {
	projectID = kmsKey.ProjectID
	}

	return fmt.Sprintf(kmsKeyNameFmt, projectID, kmsKey.Location, kmsKey.KeyRing, kmsKey.Name)
	}

	func defaultGCPMachinePoolPlatform(arch types.Architecture) gcptypes.MachinePool {
	return gcptypes.MachinePool{
	InstanceType: icgcp.DefaultInstanceTypeForArch(arch),
	OSDisk: gcptypes.OSDisk{
	DiskSizeGB: powerOfTwoRootVolumeSize,
	DiskType: "pd-ssd",
	},
	}
	}

		if kmsKeyRef.ProjectID == "" {
		logrus.Debugf("kms key project ID missing, using default project: %s", projectID)

OCPBUGS-51299: Add default values for validation to kms keys #9781

Are you sure you want to change the base?

OCPBUGS-51299: Add default values for validation to kms keys #9781

Uh oh!

Conversation

barbacbd commented Jun 12, 2025

Uh oh!

openshift-ci-robot commented Jun 12, 2025

Uh oh!

openshift-ci bot commented Jun 12, 2025

Uh oh!

tthvo left a comment

Choose a reason for hiding this comment

Uh oh!

tthvo Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

barbacbd Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

patrickdillon Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

tthvo Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

jianli-wei commented Jun 13, 2025

Uh oh!

openshift-ci-robot commented Jun 13, 2025

Uh oh!

tthvo commented Jul 1, 2025

Uh oh!

openshift-ci bot commented Jul 1, 2025

Uh oh!

Uh oh!