Skip to content
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

[OCPCLOUD-2034] Update Cloud Provider code to use new Azure/GCP specific feature gates #1505

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 29 additions & 9 deletions pkg/cloudprovider/external.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,17 @@ import (
configv1 "github.com/openshift/api/config/v1"
)

const (
var (
// ExternalCloudProviderFeature is the name of the external cloud provider feature gate.
// This is used to flag to operators that the cluster should be using the external cloud-controller-manager
// rather than the in-tree cloud controller loops.
ExternalCloudProviderFeature = "ExternalCloudProvider"
ExternalCloudProviderFeature = configv1.FeatureGateExternalCloudProvider

// ExternalCloudProviderFeatureAzure is the name of the external cloud provider feature gate for Azure.
ExternalCloudProviderFeatureAzure = configv1.FeatureGateExternalCloudProviderAzure

// ExternalCloudProviderFeatureGCP is the name of the external cloud provider feature gate for GCP.
ExternalCloudProviderFeatureGCP = configv1.FeatureGateExternalCloudProviderGCP
)

// IsCloudProviderExternal is used to check whether external cloud provider settings should be used in a component.
Expand All @@ -30,12 +36,12 @@ func IsCloudProviderExternal(platformStatus *configv1.PlatformStatus, featureGat
switch platformStatus.Type {
case configv1.GCPPlatformType:
// Platforms that are external based on feature gate presence
return isExternalFeatureGateEnabled(featureGateAccessor)
return isExternalFeatureGateEnabled(featureGateAccessor, ExternalCloudProviderFeature, ExternalCloudProviderFeatureGCP)
case configv1.AzurePlatformType:
if isAzureStackHub(platformStatus) {
return true, nil
}
return isExternalFeatureGateEnabled(featureGateAccessor)
return isExternalFeatureGateEnabled(featureGateAccessor, ExternalCloudProviderFeature, ExternalCloudProviderFeatureAzure)
case configv1.AlibabaCloudPlatformType,
configv1.AWSPlatformType,
configv1.IBMCloudPlatformType,
Expand All @@ -57,15 +63,29 @@ func isAzureStackHub(platformStatus *configv1.PlatformStatus) bool {

// isExternalFeatureGateEnabled determines whether the ExternalCloudProvider feature gate is present in the current
// feature set.
func isExternalFeatureGateEnabled(featureGateAccess featuregates.FeatureGateAccess) (bool, error) {
func isExternalFeatureGateEnabled(featureGateAccess featuregates.FeatureGateAccess, featureGateNames ...configv1.FeatureGateName) (bool, error) {
enabled, disabled, err := featureGateAccess.CurrentFeatureGates()
if err != nil {
return false, fmt.Errorf("unable to read current featuregates: %w", err)
}

enabledFeatureGates := sets.New[configv1.FeatureGateName](enabled...)
disabledFeatureGates := sets.New[configv1.FeatureGateName](disabled...)
enabledFeatureGates := sets.New(enabled...)
disabledFeatureGates := sets.New(disabled...)

// If any of the desired feature gates are disabled explictily, then the external cloud provider should not be used.
for _, featureGateName := range featureGateNames {
if disabledFeatureGates.Has(featureGateName) {
return false, nil
}
}

// If any of the desired feature gates are enabled, then the external cloud provider should be used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we end up stuck here a while, I think specific desire trumps non-specific desire.

for _, featureGateName := range featureGateNames {
if enabledFeatureGates.Has(featureGateName) {
return true, nil
}
}

// TODO left to be compatible, but we should standardize on positive checks only.
return !disabledFeatureGates.Has(configv1.FeatureGateExternalCloudProvider) && enabledFeatureGates.Has(configv1.FeatureGateExternalCloudProvider), nil
// No explicit opinion on the feature gate, assume it's not enabled.
return false, nil
}
34 changes: 34 additions & 0 deletions pkg/cloudprovider/external_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,23 @@ func TestIsCloudProviderExternal(t *testing.T) {
},
featureGate: featuregates.NewHardcodedFeatureGateAccess([]configv1.FeatureGateName{configv1.FeatureGateExternalCloudProvider}, nil),
expected: true,
}, {
name: "FeatureSet: CustomNoUpgrade (With External Feature Gate Azure), Platform: Azure",
status: &configv1.PlatformStatus{
Type: configv1.AzurePlatformType,
},
featureGate: featuregates.NewHardcodedFeatureGateAccess([]configv1.FeatureGateName{configv1.FeatureGateExternalCloudProviderAzure}, nil),
expected: true,
}, {
name: "FeatureSet: CustomNoUpgrade (With External Feature Gate Enabled but External Feature Gate Azure Disabled), Platform: Azure",
status: &configv1.PlatformStatus{
Type: configv1.AzurePlatformType,
},
featureGate: featuregates.NewHardcodedFeatureGateAccess(
[]configv1.FeatureGateName{configv1.FeatureGateExternalCloudProvider},
[]configv1.FeatureGateName{configv1.FeatureGateExternalCloudProviderAzure},
),
expected: false,
}, {
name: "Platform: Azure, CloudName: AzureStackHub",
status: &configv1.PlatformStatus{
Expand Down Expand Up @@ -178,6 +195,23 @@ func TestIsCloudProviderExternal(t *testing.T) {
},
featureGate: featuregates.NewHardcodedFeatureGateAccess([]configv1.FeatureGateName{configv1.FeatureGateExternalCloudProvider}, nil),
expected: true,
}, {
name: "FeatureSet: CustomNoUpgrade (With External Feature Gate GCP), Platform: GCP",
status: &configv1.PlatformStatus{
Type: configv1.GCPPlatformType,
},
featureGate: featuregates.NewHardcodedFeatureGateAccess([]configv1.FeatureGateName{configv1.FeatureGateExternalCloudProviderGCP}, nil),
expected: true,
}, {
name: "FeatureSet: CustomNoUpgrade (With External Feature Gate Enabled but External Feature Gate GCP Disabled), Platform: GCP",
status: &configv1.PlatformStatus{
Type: configv1.GCPPlatformType,
},
featureGate: featuregates.NewHardcodedFeatureGateAccess(
[]configv1.FeatureGateName{configv1.FeatureGateExternalCloudProvider},
[]configv1.FeatureGateName{configv1.FeatureGateExternalCloudProviderGCP},
),
expected: false,
}, {
name: "FeatureSet: TechPreviewNoUpgrade, Platform: GCP",
status: &configv1.PlatformStatus{
Expand Down