OCPBUGS-46488: Validate Encryption Keys for GCP #9328

barbacbd · 2024-12-18T23:27:31Z

** Validate install config information for GCP KMS Encryption keys.
** Update Vendor
** Add KMS Google Module

openshift-ci-robot · 2024-12-18T23:27:39Z

@barbacbd: This pull request references Jira Issue OCPBUGS-46488, 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.19.0) matches configured target version for branch (4.19.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:

** Validate install config information for GCP KMS Encryption keys.
** Update Vendor
** Add KMS Google Module

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 · 2024-12-18T23:28:01Z

/hold

Still figuring out if there is further validation/error checking.

jianli-wei · 2024-12-19T07:16:44Z

/jira-refresh

jianli-wei · 2024-12-19T07:17:06Z

/jira refresh

openshift-ci-robot · 2024-12-19T07:17:10Z

@jianli-wei: This pull request references Jira Issue OCPBUGS-46488, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.19.0) matches configured target version for branch (4.19.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:

/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.

jianli-wei · 2024-12-19T07:17:23Z

/label qe-approved

barbacbd · 2024-12-19T15:42:04Z

/hold cancel

pkg/types/gcp/validation/machinepool.go

barbacbd · 2024-12-23T12:03:32Z

/retest-required

barbacbd · 2025-01-06T18:35:37Z

/label acknowledge-critical-fixes-only

patrickdillon · 2025-01-06T21:05:56Z

pkg/types/gcp/validation/machinepool.go

@@ -80,3 +84,45 @@ func ValidateDefaultDiskType(p *gcp.MachinePool, fldPath *field.Path) field.Erro

 	return allErrs
 }
+
+func ValidateKMSKey(platform *gcp.Platform, p *types.MachinePool, fldPath *field.Path) field.ErrorList {


This validation should go in the validation run by pkg/asset. pkg/types should include minimal external dependencies to allow easy importing by other packages. The pkg/asset validation is called here:

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

Lines 52 to 75 in d2bb750

func Validate(client API, ic *types.InstallConfig) error {

allErrs := field.ErrorList{}

if err := validate.GCPClusterName(ic.ObjectMeta.Name); err != nil {

allErrs = append(allErrs, field.Invalid(field.NewPath("clusterName"), ic.ObjectMeta.Name, err.Error()))

}

allErrs = append(allErrs, validateProject(client, ic, field.NewPath("platform").Child("gcp"))...)

allErrs = append(allErrs, validateNetworkProject(client, ic, field.NewPath("platform").Child("gcp"))...)

allErrs = append(allErrs, validateRegion(client, ic, field.NewPath("platform").Child("gcp"))...)

allErrs = append(allErrs, validateZones(client, ic)...)

allErrs = append(allErrs, validateNetworks(client, ic, field.NewPath("platform").Child("gcp"))...)

allErrs = append(allErrs, validateInstanceTypes(client, ic)...)

allErrs = append(allErrs, ValidateCredentialMode(client, ic)...)

allErrs = append(allErrs, validatePreexistingServiceAccount(client, ic)...)

allErrs = append(allErrs, validateServiceAccountPresent(client, ic)...)

allErrs = append(allErrs, validateMarketplaceImages(client, ic)...)

if err := validateUserTags(client, ic.Platform.GCP.ProjectID, ic.Platform.GCP.UserTags); err != nil {

allErrs = append(allErrs, field.Invalid(field.NewPath("platform").Child("gcp").Child("userTags"), ic.Platform.GCP.UserTags, err.Error()))

}

return allErrs.ToAggregate()

}

patrickdillon · 2025-01-06T21:09:58Z

pkg/types/gcp/validation/machinepool.go

+			Parent: fmt.Sprintf("projects/%s/locations/%s", project, location),
+		}
+
+		it := kmsClient.ListKeyRings(context.Background(), req)


Can you use GetKeyRing to avoid the iterator and simplify this?

sadasu · 2025-01-07T14:00:20Z

pkg/types/gcp/validation/machinepool.go

+		}
+	}
+
+	return allErrs


It appears that we are not returning an error when the key is not found in the keyring. (I see that qe did some pre-merge testing and it works for them. Not sure if it was against this iteration of the fix.)

sadasu · 2025-01-08T15:25:24Z

/approve

openshift-ci · 2025-01-08T15:26:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sadasu

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sadasu]

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

** Validate install config information for GCP KMS Encryption keys.

** Add KMS package for GCP

openshift-ci · 2025-01-13T18:06:48Z

@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-aws-ovn-single-node	`7cebbfa`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/e2e-gcp-ovn-byo-vpc	`7cebbfa`	link	false	`/test e2e-gcp-ovn-byo-vpc`
ci/prow/e2e-aws-ovn-imdsv2	`7cebbfa`	link	false	`/test e2e-aws-ovn-imdsv2`
ci/prow/okd-scos-e2e-aws-ovn	`7cebbfa`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-aws-ovn-fips	`7cebbfa`	link	false	`/test e2e-aws-ovn-fips`
ci/prow/e2e-gcp-default-config	`7cebbfa`	link	false	`/test e2e-gcp-default-config`
ci/prow/e2e-vsphere-ovn-multi-network	`7cebbfa`	link	false	`/test e2e-vsphere-ovn-multi-network`
ci/prow/e2e-aws-default-config	`7cebbfa`	link	false	`/test e2e-aws-default-config`
ci/prow/e2e-aws-ovn-shared-vpc-edge-zones	`7cebbfa`	link	false	`/test e2e-aws-ovn-shared-vpc-edge-zones`
ci/prow/e2e-aws-ovn-edge-zones-manifest-validation	`7cebbfa`	link	true	`/test e2e-aws-ovn-edge-zones-manifest-validation`
ci/prow/e2e-azure-ovn-resourcegroup	`7cebbfa`	link	false	`/test e2e-azure-ovn-resourcegroup`
ci/prow/e2e-vsphere-static-ovn	`7cebbfa`	link	false	`/test e2e-vsphere-static-ovn`
ci/prow/e2e-aws-ovn-heterogeneous	`7cebbfa`	link	false	`/test e2e-aws-ovn-heterogeneous`
ci/prow/e2e-gcp-secureboot	`7cebbfa`	link	false	`/test e2e-gcp-secureboot`
ci/prow/images	`7cebbfa`	link	true	`/test images`
ci/prow/e2e-gcp-ovn-xpn	`7cebbfa`	link	false	`/test e2e-gcp-ovn-xpn`
ci/prow/e2e-aws-ovn-shared-vpc-custom-security-groups	`7cebbfa`	link	false	`/test e2e-aws-ovn-shared-vpc-custom-security-groups`
ci/prow/e2e-aws-ovn	`7cebbfa`	link	true	`/test e2e-aws-ovn`
ci/prow/e2e-aws-ovn-edge-zones	`7cebbfa`	link	false	`/test e2e-aws-ovn-edge-zones`
ci/prow/e2e-gcp-ovn	`7cebbfa`	link	true	`/test e2e-gcp-ovn`

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 a review from jianli-wei December 18, 2024 23:27

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 18, 2024

openshift-ci bot requested review from jhixson74 and patrickdillon December 18, 2024 23:28

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Dec 19, 2024

barbacbd force-pushed the OCPBUGS-46488 branch 2 times, most recently from c7faacb to 29e3029 Compare December 19, 2024 14:59

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

rna-afk reviewed Dec 19, 2024

View reviewed changes

pkg/types/gcp/validation/machinepool.go Outdated Show resolved Hide resolved

barbacbd force-pushed the OCPBUGS-46488 branch from 29e3029 to 118a5fb Compare December 23, 2024 12:13

openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Jan 6, 2025

patrickdillon reviewed Jan 6, 2025

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2025

barbacbd force-pushed the OCPBUGS-46488 branch from 118a5fb to 8af8eae Compare January 7, 2025 12:27

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2025

sadasu reviewed Jan 7, 2025

View reviewed changes

barbacbd force-pushed the OCPBUGS-46488 branch from 8af8eae to 61475f1 Compare January 7, 2025 21:23

barbacbd requested review from patrickdillon and sadasu January 7, 2025 21:23

