OPNET-512: config/v1/types_infrastructure: change set to atomic for networks #1873
Conversation
|
Hello @mkowalski! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/M
mkowalski
force-pushed
the
api-update-for-cno
branch
from
2135158
to
83f7f27
Compare
openshift-ci
bot
added
size/XL
| // +optional | ||
| IngressIPs []IP `json:"ingressIPs"` | ||
|
|
||
| // machineNetworks are IP networks used to connect all the OpenShift cluster | ||
| // nodes. Each network is provided in the CIDR format and should be IPv4 or IPv6, | ||
| // for example "10.0.0.0/8" or "fd00::/8". | ||
| // +listType=set | ||
| // +listType=atomic | ||
| // +kubebuilder:validation:MaxItems=32 | ||
| // +optional | ||
| MachineNetworks []CIDR `json:"machineNetworks"` |
There was a problem hiding this comment.
Need CEL to keep set semantic here and on every other MachineNetworks
There was a problem hiding this comment.
Right, adding
// +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x == y))"
and testing. Can you confirm it's okay to have CEL only in Spec and leave it out of Status as only the former is supposed to accept user input?
There was a problem hiding this comment.
Prefer to have it in both, because while only spec is supposed to be accessible by user, they can still edit status
There was a problem hiding this comment.
Fair enough. Given that API and Ingress VIP have CELs only in Spec, do you want me to also add them to Status in this PR ?
There was a problem hiding this comment.
If we are confident that isn't going to break anything, then yes.
The no breaking way to add it is to add ratcheting, prefix with self == oldSelf ||
There was a problem hiding this comment.
I think "confident" is a stretch because in the past this field was basically copy-pasted from install-config.yaml and all the validations were happening in o/installer. I believe it wouldn't break anything, but it's more faith than certainty
Was the installer doing any validation?
Yes, we have https://github.com/openshift/cluster-network-operator/blob/master/pkg/controller/infrastructureconfig/validations.go#L40-L50 that is validating that if you provide more than 1 VIP, you need to have multiple IP families. So we will not allow setting 2 IPs that are IPv4-only or IPv6-only
At what stage does this occur? Does it cause the cluster to degrade or anything?
There was a problem hiding this comment.
Was the installer doing any validation?
Yes, o/installer should prevent you from adding 2 VIPs of the same IP stack to the install-config.yaml
At what stage does this occur? Does it cause the cluster to degrade or anything?
This happens in a controller inside CNO which reconciles Infrastructure CR. So if you edit Infra with "illegal" values, the output of oc get co will show you that Network Operator is degraded and the message will be something around 2 vips of the same ip stack are not allowed
There was a problem hiding this comment.
Ok, and when were these APIs introduced as GA? I think we are safe to copy the validation to the status provided the installer has already validated this and you go degraded if the status gets edited.
If the status gets edited to invalid, does it get put back by anything?
There was a problem hiding this comment.
when were these APIs introduced as GA
#1152 (merged in August 2022) + openshift/enhancements#1048 (looks like 4.11)
If the status gets edited to invalid, does it get put back by anything?
Yes. In the master branch of CNO if you modify Status without modifying Spec, Status will get reverted because operator reconciles Status to match Spec. So for a scenario
- start from valid Spec and Status
- update with invalid Status (without modifying Spec)
you end up with no changes applied because Spec is authoritative over Status. You would need to modify Spec and Status, but then Spec is validated via CEL already now and that will stop you.
Hope this explanation is enough to support statement "we have no way of introducing illegal values into Status"
There was a problem hiding this comment.
Ack, SGTM, lets copy over the validation from spec to status and enforce in the same way there too
Edit: I see you've done that before I commented
mkowalski
changed the title
|
@mkowalski: This pull request references OPNET-512 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.16.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. |
openshift-ci-robot
added
the
jira/valid-reference
mkowalski
force-pushed
the
api-update-for-cno
branch
2 times, most recently
from
e9599f3
to
454f5bf
Compare
|
/cc @cybertron |
|
/test e2e-metal-ipi |
|
@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. |
|
The current version of the PR has been tested using the following procedure
|
|
/retest-required |
mkowalski
force-pushed
the
api-update-for-cno
branch
from
454f5bf
to
72feba0
Compare
|
@mkowalski: This pull request references OPNET-512 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.16.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. |
openshift-ci
bot
added
size/XXL
In order to handle ownership of network addresses in a more elegant way, we are switching list type from `set` to `atomic`. Thanks to this the whole list will be managed as one, instead of different owners for every entry on the list. We are adding CEL validation to fields in PlatformStatus for the consistency. As this field has already been validated by o/installer, this should not affect CRs with PlatformStatus already populated.
mkowalski
force-pushed
the
api-update-for-cno
branch
from
72feba0
to
b5cb8e3
Compare
|
/retest-required |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed, 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 |
openshift-ci
bot
added
the
approved
|
@mkowalski: all tests passed! 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. |
|
[ART PR BUILD NOTIFIER] This PR has been included in build ose-cluster-config-api-container-v4.16.0-202404291547.p0.gac9356b.assembly.stream.el9 for distgit ose-cluster-config-api. |
In order to handle ownership of network addresses in a more elegant way,
we are switching list type from
settoatomic. Thanks to this thewhole list will be managed as one, instead of different owners for every
entry on the list.
We are adding CEL validation to fields in PlatformStatus for the
consistency. As this field has already been validated by o/installer,
this should not affect CRs with PlatformStatus already populated.