Bug 2007106: Extend dual-stack network subnet validations #2660
Conversation
openshift-ci
bot
added
bugzilla/severity-urgent
|
@mkowalski: This pull request references Bugzilla bug 2007106, which is invalid:
Comment 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. |
1 similar comment
|
@mkowalski: This pull request references Bugzilla bug 2007106, which is invalid:
Comment 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. |
openshift-ci
bot
added
size/L
|
/test e2e-metal-assisted-operator-ztp |
|
/test e2e-metal-assisted-kube-api |
|
/test ci/prow/subsystem-kubeapi-aws |
|
@mkowalski: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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. |
|
/test subsystem-kubeapi-aws |
|
/hold It may be failing for a reason. Need to investigate |
openshift-ci
bot
added
the
do-not-merge/hold
|
/bugzilla refresh |
openshift-ci
bot
added
the
bugzilla/valid-bug
|
@flaper87: This pull request references Bugzilla bug 2007106, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (yobshans@redhat.com), skipping review request. 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. |
openshift-ci
bot
removed
the
bugzilla/invalid-bug
mkowalski
force-pushed
the
bz2007106
branch
from
fc6a55a
to
ebd58a1
Compare
|
/hold cancel /test e2e-metal-assisted-operator-ztp |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@mkowalski: This pull request references Bugzilla bug 2007106, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (yobshans@redhat.com), skipping review request. 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. |
| clusterNetworksWrongOrder := common.TestDualStackNetworking.ClusterNetworks | ||
| clusterNetworksWrongOrder := TestDualStackNetworkingWrongOrder.ClusterNetworks | ||
| clusterNetworksWrongOrder[0], clusterNetworksWrongOrder[1] = clusterNetworksWrongOrder[1], clusterNetworksWrongOrder[0] | ||
|
|
||
| serviceNetworksWrongOrder := common.TestDualStackNetworking.ServiceNetworks | ||
| serviceNetworksWrongOrder := TestDualStackNetworkingWrongOrder.ServiceNetworks | ||
| serviceNetworksWrongOrder[0], serviceNetworksWrongOrder[1] = serviceNetworksWrongOrder[1], serviceNetworksWrongOrder[0] | ||
|
|
||
| machineNetworksWrongOrder := common.TestDualStackNetworking.MachineNetworks | ||
| machineNetworksWrongOrder := TestDualStackNetworkingWrongOrder.MachineNetworks | ||
| machineNetworksWrongOrder[0], machineNetworksWrongOrder[1] = machineNetworksWrongOrder[1], machineNetworksWrongOrder[0] |
There was a problem hiding this comment.
Not sure...
If you look at the definition
TestDualStackNetworkingWrongOrder = common.TestNetworking{
ClusterNetworks: append(common.TestIPv4Networking.ClusterNetworks, common.TestIPv6Networking.ClusterNetworks...),
ServiceNetworks: append(common.TestIPv4Networking.ServiceNetworks, common.TestIPv6Networking.ServiceNetworks...),
MachineNetworks: append(common.TestIPv4Networking.MachineNetworks, common.TestIPv6Networking.MachineNetworks...),
APIVip: common.TestIPv4Networking.APIVip,
IngressVip: common.TestIPv4Networking.IngressVip,
}
it creates TestDualStackNetworkingWrongOrder as concatenation of IPv4 networks and IPv6 networks (so the order is still correct). Only afterwards we are doing
clusterNetworksWrongOrder := TestDualStackNetworkingWrongOrder.ClusterNetworks
clusterNetworksWrongOrder[0], clusterNetworksWrongOrder[1] = clusterNetworksWrongOrder[1], clusterNetworksWrongOrder[0]
[...]
so in the structure built above we are reversing 1st and 2nd subnet. As there is a bunch of pointers hidden inside, modifying elements of clusterNetworksWrongOrder is in fact modifying content of TestDualStackNetworkingWrongOrder.ClusterNetworks
Or did I miss something simpler here ?
| // * there are exactly two machine networks | ||
| // * the first one is IPv4 subnet | ||
| // * the second one is IPv6 subnet | ||
| func VerifyMachineNetworksDualStack(networks []*models.MachineNetwork, isDualStack bool) error { |
There was a problem hiding this comment.
Can you do some magic for these 3 duplicated functions?
There was a problem hiding this comment.
I was considering this, but my logic was as follows - today all the requirements are the same for those networks
- max 2 entries
- first one v4
- and so on...
But those requirements in the upstream OCP are not aligned, e.g. the limit of clusterNetworks is something we impose as AI, but OCP does not. It should be relatively easy for us to support it, but we'd need to test it extensively so as for today, we don't. However, it already boils down to saying that in a very near future we will have different sets of requirements for different networks. As soon as we need to implement those, we'd need to undo the magic because the same function will not be able to handle different cases.
So, instead of this back-and-forth, I went with the simplest way that is - today we have 3 functions that look almost the same, but it enables us to already now modify validators independently.
What do you think ?
There was a problem hiding this comment.
I would also add that these 3 networks are not ruled by the same assumptions, either in OCP or in Assisted. Although it looks like that right now. The implementation would also be a bit hacky.
I would go the python way here, explicit is better than implicit (and the type system is guiding us that way)
mkowalski
force-pushed
the
bz2007106
branch
2 times, most recently
from
c035bcc
to
cb23224
Compare
openshift-ci
bot
added
bugzilla/invalid-bug
|
@mkowalski: This pull request references Bugzilla bug 2007106, which is invalid:
Comment 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. |
|
/bugzilla refresh |
|
@mkowalski: This pull request references Bugzilla bug 2007106, which is invalid:
Comment 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. |
|
/bugzilla refresh |
openshift-ci
bot
added
bugzilla/valid-bug
|
@mkowalski: This pull request references Bugzilla bug 2007106, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
/cherry-pick ocm-2.4 2009759 is the backport BZ |
|
@mkowalski: once the present PR merges, I will cherry-pick it on top of ocm-2.4 in a new PR and assign it to you. 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. |
|
/test ? |
|
@mkowalski: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
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. |
|
/test e2e-ai-operator-ztp-ipv4v6-3masters-ocp-48 |
| return errors.Errorf("First machine network has to be IPv4 subnet") | ||
| } | ||
| if !IsIPv6CIDR(string(networks[1].Cidr)) { | ||
| return errors.Errorf("Second machine network has to be IPv6 subnet") |
There was a problem hiding this comment.
Have we given any thought to creating custom error types?
There was a problem hiding this comment.
Never thought about this honestly. How would that look like ?
There was a problem hiding this comment.
I may be missing something from @djzager's idea but, considering that we don't check the type of any of these errors, I'm unsure how much value custom error types would bring. Perhaps reduce duplication of error messages? Could we use constants for some those?
There was a problem hiding this comment.
I'd keep those ones as they are. The strings are only for humans to read them, we don't automate anything based on errors and based on my previous experience, it may happen that we start changing those messages very soon based on the users' understanding of them
There was a problem hiding this comment.
More of a curiosity question than trying to ask for a change.
The only thing that stands out to me as a con to using errors.Errorf was that all of our tests are just looking for some hardcoded string. The pro I've experienced is that it makes it easy to find where a failure is coming from when the strings are hardcoded this way 😎 .
This can be resolved.
| return nil | ||
| } | ||
|
|
||
| func ValidateDualStackNetworks(clusterParams interface{}) error { |
There was a problem hiding this comment.
Does this need to be public/exported?
There was a problem hiding this comment.
Nope, it does not need. It's only like this because other validators in this file are also public even if they are used only in this package. Do you prefer to make this one private or to have them all the same ?
There was a problem hiding this comment.
Generally code is easier to maintain when there is less exporting, so I would go that direction. It's very easy to come back later and export it if/when the need arises.
There was a problem hiding this comment.
I agree with @mhrivnak and @djzager here. Let's make an attempt to default to private as much as possible.
I would even be ok with a follow-up PR (that won't be backported) that makes the rest of these functions private (for the sake of consistency). Although, I would also understand it if people would rather not do such a refactor :)
| valid: false, | ||
| }, | ||
| { | ||
| element: []*string{swag.String("1.2.5.0/24"), swag.String("1002:db8::/119"), swag.String("1.2.6.0/24"), swag.String("1.2.7.0/24")}, | ||
| valid: true, | ||
| element: []*models.MachineNetwork{{Cidr: "1.2.5.0/24"}, {Cidr: "1002:db8::/119"}, {Cidr: "1.2.6.0/24"}, {Cidr: "1.2.7.0/24"}}, |
There was a problem hiding this comment.
For me and others coming here later, it would be helpful to comment in some way why these are valid or invalid.
Is this one invalid because only two are allowed?
| for _, t := range tests { | ||
| t := t |
There was a problem hiding this comment.
| for _, t := range tests { | |
| t := t | |
| for _, test := range tests { | |
| t := test |
I'm surprised with all the linting checks for shadowing that this is allowed. A little nitpicky but I think we should avoid this if we can.
There was a problem hiding this comment.
As previously, I see this pattern in 5 different tests in this file. Do we want to change it in here or follow the current style ? (cc @flaper87)
There was a problem hiding this comment.
I guess we can change this here but definitely let's not change all the cases like this in the file. Let's keep the backport as small as possible.
This commit extends the validations performed for dual-stack OCP clusters by making sure the following requirements are met if a cluster is dual-stack * exactly 2 machine networks are provided (if any) * exactly 2 service networks are provided * exactly 2 cluster networks are provided * for every list containing 2 networks, the first one has to be IPv4 and the second one IPv6 Closes-bug: #2007106 Contributes-to: #2005440 Closes: OCPBUGSM-35447 Contributes-to: OCPBUGSM-35273
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djzager, mkowalski 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 |
|
@mkowalski: 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. |
|
/retest-required |
|
@mkowalski: All pull requests linked via external trackers have merged: Bugzilla bug 2007106 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. |
|
@mkowalski: cannot checkout 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. |
|
/cherry-pick release-ocm-2.4 2009759 is the backport BZ |
|
@mkowalski: new pull request created: #2704 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. |
Assisted Pull Request
Description
This commit extends the validations performed for dual-stack OCP
clusters by making sure the following requirements are met if a cluster
is dual-stack
the second one IPv6
Closes-bug: #2007106
Contributes-to: #2005440
Closes: OCPBUGSM-35447
Contributes-to: OCPBUGSM-35273
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Manual test for single service network
KubeAPI
Rest API
The service is not crashing. Test succeeded.
Manual test for single machine network
KubeAPI
Does not throw an error yet, that's why only
Contributes-to: #2005440and notCloses-bug: #2005440Rest API
Assignees
/cc @flaper87
/cc @YuviGold
/cc @ori-amizur
Checklist
docs, README, etc)Reviewers Checklist