Skip to content
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

Add dual stack support #692

Merged
merged 4 commits into from
Mar 27, 2021
Merged

Add dual stack support #692

merged 4 commits into from
Mar 27, 2021

Conversation

aojea
Copy link
Contributor

@aojea aojea commented Jul 7, 2019

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 7, 2019
@k8s-ci-robot k8s-ci-robot requested review from krzyzacy and munnerz July 7, 2019 09:01
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 7, 2019
@aojea
Copy link
Contributor Author

aojea commented Jul 7, 2019

pkg/cluster/config/default.go Outdated Show resolved Hide resolved
pkg/cluster/config/v1alpha3/default.go Outdated Show resolved Hide resolved
@Arvinderpal
Copy link

@aojea how are you handling DNS/NAT64 in kind?

@aojea
Copy link
Contributor Author

aojea commented Jul 9, 2019

@aojea how are you handling DNS/NAT64 in kind?

not handling it, should we? conformance tests are passing without it, just modified CoreDNS to work without contacting upstream servers

@Arvinderpal
Copy link

@aojea how are you handling DNS/NAT64 in kind?

not handling it, should we? conformance tests are passing without it, just modified CoreDNS to work without contacting upstream servers

Ok, but I suspect e2e networking tests like the following that test external connectivity from pods won't pass. In that case, we'll either need to use GUA addresses or some sort of NATing with ULA addresses.

@aojea
Copy link
Contributor Author

aojea commented Jul 9, 2019

Full e2e working is the next milestone, we were able to have conformance working yesterday ... One step at s time 😅

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 19, 2019
@aojea aojea force-pushed the dualstack branch 4 times, most recently from 3205861 to 0722b04 Compare August 25, 2019 16:29
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 25, 2019
@aojea aojea force-pushed the dualstack branch 2 times, most recently from 589ed64 to b5ca840 Compare August 26, 2019 12:11
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2019
@aojea aojea force-pushed the dualstack branch 3 times, most recently from 80fc05a to c72e79e Compare August 29, 2019 16:05
@aojea
Copy link
Contributor Author

aojea commented Mar 11, 2021

@BenTheElder we should revisit this now the 1.21 will have dualstack enabled by default

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2021
Base automatically changed from master to main March 16, 2021 23:22
go.mod Outdated Show resolved Hide resolved
@BenTheElder BenTheElder added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Mar 17, 2021
@BenTheElder
Copy link
Member

generally lgtm. left comments.
in particular need to update the image, and I'm still dubious on the constant values.
we can iterate on the dependencies angle. I think we should cease importing k8s.io/* for trivial things, but we can handle that seperately.

BenTheElder and others added 4 commits March 26, 2021 11:10
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 26, 2021
@aojea
Copy link
Contributor Author

aojea commented Mar 26, 2021

/test pull-kind-conformance-parallel-dual-stack-ipv4-ipv6

generally lgtm. left comments.
in particular need to update the image, and I'm still dubious on the constant values.
we can iterate on the dependencies angle. I think we should cease importing k8s.io/* for trivial things, but we can handle that seperately.

modified constants and removed all the dependencies on k8s.io/utils

@aojea
Copy link
Contributor Author

aojea commented Mar 26, 2021

we need this for the dualstack CI job
kubernetes/test-infra#21537

once it is in kind I need to remember to change all the kubernetes jobs

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Mar 26, 2021

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

Test name Commit Details Rerun command
pull-kind-conformance-parallel 863cfc0 link /test pull-kind-conformance-parallel
pull-kind-conformance-parallel-1-14 1df6ecb link /test pull-kind-conformance-parallel-1-14

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.

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.

@BenTheElder
Copy link
Member

Antonio and I discussed this a bit and looked around quite a bit, and also discussed with the azure folks (since AKS supports this though without any high level config for it).

... There really isn't any standard way to refer to this. Kubernetes has a bunch of distinct concepts with different names surrounding dualstack and the broader networking community seems to use a variety of formats for referring to this.

https://kubernetes.io/docs/concepts/services-networking/dual-stack/

dual seems fine enough. I didn't intend for this to be a major sticking point, just a chance to find the least surprising / most obvious value for users before shipping the API, but that doesn't really seem to exist.

@BenTheElder
Copy link
Member

/test pull-kind-conformance-parallel-dual-stack-ipv4-ipv6

@BenTheElder
Copy link
Member

/lgtm
/approve
remaining nits can be addressed in follow up. past time to ship this.

for anyone following along, this has been helped along by a number of other PRs like #2043

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, BenTheElder

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@BenTheElder
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2021
@k8s-ci-robot k8s-ci-robot merged commit 5e58a0d into kubernetes-sigs:main Mar 27, 2021
@BenTheElder BenTheElder mentioned this pull request Mar 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/docker Issues or PRs related to docker area/provider/podman Issues or PRs related to podman cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants