Bug 1877984: Fix "OpenShiftSDN" to proper case when generating network config #4207
Conversation
openshift-ci-robot
added
bugzilla/severity-urgent
|
@danwinship: This pull request references Bugzilla bug 1877984, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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 kubernetes/test-infra repository. |
|
/approve |
pkg/asset/manifests/network.go
Outdated
| // For backward-compatibility reasons the installer needs to accept "OpenShiftSDN" in the | ||
| // wrong case, but other components aren't expected to deal with it. | ||
| if strings.ToLower(networkType) == "openshiftsdn" { | ||
| networkType = "OpenShiftSDN" |
There was a problem hiding this comment.
And also maybe OVNKubernetes and Kuryr? I was expecting this canonicalization to use this existing variable, which we could make public and move around if needed, and also the networking conversion handling, so we ensured that everyone consuming the install-config would have the canonical casing.
There was a problem hiding this comment.
And also maybe
OVNKubernetesandKuryr?
The original bug was only about accepting both OpenshiftSDN and OpenShiftSDN. The fix was implemented by doing a case-insensitive match, so I did the same here since theoretically someone might be doing networkType: openshiftsdn and it currently works...
CNO currently also accepts OVNKubernetes case-insensitively even though there is no historical reason for that. Given that ovn-kubernetes was not GA until now, and that we don't do case-insensitive matching on any other plugin names (including Kuryr), I think the fix for this is to make CNO stop accepting OVNKubernetes in the wrong case.
I was expecting this canonicalization to use this existing variable, which we could make public and move around if needed
Hm... I did a grep and saw that the string "OpenShiftSDN" was already hardcoded in multiple places, but looking more carefully, most of those places are unit tests. But it seems like really it ought to be defined in openshift/api ? Would you be OK with leaving it hardcoded now but fixing it to use consts from api in 4.7?
and also the networking conversion handling
wrong link (it's the same link as the previous one)
There was a problem hiding this comment.
oh wait, they are defined in openshift/api, it's just in openshift/api/operator, not openshift/api/config
There was a problem hiding this comment.
Oops. Networking conversion:
I'm fine restricting canonicalization to only values where network tooling has canonicalization logic, regardless of how that combines other installer adding canonicalization and networking removing canonicalization.
so that anything else that checks will see the correct value, and anything that looks at the generated network operator config before CNO runs will see the correct value.
danwinship
force-pushed
the
fix-openshiftsdn-case
branch
from
6d5e1f8
to
303c217
Compare
|
I guess this isn't too horrible to do in the Installer given the description and history. |
|
I don't think this change is appropriate because this makes the installer understand what these network types actually are outside of just putting in guard lanes. But it seems like people think that this should be something installer should do. So what do I know. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, knobunc, mtnbikenc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@danwinship: All pull requests linked via external trackers have merged: Bugzilla bug 1877984 has been moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
|
@danwinship: 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/test-infra repository. I understand the commands that are listed here. |
I forget exactly what happened, but way back in the beginning there was a disconnect between the installer and the cluster-network-operator about what the correct spelling of "OpenShiftSDN" was, and it was decided that CNO was going to have to accept both "OpenShiftSDN" and "OpenshiftSDN" for backward compatibility.
However, there are other components that look at the network config sometimes (in particular, MCO now needs to look at it before CNO even runs), so it doesn't make sense for the canonicalization to happen in CNO; we should just not generate the incorrect version of the network config in the first place.