-
Notifications
You must be signed in to change notification settings - Fork 150
SPLAT-2717: Migrate vSphere sync of kube-cloud-config to 3CMO #481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,18 +5,21 @@ import ( | |
| "time" | ||
|
|
||
| configv1 "github.com/openshift/api/config/v1" | ||
| "github.com/openshift/api/features" | ||
| configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" | ||
| configv1listers "github.com/openshift/client-go/config/listers/config/v1" | ||
| "github.com/openshift/cluster-config-operator/pkg/operator/operatorclient" | ||
| "github.com/openshift/library-go/pkg/controller/factory" | ||
| "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" | ||
| "github.com/openshift/library-go/pkg/operator/events" | ||
| "github.com/openshift/library-go/pkg/operator/resource/resourceapply" | ||
| operatorv1helpers "github.com/openshift/library-go/pkg/operator/v1helpers" | ||
| corev1 "k8s.io/api/core/v1" | ||
| "k8s.io/apimachinery/pkg/api/errors" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| corev1client "k8s.io/client-go/kubernetes/typed/core/v1" | ||
| "k8s.io/client-go/tools/cache" | ||
| "k8s.io/klog/v2" | ||
| ) | ||
|
|
||
| const ( | ||
|
|
@@ -34,9 +37,10 @@ type cloudConfigTransformer func(input *corev1.ConfigMap, key string, obj *confi | |
| // user specifications from `infrastructure.spec.platformSpec` to stitch together a new ConfigMap for kube cloud config. | ||
| // The stitched ConfigMap is stored at `openshift-config-managed/kube-cloud-confg`. | ||
| type KubeCloudConfigController struct { | ||
| infraClient configv1client.InfrastructureInterface | ||
| infraLister configv1listers.InfrastructureLister | ||
| configMapClient corev1client.ConfigMapsGetter | ||
| infraClient configv1client.InfrastructureInterface | ||
| infraLister configv1listers.InfrastructureLister | ||
| configMapClient corev1client.ConfigMapsGetter | ||
| featureGateAccess featuregates.FeatureGateAccess | ||
|
|
||
| // transformers stores per platform tranformer | ||
| cloudConfigTransformers map[configv1.PlatformType]cloudConfigTransformer | ||
|
|
@@ -47,11 +51,13 @@ func NewController(operatorClient operatorv1helpers.OperatorClient, | |
| infraClient configv1client.InfrastructuresGetter, infraLister configv1listers.InfrastructureLister, infraInformer cache.SharedIndexInformer, | ||
| configMapClient corev1client.ConfigMapsGetter, | ||
| openshiftConfigConfigMapInformer cache.SharedIndexInformer, openshiftConfigManagedConfigMapInformer cache.SharedIndexInformer, | ||
| featureGateAccess featuregates.FeatureGateAccess, | ||
| recorder events.Recorder) factory.Controller { | ||
| c := &KubeCloudConfigController{ | ||
| infraClient: infraClient.Infrastructures(), | ||
| infraLister: infraLister, | ||
| configMapClient: configMapClient, | ||
| featureGateAccess: featureGateAccess, | ||
| cloudConfigTransformers: cloudConfigTransformers(), | ||
| } | ||
| return factory.New(). | ||
|
|
@@ -69,7 +75,7 @@ func NewController(operatorClient operatorv1helpers.OperatorClient, | |
|
|
||
| func (c KubeCloudConfigController) sync(ctx context.Context, syncCtx factory.SyncContext) error { | ||
| obj, err := c.infraLister.Get("cluster") | ||
| if errors.IsNotFound(err) { | ||
| if apierrors.IsNotFound(err) { | ||
| syncCtx.Recorder().Warningf("KubeCloudConfigController", "Required infrastructures.%s/cluster not found", configv1.GroupName) | ||
| return nil | ||
| } | ||
|
|
@@ -87,6 +93,16 @@ func (c KubeCloudConfigController) sync(ctx context.Context, syncCtx factory.Syn | |
| platformName = currentInfra.Status.Platform | ||
| } | ||
|
|
||
| // Check if this controller should manage the kube-cloud-config for this platform | ||
| shouldManage, err := c.shouldManageCloudConfig(platformName) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !shouldManage { | ||
| syncCtx.Recorder().Eventf("KubeCloudConfigController", "Skipping kube-cloud-config management for platform %s", platformName) | ||
| return nil | ||
| } | ||
|
|
||
| sourceCloudConfigMap := currentInfra.Spec.CloudConfig.Name | ||
| sourceCloudConfigKey := currentInfra.Spec.CloudConfig.Key | ||
|
|
||
|
|
@@ -113,7 +129,7 @@ func (c KubeCloudConfigController) sync(ctx context.Context, syncCtx factory.Syn | |
| targetCloudConfigMap := targetConfigName | ||
| if len(target.Data) == 0 && len(target.BinaryData) == 0 { // delete if exists | ||
| err := c.configMapClient.ConfigMaps(operatorclient.GlobalMachineSpecifiedConfigNamespace).Delete(ctx, targetCloudConfigMap, metav1.DeleteOptions{}) | ||
| if err != nil && !errors.IsNotFound(err) { | ||
| if err != nil && !apierrors.IsNotFound(err) { | ||
| return err | ||
| } | ||
| if err == nil { | ||
|
|
@@ -160,3 +176,53 @@ func cloudConfigTransformers() map[configv1.PlatformType]cloudConfigTransformer | |
| } | ||
| return cloudConfigTransformers | ||
| } | ||
|
|
||
| // shouldManageCloudConfig determines whether this controller should manage the kube-cloud-config | ||
| // ConfigMap for the given platform type. This allows for platform-specific logic to determine | ||
| // when ownership should transfer to another operator. | ||
| func (c KubeCloudConfigController) shouldManageCloudConfig(platformType configv1.PlatformType) (bool, error) { | ||
| switch platformType { | ||
| case configv1.VSpherePlatformType: | ||
| // For vSphere, check if VSphereMultiVCenterDay2 feature gate is enabled | ||
| // When enabled, ownership transfers to cluster-cloud-controller-manager-operator | ||
| enabled, err := c.isFeatureGateEnabled(features.FeatureGateVSphereMultiVCenterDay2) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| // Return false (do not manage) if enabled, true (manage) if not enabled | ||
| return !enabled, nil | ||
|
|
||
| case configv1.AWSPlatformType, | ||
| configv1.AzurePlatformType, | ||
| configv1.GCPPlatformType, | ||
| configv1.OpenStackPlatformType, | ||
| configv1.OvirtPlatformType, | ||
| configv1.KubevirtPlatformType, | ||
| configv1.IBMCloudPlatformType, | ||
| configv1.PowerVSPlatformType, | ||
| configv1.AlibabaCloudPlatformType, | ||
| configv1.NutanixPlatformType, | ||
| configv1.ExternalPlatformType, | ||
| configv1.NonePlatformType, | ||
| configv1.EquinixMetalPlatformType, | ||
| configv1.BareMetalPlatformType: | ||
| // For all other platforms, this controller manages the cloud config | ||
| return true, nil | ||
|
|
||
| default: | ||
| // Unknown platform type, default to managing | ||
| return true, nil | ||
| } | ||
| } | ||
|
|
||
| // isFeatureGateEnabled checks if the specified feature gate is enabled in the cluster. | ||
| // It returns an error only if there's an issue retrieving the feature gate (other than NotFound). | ||
| func (c KubeCloudConfigController) isFeatureGateEnabled(gateName configv1.FeatureGateName) (bool, error) { | ||
| currentFeatures, err := c.featureGateAccess.CurrentFeatureGates() | ||
| if err != nil { | ||
| klog.Errorf("unable to get feature gates: %v", err) | ||
| return false, err | ||
| } | ||
|
Comment on lines
+221
to
+225
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't ever change result, so you can do this in the init of |
||
|
|
||
| return currentFeatures.Enabled(gateName), nil | ||
|
Comment on lines
+218
to
+227
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, let's look at the file to see the context around lines 218-227
cat -n pkg/operator/kube_cloud_config/controller.go | sed -n '210,235p'Repository: openshift/cluster-config-operator Length of output: 818 🏁 Script executed: # Find the type definition for featureGateAccess to understand what methods are available
fd -t f "controller.go" pkg/operator/kube_cloud_config/ -x cat -n {} | head -100Repository: openshift/cluster-config-operator Length of output: 5181 🏁 Script executed: # Search for the featureGateAccess field definition in the controller struct
rg "featureGateAccess" pkg/operator/kube_cloud_config/controller.go -A 2 -B 2Repository: openshift/cluster-config-operator Length of output: 1130 🏁 Script executed: # Search for AreInitialFeatureGatesObserved method in the entire codebase
rg "AreInitialFeatureGatesObserved" --type goRepository: openshift/cluster-config-operator Length of output: 1419 🏁 Script executed: # Search for the interface definition that featureGateAccess might implement
rg "FeatureGateAccess|featureGateAccess" pkg/operator/kube_cloud_config/ -A 3 -B 3Repository: openshift/cluster-config-operator Length of output: 6228 🏁 Script executed: # Find the FeatureGateAccess interface definition
rg "type FeatureGateAccess interface" --type go -A 20Repository: openshift/cluster-config-operator Length of output: 3755 🏁 Script executed: # Look for how CurrentFeatureGates() is used in the library-go vendored code
rg "CurrentFeatureGates\(\)" vendor/github.com/openshift/library-go/pkg/operator/configobserver/featuregates/ -B 3 -A 3Repository: openshift/cluster-config-operator Length of output: 5032 🏁 Script executed: # Check if there are any error checks for uninitialized feature gates
rg "AreInitialFeatureGatesObserved" vendor/github.com/openshift/library-go/pkg/operator/configobserver/featuregates/ -B 5 -A 5Repository: openshift/cluster-config-operator Length of output: 10680 🏁 Script executed: # Look at how isFeatureGateEnabled is called in the codebase to understand the impact
rg "isFeatureGateEnabled" pkg/operator/kube_cloud_config/controller.go -B 2 -A 2Repository: openshift/cluster-config-operator Length of output: 745 🏁 Script executed: # Check if there's any context where this would fail at startup
rg "sync.*defer" pkg/operator/kube_cloud_config/controller.go -A 10 | head -40Repository: openshift/cluster-config-operator Length of output: 59 Handle uninitialized feature gates gracefully during startup. Currently, any error from Suggested fix func (c KubeCloudConfigController) isFeatureGateEnabled(gateName configv1.FeatureGateName) (bool, error) {
+ if c.featureGateAccess == nil {
+ return false, nil
+ }
+ if !c.featureGateAccess.AreInitialFeatureGatesObserved() {
+ return false, nil
+ }
+
currentFeatures, err := c.featureGateAccess.CurrentFeatureGates()
if err != nil {
klog.Errorf("unable to get feature gates: %v", err)
- return false, err
+ return false, err
}
return currentFeatures.Enabled(gateName), nil
}🤖 Prompt for AI Agents |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,10 +6,12 @@ import ( | |
| "time" | ||
|
|
||
| configv1 "github.com/openshift/api/config/v1" | ||
| "github.com/openshift/api/features" | ||
| configfakeclient "github.com/openshift/client-go/config/clientset/versioned/fake" | ||
| configv1listers "github.com/openshift/client-go/config/listers/config/v1" | ||
| "github.com/openshift/cluster-config-operator/pkg/operator/operatorclient" | ||
| "github.com/openshift/library-go/pkg/controller/factory" | ||
| "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" | ||
| "github.com/openshift/library-go/pkg/operator/events" | ||
| "github.com/stretchr/testify/assert" | ||
| corev1 "k8s.io/api/core/v1" | ||
|
|
@@ -215,10 +217,14 @@ SubnetID = subnet-test | |
| } | ||
| fakeConfig := configfakeclient.NewSimpleClientset(test.inputinfra) | ||
|
|
||
| // Create a feature gate accessor with no enabled feature gates | ||
| featureGateAccessor := featuregates.NewHardcodedFeatureGateAccess(nil, nil) | ||
|
|
||
| ctrl := KubeCloudConfigController{ | ||
| infraClient: fakeConfig.ConfigV1().Infrastructures(), | ||
| infraLister: configv1listers.NewInfrastructureLister(indexerInfra), | ||
| configMapClient: fake.CoreV1(), | ||
| featureGateAccess: featureGateAccessor, | ||
| cloudConfigTransformers: cloudConfigTransformers(), | ||
| } | ||
|
|
||
|
|
@@ -252,3 +258,129 @@ SubnetID = subnet-test | |
| }) | ||
| } | ||
| } | ||
|
|
||
| func Test_sync_withVSphereMultiVCenterDay2FeatureGate(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| platformType configv1.PlatformType | ||
| inputData string | ||
| featureGateEnabled bool | ||
| expectedActions int | ||
| description string | ||
| }{ | ||
| { | ||
| name: "vSphere platform with feature gate enabled", | ||
| platformType: configv1.VSpherePlatformType, | ||
| inputData: `[Global]\ntest = value`, | ||
| featureGateEnabled: true, | ||
| expectedActions: 0, | ||
| description: "Should skip ConfigMap update when VSphereMultiVCenterDay2 is enabled on vSphere", | ||
| }, | ||
| { | ||
| name: "vSphere platform with feature gate disabled", | ||
| platformType: configv1.VSpherePlatformType, | ||
| inputData: `[Global]\ntest = value`, | ||
| featureGateEnabled: false, | ||
| expectedActions: 3, // Get source config, Get target config, Update target config | ||
| description: "Should update ConfigMap when VSphereMultiVCenterDay2 is disabled on vSphere", | ||
| }, | ||
| { | ||
| name: "AWS platform with feature gate enabled", | ||
| platformType: configv1.AWSPlatformType, | ||
| inputData: `[Global]\nVPC = vpc-test`, | ||
| featureGateEnabled: true, | ||
| expectedActions: 3, // Get source config, Get target config, Update target config | ||
| description: "Should update ConfigMap on AWS even if VSphereMultiVCenterDay2 is enabled", | ||
| }, | ||
| { | ||
| name: "Azure platform with feature gate enabled", | ||
| platformType: configv1.AzurePlatformType, | ||
| inputData: `{"resourceGroup":"test-rg"}`, | ||
| featureGateEnabled: true, | ||
| expectedActions: 3, // Get source config, Get target config, Update target config | ||
| description: "Should update ConfigMap on Azure even if VSphereMultiVCenterDay2 is enabled", | ||
| }, | ||
| { | ||
| name: "GCP platform with feature gate enabled", | ||
| platformType: configv1.GCPPlatformType, | ||
| inputData: `[global]\nsomekey = somevalue`, | ||
| featureGateEnabled: true, | ||
| expectedActions: 3, // Get source config, Get target config, Update target config | ||
| description: "Should update ConfigMap on GCP even if VSphereMultiVCenterDay2 is enabled", | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| inputInfra := &configv1.Infrastructure{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, | ||
| Spec: configv1.InfrastructureSpec{ | ||
| CloudConfig: configv1.ConfigMapFileReference{ | ||
| Name: "cluster-config-v1", | ||
| Key: "config", | ||
| }, | ||
| }, | ||
| Status: configv1.InfrastructureStatus{ | ||
| PlatformStatus: &configv1.PlatformStatus{ | ||
| Type: tc.platformType, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // Add Azure-specific platform status | ||
| if tc.platformType == configv1.AzurePlatformType { | ||
| inputInfra.Status.PlatformStatus.Azure = &configv1.AzurePlatformStatus{} | ||
| } | ||
|
|
||
| // Create a FeatureGateAccess based on the test case | ||
| var featureGateAccessor featuregates.FeatureGateAccess | ||
| if tc.featureGateEnabled { | ||
| // Create accessor with VSphereMultiVCenterDay2 enabled | ||
| featureGateAccessor = featuregates.NewHardcodedFeatureGateAccess( | ||
| []configv1.FeatureGateName{features.FeatureGateVSphereMultiVCenterDay2}, | ||
| nil, | ||
| ) | ||
| } else { | ||
| // Create accessor with VSphereMultiVCenterDay2 disabled | ||
| featureGateAccessor = featuregates.NewHardcodedFeatureGateAccess( | ||
| nil, | ||
| []configv1.FeatureGateName{features.FeatureGateVSphereMultiVCenterDay2}, | ||
| ) | ||
| } | ||
|
|
||
| fake := fake.NewClientset() | ||
| if err := fake.Tracker().Add(&corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "cluster-config-v1", Namespace: "openshift-config"}, | ||
| Data: map[string]string{"config": tc.inputData}, | ||
| }); err != nil { | ||
| t.Fatalf("failed to add cluster-config-v1 ConfigMap to fake tracker: %v", err) | ||
| } | ||
| if err := fake.Tracker().Add(&corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "kube-cloud-config", Namespace: "openshift-config-managed"}, | ||
| }); err != nil { | ||
| t.Fatalf("failed to add kube-cloud-config ConfigMap to fake tracker: %v", err) | ||
| } | ||
|
|
||
| indexerInfra := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) | ||
| if err := indexerInfra.Add(inputInfra); err != nil { | ||
| t.Fatal(err.Error()) | ||
| } | ||
|
|
||
| fakeConfig := configfakeclient.NewSimpleClientset(inputInfra) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify constructor availability/deprecation in vendored openshift config fake client.
rg -n "func NewClientset|func NewSimpleClientset|deprecated" \
vendor/github.com/openshift/client-go/config/clientset/versioned/fake -C2
# Verify current usage in this test file.
rg -n "configfakeclient.NewSimpleClientset|configfakeclient.NewClientset" \
pkg/operator/kube_cloud_config/controller_test.go -C2Repository: openshift/cluster-config-operator Length of output: 1930 🏁 Script executed: # Check the full function definitions and any deprecation markers
cat -n vendor/github.com/openshift/client-go/config/clientset/versioned/fake/clientset_generated.go | sed -n '25,100p'Repository: openshift/cluster-config-operator Length of output: 3205 🏁 Script executed: # Find all usages of NewSimpleClientset in the repository
rg -n "NewSimpleClientset|NewClientset" pkg/operator/kube_cloud_config/controller_test.goRepository: openshift/cluster-config-operator Length of output: 296 🏁 Script executed: # Check the context around line 369 in controller_test.go
sed -n '365,375p' pkg/operator/kube_cloud_config/controller_test.goRepository: openshift/cluster-config-operator Length of output: 516 🏁 Script executed: # Check context around line 218
sed -n '214,224p' pkg/operator/kube_cloud_config/controller_test.goRepository: openshift/cluster-config-operator Length of output: 576 Replace deprecated The Note: Line 218 in the same file also uses the deprecated constructor and should be updated for consistency. Suggested change- fakeConfig := configfakeclient.NewSimpleClientset(inputInfra)
+ fakeConfig := configfakeclient.NewClientset(inputInfra)🧰 Tools🪛 golangci-lint (2.11.4)[error] 369-369: SA1019: configfakeclient.NewSimpleClientset is deprecated: NewClientset replaces this with support for field management, which significantly improves server side apply testing. NewClientset is only available when apply configurations are generated (e.g. via --with-applyconfig). (staticcheck) 🤖 Prompt for AI Agents |
||
|
|
||
| ctrl := KubeCloudConfigController{ | ||
| infraClient: fakeConfig.ConfigV1().Infrastructures(), | ||
| infraLister: configv1listers.NewInfrastructureLister(indexerInfra), | ||
| configMapClient: fake.CoreV1(), | ||
| featureGateAccess: featureGateAccessor, | ||
| cloudConfigTransformers: cloudConfigTransformers(), | ||
| } | ||
|
|
||
| err := ctrl.sync(context.TODO(), | ||
| factory.NewSyncContext("KubeCloudConfigController", events.NewInMemoryRecorder("KubeCloudConfigController", clocktesting.NewFakePassiveClock(time.Now())))) | ||
|
|
||
| assert.NoError(t, err) | ||
| assert.Equal(t, tc.expectedActions, len(fake.Actions()), tc.description) | ||
| }) | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.