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

Detect CIDR IPv4 or IPv6 version to select nexthop #59749

Merged
merged 1 commit into from Feb 14, 2018

Conversation

@zioproto
Contributor

zioproto commented Feb 12, 2018

What this PR does / why we need it:

The node InternalIP is used as nexthop by the Kubernetes master to create routes in the Neutron router for Pods reachability.
If a node has more than one InternalIPs, eventually IPv4 and IPv6, a random InternalIP from the list is returned.
This can lead to the bug described in #59421
We need to check when we build a route that the CIDR and the nexthop belong to the same IP Address Family (both IPv4 or both IPv6)

Which issue(s) this PR fixes :
Fixes #59421
It is related to #55202

Special notes for your reviewer:
This is the suggested way to fix the problem after the discussion in #59502

old release note: Issue #59421 was closed: the Openstack deployment with dual stack (IPv4 and IPv6) nodes and network-plugin=kubenet is now working properly.

Release note:

Fixing a bug in OpenStack cloud provider, where dual stack deployments (IPv4 and IPv6) did not work well when using kubenet as the network plugin.
@FengyunPan2

Sounds good, can we keep getAddressByName()?
Or use a small method since we are using the same thing 2 times, maybe use it at other function later.

@zioproto

This comment has been minimized.

Contributor

zioproto commented Feb 12, 2018

@FengyunPan2 is it okay then if I pass route.DestinationCIDR as an argument to getAddressByName in order to compute in the function if I should get an IPv4 or IPv6 result ?

@k8s-ci-robot k8s-ci-robot added size/S and removed size/M labels Feb 12, 2018

@zioproto

This comment has been minimized.

Contributor

zioproto commented Feb 12, 2018

@FengyunPan2 I pushed a new amended commit. Now the implementation uses the function and there is not code duplication.