barbacbd force-pushed the OCPBUGS-46488 branch from 61475f1 to 00ffb37 Compare January 8, 2025 11:51

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

barbacbd force-pushed the OCPBUGS-46488 branch 2 times, most recently from 7a48d62 to 7ead689 Compare January 9, 2025 12:45

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2025

barbacbd added 2 commits January 13, 2025 09:25

OCPBUGS-46488: Validate Encryption Keys for GCP

b27805b

** Validate install config information for GCP KMS Encryption keys.

** Update vendor

7cebbfa

** Add KMS package for GCP

barbacbd force-pushed the OCPBUGS-46488 branch from 7ead689 to 7cebbfa Compare January 13, 2025 14:41

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-46488: Validate Encryption Keys for GCP #9328

OCPBUGS-46488: Validate Encryption Keys for GCP #9328

barbacbd commented Dec 18, 2024

openshift-ci-robot commented Dec 18, 2024

barbacbd commented Dec 18, 2024

jianli-wei commented Dec 19, 2024

jianli-wei commented Dec 19, 2024

openshift-ci-robot commented Dec 19, 2024

jianli-wei commented Dec 19, 2024

barbacbd commented Dec 19, 2024

barbacbd commented Dec 23, 2024

barbacbd commented Jan 6, 2025

patrickdillon Jan 6, 2025

patrickdillon Jan 6, 2025

sadasu Jan 7, 2025

sadasu commented Jan 8, 2025

openshift-ci bot commented Jan 8, 2025

openshift-ci bot commented Jan 13, 2025

	func Validate(client API, ic *types.InstallConfig) error {
	allErrs := field.ErrorList{}

	if err := validate.GCPClusterName(ic.ObjectMeta.Name); err != nil {
	allErrs = append(allErrs, field.Invalid(field.NewPath("clusterName"), ic.ObjectMeta.Name, err.Error()))
	}

	allErrs = append(allErrs, validateProject(client, ic, field.NewPath("platform").Child("gcp"))...)
	allErrs = append(allErrs, validateNetworkProject(client, ic, field.NewPath("platform").Child("gcp"))...)
	allErrs = append(allErrs, validateRegion(client, ic, field.NewPath("platform").Child("gcp"))...)
	allErrs = append(allErrs, validateZones(client, ic)...)
	allErrs = append(allErrs, validateNetworks(client, ic, field.NewPath("platform").Child("gcp"))...)
	allErrs = append(allErrs, validateInstanceTypes(client, ic)...)
	allErrs = append(allErrs, ValidateCredentialMode(client, ic)...)
	allErrs = append(allErrs, validatePreexistingServiceAccount(client, ic)...)
	allErrs = append(allErrs, validateServiceAccountPresent(client, ic)...)
	allErrs = append(allErrs, validateMarketplaceImages(client, ic)...)

	if err := validateUserTags(client, ic.Platform.GCP.ProjectID, ic.Platform.GCP.UserTags); err != nil {
	allErrs = append(allErrs, field.Invalid(field.NewPath("platform").Child("gcp").Child("userTags"), ic.Platform.GCP.UserTags, err.Error()))
	}

	return allErrs.ToAggregate()
	}

OCPBUGS-46488: Validate Encryption Keys for GCP #9328

Are you sure you want to change the base?

OCPBUGS-46488: Validate Encryption Keys for GCP #9328

Conversation

barbacbd commented Dec 18, 2024

openshift-ci-robot commented Dec 18, 2024

barbacbd commented Dec 18, 2024

jianli-wei commented Dec 19, 2024

jianli-wei commented Dec 19, 2024

openshift-ci-robot commented Dec 19, 2024

jianli-wei commented Dec 19, 2024

barbacbd commented Dec 19, 2024

barbacbd commented Dec 23, 2024

barbacbd commented Jan 6, 2025

patrickdillon Jan 6, 2025

Choose a reason for hiding this comment

patrickdillon Jan 6, 2025

Choose a reason for hiding this comment

sadasu Jan 7, 2025

Choose a reason for hiding this comment

sadasu commented Jan 8, 2025

openshift-ci bot commented Jan 8, 2025

openshift-ci bot commented Jan 13, 2025