OSASINFRA-3238: Improve API and Ingress VIPs validation #9438
Conversation
|
/retest |
dkokkino
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@dkokkino: This pull request references OSASINFRA-3238 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 task to target the "4.19.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 |
dkokkino
force-pushed
the
OSASINFRA-3238
branch
from
eb8ab29 to
0ecf1c7
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MaysaMacedo 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 |
|
/cc @mkowalski @mandre |
|
/test e2e-openstack-dualstack |
|
/test e2e-metal-ovn-dualstack |
|
@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-sigs/prow repository. |
|
/test e2e-metal-ipi-ovn-dualstack |
There was a problem hiding this comment.
I have a feeling the if-else logic here becomes a real spaghetti...
What we have now is
[1] if len(vips.API) == 0
[2] else if len(vips.API) <= 2
[3] if len(vips.API) >= 1
[4] if len(vips.API) == 2
and it seems to me like this could be simplified. For example [3] is redundant if you look and realize that you did not enter [1].
Thanks for pointing it out! I have removed if statement [3] and made sure the logic within that statement always runs if it enters [2] |
| name: "baremetal API VIP set to an incorrect IP Family", | ||
| installConfig: func() *types.InstallConfig { | ||
| c := validInstallConfig() | ||
| c.Networking = validDualStackNetworkingConfig() | ||
| c.Networking = InvalidPrimaryV6DualStackNetworkingConfig() | ||
| c.Platform = types.Platform{ | ||
| BareMetal: validBareMetalPlatform(), | ||
| } | ||
| c.Platform.BareMetal.APIVIPs = []string{"ffd0::"} | ||
| return c | ||
| }(), | ||
| expectedError: `platform.baremetal.apiVIPs: Invalid value: "ffd0::": VIP for the API must be of the same IP family with machine network's primary IP Family for dual-stack IPv4/IPv6`, | ||
| expectedError: `[platform.baremetal.apiVIPs: Invalid value: "ffd0::": serviceNetwork primary IP Family and primary IP family for the API VIP should match]`, |
There was a problem hiding this comment.
Please add your own test case instead of modifying the current one. They are not testing exactly the same combination, even though in today's logic of validateAPIAndIngressVIPs they run through the same code (tomorrow someone may modify the validation and one thing suddenly regresses and starts to pass)
Basically, your test is
- machine network primary v6
- service network primary v4
- cluster network primary v6
- api vip v6
and the old test is
- machine network primary v4
- service network primary v4
- cluster network primary v4
- api vip v6
There was a problem hiding this comment.
@mkowalski I added my own new tests instead of modifying the current ones as you suggested please let me know if you have any further advice.
There was a problem hiding this comment.
It is great that you added a new one! You only need now to fix the error message for existing test(s) because of your modification, i.e. look at the failure
Error: Expect "[platform.baremetal.ingressVIPs: Invalid value: "ffd0::": machineNetwork primary IP Family and primary IP family for the Ingress VIP should match, platform.baremetal.ingressVIPs: Invalid value: "ffd0::": serviceNetwork primary IP Family and primary IP family for the Ingress VIP should match, platform.baremetal.ingressVIPs: Invalid value: "ffd0::": clusterNetwork primary IP Family and primary IP family for the Ingress VIP should match]" to match "platform.baremetal.ingressVIPs: Invalid value: "ffd0::": VIP for the Ingress must be of the same IP family with machine network's primary IP Family for dual-stack IPv4/IPv6"
In the past there was only one error message for the mismatch. Now you append an error for every of machine, service, and cluster networks. Old test(s) do not know about this change.
You need to go one-by-one over whatever unit test(s) failed and fix expected error messages.
|
Thanks a lot! The logic looks much more elegant now /test e2e-metal-ovn-dualstack |
|
@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-sigs/prow repository. |
|
/test e2e-metal-ipi-ovn-dualstack |
|
/test e2e-openstack-dualstack |
dkokkino
force-pushed
the
OSASINFRA-3238
branch
from
7d82218 to
0e1f225
Compare
• This commit enhances VIPs validation to ensure the primary IP family of the VIPs matches the primary IP family of all the network fields. • Code repurposed from prior closed PR (openshift#7504) Co-Authored-By: Maysa Macedo <[email protected]> Co-Authored-By: Danny Kokkinos <[email protected]>
dkokkino
force-pushed
the
OSASINFRA-3238
branch
from
0e1f225 to
138870e
Compare
|
/test e2e-openstack-dualstack |
|
@dkokkino: 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. |
• This commit enhances VIPs validation to ensure the primary IP family of the VIPs matches the primary IP family of all the network fields.
• Code repurposed from prior closed PR (#7504)