CORS-4391: GCP: Encrypt Storage Registry and Bootstrap Ignition with KMS Key - #10734
CORS-4391: GCP: Encrypt Storage Registry and Bootstrap Ignition with KMS Key#10734patrickdillon wants to merge 7 commits into
Conversation
Add the imageregistry/v1 API types to vendor for use in generating the ImageRegistry CR manifest during GCP installations with customer-managed KMS encryption.
Add types and helpers for customer-managed KMS key configuration on GCS buckets used by Ignition bootstrap and the image registry. - Ignition/IgnitionStorage/Registry/RegistryStorage config types - GetStorageKMSKey helper to retrieve the storage KMS key from defaultMachinePlatform.osDisk.encryptionKey.kmsKey - FormatKMSKeyResourcePath to format KMSKeyReference as a full GCP resource path JIRA: CORS-4406
Add validation for customer-managed KMS keys used to encrypt GCS buckets for Ignition bootstrap and image registry. - Extract common validateKMSKeyReference helper for reuse across disk and storage encryption validation - Validate ignition and registry storage encryption keys - Handle permission failures gracefully with warnings - Use correct field paths for controlPlane/compute (top-level install-config fields, not nested under platform.gcp) JIRA: CORS-4407
Configure BucketEncryption.DefaultKMSKeyName when a customer-managed KMS key is set in the install-config, encrypting the bootstrap ignition GCS bucket from creation. JIRA: CORS-4408
Generate the ImageRegistry CR manifest with spec.storage.gcs.keyID when a customer-managed KMS key is configured for registry storage. Wire ImageRegistryConfig into the Openshift manifest pipeline. JIRA: CORS-4410
Automatically grant Google-managed and node service accounts permission to use customer-managed KMS keys: - Master SA: key-scoped permission for bootstrap ignition and registry buckets (not project-wide) - Compute Engine SA: permission for OS disk encryption - Cloud Storage SA: permission for bucket encryption GrantKMSKeyIAMPermission uses key-level IAM policies with exponential backoff for concurrent modifications. Supports PSC endpoints and uses CredentialOptions matching the existing KMS client pattern. JIRA: CORS-4411
Document customer-managed KMS encryption setup: - Required installer SA permissions for KMS IAM policy management - Automatic permission grants to Cloud Storage, Compute Engine, and master node service accounts - Note that defaultMachinePlatform.osDisk.encryptionKey.kmsKey is used for all encryption (OS disks, ignition, registry) JIRA: CORS-4413
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@patrickdillon: This pull request references CORS-4391 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. 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. |
📝 WalkthroughWalkthroughAdds customer-managed KMS support for GCP bootstrap storage, OS disks, and image registry storage. It validates KMS references, grants required service-account permissions, generates the image registry manifest, updates asset wiring, and documents the configuration. ChangesGCP KMS integration
Estimated code review effort: 4 (Complex) | ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test e2e-gcp-kms-encryption |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/asset/installconfig/gcp/validation.go (1)
933-967: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftFix the "safe to warn" check for an invalid
defaultMachinePlatformKMS key.
validatedComputeKeysis settrueas soon as one compute pool has an explicit, valid override. It does not require that every compute pool override the key. A compute pool that omitsosDisk.encryptionKey.kmsKeyinheritsdefaultMachinePlatform's key at provisioning time (see theSet()merge inPreProvision). So if pool A overrides with a valid key and pool B does not override at all,validatedComputeKeysistrue,allErrsstays empty, and an invaliddefaultMachinePlatformkey gets downgraded to alogrus.Warneven though pool B will try to use that same invalid key during infrastructure provisioning. This turns a config-time validation failure into a much more expensive provisioning-time failure.Track whether every compute pool that relies on the default (i.e., has no explicit override) exists, not just whether one override succeeded.
Separately, the control-plane check returns immediately on error (
return append(allErrs, err)at line 940), while the compute loop aggregates errors and keeps going. Consider aggregating the control-plane error the same way, so a single validation run reports all invalid KMS references instead of only the first one.🐛 Proposed fix for the compute-key tracking
- validatedComputeKeys := false + allComputeOverridden := true for idx, mp := range ic.Compute { - if mp.Platform.GCP != nil && mp.Platform.GCP.OSDisk.EncryptionKey != nil && mp.Platform.GCP.OSDisk.EncryptionKey.KMSKey != nil { - if err := validateKMSKeyReference(client, mp.Platform.GCP.OSDisk.EncryptionKey.KMSKey, ic.GCP.ProjectID, field.NewPath("compute").Index(idx).Child("platform", "gcp", "osDisk", "encryptionKey", "kmsKey")); err != nil { - allErrs = append(allErrs, err) - } else { - validatedComputeKeys = true - } + if mp.Platform.GCP == nil || mp.Platform.GCP.OSDisk.EncryptionKey == nil || mp.Platform.GCP.OSDisk.EncryptionKey.KMSKey == nil { + // This pool has no explicit override and inherits defaultMachinePlatform's key. + allComputeOverridden = false + continue + } + if err := validateKMSKeyReference(client, mp.Platform.GCP.OSDisk.EncryptionKey.KMSKey, ic.GCP.ProjectID, field.NewPath("compute").Index(idx).Child("platform", "gcp", "osDisk", "encryptionKey", "kmsKey")); err != nil { + allErrs = append(allErrs, err) } } defaultMp := ic.GCP.DefaultMachinePlatform if defaultMp != nil && defaultMp.OSDisk.EncryptionKey != nil && defaultMp.OSDisk.EncryptionKey.KMSKey != nil { if err := validateKMSKeyReference(client, defaultMp.OSDisk.EncryptionKey.KMSKey, ic.GCP.ProjectID, fieldPath.Child("defaultMachinePlatform").Child("osDisk").Child("encryptionKey").Child("kmsKey")); err != nil { - if validatedControlPlaneKey && (validatedComputeKeys && len(allErrs) == 0) { + if validatedControlPlaneKey && allComputeOverridden && len(allErrs) == 0 { logrus.Warn("defaultMachinePlatform.osDisk.encryptionKey.kmsKey is not valid, but compute and control plane keys are valid") } else { return append(allErrs, err) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/asset/installconfig/gcp/validation.go` around lines 933 - 967, Update validatePlatformKMSKeys to track whether every compute pool has an explicit valid KMS override, rather than setting validatedComputeKeys true after only one success; treat any pool without an override as relying on defaultMachinePlatform and prevent the warning path when such a pool exists. Also aggregate control-plane validation errors like the compute loop instead of returning immediately, while preserving the existing error paths and warning behavior otherwise.pkg/infrastructure/gcp/clusterapi/clusterapi.go (1)
61-102: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winControl-plane KMS grants are skipped when a pre-created service account is used.
The Compute Engine SA OS-disk-key grant (Lines 82-91) and the master SA storage-key grant (Lines 74-80) sit inside the
elsebranch, so they run only whenPreProvisioncreates a new master service account. WhencontrolPlaneMpool.ServiceAccountis set to a pre-created service account, both grants are skipped.The Compute Engine service account is a Google-managed service agent, independent of the control-plane node's own service account. It always needs permission on the OS disk KMS key to perform disk encryption, regardless of whether the node's service account is pre-created or newly created. The compute-node loop (Lines 124-154) already grants this permission unconditionally, outside its equivalent
if createSA {...}guard. The control-plane path should follow the same pattern.With the current code, a pre-created control-plane service account combined with an OS disk KMS key configuration will fail at install time or at first disk operation due to missing IAM permission on the key.
docs/user/gcp/iam.md (Lines 54-64) states the Compute Engine and master node grants happen automatically in
PreProvision; this statement does not hold for the pre-created service account case as currently coded.🔧 Proposed fix: move the Compute Engine SA OS-disk-key grant outside the `else` branch
if controlPlaneMpool.ServiceAccount != "" { logrus.Debugf("Using pre-created ServiceAccount for control plane nodes") } else { // Create ServiceAccount for control plane nodes logrus.Debugf("Creating ServiceAccount for control plane nodes") masterSA, err := CreateServiceAccount(ctx, in.InfraID, projectID, "master", in.InstallConfig.Config.GCP.Endpoint) if err != nil { return fmt.Errorf("failed to create master serviceAccount: %w", err) } if err = AddServiceAccountRoles(ctx, projectID, masterSA, GetMasterRoles(), in.InstallConfig.Config.GCP.Endpoint); err != nil { return fmt.Errorf("failed to add master roles: %w", err) } - - // Grant master SA KMS permissions scoped to the specific storage key. - // Master nodes need these for bootstrap ignition bucket and registry operator. - if storageKMSKey := gcptypes.GetStorageKMSKey(in.InstallConfig.Config.GCP); storageKMSKey != nil { - if err = GrantKMSKeyIAMPermission(ctx, storageKMSKey, projectID, masterSA, "roles/cloudkms.cryptoKeyEncrypterDecrypter", in.InstallConfig.Config.GCP.Endpoint); err != nil { - return fmt.Errorf("failed to grant master SA KMS permission: %w", err) - } - } - - // Grant Compute Engine service account permission to use OS disk KMS key if configured. - if controlPlaneMpool.OSDisk.EncryptionKey != nil && controlPlaneMpool.OSDisk.EncryptionKey.KMSKey != nil { - computeEngineSA, err := GetComputeEngineServiceAccount(ctx, projectID, in.InstallConfig.Config.GCP.Endpoint) - if err != nil { - return fmt.Errorf("failed to get Compute Engine service account for project %s: %w", projectID, err) - } - if err = GrantKMSKeyIAMPermission(ctx, controlPlaneMpool.OSDisk.EncryptionKey.KMSKey, projectID, computeEngineSA, "roles/cloudkms.cryptoKeyEncrypterDecrypter", in.InstallConfig.Config.GCP.Endpoint); err != nil { - return fmt.Errorf("failed to grant Compute Engine SA KMS permission for control plane: %w", err) - } - } + controlPlaneMpool.ServiceAccount = masterSA + + // Grant master SA KMS permissions scoped to the specific storage key. + // Master nodes need these for bootstrap ignition bucket and registry operator. + if storageKMSKey := gcptypes.GetStorageKMSKey(in.InstallConfig.Config.GCP); storageKMSKey != nil { + if err = GrantKMSKeyIAMPermission(ctx, storageKMSKey, projectID, masterSA, "roles/cloudkms.cryptoKeyEncrypterDecrypter", in.InstallConfig.Config.GCP.Endpoint); err != nil { + return fmt.Errorf("failed to grant master SA KMS permission: %w", err) + } + } // Add additional roles for shared VPC if len(in.InstallConfig.Config.Platform.GCP.NetworkProjectID) > 0 { projID := in.InstallConfig.Config.Platform.GCP.NetworkProjectID // Add roles needed for creating firewalls roles := GetSharedVPCRoles() if err = AddServiceAccountRoles(ctx, projID, masterSA, roles, in.InstallConfig.Config.GCP.Endpoint); err != nil { return fmt.Errorf("failed to add roles for shared VPC: %w", err) } } } + + // Grant Compute Engine service account permission to use OS disk KMS key if configured. + // This must run regardless of whether the control-plane service account is pre-created, + // since the Compute Engine service agent identity is independent of the node's own SA. + if controlPlaneMpool.OSDisk.EncryptionKey != nil && controlPlaneMpool.OSDisk.EncryptionKey.KMSKey != nil { + computeEngineSA, err := GetComputeEngineServiceAccount(ctx, projectID, in.InstallConfig.Config.GCP.Endpoint) + if err != nil { + return fmt.Errorf("failed to get Compute Engine service account for project %s: %w", projectID, err) + } + if err := GrantKMSKeyIAMPermission(ctx, controlPlaneMpool.OSDisk.EncryptionKey.KMSKey, projectID, computeEngineSA, "roles/cloudkms.cryptoKeyEncrypterDecrypter", in.InstallConfig.Config.GCP.Endpoint); err != nil { + return fmt.Errorf("failed to grant Compute Engine SA KMS permission for control plane: %w", err) + } + }This diff is illustrative. Confirm
controlPlaneMpool.ServiceAccountis safe to overwrite locally, and decide whether the master-SA storage-key grant should also move outside theelsebranch (matching the "automatic" language in docs/user/gcp/iam.md), or whether that grant is intentionally scoped to newly created service accounts only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/infrastructure/gcp/clusterapi/clusterapi.go` around lines 61 - 102, Move the Compute Engine service-account OS-disk KMS grant out of the service-account creation else branch so it executes whenever controlPlaneMpool.OSDisk.EncryptionKey.KMSKey is configured, including pre-created service-account flows. Preserve the existing GetComputeEngineServiceAccount and GrantKMSKeyIAMPermission error handling. Also ensure the master SA storage-key grant in the control-plane provisioning path is applied for pre-created service accounts when required by the documented automatic-grant behavior, without overwriting the configured service-account value.
🧹 Nitpick comments (2)
pkg/asset/manifests/gcp/imageregistry_test.go (1)
153-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the replica-count branch in
Generate().None of these tests call
ImageRegistryConfig.Generate()directly.TestImageRegistryConfigGenerationhardcodes"replicas": 2in a hand-built map, so the single-node branch (replicas := 1whencontrolPlaneReplicas == 1) is never exercised. Add a test that callsGenerate()withinstallConfig.Config.ControlPlane.Replicasset to1and to3, and asserts the resultingConfigFilecontainsreplicas: 1andreplicas: 2respectively.As per path instructions, test files should "Verify edge cases are covered, especially for validation and defaulting logic."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/asset/manifests/gcp/imageregistry_test.go` around lines 153 - 260, Extend TestImageRegistryConfigGeneration with direct ImageRegistryConfig.Generate() cases using installConfig.Config.ControlPlane.Replicas set to 1 and 3. Assert the generated ConfigFile contains replicas: 1 for a single control-plane replica and replicas: 2 for three replicas, covering both branches instead of relying on the hardcoded hand-built YAML map.Source: Path instructions
pkg/infrastructure/gcp/clusterapi/iam.go (1)
239-391: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd unit tests for the new GCP KMS IAM provisioning paths. Cover
getProjectNumber, both service-account helpers,GrantKMSKeyIAMPermission,addMemberToKMSPolicy, and the KMS branches inProvider.PreProvision, including pre-created service accounts and duplicate keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/infrastructure/gcp/clusterapi/iam.go` around lines 239 - 391, Add unit tests covering the new GCP KMS IAM provisioning paths in pkg/infrastructure/gcp/clusterapi/iam.go (lines 239-391): test getProjectNumber, GetCloudStorageServiceAccount, GetComputeEngineServiceAccount, GrantKMSKeyIAMPermission, and addMemberToKMSPolicy, including success, error, duplicate-member, and retry behavior. Also test the KMS branches in Provider.PreProvision in pkg/infrastructure/gcp/clusterapi/clusterapi.go (lines 49-168), covering pre-created service accounts and duplicate keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/user/gcp/customization.md`:
- Line 26: Add a Go deprecation directive comment immediately above the
KMSKeyServiceAccount field, using the exact `// Deprecated:` prefix and matching
the deprecation guidance already documented in customization.md, so generated
API documentation marks this field as deprecated.
In `@pkg/asset/installconfig/gcp/validation.go`:
- Around line 911-929: Add a context.Context parameter to the Validate entry
point and thread it through validatePlatformKMSKeys into
validateKMSKeyReference. Replace context.TODO() in validateKMSKeyReference with
the propagated context when calling client.GetKeyRing, and update all affected
call sites to pass their existing caller context.
In `@pkg/infrastructure/gcp/clusterapi/iam.go`:
- Around line 281-361: Update GrantKMSKeyIAMPermission to append the Cloud KMS
endpoint option to opts when ShouldUseEndpointForInstaller(endpoint) is true,
using CreateEndpointOption with the cloudkms service constant. Define the
cloudkms constant in services.go, and ensure the resulting opts are passed to
kms.NewKeyManagementClient while preserving default endpoint behavior otherwise.
---
Outside diff comments:
In `@pkg/asset/installconfig/gcp/validation.go`:
- Around line 933-967: Update validatePlatformKMSKeys to track whether every
compute pool has an explicit valid KMS override, rather than setting
validatedComputeKeys true after only one success; treat any pool without an
override as relying on defaultMachinePlatform and prevent the warning path when
such a pool exists. Also aggregate control-plane validation errors like the
compute loop instead of returning immediately, while preserving the existing
error paths and warning behavior otherwise.
In `@pkg/infrastructure/gcp/clusterapi/clusterapi.go`:
- Around line 61-102: Move the Compute Engine service-account OS-disk KMS grant
out of the service-account creation else branch so it executes whenever
controlPlaneMpool.OSDisk.EncryptionKey.KMSKey is configured, including
pre-created service-account flows. Preserve the existing
GetComputeEngineServiceAccount and GrantKMSKeyIAMPermission error handling. Also
ensure the master SA storage-key grant in the control-plane provisioning path is
applied for pre-created service accounts when required by the documented
automatic-grant behavior, without overwriting the configured service-account
value.
---
Nitpick comments:
In `@pkg/asset/manifests/gcp/imageregistry_test.go`:
- Around line 153-260: Extend TestImageRegistryConfigGeneration with direct
ImageRegistryConfig.Generate() cases using
installConfig.Config.ControlPlane.Replicas set to 1 and 3. Assert the generated
ConfigFile contains replicas: 1 for a single control-plane replica and replicas:
2 for three replicas, covering both branches instead of relying on the hardcoded
hand-built YAML map.
In `@pkg/infrastructure/gcp/clusterapi/iam.go`:
- Around line 239-391: Add unit tests covering the new GCP KMS IAM provisioning
paths in pkg/infrastructure/gcp/clusterapi/iam.go (lines 239-391): test
getProjectNumber, GetCloudStorageServiceAccount, GetComputeEngineServiceAccount,
GrantKMSKeyIAMPermission, and addMemberToKMSPolicy, including success, error,
duplicate-member, and retry behavior. Also test the KMS branches in
Provider.PreProvision in pkg/infrastructure/gcp/clusterapi/clusterapi.go (lines
49-168), covering pre-created service accounts and duplicate keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 94aae9ee-8a1b-4417-a568-5b2624b53620
⛔ Files ignored due to path filters (10)
vendor/github.com/openshift/api/imageregistry/v1/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/imageregistry/v1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/imageregistry/v1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/imageregistry/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/imageregistry/v1/types_imagepruner.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/imageregistry/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/github.com/openshift/api/imageregistry/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/github.com/openshift/api/imageregistry/v1/zz_generated.model_name.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/github.com/openshift/api/imageregistry/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (13)
docs/user/gcp/customization.mddocs/user/gcp/iam.mdgo.modpkg/asset/ignition/bootstrap/gcp/storage.gopkg/asset/ignition/bootstrap/gcp/storage_test.gopkg/asset/installconfig/gcp/validation.gopkg/asset/installconfig/gcp/validation_test.gopkg/asset/manifests/gcp/imageregistry.gopkg/asset/manifests/gcp/imageregistry_test.gopkg/asset/manifests/openshift.gopkg/infrastructure/gcp/clusterapi/clusterapi.gopkg/infrastructure/gcp/clusterapi/iam.gopkg/types/gcp/platform.go
| * `location` (string): The GCP location in which the Key Ring exists. | ||
| * `projectID` (optional string): The ID of the Project in which the KMS Key Ring exists. Defaults to the VM ProjectID if not set. | ||
| * `kmsKeyServiceAccount` (optional string): The service account being used for the encryption request for the given KMS key. If absent, the [Compute Engine default service account][default-service-account] is used. | ||
| * `kmsKeyServiceAccount` (optional string): **Deprecated**. This field is no longer needed as the installer automatically grants the Compute Engine service account permission to use the KMS key. The Compute Engine service account (`service-{PROJECT_NUMBER}@compute-system.iam.gserviceaccount.com`), not the instance service account, performs OS disk encryption. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check kmsKeyServiceAccount field definition and deprecation comment.
rg -n -B3 'kmsKeyServiceAccount' --type=goRepository: openshift/installer
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all repository references ---'
rg -n -C 4 'kmsKeyServiceAccount|KMSKeyServiceAccount|KmsKeyServiceAccount' . || true
printf '%s\n' '--- candidate GCP type files ---'
git ls-files | rg '(^|/)(pkg/types/gcp|types/gcp)/|gcp.*\.(go|yaml|json)$' | head -200Repository: openshift/installer
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
path = Path("pkg/types/gcp/machinepools.go")
text = path.read_text()
match = re.search(
r"(?P<comments>(?:(?:\s*//.*\n)+))\s*"
r"KMSKeyServiceAccount\s+string\s+`json:\"kmsKeyServiceAccount,omitempty\"`",
text,
)
if not match:
raise SystemExit("field not found")
comments = match.group("comments")
print("field:", "KMSKeyServiceAccount string `json:\"kmsKeyServiceAccount,omitempty\"`")
print("doc_comments:")
print(comments.rstrip())
print("has_deprecated_directive:", bool(re.search(r"(?m)^\s*//\s*Deprecated:", comments)))
PYRepository: openshift/installer
Length of output: 627
Add a // Deprecated: comment to KMSKeyServiceAccount. The Go type currently has no deprecation directive, so generated API documentation is inconsistent with docs/user/gcp/customization.md.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/user/gcp/customization.md` at line 26, Add a Go deprecation directive
comment immediately above the KMSKeyServiceAccount field, using the exact `//
Deprecated:` prefix and matching the deprecation guidance already documented in
customization.md, so generated API documentation marks this field as deprecated.
| // validateKMSKeyReference validates a KMS key reference by checking if the key ring exists. | ||
| // Returns a field.Error on failure, or nil on success. | ||
| // The defaultProjectID is used if the kmsKeyRef.ProjectID is empty. | ||
| func validateKMSKeyReference(client API, kmsKeyRef *gcp.KMSKeyReference, defaultProjectID string, fldPath *field.Path) *field.Error { | ||
| if kmsKeyRef == nil { | ||
| return nil | ||
| } | ||
|
|
||
| // Create a copy with the project ID filled in if not specified | ||
| kmsKeyRefCopy := *kmsKeyRef | ||
| if kmsKeyRefCopy.ProjectID == "" { | ||
| kmsKeyRefCopy.ProjectID = defaultProjectID | ||
| } | ||
|
|
||
| if _, err := client.GetKeyRing(context.TODO(), &kmsKeyRefCopy); err != nil { | ||
| return field.Invalid(fldPath.Child("keyRing"), kmsKeyRef.KeyRing, err.Error()) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether GCP install-config validation already threads a context.Context.
rg -n -B3 -A15 'func Validate(ForProvisioning)?\(' pkg/asset/installconfig/gcp/validation.go
rg -n 'validatePlatformKMSKeys\(' pkg/asset/installconfig/gcp/validation.goRepository: openshift/installer
Length of output: 2442
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- validation entry points and KMS call sites ---'
sed -n '1,90p' pkg/asset/installconfig/gcp/validation.go
sed -n '900,980p' pkg/asset/installconfig/gcp/validation.go
printf '%s\n' '--- GetKeyRing declarations and callers ---'
rg -n -B4 -A12 'GetKeyRing|validateKMSKeyReference|GrantKMSKeyIAMPermission' --glob '*.go' .
printf '%s\n' '--- KMS validation tests ---'
rg -n -B5 -A15 'KMS|kmsKey|validatePlatformKMSKeys' pkg/asset/installconfig/gcp --glob '*_test.go'Repository: openshift/installer
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
p = Path("pkg/asset/installconfig/gcp/validation.go")
lines = p.read_text().splitlines()
for start, end in [(45, 80), (925, 975)]:
print(f"--- {p}:{start}-{end} ---")
for n in range(start, min(end, len(lines)) + 1):
print(f"{n}:{lines[n-1]}")
PY
printf '%s\n' '--- exact context usage in GCP validation ---'
rg -n 'context\.(TODO|Background|With)|context\.Context|Validate\(' pkg/asset/installconfig/gcp/validation.go
printf '%s\n' '--- API interface and implementation candidates ---'
rg -n -B5 -A15 'type API interface|func .*GetKeyRing|GetKeyRing\(' --glob '*.go' .Repository: openshift/installer
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- GCP KMS client implementation ---'
sed -n '770,835p' pkg/asset/installconfig/gcp/client.go
printf '%s\n' '--- IAM timeout pattern ---'
rg -n -B8 -A18 'GrantKMSKeyIAMPermission|WithTimeout' pkg/infrastructure/gcp/clusterapi/iam.go
printf '%s\n' '--- Validate call sites ---'
rg -n -B3 -A5 '\bgcp\.Validate\(|installconfig/gcp.*Validate|Validate\(.*gcp' --glob '*.go' --glob '!vendor/**' --glob '!cluster-api/**' .
printf '%s\n' '--- validation tests and direct helper calls ---'
rg -n -B3 -A5 'Validate\(|validatePlatformKMSKeys|validateKMSKeyReference' pkg/asset/installconfig/gcp/*_test.goRepository: openshift/installer
Length of output: 8110
Propagate cancellation to KMS validation.
Validate does not currently receive a context.Context, so add it at the entry point and pass it through validatePlatformKMSKeys to validateKMSKeyReference. GetKeyRing has an internal timeout, but context.TODO() prevents callers from cancelling the KMS request.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/asset/installconfig/gcp/validation.go` around lines 911 - 929, Add a
context.Context parameter to the Validate entry point and thread it through
validatePlatformKMSKeys into validateKMSKeyReference. Replace context.TODO() in
validateKMSKeyReference with the propagated context when calling
client.GetKeyRing, and update all affected call sites to pass their existing
caller context.
Source: Path instructions
| // GrantKMSKeyIAMPermission grants a service account permission to use a KMS key by updating | ||
| // the key's IAM policy. This is required to allow Google-managed service accounts (like Cloud Storage | ||
| // and Compute Engine) to encrypt/decrypt data using customer-managed KMS keys. | ||
| // | ||
| // The function uses a read-modify-write pattern with exponential backoff to handle concurrent | ||
| // policy updates. It requires the installer service account to have cloudkms.cryptoKeys.getIamPolicy | ||
| // and cloudkms.cryptoKeys.setIamPolicy permissions. | ||
| func GrantKMSKeyIAMPermission(ctx context.Context, kmsKey *gcptypes.KMSKeyReference, projectID, serviceAccountEmail, role string, endpoint *gcptypes.PSCEndpoint) error { | ||
| ctx, cancel := context.WithTimeout(ctx, time.Minute*2) | ||
| defer cancel() | ||
|
|
||
| ssn, err := gcp.GetSession(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get session: %w", err) | ||
| } | ||
|
|
||
| opts, err := gcp.CredentialOptions(ssn) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get credential options: %w", err) | ||
| } | ||
|
|
||
| kmsClient, err := kms.NewKeyManagementClient(ctx, opts...) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create KMS client: %w", err) | ||
| } | ||
| defer kmsClient.Close() | ||
|
|
||
| resourceName := gcptypes.FormatKMSKeyResourcePath(kmsKey, projectID) | ||
| member := fmt.Sprintf("serviceAccount:%s", serviceAccountEmail) | ||
|
|
||
| // Use exponential backoff for IAM policy updates to handle concurrent modifications | ||
| backoff := wait.Backoff{ | ||
| Duration: 2 * time.Second, | ||
| Factor: 2.0, | ||
| Jitter: 1.0, | ||
| Steps: retryCount, | ||
| } | ||
|
|
||
| var lastErr error | ||
| if waitErr := wait.ExponentialBackoffWithContext(ctx, backoff, func(ctx context.Context) (bool, error) { | ||
| // Get current IAM policy | ||
| req := &iampb.GetIamPolicyRequest{ | ||
| Resource: resourceName, | ||
| } | ||
| policy, err := kmsClient.GetIamPolicy(ctx, req) | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to get IAM policy for KMS key %s: %w", resourceName, err) | ||
| } | ||
|
|
||
| // Add the member to the role | ||
| if err := addMemberToKMSPolicy(policy, role, member); err != nil { | ||
| return false, fmt.Errorf("failed to add member to KMS policy: %w", err) | ||
| } | ||
|
|
||
| // Set the updated IAM policy | ||
| setReq := &iampb.SetIamPolicyRequest{ | ||
| Resource: resourceName, | ||
| Policy: policy, | ||
| } | ||
| _, err = kmsClient.SetIamPolicy(ctx, setReq) | ||
| if err != nil { | ||
| // Check for concurrent modification errors (gRPC status codes) | ||
| if st, ok := status.FromError(err); ok && (st.Code() == codes.Aborted || st.Code() == codes.FailedPrecondition) { | ||
| lastErr = err | ||
| logrus.Debugf("Concurrent KMS IAM policy changes, restarting read/modify/write") | ||
| return false, nil | ||
| } | ||
| return false, fmt.Errorf("failed to set IAM policy for KMS key %s: %w", resourceName, err) | ||
| } | ||
|
|
||
| logrus.Debugf("Granted %s to %s on KMS key %s", role, serviceAccountEmail, resourceName) | ||
| return true, nil | ||
| }); waitErr != nil { | ||
| if wait.Interrupted(waitErr) { | ||
| return fmt.Errorf("failed to set KMS IAM policy for key %s after retries: %w", resourceName, lastErr) | ||
| } | ||
| return waitErr | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the GCP service-name constant used for KMS endpoint overrides.
rg -n 'ServiceNameGCP' --type=goRepository: openshift/installer
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate symbols ---'
rg -n -C 4 'ShouldUseEndpointForInstaller|CreateEndpointOption|ServiceName|KMS|GrantKMSKeyIAMPermission|getProjectNumber|CreateServiceAccount' pkg/infrastructure/gcp/clusterapi pkg/infrastructure/gcp --glob '*.go' | head -240
printf '%s\n' '--- iam.go context ---'
sed -n '1,390p' pkg/infrastructure/gcp/clusterapi/iam.goRepository: openshift/installer
Length of output: 35210
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- service-name definitions and uses ---'
rg -n -C 3 'ServiceNameGCP[A-Za-z0-9_]*' --glob '*.go' .
printf '%s\n' '--- endpoint option implementation ---'
rg -n -C 8 'func CreateEndpointOption|CreateEndpointOption' pkg --glob '*.go'
printf '%s\n' '--- KMS client construction and endpoint tests ---'
rg -n -C 5 'NewKeyManagementClient|ServiceNameGCPKMS|kms.*endpoint|endpoint.*kms|PSCEndpoint' pkg/infrastructure/gcp --glob '*_test.go' --glob '*.go' | head -260Repository: openshift/installer
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- GrantKMSKeyIAMPermission call sites ---'
rg -n -C 5 'GrantKMSKeyIAMPermission' --glob '*.go' .
printf '%s\n' '--- focused service and test references ---'
rg -n 'ServiceNameGCP|CreateServiceEndpoint|CreateEndpointOption|NewKeyManagementClient|GrantKMSKeyIAMPermission' pkg/asset/installconfig/gcp pkg/infrastructure/gcp/clusterapi --glob '*.go' --glob '*_test.go' | grep -E 'services.go|iam.go|_test.go|NewKeyManagementClient|GrantKMS|ServiceNameGCP' | head -180
printf '%s\n' '--- KMS module API metadata ---'
rg -n 'cloud.google.com/go/kms|WithEndpoint' go.mod go.sum vendor 2>/dev/null | head -100Repository: openshift/installer
Length of output: 29172
🌐 Web query:
Google Cloud Private Service Connect published services Cloud KMS API endpoint service name cloudkms
💡 Result:
To access the Cloud Key Management Service (KMS) via Private Service Connect (PSC), you do not use a single published service name like a private producer service. Instead, you access it as a Google API using a regional or global endpoint [1][2][3]. The Cloud KMS API service identifier is cloudkms.googleapis.com [4][5][6]. When using Private Service Connect to access regional Google APIs, you target regional endpoints that follow this pattern: cloudkms.REGION.rep.googleapis.com [7] For example, a regional endpoint for Cloud KMS in the us-central1 region would be: cloudkms.us-central1.rep.googleapis.com [7] To configure this, you create a regional Private Service Connect endpoint and direct traffic to these regional API targets [8][9]. You can also use Private Service Connect bundles for more general Google API access, but for specific regional requirements, the regional endpoint pattern is the standard approach [8][1][9].
Citations:
- 1: https://docs.cloud.google.com/vpc/docs/configure-private-service-connect-apis
- 2: https://docs.cloud.google.com/vpc/docs/private-service-connect
- 3: https://cloud.google.com/vpc/docs/private-access-options
- 4: https://cloud.google.com/kms/docs/reference/rest
- 5: https://docs.cloud.google.com/kms/docs/reference/rpc
- 6: https://docs.cloud.google.com/vpc-service-controls/docs/supported-products
- 7: https://cloud.google.com/vpc/docs/regional-service-endpoints
- 8: https://docs.cloud.google.com/vpc/docs/access-regional-google-apis-endpoints
- 9: https://cloud.google.com/vpc/docs/access-regional-google-apis-endpoints
🌐 Web query:
site:cloud.google.com/vpc/docs "cloudkms-" "p.googleapis.com" Private Service Connect
💡 Result:
The domain name "p.googleapis.com" is associated with Private Service Connect (PSC) endpoints used to access Google APIs privately [1]. When you create a Private Service Connect endpoint to access Google APIs, you often configure DNS records to route traffic to these endpoints [2][1], and internal hostnames for these services follow the pattern of "{service-name}.p.googleapis.com" [1]. For Google Cloud Key Management Service (Cloud KMS), which is represented by the "cloudkms.googleapis.com" API, Private Service Connect supports private access through specific configurations [3][4]. Key points regarding Cloud KMS and Private Service Connect: 1. Regional Service Endpoints: You can use Private Service Connect regional endpoints and Private Service Connect backends to connect to regional service endpoints for Cloud KMS [3]. For example, regional endpoints exist for various regions, such as "cloudkms.us-central1.rep.googleapis.com" [3]. 2. Access Methods: - Private Service Connect Endpoints: You can create a regional PSC endpoint to send requests to a specific regional API endpoint [2]. - Private Service Connect Backends: You can configure an internal Application Load Balancer with a PSC network endpoint group (NEG) backend to access regional Google APIs, including Cloud KMS [5]. 3. DNS Configuration: When using PSC endpoints, you must configure private DNS entries so that your clients correctly resolve the API hostnames to the internal IP address of your Private Service Connect endpoint [2]. By using these configurations, traffic to Cloud KMS stays entirely within the Google Cloud network rather than traversing the public internet [4].
Citations:
- 1: https://cloud.google.com/vpc/docs/about-accessing-google-apis-endpoints
- 2: https://cloud.google.com/vpc/docs/access-regional-google-apis-endpoints
- 3: https://cloud.google.com/vpc/docs/regional-service-endpoints
- 4: https://docs.cloud.google.com/vpc/docs/private-service-connect
- 5: https://cloud.google.com/vpc/docs/configure-private-service-connect-controls
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- PSC endpoint type and service URL rules ---'
rg -n -C 8 'type PSCEndpoint|PSCEndpoint struct|ShouldUseEndpointForInstaller|p.googleapis.com|CreateServiceEndpoint' pkg/types/gcp pkg/asset/installconfig/gcp --glob '*.go'
printf '%s\n' '--- KMS client endpoint option path ---'
sed -n '740,790p' pkg/asset/installconfig/gcp/client.go
sed -n '1,90p' pkg/asset/installconfig/gcp/services.go
sed -n '440,510p' vendor/cloud.google.com/go/kms/apiv1/key_management_client.goRepository: openshift/installer
Length of output: 14060
🌐 Web query:
site:github.com/openshift/installer "ServiceNameGCPKMS"
💡 Result:
The term "ServiceNameGCPKMS" is not a standard, publicly documented identifier used by Google Cloud Platform (GCP) official APIs or services [1][2][3]. Based on technical context, this string appears to be a variable name, constant, or internal identifier used within the OpenShift Installer codebase (a tool for deploying OpenShift on various platforms, including GCP) to refer to the GCP Key Management Service (KMS) or related client configurations [1][2][3]. In the OpenShift installer source code, developers manage GCP service interactions by defining constants for various Google Cloud APIs (such as Compute, IAM, and Cloud Resource Manager) [2]. While the installer utilizes the official Google Cloud KMS Go client library (cloud.google.com/go/kms/apiv1) to interact with GCP KMS for disk encryption and other security features, "ServiceNameGCPKMS" is not a recognized public service name within the broader Google Cloud ecosystem [1][2][3]. If you encountered this in logs or configuration files, it is likely specific to the internal implementation of the software or infrastructure-as-code tool you are using [1][4]. For official GCP KMS service information, you should refer to the Google Cloud Key Management Service documentation, which does not utilize this specific naming convention [3].
Citations:
- 1: https://github.com/openshift/installer/blob/master/pkg/asset/installconfig/gcp/client.go
- 2: 443742a
- 3: 89b3733
- 4: OCPBUGS-52203: Find GCP KMS keys #9597
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- repository history references without full history output ---'
git log --all --oneline -- pkg/asset/installconfig/gcp/services.go pkg/infrastructure/gcp/clusterapi/iam.go | head -40
printf '%s\n' '--- KMS generated client defaults and endpoint override handling ---'
rg -n -C 6 'defaultEndpoint|WithEndpoint|NewKeyManagementClient|KeyManagementClient' vendor/cloud.google.com/go/kms/apiv1/key_management_client.go vendor/cloud.google.com/go/kms/apiv1/auxiliary.go | head -180Repository: openshift/installer
Length of output: 20844
Honor the PSC endpoint for Cloud KMS. When ShouldUseEndpointForInstaller(endpoint) is true, append a Cloud KMS endpoint option before creating the client. Add the cloudkms service constant in pkg/asset/installconfig/gcp/services.go and use it with CreateEndpointOption; otherwise these IAM calls use the default endpoint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/infrastructure/gcp/clusterapi/iam.go` around lines 281 - 361, Update
GrantKMSKeyIAMPermission to append the Cloud KMS endpoint option to opts when
ShouldUseEndpointForInstaller(endpoint) is true, using CreateEndpointOption with
the cloudkms service constant. Define the cloudkms constant in services.go, and
ensure the resulting opts are passed to kms.NewKeyManagementClient while
preserving default endpoint behavior otherwise.
|
@patrickdillon: 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. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
3 participants
Reworks #10553
/cc @rochacbruno
Summary by CodeRabbit
New Features
Bug Fixes
Documentation