CORS-4180: Allow AWS and Azure as platforms that support dual-stack on Day-0 #2804
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,12 @@ var dualStackPlatforms = sets.NewString( | |
| string(configv1.OpenStackPlatformType), | ||
| ) | ||
|
|
||
| var conversionToDualStackPlatforms = sets.NewString( | ||
| string(configv1.BareMetalPlatformType), | ||
| string(configv1.NonePlatformType), | ||
| string(configv1.VSpherePlatformType), | ||
| ) | ||
|
|
||
| const ( | ||
| pluginName = "networking-console-plugin" | ||
| ) | ||
|
|
@@ -47,6 +53,9 @@ func Render(operConf *operv1.NetworkSpec, clusterConf *configv1.NetworkSpec, man | |
| var progressing bool | ||
| objs := []*uns.Unstructured{} | ||
|
|
||
| // Update the list of supported DualStack platforms based on enabled feature gates. | ||
| updateDualStackPlatforms(featureGates) | ||
|
Contributor
There was a problem hiding this comment.
Edit: this isn't a problem since we use a set. I still don't like the fact that it tries to modify the global variable so often. |
||
|
|
||
|
Comment on lines
+56
to
+58
Member
There was a problem hiding this comment. @sadasu this is added in the This means when the cluster-network-operator is running in the cluster, such function is not called, right? And the list is not modified at all?
Member
There was a problem hiding this comment. /lgtm cancel Pending question above 👀
Member
There was a problem hiding this comment. I think we should have the function WDYT?
Member
There was a problem hiding this comment. Whoops, nvmind... It is also called during reconcilation 😓 so its fine. Please ignore it :D |
||
| // render cloud network config controller **before** the network plugin. | ||
| // the network plugin is dependent upon having the cloud network CRD | ||
| // defined as to initialize its watcher, otherwise it will error and crash | ||
|
|
@@ -400,13 +409,13 @@ func isNetworkChangeSafe(prev, next *operv1.NetworkSpec, infraRes *bootstrap.Inf | |
| return isClusterNetworkChangeSafe(prev, next) | ||
| } | ||
|
|
||
| // Validate that this is a platform that supports DualStack. If it does, then check if | ||
| // migration to DualStack on day-2 is allowed. | ||
| if len(prev.ServiceNetwork) < len(next.ServiceNetwork) { | ||
| if !isSupportedDualStackPlatform(infraRes.PlatformType) { | ||
| return errors.Errorf("%s is not one of the supported platforms for dual stack (%s)", infraRes.PlatformType, | ||
| strings.Join(dualStackPlatforms.List(), ", ")) | ||
| } else if !isConversionSupportedDualStackPlatform(infraRes.PlatformType) { | ||
|
Contributor
There was a problem hiding this comment. I know you didn't introduce this but we can simplify the code and ditch the |
||
| return errors.Errorf("%s does not allow conversion to dual-stack cluster", infraRes.PlatformType) | ||
| } | ||
| } | ||
|
|
@@ -977,6 +986,10 @@ func isSupportedDualStackPlatform(platformType configv1.PlatformType) bool { | |
| return dualStackPlatforms.Has(string(platformType)) | ||
| } | ||
|
|
||
| func isConversionSupportedDualStackPlatform(platformType configv1.PlatformType) bool { | ||
| return conversionToDualStackPlatforms.Has(string(platformType)) | ||
| } | ||
|
|
||
| func renderAdditionalRoutingCapabilities(conf *operv1.NetworkSpec, manifestDir string) ([]*uns.Unstructured, error) { | ||
| if conf == nil || conf.AdditionalRoutingCapabilities == nil { | ||
| return nil, nil | ||
|
|
@@ -999,3 +1012,16 @@ func renderAdditionalRoutingCapabilities(conf *operv1.NetworkSpec, manifestDir s | |
|
|
||
| return out, nil | ||
| } | ||
|
|
||
| // updateDualStackPlatforms checks for enabled feature gates and adds to the list | ||
| // of platforms that support DualStack on Day-0. Currently the 2 platforms added here | ||
| // donot support conversion to DualStack on Day-2. When that happens, we will need to | ||
| // update `conversionToDualStackPlatforms` too. | ||
| func updateDualStackPlatforms(featureGates featuregates.FeatureGate) { | ||
|
Contributor
There was a problem hiding this comment. I don't like the fact that we are modifying the global variable through this function. My proposal is to set the supported platforms as part of the
Author
There was a problem hiding this comment. @kyrtapz happy to make that change but I am not sure how future proof that is. As long as the
Contributor
There was a problem hiding this comment. With the change I proposed we should get rid of the global |
||
| if featureGates.Enabled(apifeatures.FeatureGateAWSDualStackInstall) { | ||
| dualStackPlatforms.Insert(string(configv1.AWSPlatformType)) | ||
| } | ||
| if featureGates.Enabled(apifeatures.FeatureGateAzureDualStackInstall) { | ||
| dualStackPlatforms.Insert(string(configv1.AzurePlatformType)) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Previously, only
OpenStackis rejected. Just wondering if we confirmed this is OK?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code comments seem to indicate so, but I have 0 clue 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the "allow" list so it contains the list of platforms that allow conversion to DualStack on Day-2. The code earlier was looking at OpenStack platform as the reject list. It could be implemented either ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sounds good! I was just unsure if there are any other platforms that allow dualstack, which is not yet accounted for here...