Skip to content

WIP: Fix HostPortOrFile to support IPv6 addresses with zone#19

Closed
bcrochet wants to merge 1 commit intoopenshift:masterfrom
bcrochet:hostportorfile
Closed

WIP: Fix HostPortOrFile to support IPv6 addresses with zone#19
bcrochet wants to merge 1 commit intoopenshift:masterfrom
bcrochet:hostportorfile

Conversation

@bcrochet
Copy link
Copy Markdown
Member

  1. The HostPortOrFile tests don't have any IPv6 tests. This adds some.
  2. The HostPortOrFile breaks if any of the addresses have IPv6 zone
    defined. ParseIP does not handle %zone anymore.

Signed-off-by: Brad P. Crochet brad@redhat.com

1. Why is this pull request needed and what does it do?

2. Which issues (if any) are related?

3. Which documentation changes (if any) need to be made?

4. Does this introduce a backward incompatible change or deprecation?

1. The HostPortOrFile tests don't have any IPv6 tests. This adds some.
2. The HostPortOrFile breaks if any of the addresses have IPv6 zone
defined. ParseIP does not handle %zone anymore.

Signed-off-by: Brad P. Crochet <brad@redhat.com>
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 14, 2020
@ironcladlou
Copy link
Copy Markdown

I think this patch should go upstream first, what do you all think?

@openshift-ci-robot
Copy link
Copy Markdown

@bcrochet: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws 6667a0e link /test e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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.

@hardys
Copy link
Copy Markdown

hardys commented Jan 15, 2020

I think this patch should go upstream first, what do you all think?

Agreed, the related upstream fix is coredns#3527 but @bcrochet pushed this backport to enable testing of this on OCP builds (with a locally overridden image) - this can probably be marked WIP until the upstream fix lands.

@bcrochet
Copy link
Copy Markdown
Member Author

I think this patch should go upstream first, what do you all think?

Agreed, the related upstream fix is coredns#3527 but @bcrochet pushed this backport to enable testing of this on OCP builds (with a locally overridden image) - this can probably be marked WIP until the upstream fix lands.

Precisely. Upstream PR here: coredns#3527

@bcrochet bcrochet changed the title Fix HostPortOrFile to support IPv6 addresses with zone WIP: Fix HostPortOrFile to support IPv6 addresses with zone Jan 15, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 15, 2020
@ironcladlou
Copy link
Copy Markdown

Thanks. What I'd expect to happen here is:

  1. Get the upstream patch merged
  2. Open a PR to openshift/coredns updating to the latest upstream commit

What we want to avoid is carrying a downstream patch.

@bcrochet
Copy link
Copy Markdown
Member Author

Thanks. What I'd expect to happen here is:

1. Get the upstream patch merged

2. Open a PR to openshift/coredns updating to the latest upstream commit

What we want to avoid is carrying a downstream patch.

I completely agree. That's why I started upstream first. I've marked this as WIP, and will close when the upstream is merged. And do as you've said, open a new PR with a rebase.

@eparis
Copy link
Copy Markdown
Member

eparis commented Jan 16, 2020

I'm ok with this sitting a day or 2. But we can not let things like this sit for more than about 2 days waiting on upstream. I don't care if we do a CARRY/UPSTREAM and it gets dropped next week. I need master working now. Not 'sometime' even if we have to do extra work and then back it back out.

So keep pushing upstream. But if upstream isn't done by say, this weekend, this needs to merge. Upstream can not block us getting IPv6 support in both master and 4.3.

@ironcladlou
Copy link
Copy Markdown

Looks like upstream is probably just about ready. No objection to merging by the weekend in advance of upstream.

@eparis
Copy link
Copy Markdown
Member

eparis commented Jan 16, 2020

thanks Dan!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants