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.

Sign up for GitHub

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

Jump to bottom

Remove requirement that Endpoints IPs be IPv4 #23317

Merged
merged 1 commit into from
Apr 21, 2016
Merged

Remove requirement that Endpoints IPs be IPv4 #23317

merged 1 commit into from
Apr 21, 2016

Conversation

aanm
Copy link
Contributor

@aanm aanm commented Mar 22, 2016

Signed-off-by: André Martins aanm90@gmail.com

Release Note: The Endpoints API object now allows IPv6 addresses to be stored. Other components of the system are not ready for IPv6 yet, and many cloud providers are not IPv6 compatible, but installations that use their own controller logic can now store v6 endpoints.

@k8s-bot
Copy link

k8s-bot commented Mar 22, 2016

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

2 similar comments
@k8s-bot
Copy link

k8s-bot commented Mar 22, 2016

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Mar 22, 2016

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 22, 2016
@bgrant0607
Copy link
Member

cc @kubernetes/sig-network @kubernetes/rh-networking @kubernetes/goog-cluster

@bgrant0607 bgrant0607 assigned ArtfulCoder and unassigned bgrant0607 Mar 22, 2016
@thockin
Copy link
Member

thockin commented Mar 23, 2016

This had to happen eventually. @ArtfulCoder for final approval - can you think of anything this might cause trouble for?

@marun marun mentioned this pull request Mar 23, 2016
Merged
@danwinship
Copy link
Contributor

I think kube-proxy needs to be updated to handle IPv6 endpoints first. Otherwise if you're using the iptables-based proxier, adding an IPv6 endpoint would break all further service updates until that endpoint was deleted.

@tgraf
Copy link
Contributor

tgraf commented Mar 31, 2016

@danwinship: Right, we are running without kube-proxy for now. The addresses are globally reachable anyway. We are working on fixing kube-proxy thuogh. Would you agree with failing appropriately in kube-proxy at first in the event of IPv6 addresses and then add additional support for ip6tables?

thockin
thockin reviewed Mar 31, 2016
View reviewed changes
pkg/util/validation/validation.go
@@ -160,6 +160,11 @@ func IsValidIPv4(value string) bool {
return net.ParseIP(value) != nil && net.ParseIP(value).To4() != nil
}

// IsValidIP tests that the argument is a valid IP address.
func IsValidIP(value string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

if nobody else calls IsValidIPv4() why not just get rid of that function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, you're right. Removed.

@thockin
Copy link
Member

thockin commented Mar 31, 2016

@ArtfulCoder are you going to review this or should I take it back?

@thockin
Copy link
Member

thockin commented Mar 31, 2016

There are about 100 things that have to change to actually support ipv6.
This is just one of them. :)

On Thu, Mar 31, 2016 at 12:21 AM, Thomas Graf notifications@github.com
wrote:

@danwinship https://github.com/danwinship: Right, we are running
without kube-proxy for now. The addresses are globally reachable anyway. We
are working on fixing kube-proxy thuogh. Would you agree with failing
appropriately in kube-proxy at first in the event of IPv6 addresses and
then add additional support for ip6tables?

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
#23317 (comment)

@danwinship
Copy link
Contributor

This isn't just a case of kube-proxy "not supporting" IPv6 though. Adding an IPv6 endpoint to a service will break all further updates to the service until the IPv6 endpoint is removed.