@@ -520,12 +521,12 @@ func getAddressByName(client *gophercloud.ServiceClient, name types.NodeName) (s
}
for _, addr := range addrs {
if addr.Type == v1.NodeInternalIP {
if (addr.Type == v1.NodeInternalIP) && (govalidator.IsIPv6(addr.Address) == needIPv6) {

This comment has been minimized.

@leblancd

leblancd Feb 12, 2018

Contributor

Might be better to use the more generic 'net' package (instead of asaskevich/govalidator) to determine whether the address is IPv6, e.g.:

isIPv6 := net.ParseIP(addr.Address).To4() == nil
if addr.Type == v1.NodeInternalIP && isIPv6 == needIPv6 {
    return addr.Address, nil
}

This comment has been minimized.

@zioproto

zioproto Feb 12, 2018

Contributor

hello @leblancd
I checked https://stackoverflow.com/questions/22751035/golang-distinguish-ipv4-ipv6

Please check in particular this answer: https://stackoverflow.com/a/48519490/1506396
It is the exaplanation why to use govalidator instead of the net package.

This comment has been minimized.

@leblancd

leblancd Feb 12, 2018

Contributor

Hi @zioproto:
I would respectfully disagree with what the poster is claiming in this stackoverflow answer. Specifically, the list of addresses that are labeled "Valid IPv6 notations" should be labeled instead "IPv4-mapped IPv6 Notations" (RFC4291 and RFC4038). These addresses have an IPv6 format, but are intended to be internal representations of IPv4 addresses on the wire. They're essentially IPv4 addresses encapsulated in an IPv6 frame, and this is done so that dual-stack software can conveniently handle both IPv4 and IPv6 addresses as 16-byte entities. The net package's To4() takes care to look for this pattern in a 16-byte slice, and (correctly, I believe) creates an 4-byte (IPv4) slice from the lowest 4 bytes (since Golang can handle slices of arbitrary length, so there's no need to perpetuate the 16-byte internal representation of IPv4-mapped IPv6 addresses). In the examples given in the post, the only addresses that the net's To4() gets wrong, in my opinion, are the examples with :port appended, but To4() assumes that the caller has taken care of eliminating any :port (e.g. with splitHostPort()).

@@ -146,7 +148,10 @@ func (r *Routes) CreateRoute(ctx context.Context, clusterName string, nameHint s
onFailure := newCaller()
addr, err := getAddressByName(r.compute, route.TargetNode)
IP, _, _ := net.ParseCIDR(route.DestinationCIDR)
CIDRisV6 := govalidator.IsIPv6(IP.String())

This comment has been minimized.

@leblancd

leblancd Feb 12, 2018

Contributor

Same comment as before, can use 'net' package to test for IPv6, e.g.:

CIDRisV6 := IP.To4() == nil
@dims

This comment has been minimized.

Member

dims commented Feb 12, 2018

/ok-to-test

@leblancd

This comment has been minimized.

Contributor

leblancd commented Feb 12, 2018

Looks good to me. Thanks, @zioproto for taking care of this!

@leblancd

This comment has been minimized.

Contributor

leblancd commented Feb 12, 2018

/test pull-kubernetes-bazel-test

@FengyunPan2

This comment has been minimized.

Contributor

FengyunPan2 commented Feb 13, 2018

@zioproto Yeah, nice work.
/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 13, 2018

@FengyunPan2: changing LGTM is restricted to assignees, and only kubernetes org members may be assigned issues.

In response to this:

@zioproto Yeah, nice work.
/lgtm

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.

@dims

This comment has been minimized.

Member

dims commented Feb 13, 2018

/approve
/lgtm

return addr.Address, nil
}
}
return addrs[0].Address, nil
return "", ErrNoAddressFound

This comment has been minimized.

@anguslees

anguslees Feb 13, 2018

Member

With this last change, this function will only return InternalIPs. It should instead try ExternalIPs before giving up.

Aside: An alternate version might be to pass an address family down to getAddressesByName and thence nodeAddresses (with some value to indicate "all" families) and then do the filtering at the lowest level, when we know the address family from the openstack data structure. Happy to go with whatever version you think is best.

@@ -146,7 +147,10 @@ func (r *Routes) CreateRoute(ctx context.Context, clusterName string, nameHint s
onFailure := newCaller()
addr, err := getAddressByName(r.compute, route.TargetNode)
IP, _, _ := net.ParseCIDR(route.DestinationCIDR)
CIDRisV6 := IP.To4() == nil

This comment has been minimized.

@anguslees

anguslees Feb 13, 2018

Member

Both IP and CIDRisV6 should have (at least) a lowercased first letter, to conform with golang's public/private styleguide/rules.

@@ -219,7 +223,10 @@ func (r *Routes) DeleteRoute(ctx context.Context, clusterName string, route *clo
onFailure := newCaller()
addr, err := getAddressByName(r.compute, route.TargetNode)
IP, _, _ := net.ParseCIDR(route.DestinationCIDR)
CIDRisV6 := IP.To4() == nil

This comment has been minimized.

@anguslees

anguslees Feb 13, 2018

Member

Ditto lowercased first letter of local variable.

@dims

This comment has been minimized.

Member

dims commented Feb 13, 2018

/hold

@dims

This comment has been minimized.

Member

dims commented Feb 13, 2018

/lgtm cancel

@zioproto

This comment has been minimized.

Contributor

zioproto commented Feb 13, 2018

/test pull-kubernetes-unit

@dims

This comment has been minimized.

Member

dims commented Feb 13, 2018

/hold cancel

@anguslees please take another look

@dims

This comment has been minimized.

Member

dims commented Feb 13, 2018

/lgtm

@dims

This comment has been minimized.

Member

dims commented Feb 13, 2018

/lgtm cancel

@dims

This comment has been minimized.

Member

dims commented Feb 13, 2018

weird, i wanted to indicate lgtm without approving ... bot applied both! never ming. just waiting for @anguslees :)

@anguslees

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 14, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 14, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anguslees, FengyunPan2, zioproto

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 OWNERS Files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 14, 2018

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

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 14, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@zioproto

This comment has been minimized.

Contributor

zioproto commented Feb 14, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 14, 2018

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

@zioproto

This comment has been minimized.

Contributor

zioproto commented Feb 14, 2018

/test pull-kubernetes-unit

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 14, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 14, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit 0dda5c8 into kubernetes:master Feb 14, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation zioproto authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@zioproto

This comment has been minimized.

Contributor

zioproto commented Feb 14, 2018

/cherrypick-candidate

k8s-merge-robot added a commit that referenced this pull request Feb 25, 2018

Merge pull request #59868 from dims/automated-cherry-pick-of-#59749-u…
…pstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #59749: Detect CIDR IPv4 or IPv6 version to select nexthop

Cherry pick of #59749 on release-1.9.

#59749: Detect CIDR IPv4 or IPv6 version to select nexthop

```release-note
Fixing a bug in OpenStack cloud provider, where dual stack deployments (IPv4 and IPv6) did not work well when using kubenet as the network plugin.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment