New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-27222: Allow modified IPv6 address as CPIC name #1727
OCPBUGS-27222: Allow modified IPv6 address as CPIC name #1727
Conversation
@dulek: This pull request references Jira Issue OCPBUGS-27222, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
Hello @dulek! Some important instructions when contributing to openshift/api: |
/jira refresh |
@dulek: This pull request references Jira Issue OCPBUGS-27222, 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 Jira (itbrown@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 openshift-eng/jira-lifecycle-plugin repository. |
tested locally with ipv6 subnet in dhcpv6-stateful mode, egressip worked fine:
|
Thanks @dulek!
As per https://github.com/openshift/enhancements/blob/master/enhancements/network/cloud-egress-ip.md#crd:
This is already done in ovn-kubernetes: |
10867be
to
13dccf7
Compare
/lgtm |
/approve
FTR, this is not true, and the linked doc even agrees ("Most resource types require a name that can be used as a DNS subdomain name", emphasis added). The It's possible that CRD validation forces the RFC-1123 constraint? I'm not exactly sure where in the code the top level of CRD validation is. At any rate, as Patryk notes, we have defined CloudPrivateIPConfig to work in this particular way already, so... |
With the new validation you are requiring that each segment in the IP address is fully spelt out, with no abbreviation ( |
Yup. You can see that in the ovn-k code Patryk linked to:
|
/lgtm |
/assign @sjenning |
13dccf7
to
373b311
Compare
@@ -7,4 +7,4 @@ | |||
type: string | |||
anyOf: | |||
- format: ipv4 | |||
- format: ipv6 | |||
- pattern: '^[0-9a-f]{4}(\.[0-9a-f]{4}){7}$' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, please restore the final new line character
CloudPrivateIPConfig's name is supposed to be an IPv4 or IPv6 address. However, the K8s constraints do not allow colons in the names of the resources, so for IPv6 addresses the names are the addresses fully expanded with colons replaced with dots. E.g. `fc00:f853:ccd:e793::54` becomes `fc00.f853.0ccd.e793.0000.0000.0000.0054`. This means that standard K8s validation of `format: ipv6` will not work as it'll try parsing the address using golang's `net` library and that will fail due to `s/\:/\./`. This commit replaces `format: ipv6` with a pattern that matches fully expanded IPv6 addresses to fix the problem.
373b311
to
fd1e443
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, dulek, JoelSpeed, kyrtapz 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 |
@dulek: 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. |
@dulek: Jira Issue OCPBUGS-27222: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-27222 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift/api#1727 fixes issue where IPv6 CloudPrivateIPConfigs cannot be created because of wrong name validation. This commit imports this change into the CRD created by CNO when deploying cloud-network-config-controller.
openshift/api#1727 fixes issue where IPv6 CloudPrivateIPConfigs cannot be created because of wrong name validation. This commit imports this change into the CRD created by CNO when deploying cloud-network-config-controller.
openshift/api#1727 fixes issue where IPv6 CloudPrivateIPConfigs cannot be created because of wrong name validation. This commit imports this change into the CRD created by CNO when deploying cloud-network-config-controller.
openshift/api#1727 fixes issue where IPv6 CloudPrivateIPConfigs cannot be created because of wrong name validation. This commit imports this change into the CRD created by CNO when deploying cloud-network-config-controller.
[ART PR BUILD NOTIFIER] This PR has been included in build ose-cluster-config-api-container-v4.16.0-202401311812.p0.g4a45f44.assembly.stream for distgit ose-cluster-config-api. |
/cherry-pick release-4.15 |
@dulek: #1727 failed to apply on top of branch "release-4.15":
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. |
Fix included in accepted release 4.16.0-0.nightly-2024-02-02-224339 |
Fix included in accepted release 4.16.0-0.nightly-2024-02-17-013806 |
CloudPrivateIPConfig's name is supposed to be an IPv4 or IPv6 address. However, the K8s constraints do not allow colons in the names of the resources, so for IPv6 addresses the names are the addresses fully expanded with colons replaced with dots. E.g.
fc00:f853:ccd:e793::54
becomesfc00.f853.0ccd.e793.0000.0000.0000.0054
. This means that standard K8s validation offormat: ipv6
will not work as it'll try parsing the address using golang'snet
library and that will fail due tos/\:/\./
.This commit replaces
format: ipv6
with a pattern that matches fully expanded IPv6 addresses to fix the problem.