Bug 1780376: Exclude IPv6 Registry Locations #52
Conversation
Exclude IPv6 addresses from the list of registry locations. These are invalid locations to reference in an image pull spec. Use IPv4 if IPv6 address is convertable.
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@adambkaplan: This pull request references Bugzilla bug 1780376, 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. 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-robot
added
size/L
| ipv4 := ip.To4() | ||
| if ipv4 != nil { | ||
| ret = append(ret, net.JoinHostPort(ipv4.String(), fmt.Sprintf("%d", svcPort))) | ||
| } |
There was a problem hiding this comment.
If the IP address is an IPv6 address, ip.To4() will return nil.
Note that IPv6 addresses that start with ::FFFF: can be converted to IPv4.
|
/cc @danwinship |
|
Do you have any more info about why IPv4 is valid, but IPv6 is not? Is this a feature gap somewhere? Is this gap tracked somewhere? What's the impact for a cluster that only has IPv6? |
|
@russellb @danwinship IPv6 addresses are not valid hostnames in an image pull spec. This is not an issue for the internal registry because we have a DNS name for its service within the cluster. I believe we kept this around because in the early days of OCP 3.x we did not have a service name for the registry, and hence used the |
|
/lgtm The change looks right to me if IPv6 addresses are not supported. I would find it helpful to have links to any relevant references to where this might be discussed or tracked in an appropriate upstream, in case anyone wants to learn more, or go look at how to add support for this in the future. Thanks again for the fix! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, russellb 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 |
|
@adambkaplan: All pull requests linked via external trackers have merged. Bugzilla bug 1780376 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. |
|
@russellb I believe the issue is that IPv6 addresses use colons to separate the address blocks, rather than periods. This confuses how upstream pull spec parsers work, which rely on a final ":" to denote a tag name. |
|
/cherry-pick 4.3 @adambkaplan can you help with the 4.3 backport? |
|
@russellb: cannot checkout 4.3: error checking out 4.3: exit status 1. output: error: pathspec '4.3' did not match any file(s) known to git. 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-4.3 |
|
@russellb: new pull request created: #57 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. |
Exclude IPv6 addresses from the list of registry locations.
These are invalid locations to reference in an image pull spec.
Use IPv4 if IPv6 address is convertable.