(I had originally thought it would break all services, but after testing it out I guess that's not true. The iptables proxier code updates each endpoint table separately, so it's only the updates of the table with the IPv6 endpoint that will fail:

Mar 31 10:59:52 openshift-master openshift[4464]: E0331 10:59:52.205674    4464 proxier.go:941] Failed to execute iptables-restore: exit status 2 (iptables-restore v1.4.21: host/network `[2600' not found

So I guess this isn't terrible.)

@aanm aanm force-pushed the removing-ipv4-enforcement branch from a806a67 to b15592f Compare March 31, 2016 14:54
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 31, 2016
@ArtfulCoder
Copy link
Contributor

reviewing now

@ArtfulCoder
Copy link
Contributor

I discussed offline with Tim.
We can accept the changes, if we can update the documentation on the IP field and doc to mention that IPV6 is accepted, but not supported on all platforms and certain kubernetes components are not ipv6 ready (like kube proxy)

@aanm
Copy link
Contributor Author

aanm commented Apr 7, 2016

@ArtfulCoder Thanks for the feedback. Can you please point me out which docs are relevant to be updated? Thank you!

@thockin
Copy link
Member

thockin commented Apr 8, 2016

The docs to update are the comments on type EndpointAddress field IP in pkg/api/types.go and pkg/api/v1/types.go. You'll have to re-run the docs generators (./hack/update-all.sh) and add all the modified files to this PR

@aanm aanm force-pushed the removing-ipv4-enforcement branch from b15592f to e7bb157 Compare April 8, 2016 14:43
@aanm
Copy link
Contributor Author

aanm commented Apr 8, 2016

Thanks. Out of curiosity, why there are 2 type EndpointAddress?

I've pushed the latest changes with proper documentation.

Sigh I've to fix the conflicts first

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 8, 2016
@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 8, 2016

GCE e2e build/test passed for commit 9885886d7d00bfbdc104bcf049f5d8082af9a61d.

@aanm aanm force-pushed the removing-ipv4-enforcement branch from 9885886 to 162d64b Compare April 11, 2016 16:16
aanm
aanm reviewed Apr 11, 2016
View reviewed changes
pkg/api/validation/validation_test.go
@@ -4909,13 +4909,13 @@ func TestValidateEndpoints(t *testing.T) {
ObjectMeta: api.ObjectMeta{Name: "mysvc", Namespace: "namespace"},
Subsets: []api.EndpointSubset{
{
Addresses: []api.EndpointAddress{{IP: "2001:0db8:85a3:0042:1000:8a2e:0370:7334"}},
Addresses: []api.EndpointAddress{{IP: "[2001:0db8:85a3:0042:1000:8a2e:0370:7334]"}},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't sure if I should have removed this test so I added a couple of square brackets to force failing

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 11, 2016

GCE e2e build/test passed for commit 162d64b14be9e45798ca1c11cbc9438c868d399c.

@thockin thockin added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge and removed needs-ok-to-merge labels Apr 11, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2016
@aanm aanm force-pushed the removing-ipv4-enforcement branch from 162d64b to 6df2c3d Compare April 12, 2016 13:43
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 12, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 12, 2016

GCE e2e build/test passed for commit 6df2c3de4a6689de59c186a0c72f2ecf7a8938eb.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2016
@aanm
Removing IPv4 enforcement on Endpoints
c1a360b 
Signed-off-by: André Martins <aanm90@gmail.com>
@aanm aanm force-pushed the removing-ipv4-enforcement branch from 6df2c3d to c1a360b Compare April 14, 2016 15:20
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 14, 2016

GCE e2e build/test passed for commit c1a360b.

@aanm
Copy link
Contributor Author

aanm commented Apr 21, 2016

Ping @thockin
I had to rebase because of conflicts.

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 21, 2016
@thockin thockin changed the title Removing IPv4 enforcement on Endpoints Remove requirement that Endpoints IPs be IPv4 Apr 21, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 21, 2016

GCE e2e build/test passed for commit c1a360b.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Apr 21, 2016

GCE e2e build/test passed for commit c1a360b.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 09adffb into kubernetes:master Apr 21, 2016
@aanm aanm deleted the removing-ipv4-enforcement branch April 21, 2016 17:04
@bachp bachp mentioned this pull request Nov 13, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ArtfulCoder ArtfulCoder

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@aanm @k8s-bot @bgrant0607 @thockin @danwinship @tgraf @ArtfulCoder @k8s-github-robot @googlebot