CORS-4180: Allow AWS and Azure as platforms that support dual-stack on Day-0 #2804
Conversation
sadasu
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sadasu: This pull request references CORS-4180 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 the "4.21.0" version, but no target version was set. In 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. |
|
@sadasu: This pull request references CORS-4180 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 the "4.21.0" version, but no target version was set. In 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. |
|
/retest-required |
|
This looks good to me. AWS tests are failing to find a lease... i think we're hitting ci infra problems. |
|
/retest-required |
sadasu
force-pushed
the
update-for-dualstack
branch
from
4696a48 to
621b294
Compare
|
@sadasu: This pull request references CORS-4180 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 the "4.21.0" version, but no target version was set. In 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. |
sadasu
force-pushed
the
update-for-dualstack
branch
from
621b294 to
9fbf4db
Compare
|
/jira-refresh |
|
/retest-required |
|
/cc |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/retest |
|
/lgtm |
pkg/network/render.go
Outdated
| return nil, progressing, err | ||
| } | ||
|
|
||
| updateDualStackPlatforms(featureGates) |
There was a problem hiding this comment.
Should we move the function to earlier in the func Render so that any rendering, if any later on, can always pick up the most up-to-date list of supported dualstack platform?
pkg/network/render.go
Outdated
| var conversionToDualStackPlatforms = sets.NewString( | ||
| string(configv1.BareMetalPlatformType), | ||
| string(configv1.NonePlatformType), | ||
| string(configv1.VSpherePlatformType), | ||
| ) |
There was a problem hiding this comment.
question: Previously, only OpenStack is rejected. Just wondering if we confirmed this is OK?
There was a problem hiding this comment.
The code comments seem to indicate so, but I have 0 clue 😞
There was a problem hiding this comment.
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.
Oh sounds good! I was just unsure if there are any other platforms that allow dualstack, which is not yet accounted for here...
sadasu
force-pushed
the
update-for-dualstack
branch
from
9fbf4db to
43ceb4d
Compare
openshift-merge-robot
added
the
needs-rebase
|
Maybe another rebase? |
sadasu
force-pushed
the
update-for-dualstack
branch
from
43ceb4d to
d95a14b
Compare
openshift-merge-robot
removed
the
needs-rebase
pkg/network/render.go
Outdated
| // Update the list of supported DualStack platforms based on enabled feature gates. | ||
| updateDualStackPlatforms(featureGates) | ||
|
|
There was a problem hiding this comment.
@sadasu this is added in the Render function is, I believe, is called when rendering manifests for bootstrap.
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?
There was a problem hiding this comment.
/lgtm cancel
Pending question above 👀
There was a problem hiding this comment.
I think we should have the function isSupportedDualStackPlatform keeps the list of supported platforms (i.e. both static and feature-gated items), instead of having a global variable and modifying them at runtime.
WDYT?
There was a problem hiding this comment.
Whoops, nvmind... It is also called during reconcilation 😓 so its fine. Please ignore it :D
pkg/network/render.go
Outdated
| // 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) { |
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 bootstrap.InfraStatus and read it from there. Same applies to conversionToDualStackPlatforms.
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 dualStackPlatforms is in place, we run the risk of someone using that global instead of the InfraStatus.
There was a problem hiding this comment.
With the change I proposed we should get rid of the global dualStackPlatforms variable.
There was a problem hiding this comment.
Thanks @sadasu!
The new approach makes sense to me and it worked when testing with openshift/installer#9930. Though obviously I am not the expert here, maybe @kyrtapz can have the final thought 🙏
pkg/network/render.go
Outdated
| objs := []*uns.Unstructured{} | ||
|
|
||
| // Update the list of supported DualStack platforms based on enabled feature gates. | ||
| updateDualStackPlatforms(featureGates) |
There was a problem hiding this comment.
Calling it here would mean adding elements to dualStackPlatforms every time Render is called, we cannot do that.
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.
pkg/network/render.go
Outdated
| return errors.Errorf("%s is not one of the supported platforms for dual stack (%s)", infraRes.PlatformType, | ||
| strings.Join(dualStackPlatforms.List(), ", ")) | ||
| } else if string(configv1.OpenStackPlatformType) == string(infraRes.PlatformType) { | ||
| } else if !isConversionSupportedDualStackPlatform(infraRes.PlatformType) { |
There was a problem hiding this comment.
I know you didn't introduce this but we can simplify the code and ditch the else if since the if returns.
something like this:
if !isSupportedDualStackPlatform(infraRes.PlatformType) {
return ...
}
if !isConversionSupportedDualStackPlatform(infraRes.PlatformType) {
return ...
}
Bringing in the feature gates added for AWS and Azure DualStack support.
This is required to bring in the lastest featuregates
sadasu
force-pushed
the
update-for-dualstack
branch
from
d95a14b to
2dffbc8
Compare
WalkthroughGo module versions were bumped. Feature-gate support for dual-stack was added and propagated through controllers, validation, rendering, and tests by threading a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (179)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_apiserver.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_cluster_operator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_version.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.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**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/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/console/v1/types_console_cli_download.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/console/v1/types_console_link.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/console/v1/zz_generated.featuregated-crd-manifests.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/features/legacyfeaturegates.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/imageregistry/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/imageregistry/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/legacyconfig/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/legacyconfig/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machinehealthcheck.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types_machineconfignode.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types_machineosbuild.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types_machineosconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_machineosbuild.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_machineosconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.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_machineconfiguration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/azureplatformstatus.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/alertmanagerconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/alertmanagercustomconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/audit.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clustermonitoringspec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/containerresource.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/metricsserverconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/irreconcilablechangediff.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfignodespec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfignodespecconfigimage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfignodestatus.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfignodestatusconfigimage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/buildinputs.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/buildoutputs.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/imagesecretobjectreference.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/machineconfigpoolreference.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/machineosbuild.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/machineosbuilderreference.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/machineosbuildspec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/machineosbuildstatus.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/machineosconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/machineosconfigreference.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/machineosconfigspec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/machineosconfigstatus.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/machineoscontainerfile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/machineosimagebuilder.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/objectreference.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/renderedmachineconfigreference.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/clientset/versioned/typed/machineconfiguration/v1alpha1/generated_expansion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/clientset/versioned/typed/machineconfiguration/v1alpha1/machineconfiguration_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/clientset/versioned/typed/machineconfiguration/v1alpha1/machineosbuild.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/clientset/versioned/typed/machineconfiguration/v1alpha1/machineosconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/informers/externalversions/generic.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/informers/externalversions/machineconfiguration/v1alpha1/interface.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/informers/externalversions/machineconfiguration/v1alpha1/machineosbuild.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/informers/externalversions/machineconfiguration/v1alpha1/machineosconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/listers/machineconfiguration/v1alpha1/expansion_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/listers/machineconfiguration/v1alpha1/machineosbuild.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machineconfiguration/listers/machineconfiguration/v1alpha1/machineosconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/bootimageskewenforcementconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/bootimageskewenforcementstatus.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/clusterbootimageautomatic.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/clusterbootimagemanual.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/irreconcilablevalidationoverrides.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/machineconfigurationspec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/machineconfigurationstatus.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/utils.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/annotations.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/operator/resource/resourceapply/storage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/networking.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/status/status_controller.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/status/version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/v1helpers/informers.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/http2.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_go15_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/plan9/pwd_plan9.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/affinity_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/mkerrors.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_darwin.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/syscall_solaris.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_386.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_arm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_arm64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_loong64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mipsle.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_riscv64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_s390x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_sparc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsyscall_solaris_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_386.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_arm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_arm64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_loong64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_mips.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_mips64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_mips64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_mipsle.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_ppc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_ppc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_ppc64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_riscv64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_s390x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zsysnum_linux_sparc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_386.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_arm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_arm64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_loong64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_mips.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_mips64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_mips64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_mipsle.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_ppc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_ppc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_ppc64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_riscv64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_s390x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_linux_sparc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/registry/zsyscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/types_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/windows/zsyscall_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/term/term_windows.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/term/terminal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/astutil/enclosing.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/packages/doc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/imports/source_modindex.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/modindex/directories.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/modindex/index.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/modindex/modindex.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/modindex/symbols.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/modindex/types.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (7)
go.mod(3 hunks)pkg/controller/clusterconfig/clusterconfig_controller.go(3 hunks)pkg/controller/operconfig/cluster.go(1 hunks)pkg/network/cluster_config.go(3 hunks)pkg/network/cluster_config_test.go(9 hunks)pkg/network/render.go(2 hunks)pkg/network/render_test.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/controller/clusterconfig/clusterconfig_controller.gopkg/controller/operconfig/cluster.gopkg/network/cluster_config_test.gopkg/network/cluster_config.gopkg/network/render.gopkg/network/render_test.gogo.mod
🔇 Additional comments (2)
go.mod (1)
22-22: Security verification complete; manually verify openshift package feature gate support.The golang.org/x package updates (net v0.43.0, crypto v0.42.0, sys v0.36.0) are secure with no active CVE advisories. All known vulnerabilities in these packages have been fixed in earlier patches.
However, verify that the openshift packages (api, client-go, library-go) actually include the DualStack feature gate support referenced in the PR by checking their changelogs or repository commits. Cross-package version compatibility within the openshift ecosystem should also be confirmed.
pkg/controller/clusterconfig/clusterconfig_controller.go (1)
29-37: Feature-gate plumbing looks solid.Passing the
featureGateshandle from controller setup through the reconciler down intoValidateClusterConfigkeeps validation aligned with the operator’s gate state. Thanks for keeping the wiring tight.Also applies to: 60-64, 96-96
| return errors.Errorf("%s is not one of the supported platforms for dual stack (%s)", infraRes.PlatformType, | ||
| strings.Join(dualStackPlatforms.List(), ", ")) | ||
| } else if string(configv1.OpenStackPlatformType) == string(infraRes.PlatformType) { | ||
| if !isConversionToDualStackSupported(infraRes.PlatformType) { |
There was a problem hiding this comment.
@kyrtapz please note here that I am only checking if the conversion to DualStack is possible. I removed the check !isSupportedDualStackPlatform() because the method isNetworkChangeSafe() is checking for Day-2 changes.
There was a problem hiding this comment.
I guess this is fine under the assumption: if a platform supports converting to dual-stack, it really should support dual-stack, right 😁?
The difference here is just error message :D
|
|
||
| func isSupportedDualStackPlatform(platformType configv1.PlatformType, featureGates featuregates.FeatureGate) bool { | ||
| switch platformType { | ||
| case configv1.BareMetalPlatformType, configv1.NonePlatformType, configv1.VSpherePlatformType, configv1.OpenStackPlatformType: |
There was a problem hiding this comment.
Do you want me to add Platform: External to this list? It is treated identical to Platform:None in other places.
|
|
||
| func isConversionToDualStackSupported(platformType configv1.PlatformType) bool { | ||
| switch platformType { | ||
| case configv1.BareMetalPlatformType, configv1.NonePlatformType, configv1.VSpherePlatformType: |
There was a problem hiding this comment.
Ditto:Do you want me to add Platform: External to this list? It is treated identical to Platform:None in the Installer.
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
This approach looks better to me 👍!
I tested with openshift/installer#9930 and custom release image quay.io/thvo/origin-release:v4.21.0-preview-1. The install completed and resulted in an AWS dualstack cluster (i.e. CNI is happy).
I have a few suggestion along with CodeRabbit 👀
pkg/network/render.go
Outdated
| // Validate that this is either a BareMetal or None PlatformType. For all other | ||
| // PlatformTypes, migration to DualStack is prohibited |
There was a problem hiding this comment.
We should probably update this comment too (or remove it) since Vsphere also support dualstack conversion.
| // You can't use dual-stack if enabled on an unsupported platform | ||
| infrastructure.Status.PlatformStatus.Type = configv1.GCPPlatformType | ||
| infrastructure.Status.PlatformStatus.GCP = &configv1.GCPPlatformStatus{ | ||
| Region: "us-west1", | ||
| } |
There was a problem hiding this comment.
Should we also have test cases that validate if there is no error when a platform:
- Support dualstack out of the box.
- Support dualstack with Feature gate.
There was a problem hiding this comment.
Should we also have test cases that validate if there is no error when a platform:
- Support dualstack out of the box.
This already exists
- Support dualstack with Feature gate.
Added a test for that.
| return errors.Errorf("%s is not one of the supported platforms for dual stack (%s)", infraRes.PlatformType, | ||
| strings.Join(dualStackPlatforms.List(), ", ")) | ||
| } else if string(configv1.OpenStackPlatformType) == string(infraRes.PlatformType) { | ||
| if !isConversionToDualStackSupported(infraRes.PlatformType) { |
There was a problem hiding this comment.
I guess this is fine under the assumption: if a platform supports converting to dual-stack, it really should support dual-stack, right 😁?
The difference here is just error message :D
1. Added AWS and Azure as platforms that support DualStack on Day-0 when the right featuregates are enabled. 2. Removed the global constant that kept track of platforms that supported DualStack. 3. Added AWS, Azure and OpenStack as platforms that do not support conversion to DualStack on Day-2. 4. Updated tests to reflect the newly supported platforms and when they support DualStack.
sadasu
force-pushed
the
update-for-dualstack
branch
from
2dffbc8 to
e012af9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/network/render_test.go (1)
356-368: Consider deduplicating feature gate helper.This helper
getDefaultFeatureGatesWithDualStack()appears to be duplicated inpkg/network/cluster_config_test.go(lines 41-53). Consider extracting it to a shared test utility file to avoid duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
pkg/controller/clusterconfig/clusterconfig_controller.go(3 hunks)pkg/controller/operconfig/cluster.go(1 hunks)pkg/network/cluster_config.go(3 hunks)pkg/network/cluster_config_test.go(10 hunks)pkg/network/render.go(2 hunks)pkg/network/render_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/clusterconfig/clusterconfig_controller.go
- pkg/controller/operconfig/cluster.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/network/cluster_config_test.gopkg/network/render.gopkg/network/render_test.gopkg/network/cluster_config.go
🔇 Additional comments (13)
pkg/network/render.go (3)
1001-1008: LGTM!The Day-2 conversion logic correctly restricts dual-stack conversion to platforms that support it, excluding AWS, Azure, and OpenStack as intended.
396-401: LGTM!The simplified Day-2 dual-stack conversion check correctly validates platform support using the new helper function and provides a clear error message.
987-999: ****The review comment is based on an incorrect premise.
ExternalPlatformTypedoes not exist in this codebase—a search for it produces no results. The functionisSupportedDualStackPlatformcorrectly includes the platforms that support dual-stack without feature gates (BareMetal, None, VSphere, OpenStack) and gates AWS and Azure behind their respective feature flags. No changes are needed.Likely an incorrect or invalid review comment.
pkg/network/render_test.go (2)
237-259: LGTM!The test correctly validates that only BareMetal, None, and VSphere platforms support Day-2 dual-stack conversion, using GCP as the negative test case. The error message validation is precise.
421-423: LGTM!The test correctly constructs and passes feature gates to the
Renderfunction, exercising the new feature gate integration.pkg/network/cluster_config_test.go (5)
79-83: LGTM!The
haveErrorhelper correctly uses thecfgparameter passed to it (line 81), addressing the previous review concern. The feature gates are appropriately captured from the outer scope.
323-336: LGTM!The helper clearly simulates a scenario where Azure dual-stack feature gate is disabled, enabling testing of validation failures when required feature gates are missing.
407-423: LGTM!The test correctly validates that GCP platform rejects dual-stack configuration with an appropriate error message.
424-440: LGTM!The test correctly validates that AWS platform allows dual-stack when the AWS dual-stack feature gate is enabled.
441-457: LGTM!The test correctly validates that Azure platform rejects dual-stack when the Azure dual-stack feature gate is disabled, providing comprehensive coverage for feature-gated platform validation. This addresses the previous review comment requesting test cases for feature-gated platforms.
pkg/network/cluster_config.go (3)
35-42: LGTM!The signature update correctly adds feature gate support to the public validation API and properly threads it through to the internal validation function.
44-44: LGTM!The internal validation function signature correctly accepts feature gates for use in platform-specific validation.
120-124: LGTM!The dual-stack platform validation correctly uses the feature-gate-aware helper function and provides a clear error message when the platform doesn't support dual-stack.
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
/lgtm
Based on #2804 (review), looks good and worked for me!
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jhixson74, sadasu, tthvo The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified by @tthvo |
openshift-ci-robot
added
the
verified
|
@tthvo: This PR has been marked as verified by In 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. |
|
/retest |
|
@sadasu: The following tests failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
7 participants
Uh oh!
There was an error while loading. Please reload this page.