SPLAT-2651: Added support to mange kube-cloud-config for vSphere in openshift-config-managed#442
Draft
vr4manta wants to merge 4 commits intoopenshift:mainfrom
Draft
SPLAT-2651: Added support to mange kube-cloud-config for vSphere in openshift-config-managed#442vr4manta wants to merge 4 commits intoopenshift:mainfrom
vr4manta wants to merge 4 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vr4manta: This pull request references SPLAT-2651 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 either version "4.22." or "openshift-4.22.", but it targets "openshift-5.0" instead. 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughThis PR updates Go module dependencies with a temporary replace directive, refactors the cloud config sync controller to support feature-gated managed ConfigMap ownership for vSphere platforms, switches the transformer to use the openshift/library-go vSphere package, expands RBAC permissions for ConfigMap write operations, and adds comprehensive tests for managed config synchronization while removing deprecated INI parsing tests. Changes
Sequence Diagram(s)sequenceDiagram
actor Controller as Cloud Config<br/>Sync Controller
participant FGA as FeatureGateAccess
participant TF as CloudConfig<br/>Transformer
participant SCM as Source ConfigMap<br/>(openshift-config)
participant MCM as Managed ConfigMap<br/>(openshift-config-managed)
participant TCM as Target ConfigMap<br/>(kube-cloud-config)
Controller->>FGA: Fetch and validate<br/>feature gates
alt Feature gates missing
Controller-->>Controller: Fail to degraded<br/>condition
else Proceed
Controller->>Controller: Check if platform<br/>supports managed mode<br/>(vSphere +<br/>VSphereMultiVCenterDay2)
alt Managed platform
Controller->>SCM: Check if source<br/>ConfigMap exists
alt Source missing
Controller->>MCM: Initialize empty<br/>source config
else Source exists
Controller->>SCM: Read source config
end
Controller->>TF: Transform config
Controller->>MCM: Create or update<br/>managed ConfigMap
else Non-managed platform
Controller->>SCM: Read source config
Controller->>TF: Transform config
Controller->>TCM: Sync to target<br/>ConfigMap
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
Contributor
|
Skipping CI for Draft Pull Request. |
|
@vr4manta: This pull request references SPLAT-2651 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 either version "4.22." or "openshift-4.22.", but it targets "openshift-5.0" instead. 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. |
Contributor
|
[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 |
|
@vr4manta: This pull request references SPLAT-2651 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 either version "4.22." or "openshift-4.22.", but it targets "openshift-5.0" instead. 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. |
JoelSpeed
reviewed
Apr 21, 2026
Comment on lines
+35
to
+43
| // isFeatureGateEnabled checks if a feature gate is enabled. | ||
| // Returns false if the feature gate is nil. | ||
| // This provides a nil-safe way to check feature gates. | ||
| func isFeatureGateEnabled(featureGates featuregates.FeatureGate, featureName configv1.FeatureGateName) bool { | ||
| if featureGates == nil { | ||
| return false | ||
| } | ||
| return featureGates.Enabled(featureName) | ||
| } |
Contributor
There was a problem hiding this comment.
Do we really need a nil safe way to check? Shouldn't the featuregates.FeatureGate always be initialised? Could this lead to a programatic error with bad behaviour because we missed populating the gate somewhere?
Contributor
Author
There was a problem hiding this comment.
I can remove this. Its been encouraged by our AI friends to do this. I'll remove and make sure code rabbit is happy.
Comment on lines
+653
to
+666
| AfterEach(func() { | ||
| deleteOptions := &client.DeleteOptions{ | ||
| GracePeriodSeconds: ptr.To[int64](0), | ||
| } | ||
|
|
||
| allCMs := &corev1.ConfigMapList{} | ||
| Expect(cl.List(ctx, allCMs)).To(Succeed()) | ||
| for _, cm := range allCMs.Items { | ||
| Expect(cl.Delete(ctx, cm.DeepCopy(), deleteOptions)).To(Succeed()) | ||
| Eventually(func() error { | ||
| return cl.Get(ctx, client.ObjectKeyFromObject(cm.DeepCopy()), &corev1.ConfigMap{}) | ||
| }).Should(MatchError(apierrors.IsNotFound, "IsNotFound")) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
Pretty sure we have some test utils for this that we use in CAPI, I wonder if they could also be used here 🤔
Something like https://github.com/openshift/cluster-api-actuator-pkg/blob/master/testutils/cleanup.go
Contributor
Author
There was a problem hiding this comment.
nice, i can update changes to use that.
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
PR needs rebase. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/controllers/cloud_config_sync_controller.go (1)
362-365: Note: Target ConfigMap always updated regardless of equality.
syncCloudConfigDatapassescheckEquality: false, meaning the target ConfigMap in the managed namespace will always be updated. This may cause unnecessary resource version churn. Consider if equality checking should be enabled here as well.💡 Consider enabling equality check
func (r *CloudConfigReconciler) syncCloudConfigData(ctx context.Context, source *corev1.ConfigMap) error { - // Use the generic helper, no equality check (always update for target CM) - return r.syncConfigMapToTarget(ctx, source, syncedCloudConfigMapName, r.ManagedNamespace, false) + // Use the generic helper with equality check to reduce churn + return r.syncConfigMapToTarget(ctx, source, syncedCloudConfigMapName, r.ManagedNamespace, true) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/cloud_config_sync_controller.go` around lines 362 - 365, syncCloudConfigData currently calls syncConfigMapToTarget with checkEquality set to false which forces updates even when nothing changed; change the call in syncCloudConfigData to pass checkEquality=true so the reconciler will compare source vs target and skip updates when equal (keep using syncedCloudConfigMapName and r.ManagedNamespace). Confirm syncConfigMapToTarget's equality logic covers data/labels/annotations you care about and add/update a unit/integration test for syncCloudConfigData to assert no update occurs when source and target are identical.pkg/controllers/cloud_config_sync_controller_test.go (1)
36-56: Consider using these constants in tests.The
defaultVSphereINIConfiganddefaultVSphereYAMLConfigconstants are defined but the tests at lines 669-777 use inline strings like"global:\n server: test\n"instead. For consistency and maintainability, consider using the defined constants in the test assertions where appropriate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/cloud_config_sync_controller_test.go` around lines 36 - 56, Tests construct expected vSphere config strings inline; replace those inline literals with the existing constants defaultVSphereINIConfig and defaultVSphereYAMLConfig so assertions use the defined canonical values. Locate the test cases that assert vSphere config contents (they currently embed strings like "global:\n server: test\n" within the test body) and swap those string literals with the matching constant (defaultVSphereINIConfig for INI-based expectations and defaultVSphereYAMLConfig for YAML-based expectations) to ensure consistency and maintainability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 9-11: The temporary replace directive that points to the personal
fork (replace github.com/openshift/library-go => github.com/vr4manta/library-go
v0.0.0-...) must be removed: delete that replace line from go.mod so the project
uses the upstream github.com/openshift/library-go module; if you need to track
the change add a TODO comment or a repository-level note to reintroduce it only
if the upstream PR for the vSphere cloudprovider hasn't merged yet and ensure go
mod tidy/update runs after removal.
In `@pkg/cloud/vsphere/vsphere_config_transformer.go`:
- Line 10: The code imports the non-upstream package alias ccmConfig from
"github.com/openshift/library-go/pkg/cloudprovider/vsphere" which currently only
exists in a temporary fork referenced via a replace in go.mod; resolve this by
removing the temporary dependency: either wait until the upstream PR is merged
then update go.mod to remove the replace and ensure the import path
"github.com/openshift/library-go/pkg/cloudprovider/vsphere" is valid, or change
the import back to the fork's module path used in go.mod (e.g.,
"github.com/vr4manta/library-go/…") consistently and keep the replace until
upstream merges; update any references to ccmConfig in
vsphere_config_transformer.go accordingly and run go mod tidy to verify the
module graph.
In `@pkg/controllers/cloud_config_sync_controller_test.go`:
- Around line 138-150: Remove the dead helpers makeManagedCloudConfigINI and
makeManagedCloudConfigYAML from
pkg/controllers/cloud_config_sync_controller_test.go since they are not
referenced by any tests; delete both function definitions
(makeManagedCloudConfigINI and makeManagedCloudConfigYAML) to eliminate unused
code, or alternatively replace inline test config strings with calls to these
helpers where appropriate—prefer deleting the unused functions to keep the test
file clean.
In `@pkg/controllers/cloud_config_sync_controller.go`:
- Around line 171-176: The controller currently sets
sourceCM.Data[defaultConfigKey] = "" when
shouldManageManagedConfigMap(platformType, features) is true, but the vSphere
CloudConfigTransformer (ccmConfig.ReadConfig) rejects empty input; change the
init behavior so we do NOT seed an empty string for platforms whose transformer
requires non-empty config (e.g., vSphere): either (A) skip creating
sourceCM.Data[defaultConfigKey] for those platforms so the transformer sees no
source, or (B) populate sourceCM with a minimal valid config template for that
platform; implement this by checking platformType before setting
defaultConfigKey (use the existing shouldManageManagedConfigMap and add a guard
for vSphere or a helper like platformRequiresConfig), and ensure
CloudConfigTransformer/ccmConfig.ReadConfig will receive either a valid config
or no source instead of an empty string.
---
Nitpick comments:
In `@pkg/controllers/cloud_config_sync_controller_test.go`:
- Around line 36-56: Tests construct expected vSphere config strings inline;
replace those inline literals with the existing constants
defaultVSphereINIConfig and defaultVSphereYAMLConfig so assertions use the
defined canonical values. Locate the test cases that assert vSphere config
contents (they currently embed strings like "global:\n server: test\n" within
the test body) and swap those string literals with the matching constant
(defaultVSphereINIConfig for INI-based expectations and defaultVSphereYAMLConfig
for YAML-based expectations) to ensure consistency and maintainability.
In `@pkg/controllers/cloud_config_sync_controller.go`:
- Around line 362-365: syncCloudConfigData currently calls syncConfigMapToTarget
with checkEquality set to false which forces updates even when nothing changed;
change the call in syncCloudConfigData to pass checkEquality=true so the
reconciler will compare source vs target and skip updates when equal (keep using
syncedCloudConfigMapName and r.ManagedNamespace). Confirm
syncConfigMapToTarget's equality logic covers data/labels/annotations you care
about and add/update a unit/integration test for syncCloudConfigData to assert
no update occurs when source and target are identical.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a7722981-b9e5-47d5-9ac7-64fe792fe494
⛔ Files ignored due to path filters (64)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_dns.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_dnses-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_dnses-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_dnses-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_dnses-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_dnses-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/envtest-releases.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/library-go/pkg/cloudprovider/vsphere/config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/cloudprovider/vsphere/ini.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/cloudprovider/vsphere/yaml.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/cert_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/keygen.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/client_cert_rotation_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/signer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/target.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/profile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/provider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/resolve.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/types.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
go.modmanifests/0000_26_cloud-controller-manager-operator_02_rbac_operator.yamlpkg/cloud/vsphere/vsphere_cloud_config/config_test.gopkg/cloud/vsphere/vsphere_config_transformer.gopkg/controllers/cloud_config_sync_controller.gopkg/controllers/cloud_config_sync_controller_test.go
💤 Files with no reviewable changes (1)
- pkg/cloud/vsphere/vsphere_cloud_config/config_test.go
Comment on lines
+9
to
+11
| // Temporary while testing: go get github.com/vr4manta/library-go@SPLAT-2651 | ||
| replace github.com/openshift/library-go => github.com/vr4manta/library-go v0.0.0-20260423150140-79a3161b14ce | ||
|
|
There was a problem hiding this comment.
Remove temporary replace directive before merge.
The comment explicitly states this is temporary for testing. This replace directive pointing to a personal fork (github.com/vr4manta/library-go) must be removed before merging to main. The upstream openshift/library-go should contain the required vSphere cloudprovider package.
Consider tracking this with a TODO or ensuring the dependent library-go PR is merged first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` around lines 9 - 11, The temporary replace directive that points to
the personal fork (replace github.com/openshift/library-go =>
github.com/vr4manta/library-go v0.0.0-...) must be removed: delete that replace
line from go.mod so the project uses the upstream
github.com/openshift/library-go module; if you need to track the change add a
TODO comment or a repository-level note to reintroduce it only if the upstream
PR for the vSphere cloudprovider hasn't merged yet and ensure go mod tidy/update
runs after removal.
| "k8s.io/utils/net" | ||
|
|
||
| ccmConfig "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/vsphere/vsphere_cloud_config" | ||
| ccmConfig "github.com/openshift/library-go/pkg/cloudprovider/vsphere" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the vSphere cloudprovider package exists in the upstream library-go
# This verifies if the fork changes have been merged
echo "Checking if the vsphere cloudprovider package exists in upstream library-go..."
curl -s -o /dev/null -w "%{http_code}" "https://api.github.com/repos/openshift/library-go/contents/pkg/cloudprovider/vsphere"
echo ""
# Also check for any open PRs from vr4manta to openshift/library-go
echo "Checking for related PRs..."
gh pr list --repo openshift/library-go --author vr4manta --state all --limit 5Repository: openshift/cluster-cloud-controller-manager-operator
Length of output: 419
Resolve the temporary library-go fork dependency before merge.
The import path github.com/openshift/library-go/pkg/cloudprovider/vsphere does not exist in the upstream openshift/library-go repository. The code currently relies on a temporary replace directive in go.mod pointing to github.com/vr4manta/library-go. An open PR (#2175: "SPLAT-2651: Added vSphere cloud config modules from 3CMO") exists to add this package to upstream, but it remains unmerged. Either merge the upstream PR and remove the replace directive, or delay this PR until the fork changes are upstream.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cloud/vsphere/vsphere_config_transformer.go` at line 10, The code imports
the non-upstream package alias ccmConfig from
"github.com/openshift/library-go/pkg/cloudprovider/vsphere" which currently only
exists in a temporary fork referenced via a replace in go.mod; resolve this by
removing the temporary dependency: either wait until the upstream PR is merged
then update go.mod to remove the replace and ensure the import path
"github.com/openshift/library-go/pkg/cloudprovider/vsphere" is valid, or change
the import back to the fork's module path used in go.mod (e.g.,
"github.com/vr4manta/library-go/…") consistently and keep the replace until
upstream merges; update any references to ccmConfig in
vsphere_config_transformer.go accordingly and run go mod tidy to verify the
module graph.
Comment on lines
+138
to
+150
| func makeManagedCloudConfigINI() *corev1.ConfigMap { | ||
| return &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{ | ||
| Name: managedCloudConfigMapName, | ||
| Namespace: OpenshiftManagedConfigNamespace, | ||
| }, Data: map[string]string{"cloud.conf": defaultVSphereINIConfig}} | ||
| } | ||
|
|
||
| func makeManagedCloudConfigYAML() *corev1.ConfigMap { | ||
| return &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{ | ||
| Name: managedCloudConfigMapName, | ||
| Namespace: OpenshiftManagedConfigNamespace, | ||
| }, Data: map[string]string{"cloud.conf": defaultVSphereYAMLConfig}} | ||
| } |
There was a problem hiding this comment.
Remove unused helper functions.
Static analysis indicates makeManagedCloudConfigINI() and makeManagedCloudConfigYAML() are unused. The tests use inline config strings instead. Either utilize these helpers in the tests or remove them to avoid dead code.
🧹 Proposed fix: Remove unused functions
-func makeManagedCloudConfigINI() *corev1.ConfigMap {
- return &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{
- Name: managedCloudConfigMapName,
- Namespace: OpenshiftManagedConfigNamespace,
- }, Data: map[string]string{"cloud.conf": defaultVSphereINIConfig}}
-}
-
-func makeManagedCloudConfigYAML() *corev1.ConfigMap {
- return &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{
- Name: managedCloudConfigMapName,
- Namespace: OpenshiftManagedConfigNamespace,
- }, Data: map[string]string{"cloud.conf": defaultVSphereYAMLConfig}}
-}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func makeManagedCloudConfigINI() *corev1.ConfigMap { | |
| return &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{ | |
| Name: managedCloudConfigMapName, | |
| Namespace: OpenshiftManagedConfigNamespace, | |
| }, Data: map[string]string{"cloud.conf": defaultVSphereINIConfig}} | |
| } | |
| func makeManagedCloudConfigYAML() *corev1.ConfigMap { | |
| return &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{ | |
| Name: managedCloudConfigMapName, | |
| Namespace: OpenshiftManagedConfigNamespace, | |
| }, Data: map[string]string{"cloud.conf": defaultVSphereYAMLConfig}} | |
| } |
🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 138-138: func makeManagedCloudConfigINI is unused
(unused)
[error] 145-145: func makeManagedCloudConfigYAML is unused
(unused)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controllers/cloud_config_sync_controller_test.go` around lines 138 - 150,
Remove the dead helpers makeManagedCloudConfigINI and makeManagedCloudConfigYAML
from pkg/controllers/cloud_config_sync_controller_test.go since they are not
referenced by any tests; delete both function definitions
(makeManagedCloudConfigINI and makeManagedCloudConfigYAML) to eliminate unused
code, or alternatively replace inline test config strings with calls to these
helpers where appropriate—prefer deleting the unused functions to keep the test
file clean.
Comment on lines
+171
to
+176
| klog.Warningf("cloud-config not found in either openshift-config-managed or openshift-config namespace") | ||
| // For platforms we manage, create an empty source that will be populated by the transformer | ||
| if shouldManageManagedConfigMap(platformType, features) { | ||
| klog.Infof("Initializing empty config for platform %s", platformType) | ||
| sourceCM.Data = map[string]string{defaultConfigKey: ""} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how ReadConfig handles empty input in the transformer
ast-grep --pattern 'func CloudConfigTransformer($_, $_, $_, $_) ($_, $_) {
$$$
}'
# Also check if there's any validation before calling the transformer
rg -n "cloudConfigTransformerFn\(" pkg/controllers/Repository: openshift/cluster-cloud-controller-manager-operator
Length of output: 15059
🏁 Script executed:
# Find the ReadConfig function implementation
rg -n "func ReadConfig\(" --type go
# Check the ccmConfig package to understand how ReadConfig handles empty input
fd -e go -x grep -l "func ReadConfig" {}
# Look at the context around line 201 in the controller where the transformer is called
sed -n '190,210p' pkg/controllers/cloud_config_sync_controller.goRepository: openshift/cluster-cloud-controller-manager-operator
Length of output: 1700
🏁 Script executed:
# Check imports and find which ReadConfig is used
head -30 pkg/cloud/vsphere/vsphere_config_transformer.go
# Check the library-go ReadConfig implementation
cat vendor/github.com/openshift/library-go/pkg/cloudprovider/vsphere/config.go
# Also check the k8s cloud-provider-vsphere ReadConfig
sed -n '258,300p' vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/config.goRepository: openshift/cluster-cloud-controller-manager-operator
Length of output: 4500
The vSphere CloudConfigTransformer will fail when passed an empty config string.
When config is not found in either namespace, an empty string is set for defaultConfigKey (line 175). However, the vSphere CloudConfigTransformer calls ccmConfig.ReadConfig([]byte(source)) which explicitly rejects empty input, returning an error: "vSphere config is empty". This will cause the transformer to fail at line 37-39 with "failed to read the cloud.conf". While the controller's error handling will catch this at lines 207-211, consider whether initializing with an empty config is the right approach for platforms that require populated configuration data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controllers/cloud_config_sync_controller.go` around lines 171 - 176, The
controller currently sets sourceCM.Data[defaultConfigKey] = "" when
shouldManageManagedConfigMap(platformType, features) is true, but the vSphere
CloudConfigTransformer (ccmConfig.ReadConfig) rejects empty input; change the
init behavior so we do NOT seed an empty string for platforms whose transformer
requires non-empty config (e.g., vSphere): either (A) skip creating
sourceCM.Data[defaultConfigKey] for those platforms so the transformer sees no
source, or (B) populate sourceCM with a minimal valid config template for that
platform; implement this by checking platformType before setting
defaultConfigKey (use the existing shouldManageManagedConfigMap and add a guard
for vSphere or a helper like platformRequiresConfig), and ensure
CloudConfigTransformer/ccmConfig.ReadConfig will receive either a valid config
or no source instead of an empty string.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SPLAT-2651
Changes
Enhanced the cluster-cloud-controller-manager-operator (CCCMO) to manage the openshift-config-managed/kube-cloud-config ConfigMap for vSphere platforms,
migrating ownership from the Cluster Config Operator (CCO) to CCCMO. This enables CCCMO to:
Tests covering:
Enhanced RBAC for openshift-config-managed namespace - Lines 284-295
Added verbs to existing Role:
verbs:
- get
- list
- watch
- create # NEW: For initial ConfigMap creation (migration from CCO)
- update # NEW: For ongoing updates
- patch # NEW: For ongoing updates
Purpose: Allow CCCMO service account to manage ConfigMaps in openshift-config-managed namespace
Feature Gate Integration
Feature Gate: VSphereMultiVCenterDay2
Before This Enhancement:
After This Enhancement:
Migration Path from CCO to CCCMO
- Creates ConfigMap if CCO never created it
- Updates ConfigMap with transformed content
- Becomes single source of truth
Summary by CodeRabbit
Release Notes
Bug Fixes & Improvements
openshift-config-managednamespace, including create, update, and patch operations.Dependencies