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

Adds Support for Node Resource IPv6 Addressing #45551

Merged
merged 1 commit into from Nov 15, 2017

Conversation

danehans
Copy link

@danehans danehans commented May 9, 2017

What this PR does / why we need it:
This PR adds support for the following:

  1. A node resource to be assigned an IPv6 address.
  2. Expands IPv4/v6 address validation checks.

Which issue this PR fixes:
Fixes Issue #44848 (in combination with PR #45116).

Special notes for your reviewer:
This PR is part of a larger effort, Issue #1443 to add IPv6 support to k8s.

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 9, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels May 9, 2017
@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 9, 2017
@k82cn
Copy link
Member

k82cn commented May 9, 2017

fyi: @Spindel , IPv6 related.

@dcbw dcbw added sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. labels May 18, 2017
@dcbw
Copy link
Member

dcbw commented May 18, 2017

This change in isolation looks OK to me, but I worry a lot about everything else, since many things use the node address through the API. If this change is made, and the node has only an IPv6 address, can it register itself with the apiserver using that IPv6 address?

Obviously we need IPv6 support everywhere in Kube, and adding it in small pieces is a good thing, but we need to figure out what the failure cases are so they don't bite people unawares over the next couple releases while v6 gets added.

@danehans
Copy link
Author

@dcbw thanks for checking out my PR. I agree that we must be sensitive to breaking functionality while adding IPv6. I'm open to guidance from you and others on the best approach to adding IPv6 functionality while preserving stability.

As you mentioned, myself and several others are working on small PR's to add IPv6 instead of one massive PR. Should these small IPv6 PR's be labeled and sit in a queue until they can be collectively tested for e2e functionality?

If this change is made, and the node has only an IPv6 address, can it register itself with the apiserver using that IPv6 address?

It's my understanding that this PR will allow a node to register itself with the apiserver using an IPv6 address. This is my understanding:

  1. The node resource contains a status field representing the most recently observed status of the node.
  2. The status field contains an addresses field representing a list of addresses reachable to the node.
  3. Each address has an associated type.
  4. The 3 supported types are: Hostname, ExternalIP or InternalIP.
  5. Of the 3 address types, InternalIP is used by the kubelet to set the node resource address with the apiserver.
  6. The Hostname and InternalIP are set by the setNodeAdrress method.

Prior to this PR, only an IPv4 address could be set for InternalIP. In addition to adding IPv6 support to this method. I also expanded the ValidateNodeIP method to support additional invalid IPv4/IPv6 addresses such as multicast, unspecified, etc..

@danehans
Copy link
Author

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 23, 2017
@pmorie
Copy link
Member

pmorie commented May 26, 2017

I also have the same concerns that @dcbw has. @danehans, have you written a proposal for this change yet? I think that is probably the right thing to do in this case, and get thorough review from sig-node and sig-network.

@danehans
Copy link
Author

@pmorie thank you for the review. I have not written a proposal for this change. I was anticipating writing a proposal for changes such as multiple pod ip's that effect the api design, but not for low-level functionality such as this. I understand this change can impact dependent services. In this change, I maintain backwards compatibility and continue to prefer v4 over v6. I will look closer at expanding unit tests for this change and would appreciate any guidance on expanding integration/e2e tests. However, I will work on a design proposal if it's required for the PR to get merged.

@danehans
Copy link
Author

danehans commented Jun 1, 2017

/area ipv6

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 7, 2017
@danehans danehans force-pushed the node_v6 branch 2 times, most recently from ae190a2 to 6e0fe10 Compare June 8, 2017 18:37
@k8s-github-robot k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 8, 2017
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2017
case kl.nodeIP.To16() != nil:
hostDNS = []string{"::1"}
hostSearch = []string{"."}
default:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks some of the conditional logic got munged here:
Seems like this should be:

if kl.resolverConfig == "" {
  switch {
    ...
    case kl.nodeIP.To16() != nil:
      ...
  }
} else {
  hostSearch = kl.formDNSSearchForDNSDefault(hostSearch, pod)
}

// Honor IP limitations set in setNodeStatus()
if kl.nodeIP.IsLoopback() {
if nodeIP.IsLoopback() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably should preserve at the top before the other checks:

if kl.nodeIP.To4() == nil && kl.nodeIP.To6() == nil {
  return fmt.Errorf("nodeIP must be a valid IP address")
}

@cblecker
Copy link
Member

cblecker commented Nov 7, 2017

I1106 23:01:55.779] Verifying hack/make-rules/../../hack/verify-govet.sh
W1106 23:05:48.167] pkg/kubelet/kubelet_node_status.go:1002: comparison of function To4 == nil is always false
W1106 23:05:48.167] pkg/kubelet/kubelet_node_status.go:1002: comparison of function To16 == nil is always false
W1106 23:05:48.202] exit status 1
W1106 23:06:49.457] make[1]: *** [vet] Error 1
I1106 23:06:49.558] Makefile:363: recipe for target 'vet' failed
I1106 23:06:49.559] FAILED   hack/make-rules/../../hack/verify-govet.sh	294s

@danehans
Copy link
Author

danehans commented Nov 7, 2017

/test pull-kubernetes-unit

@danehans
Copy link
Author

danehans commented Nov 9, 2017

@bowei I have updated the PR based on your feedback and tests look good. This PR is needed for IPv6 alpha support in 1.9.

cc: @luxas

Adds support for the following:

1. A node resource to be assigned an IPv6 address.
2. Expands IPv4/v6 address validation checks.

Which issue this PR fixes:
fixes kubernetes#44848 in combination with PR kubernetes#45116

Special notes for your reviewer:

Release note:
With this PR, nodes can be assigned an IPv6 address. An IPv4 address is
preferred over an IPv6 address. IP address validation has been expanded
to check for multicast, link-local and unspecified addresses.
@danehans
Copy link
Author

@bowei @MrHohn can you review the updated PR when you have a moment?

@danehans
Copy link
Author

@thockin anything needed to get this PR merged? We need this for IPv6 alpha support in 1.9.

@MrHohn
Copy link
Member

MrHohn commented Nov 14, 2017

/lgtm

Though still needs approval from @thockin
/assign @thockin

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2017
@thockin
Copy link
Member

thockin commented Nov 15, 2017

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danehans, dcbw, MrHohn, pmichali, thockin

Associated issue: 44848

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2017
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

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/ipv6 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. release-note-none Denotes a PR that doesn't merit a release note. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet