SPLAT-2651: Added support to manage kube-cloud-config for vSphere in openshift-config-managed#442
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)
Comment |
|
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. |
|
@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. |
| // 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) | ||
| } |
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?
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.
| 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")) | ||
| } | ||
| }) |
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
There was a problem hiding this comment.
nice, i can update changes to use that.
There was a problem hiding this comment.
I tried to but it seems to cause some errors. I then got claude to take a look and I was told:
● Based on my analysis, I can see that in the "vSphere managed config sync" Describe block, the tests only create ConfigMaps in the
OpenshiftManagedConfigNamespace namespace (lines 689, 700, 733).
However, there's an issue with using testutils.CleanupResources() directly: it will try to delete the namespace after cleanup (if the namespace parameter is
not empty), which you don't want because OpenshiftManagedConfigNamespace is a shared namespace created in BeforeSuite.
Looking at the CleanupResources function signature:
func CleanupResources(g gomega.Gomega, ctx context.Context, cfg *rest.Config, k8sClient client.Client, namespace string, objs ...client.Object)
After cleaning up resources, it calls removeNamespace() if namespace is not empty, which would delete the OpenshiftManagedConfigNamespace.
Recommended Solution
The best approach is to keep the for loop but add a namespace filter to only clean up ConfigMaps in the specific namespace where this test creates them
![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
| 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.
| 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.
| 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.
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
Just one comment, otherwise LGTM
| // Only initialize Nodes if there's at least one non-empty field to set | ||
| // This prevents empty "nodes: {}" from appearing in the marshaled YAML | ||
| if nodeNetworking.External.Network == "" && | ||
| externalNetworkSubnetCIDR == "" && | ||
| excludeExternalNetworkSubnetCIDR == "" && | ||
| nodeNetworking.Internal.Network == "" && | ||
| internalNetworkSubnetCIDR == "" && | ||
| excludeInternalNetworkSubnetCIDR == "" { | ||
| return | ||
| } |
There was a problem hiding this comment.
Would adding omitzero to the struct mean we don't have to do this?
There was a problem hiding this comment.
hmm, I'll try that. It currently has omitempty. That will require a change to the library-go, but shouldn't be bad.
There was a problem hiding this comment.
i tried omit zero but didn't seem to work.
According to Claude:
● Looking at your code, I can see the issue. The omitzero tag you added in line 143 of yaml.go isn't actually supported by the sigs.k8s.io/yaml package you're
using. That package delegates to Go's standard encoding/json, which only supports omitempty, not omitzero.
This seems to be accurate:
Whats interesting is they do have omitzero in the json package. /shrug
So claude is clearly wrong here. |
|
@vr4manta I've created a go playground which demonstrates that it is supported |
|
@JoelSpeed I made a PR in library-go to change the behavior of the vsphere cloud config struct. Now with the omitzero it should work. Key was to change the field to no longer be a pointer. That was my issue. Thanks for the suggestion. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
vr4manta
changed the title
|
@vr4manta: 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. |
|
/verified by @vr4manta |
openshift-ci-robot
added
the
verified
|
@vr4manta: This PR has been marked as verified 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. |
3 participants
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