Skip to content

Commit

Permalink
OCPCLOUD-2010: Add external platform type support in IsCloudProviderE…
Browse files Browse the repository at this point in the history 
…xternal

This change enables the external platform when external platform type is
set and when cloud controller manager mode is set to External.

Since cloud controller manager mode is controlled from the
`PlatformSpec` struct, I had to change the signature of
`IsCloudProviderExternal` to take the `Infrastructure` struct in
parameter.

PR on API side: openshift/api#1434
  • Loading branch information
@adriengentil
adriengentil committed Apr 6, 2023
1 parent 6ed98e0 commit e052292
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 5 deletions.
18 changes: 17 additions & 1 deletion pkg/cloudprovider/external.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ const (
// IsCloudProviderExternal is used to check whether external cloud provider settings should be used in a component.
// It checks whether the ExternalCloudProvider feature gate is enabled and whether the ExternalCloudProvider feature
// has been implemented for the platform.
func IsCloudProviderExternal(platformStatus *configv1.PlatformStatus, featureGate *configv1.FeatureGate) (bool, error) {
func IsCloudProviderExternal(infrastructure *configv1.Infrastructure, featureGate *configv1.FeatureGate) (bool, error) {
platformStatus := infrastructure.Status.PlatformStatus
if platformStatus == nil {
return false, fmt.Errorf("platformStatus is required")
}
Expand All @@ -35,12 +36,27 @@ func IsCloudProviderExternal(platformStatus *configv1.PlatformStatus, featureGat
configv1.PowerVSPlatformType,
configv1.VSpherePlatformType:
return true, nil
case configv1.ExternalPlatformType:
return isExternalPlatformCCMEnabled(infrastructure)
default:
// Platforms that do not have external cloud providers implemented
return false, nil
}
}

func isExternalPlatformCCMEnabled(infrastructure *configv1.Infrastructure) (bool, error) {
platformSpec := &infrastructure.Spec.PlatformSpec
if platformSpec.External == nil {
return false, nil
}

if platformSpec.External.CloudControllerManager.State == configv1.CloudControllerManagerExternal {
return true, nil
}

return false, nil
}

// isExternalFeatureGateEnabled determines whether the ExternalCloudProvider feature gate is present in the current
// feature set.
func isExternalFeatureGateEnabled(featureGate *configv1.FeatureGate) (bool, error) {
Expand Down
60 changes: 59 additions & 1 deletion pkg/cloudprovider/external_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
func TestIsCloudProviderExternal(t *testing.T) {
cases := []struct {
name string
spec configv1.PlatformSpec
status *configv1.PlatformStatus
featureGate *configv1.FeatureGate
expected bool
Expand Down Expand Up @@ -161,6 +162,54 @@ func TestIsCloudProviderExternal(t *testing.T) {
},
featureGate: nil,
expected: true,
}, {
name: "No FeatureGate, Platform: External, CloudControllerManager.State = External",
spec: configv1.PlatformSpec{
External: &configv1.ExternalPlatformSpec{
CloudControllerManager: configv1.CloudControllerManagerSpec{
State: configv1.CloudControllerManagerExternal,
},
},
},
status: &configv1.PlatformStatus{
Type: configv1.ExternalPlatformType,
},
featureGate: nil,
expected: true,
}, {
name: "No FeatureGate, Platform: External, CloudControllerManager.State = None",
spec: configv1.PlatformSpec{
External: &configv1.ExternalPlatformSpec{
CloudControllerManager: configv1.CloudControllerManagerSpec{
State: configv1.CloudControllerManagerNone,
},
},
},
status: &configv1.PlatformStatus{
Type: configv1.ExternalPlatformType,
},
featureGate: nil,
expected: false,
}, {
name: "No FeatureGate, Platform: External, CloudControllerManager.State is empty",
spec: configv1.PlatformSpec{
External: &configv1.ExternalPlatformSpec{},
},
status: &configv1.PlatformStatus{
Type: configv1.ExternalPlatformType,
},
featureGate: nil,
expected: false,
}, {
name: "No FeatureGate, Platform: External, ExternalPlatformSpec is nil",
spec: configv1.PlatformSpec{
External: nil,
},
status: &configv1.PlatformStatus{
Type: configv1.ExternalPlatformType,
},
featureGate: nil,
expected: false,
}, {
name: "FeatureSet: CustomNoUpgrade (With External Feature Gate Enabled), Platform: Nutanix",
status: &configv1.PlatformStatus{
Expand Down Expand Up @@ -370,7 +419,16 @@ func TestIsCloudProviderExternal(t *testing.T) {
}}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
got, err := IsCloudProviderExternal(c.status, c.featureGate)
infrastructure := &configv1.Infrastructure{
Status: configv1.InfrastructureStatus{
PlatformStatus: c.status,
},
Spec: configv1.InfrastructureSpec{
PlatformSpec: c.spec,
},
}

got, err := IsCloudProviderExternal(infrastructure, c.featureGate)
if c.expectedErr != nil {
if err == nil {
t.Errorf("expected error: %v, but got no error", c.expectedErr)
Expand Down
7 changes: 4 additions & 3 deletions pkg/operator/configobserver/cloudprovider/observe_cloudprovider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func (c *cloudProviderObserver) ObserveCloudProviderNames(genericListers configo
return existingConfig, append(errs, err)
}

external, err := IsCloudProviderExternal(listers, infrastructure.Status.PlatformStatus)
external, err := IsCloudProviderExternal(listers, infrastructure)
if err != nil {
recorder.Warningf("ObserveCloudProviderNames", "Could not determine external cloud provider state: %v", err)
return existingConfig, append(errs, err)
Expand Down Expand Up @@ -151,7 +151,7 @@ func (c *cloudProviderObserver) ObserveCloudProviderNames(genericListers configo
// IsCloudProviderExternal is used to determine if the cluster should use external cloud providers.
// Currently, this is opt in via a feature gate. If no feature gate is present, the cluster should remain
// using the in-tree implementation.
func IsCloudProviderExternal(listers InfrastructureLister, platform *configv1.PlatformStatus) (bool, error) {
func IsCloudProviderExternal(listers InfrastructureLister, infrastructure *configv1.Infrastructure) (bool, error) {
featureGate, err := listers.FeatureGateLister().Get("cluster")
if errors.IsNotFound(err) {
// No feature gate is set, therefore cannot be external.
Expand All @@ -161,7 +161,7 @@ func IsCloudProviderExternal(listers InfrastructureLister, platform *configv1.Pl
return false, fmt.Errorf("could not fetch featuregate: %v", err)
}

external, err := cloudprovider.IsCloudProviderExternal(platform, featureGate)
external, err := cloudprovider.IsCloudProviderExternal(infrastructure, featureGate)
if err != nil {
return false, fmt.Errorf("could not determine if cloud provider is external from featuregate: %v", err)
}
Expand Down Expand Up @@ -194,6 +194,7 @@ func GetPlatformName(platformType configv1.PlatformType, recorder events.Recorde
case configv1.KubevirtPlatformType:
case configv1.AlibabaCloudPlatformType:
case configv1.PowerVSPlatformType:
case configv1.ExternalPlatformType:
default:
// the new doc on the infrastructure fields requires that we treat an unrecognized thing the same bare metal.
// TODO find a way to indicate to the user that we didn't honor their choice
Expand Down
39 changes: 39 additions & 0 deletions pkg/operator/configobserver/cloudprovider/observe_cloudprovider_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ func (l FakeInfrastructureLister) FeatureGateLister() configlistersv1.FeatureGat
func TestObserveCloudProviderNames(t *testing.T) {
cases := []struct {
name string
infrastructureSpec configv1.InfrastructureSpec
infrastructureStatus configv1.InfrastructureStatus
fgSelection configv1.FeatureGateSelection
expected string
Expand Down Expand Up @@ -258,6 +259,43 @@ func TestObserveCloudProviderNames(t *testing.T) {
},
},
cloudProviderCount: 0,
}, {
name: "External platform, CloudControllerManager.State = External",
infrastructureSpec: configv1.InfrastructureSpec{
PlatformSpec: configv1.PlatformSpec{
External: &configv1.ExternalPlatformSpec{
CloudControllerManager: configv1.CloudControllerManagerSpec{
State: configv1.CloudControllerManagerExternal,
},
},
},
},
infrastructureStatus: configv1.InfrastructureStatus{
Platform: configv1.ExternalPlatformType,
PlatformStatus: &configv1.PlatformStatus{
Type: configv1.ExternalPlatformType,
},
},
expected: "external",
cloudProviderCount: 1,
}, {
name: "External platform, CloudControllerManager.State = None",
infrastructureSpec: configv1.InfrastructureSpec{
PlatformSpec: configv1.PlatformSpec{
External: &configv1.ExternalPlatformSpec{
CloudControllerManager: configv1.CloudControllerManagerSpec{
State: configv1.CloudControllerManagerNone,
},
},
},
},
infrastructureStatus: configv1.InfrastructureStatus{
Platform: configv1.ExternalPlatformType,
PlatformStatus: &configv1.PlatformStatus{
Type: configv1.ExternalPlatformType,
},
},
cloudProviderCount: 0,
}, {
name: "empty or unknown platform",
infrastructureStatus: configv1.InfrastructureStatus{
Expand Down Expand Up @@ -288,6 +326,7 @@ func TestObserveCloudProviderNames(t *testing.T) {
ObjectMeta: v1.ObjectMeta{
Name: "cluster",
},
Spec: c.infrastructureSpec,
Status: c.infrastructureStatus,
}
if err := indexer.Add(infra); err != nil {
Expand Down

0 comments on commit e052292

Please sign in to comment.