GCP-367: Implement GCP Cloud Controller Manager component#7677
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@cristianoveiga: This pull request references GCP-367 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 story to target the "4.22.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. |
|
Skipping CI for Draft Pull Request. |
|
@cristianoveiga: This pull request references GCP-367 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 story to target the "4.22.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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds GCP cloud-controller-manager support: new component registration, component implementation and config adaptation, manifests (ConfigMap, Deployment), unit tests and fixtures, workload-identity credential wiring/validation for GCP, and CLI wiring for CloudController service account. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
area/control-plane-operator
cristianoveiga
force-pushed
the
GCP-367-implement-gcp-cloud-controller-manager
branch
from
7a44306 to
dadf4e8
Compare
|
@cristianoveiga: This pull request references GCP-367 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 story to target the "4.22.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. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp.go`:
- Around line 343-347: ReconcileCredentials is syncing a
cloud-controller-manager secret using
hcluster.Spec.Platform.GCP.WorkloadIdentity.ServiceAccountsEmails.CloudController
even though validateWorkloadIdentityConfiguration does not require that field;
add an explicit validation in validateWorkloadIdentityConfiguration to return an
error when wif.ServiceAccountsEmails.CloudController == "" so the
ValidGCPWorkloadIdentity condition fails early and ReconcileCredentials won't
proceed to sync a missing CloudController secret. Update
validateWorkloadIdentityConfiguration to check
ServiceAccountsEmails.CloudController, return a descriptive error (e.g., "cloud
controller service account email is required"), and ensure callers handle that
error to prevent later failure in ReconcileCredentials.
hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp.go
Show resolved
Hide resolved
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp.go (1)
343-351:⚠️ Potential issue | 🟠 MajorAvoid map key collisions when multiple roles share the same GSA.
The map-based iteration silently collapses duplicate email entries. If a user configures the same service account email for multiple roles, only one secret will be synced and the others will be skipped without error. Use an ordered slice of pairs instead to ensure all role credentials are reconciled.
🔧 Proposed fix
- for email, secret := range map[string]*corev1.Secret{ - hcluster.Spec.Platform.GCP.WorkloadIdentity.ServiceAccountsEmails.NodePool: NodePoolManagementCredsSecret(controlPlaneNamespace), - hcluster.Spec.Platform.GCP.WorkloadIdentity.ServiceAccountsEmails.ControlPlane: ControlPlaneOperatorCredsSecret(controlPlaneNamespace), - hcluster.Spec.Platform.GCP.WorkloadIdentity.ServiceAccountsEmails.CloudController: CloudControllerCredsSecret(controlPlaneNamespace), - } { - if err := syncSecret(secret, email); err != nil { + for _, item := range []struct { + email string + secret *corev1.Secret + }{ + {hcluster.Spec.Platform.GCP.WorkloadIdentity.ServiceAccountsEmails.NodePool, NodePoolManagementCredsSecret(controlPlaneNamespace)}, + {hcluster.Spec.Platform.GCP.WorkloadIdentity.ServiceAccountsEmails.ControlPlane, ControlPlaneOperatorCredsSecret(controlPlaneNamespace)}, + {hcluster.Spec.Platform.GCP.WorkloadIdentity.ServiceAccountsEmails.CloudController, CloudControllerCredsSecret(controlPlaneNamespace)}, + } { + if err := syncSecret(item.secret, item.email); err != nil { errs = append(errs, err) } }
|
/retest-required |
cristianoveiga
force-pushed
the
GCP-367-implement-gcp-cloud-controller-manager
branch
from
a14e0dc to
ffe0a81
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@control-plane-operator/controllers/hostedcontrolplane/testdata/gcp-cloud-controller-manager/IBMCloud/zz_fixture_TestControlPlaneComponents_gcp_cloud_controller_manager_deployment.yaml`:
- Around line 88-89: The IBMCloud fixture's container spec only sets
securityContext.readOnlyRootFilesystem and is missing the GCP fixture's
hardening fields and sidecar/volume; update the IBMCloud deployment spec to add
securityContext.allowPrivilegeEscalation: false,
securityContext.capabilities.drop: [ALL], securityContext.runAsNonRoot: true to
the same container spec (or document why platform differences are required), and
add the token-minter sidecar container plus the cloud-token volume mount/volume
definition to mirror the GCP fixture (or add a comment explaining why
token-minter/cloud-token are intentionally omitted).
In
`@control-plane-operator/controllers/hostedcontrolplane/testdata/gcp-cloud-controller-manager/zz_fixture_TestControlPlaneComponents_gcp_cloud_controller_manager_deployment.yaml`:
- Around line 88-90: The container securityContext only sets
readOnlyRootFilesystem; update the deployment container specs (the containers
that currently have securityContext and terminationMessagePolicy) to add
allowPrivilegeEscalation: false and runAsNonRoot: true (assuming the images
support non-root) alongside the existing readOnlyRootFilesystem setting; ensure
you apply this change to both container blocks present (the one with
securityContext/readOnlyRootFilesystem and the other similar block) so both
containers prevent privilege escalation and run as non-root.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/gcp-cloud-controller-manager/deployment.yaml`:
- Around line 18-51: The cloud-controller-manager container runs as root and
allows privilege escalation; update the Deployment's pod spec for the container
named "cloud-controller-manager" to add a restrictive securityContext: set
runAsNonRoot: true and a non-root runAsUser (e.g., 1000), set
allowPrivilegeEscalation: false, drop ALL capabilities (capabilities.drop:
["ALL"]), enable readOnlyRootFilesystem: true and set a seccompProfile
(RuntimeDefault) and/or restricted SELinux/AppArmor context as available; apply
these changes in the container spec and update any test fixtures that assert on
the old defaults to expect the new securityContext values.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/gcp/config.go (1)
22-24: Consider validatingnetworkNameis not empty.If
gcpPlatform.NetworkConfig.Network.Nameis empty, the CCM will receive an emptynetwork-namein cloud.conf, which may cause runtime failures. Consider adding validation or logging a warning.♻️ Proposed validation
projectID := gcpPlatform.Project networkName := gcpPlatform.NetworkConfig.Network.Name + if networkName == "" { + return fmt.Errorf("network name is required in GCP platform configuration") + } subnetworkName := "" // Subnetwork is optional for CCM
.../IBMCloud/zz_fixture_TestControlPlaneComponents_gcp_cloud_controller_manager_deployment.yaml
Show resolved
Hide resolved
...r-manager/zz_fixture_TestControlPlaneComponents_gcp_cloud_controller_manager_deployment.yaml
Show resolved
Hide resolved
...erator/controllers/hostedcontrolplane/v2/assets/gcp-cloud-controller-manager/deployment.yaml
Show resolved
Hide resolved
|
/uncc @bryan-cox |
...erator/controllers/hostedcontrolplane/v2/assets/gcp-cloud-controller-manager/deployment.yaml
Outdated
Show resolved
Hide resolved
|
/lgtm |
|
/verified later @cristianoveiga |
openshift-ci
bot
added
the
approved
|
/retest-required |
1 similar comment
|
/retest-required |
|
/verified later @cristianoveiga |
openshift-ci-robot
added
verified-later
verified
|
@cristianoveiga: This PR has been marked to be verified later by 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. |
Implements the GCP Cloud Controller Manager (CCM) for hosted control planes, replacing the need for a custom ProviderID controller. The CCM handles node initialization (provider ID, zone labels, taint removal) and enables LoadBalancer service support for GCP hosted clusters. Key implementation details: - Create CCM component under control-plane-operator v2/cloud_controller_manager/gcp/ - Create CCM assets (deployment, config) under v2/assets/gcp-cloud-controller-manager/ - Configure token minter sidecar for WIF with kube-system/cloud-controller-manager SA - Deploy CCM with --controllers=*,-nodeipam (nodeipam handled by cluster network operator) - Add cloud-controller-manager-creds secret creation to hypershift-operator ReconcileCredentials, reusing existing buildGCPWorkloadIdentityCredentials - Support image override via hypershift.openshift.io/image-overrides annotation The CCM uses the pre-created cloud-controller-manager-creds secret from the hypershift-operator, following the same pattern as node-management-creds and control-plane-operator-creds for consistent credential management. Closes: GCP-367 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add missing validation for CloudController service account email in validateWorkloadIdentityConfiguration. This ensures early failure with actionable error message when the field is empty, rather than failing later during credential secret creation.
Add GCP platform spec to TestControlPlaneComponents to ensure the GCP CCM component properly reconciles during tests. Also generates test fixtures for the component deployment, configmap, and controlplanecomponent resources across different feature sets and platform types. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The subnetwork-name field was always set to an empty string since the GCP CCM can discover the subnetwork from node metadata. Remove it from the config template and adapter to avoid including an empty configuration value. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add missing AROSwift feature gate test fixtures after rebasing onto main which introduced the AROSwift feature set. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
cristianoveiga
force-pushed
the
GCP-367-implement-gcp-cloud-controller-manager
branch
from
9190f2f to
527c0d9
Compare
openshift-ci-robot
removed
verified
|
/verified later @cristianoveiga |
openshift-ci-robot
added
verified-later
verified
|
@cristianoveiga: This PR has been marked to be verified later by 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. |
|
/lgtm |
|
Scheduling tests matching the |
|
/retest-required |
|
/test verify-deps |
|
@cristianoveiga: all tests passed! 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. |
6 participants
What this PR does / why we need it:
Implements the GCP Cloud Controller Manager (CCM) for hosted control planes, replacing the need for a custom ProviderID controller. The CCM handles node initialization (provider ID, zone labels, taint removal) and enables LoadBalancer service support for GCP hosted clusters.
Key implementation details:
The CCM uses the pre-created cloud-controller-manager-creds secret from the hypershift-operator, following the same pattern as node-management-creds and control-plane-operator-creds for consistent credential management.
Which issue(s) this PR fixes:
Fixes GCP-367
Special notes for your reviewer:
Checklist: