Changes to support IPv6 addresses on nodes #268

sdmodi · 2021-08-20T21:52:28Z

These changes populate the Node object with the IPv6 address as well as the IPv6 podCIDR. This is done only for clusters with stackType as IPV4_IPV6. This patch handles both the internal and external IPv6 addresses.

k8s-ci-robot · 2021-08-20T21:52:36Z

Hi @sdmodi. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

sdmodi · 2021-08-20T23:30:44Z

/cc @basantsa1989

k8s-ci-robot · 2021-08-20T23:30:45Z

@sdmodi: GitHub didn't allow me to request PR reviews from the following users: basant1989.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @Basant1989

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.

MrHohn · 2021-08-20T23:33:23Z

/ok-to-test

sdmodi · 2021-08-24T21:57:20Z

/test cloud-provider-gcp-e2e-full

sdmodi · 2021-08-24T22:00:50Z

Joe/Jiahui would you please review this. Thanks!

sdmodi · 2021-08-24T22:42:07Z

/test cloud-provider-gcp-e2e-full

sdmodi · 2021-08-25T16:30:03Z

/assign @bowei

basantsa1989 · 2021-08-25T18:45:47Z

providers/gce/gce.go

@@ -392,6 +397,10 @@ func generateCloudConfig(configFile *ConfigFile) (cloudConfig *CloudConfig, err
 		cloudConfig.SecondaryRangeName = configFile.Global.SecondaryRangeName
 	}

+	if configFile != nil && configFile.Global.StackType != "" {


We can skip this check and let it set it to "" anyway?

basantsa1989 · 2021-08-25T18:46:53Z

providers/gce/gce_instances.go

@@ -116,6 +118,27 @@ func (g *Cloud) NodeAddresses(ctx context.Context, nodeName types.NodeName) ([]v
 				}
 				nodeAddresses = append(nodeAddresses, v1.NodeAddress{Type: v1.NodeInternalIP, Address: internalIP})

+				if g.stackType == "IPV4_IPV6" {
+					// Handling only the internal v6 address. External v6 addresses will be handled once vendor apis are updated.


We should remove or update this comment now that we support external too.

basantsa1989 · 2021-08-25T18:56:47Z

providers/gce/gce_instances.go

+					if internalIPV6 != "" {
+						nodeAddresses = append(nodeAddresses, v1.NodeAddress{Type: v1.NodeInternalIP, Address: internalIPV6})
+					} else {
+						klog.Warningf("internal IPV6 range is empty")


Handle the external IPV6 addresses right here?

We don't need to. Both the internal and external IPv6 addresses are written to this same field in the metadata.

Cool!
Nit: Rename 'internalIPV6s' and 'internalIPV6Arr' to something generic like ipv6s, ipv6Arr.

Could we include the node name in the warning log - might be helpful when debugging.

basantsa1989 · 2021-08-25T19:02:19Z

providers/gce/gce_instances.go

 	}

 	return nodeAddresses, nil
 }

+func getIPV6AddressFromInterface(nic *computealpha.NetworkInterface) string {
+	ipv6Addr := nic.Ipv6Address
+	if ipv6Addr == "" && nic.Ipv6AccessType == "EXTERNAL" {


There was a discussion by gce team on supporting both public and private v6 IPs on the same interface. We may have to revisit this if that is the case.

Yes. For now, VMs just get one IPv6 address.

Question about the access type - does "INTERNAL" correspond to directpath? What happen if the customer enables both dualstack and directpath?

INTERNAL does not directly correspond to directpath. INTERNAL means that the subnet has only private IPv6 addresses. Today directpath uses INTERNAL addresses. When dual stack is enabled with directpath on a subnet with INTERNAL addresses, everything just works. Directpath and dual stack use the same IPs. Directpath on subnets with EXTERNAL addresses is not working currently. The GCE team is trying to figure something out.

Got it, thanks for the clarification. Make sense, seems like we will worry about the directpath use case later then.

sdmodi

Addressed the other comments

sdmodi · 2021-08-25T19:58:27Z

providers/gce/gce_instances.go

+					if internalIPV6 != "" {
+						nodeAddresses = append(nodeAddresses, v1.NodeAddress{Type: v1.NodeInternalIP, Address: internalIPV6})
+					} else {
+						klog.Warningf("internal IPV6 range is empty")


We don't need to. Both the internal and external IPv6 addresses are written to this same field in the metadata.

sdmodi · 2021-08-25T19:58:52Z

providers/gce/gce_instances.go

 	}

 	return nodeAddresses, nil
 }

+func getIPV6AddressFromInterface(nic *computealpha.NetworkInterface) string {
+	ipv6Addr := nic.Ipv6Address
+	if ipv6Addr == "" && nic.Ipv6AccessType == "EXTERNAL" {


Yes. For now, VMs just get one IPv6 address.

sdmodi · 2021-08-25T20:39:10Z

/test cloud-provider-gcp-verify-all

sdmodi · 2021-08-25T21:00:21Z

/test cloud-provider-gcp-verify-all

jiahuif · 2021-08-25T21:11:59Z

There is a govet error

[*] Verifying govet...
# k8s.io/cloud-provider-gcp/cmd/gke-gcloud-auth-plugin
cmd/gke-gcloud-auth-plugin/main.go:99:37: k8s.io/apimachinery/pkg/apis/meta/v1.Time composite literal uses unkeyed fields
cmd/gke-gcloud-auth-plugin/main.go:113:27: k8s.io/apimachinery/pkg/apis/meta/v1.Time composite literal uses unkeyed fields

jiahuif · 2021-08-25T21:35:49Z

sent a PR to fix that #269

jiahuif · 2021-08-25T23:31:50Z

providers/gce/gce.go

@@ -166,6 +166,9 @@ type Cloud struct {
 	s *cloud.Service

 	metricsCollector loadbalancerMetricsCollector
+	// stackType indicates whether the cluster is a single stack IPv4, single
+	// stack IPv6 or a dual stack cluster
+	stackType string


Can we have the list of all possible values as constants? something like

type StackType string const NetworkStackDualStack StackType = "IPV4_IPV6"

This way we can eliminate the magic number below.

jiahuif · 2021-08-30T18:06:41Z

I wonder if there is a way to test this part of code right now. There is a fakeGCP so it seems that integration tests are possible.

sdmodi · 2021-08-30T18:30:04Z

I thought through how we could test this. The real issue is that the underlying code calls the metadata server which does not have the right architecture to fake.

I can add tests for the part of the code that calls GCP API. Let me take a stab at doing that. Thanks!

sdmodi · 2021-08-31T04:31:57Z

I have added unit tests wherever I could. Please take a look.

@basantsa1989 would you please review this again. Thanks!

basantsa1989

/lgtm
Kudos on the test coverage :)

basantsa1989 · 2021-09-01T01:09:30Z

providers/gce/gce_instances.go

@@ -117,6 +119,27 @@ func (g *Cloud) NodeAddresses(ctx context.Context, nodeName types.NodeName) ([]v
 				}
 				nodeAddresses = append(nodeAddresses, v1.NodeAddress{Type: v1.NodeInternalIP, Address: internalIP})

+				if g.stackType == NetworkStackDualStack {
+					// Both internal and external IPv6 addresses are written to this array
+					internalIPV6s, err := metadata.Get(fmt.Sprintf(networkInterfaceIPV6, nic))


Nit: Since we handle both internal and external IPv6 addresses, we can rename 'internalIPV6s' and 'internalIPV6Arr' to something generic like ipv6s, ipv6Arr.

k8s-ci-robot · 2021-09-01T01:10:55Z

@basantsa1989: changing LGTM is restricted to collaborators

In response to this:

/lgtm
Kudos on the test coverage :)

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.

jiahuif · 2021-09-01T16:37:38Z

lgtm for the change to the cloud provider. I do not have enough expertise with networking, though.

sdmodi · 2021-09-01T16:54:42Z

/assign @MrHohn

Zihong, would you please take a look at these changes from a networking point of view. Thanks!

MrHohn

Got a few questions on the stacktype but overall LGTM.

providers/gce/gce.go

MrHohn · 2021-09-01T17:04:38Z

providers/gce/gce.go

@@ -393,6 +404,10 @@ func generateCloudConfig(configFile *ConfigFile) (cloudConfig *CloudConfig, err
 		cloudConfig.SecondaryRangeName = configFile.Global.SecondaryRangeName
 	}

+	if configFile != nil {
+		cloudConfig.StackType = configFile.Global.StackType


What would be the default value for this StackType field? Is empty the same as IPV4?

We should treat empty the same as IPV4

Yep seems good. Was thinking what would happen when using this with old clusters.

MrHohn · 2021-09-01T17:07:16Z

providers/gce/gce_instances.go

+					if internalIPV6 != "" {
+						nodeAddresses = append(nodeAddresses, v1.NodeAddress{Type: v1.NodeInternalIP, Address: internalIPV6})
+					} else {
+						klog.Warningf("internal IPV6 range is empty")


Could we include the node name in the warning log - might be helpful when debugging.

MrHohn · 2021-09-01T17:13:06Z

providers/gce/gce_instances.go

 	}

 	return nodeAddresses, nil
 }

+func getIPV6AddressFromInterface(nic *computealpha.NetworkInterface) string {
+	ipv6Addr := nic.Ipv6Address
+	if ipv6Addr == "" && nic.Ipv6AccessType == "EXTERNAL" {


Question about the access type - does "INTERNAL" correspond to directpath? What happen if the customer enables both dualstack and directpath?

bowei · 2021-09-01T17:24:22Z

providers/gce/gce.go

@@ -94,6 +94,12 @@ var _ cloudprovider.Zones = (*Cloud)(nil)
 var _ cloudprovider.PVLabeler = (*Cloud)(nil)
 var _ cloudprovider.Clusters = (*Cloud)(nil)

+type StackType string
+
+const NetworkStackDualStack StackType = "IPV4_IPV6"


is there a reference from where this constant is coming from

These are in the comments of the go sdk.
https://pkg.go.dev/google.golang.org/api@v0.56.0/compute/v0.alpha
(search for "IPV4_IPV6")
Unfortunately the generated code did not have the constants directly avaliable.

This is actually being set by the GKE cluster server. This value is going to be written to gce.conf.

These changes populate the Node object with the IPv6 address as well as the IPv6 podCIDR. This is done only for clusters with stackType as IPV4_IPV6

MrHohn · 2021-09-01T21:59:16Z

providers/gce/gce_instances.go

 	}

 	return nodeAddresses, nil
 }

+func getIPV6AddressFromInterface(nic *computealpha.NetworkInterface) string {
+	ipv6Addr := nic.Ipv6Address
+	if ipv6Addr == "" && nic.Ipv6AccessType == "EXTERNAL" {


Got it, thanks for the clarification. Make sense, seems like we will worry about the directpath use case later then.

MrHohn · 2021-09-01T21:59:35Z

providers/gce/gce.go

@@ -393,6 +404,10 @@ func generateCloudConfig(configFile *ConfigFile) (cloudConfig *CloudConfig, err
 		cloudConfig.SecondaryRangeName = configFile.Global.SecondaryRangeName
 	}

+	if configFile != nil {
+		cloudConfig.StackType = configFile.Global.StackType


Yep seems good. Was thinking what would happen when using this with old clusters.

k8s-ci-robot · 2021-09-01T22:01:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: basantsa1989, MrHohn, sdmodi

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:

~~providers/gce/OWNERS~~ [MrHohn]

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

MrHohn · 2021-09-01T22:45:11Z

/lgtm

A recent change kubernetes#268 introduced IPv6 support, but it also began to require Google Cloud Compute Aplha API access. Unfortunately by default projects don't have this access and therefore, when the CCM fails with: googleapi: Error 403: Required 'Alpha Access' permission for 'Compute API', forbidden This commit starts using Alpha API only for dual stack deployments, where it's really required. For all other cases we will continue to use GA API. Fixes: kubernetes#281

kubernetes#268 introduced IPv6 support, but it also began to require Google Cloud Compute Aplha API access. Unfortunately by default projects don't have this access and therefore, when the CCM fails with: googleapi: Error 403: Required 'Alpha Access' permission for 'Compute API', forbidden This commit starts using Alpha API only for dual stack deployments, where it's really required. For all other cases we will continue to use GA API. Fixes: kubernetes#281

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 20, 2021

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 20, 2021

k8s-ci-robot requested review from jiahuif and jpbetz August 20, 2021 21:52

sdmodi mentioned this pull request Aug 20, 2021

Handle internal IPV6 addresses from gce nodes if cluster is dual stack #240

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 20, 2021

sdmodi force-pushed the ipv6-changes branch from 5b7b4f2 to 3648504 Compare August 20, 2021 23:43

k8s-ci-robot assigned bowei Aug 25, 2021

basantsa1989 reviewed Aug 25, 2021

View reviewed changes

sdmodi force-pushed the ipv6-changes branch from 3648504 to a98d280 Compare August 25, 2021 19:57

sdmodi commented Aug 25, 2021

View reviewed changes

jiahuif reviewed Aug 25, 2021

View reviewed changes

sdmodi force-pushed the ipv6-changes branch 2 times, most recently from 597be0f to 8e0bf35 Compare August 30, 2021 17:48

sdmodi force-pushed the ipv6-changes branch from 8e0bf35 to eb3fb99 Compare August 31, 2021 04:31

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 31, 2021

sdmodi force-pushed the ipv6-changes branch from eb3fb99 to f6b5ab9 Compare August 31, 2021 04:34

basantsa1989 approved these changes Sep 1, 2021

View reviewed changes

sdmodi force-pushed the ipv6-changes branch from f6b5ab9 to c3d38e4 Compare September 1, 2021 16:35

k8s-ci-robot assigned MrHohn Sep 1, 2021

MrHohn approved these changes Sep 1, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2021

bowei reviewed Sep 1, 2021

View reviewed changes

Changes to support IPv6 addresses on nodes

ab4d7db

These changes populate the Node object with the IPv6 address as well as the IPv6 podCIDR. This is done only for clusters with stackType as IPV4_IPV6

sdmodi force-pushed the ipv6-changes branch from c3d38e4 to ab4d7db Compare September 1, 2021 20:46

MrHohn approved these changes Sep 1, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2021

k8s-ci-robot merged commit 7f05bd0 into kubernetes:master Sep 1, 2021

Fedosin mentioned this pull request Oct 14, 2021

"Forbidden" error code when using GCP CCM #281

Closed

Fedosin mentioned this pull request Oct 14, 2021

Use Alpha Compute API only in case of dual stack #282

Merged

Changes to support IPv6 addresses on nodes #268

Changes to support IPv6 addresses on nodes #268

Conversation

sdmodi commented Aug 20, 2021

k8s-ci-robot commented Aug 20, 2021

sdmodi commented Aug 20, 2021 • edited Loading

k8s-ci-robot commented Aug 20, 2021

MrHohn commented Aug 20, 2021

sdmodi commented Aug 24, 2021

sdmodi commented Aug 24, 2021

sdmodi commented Aug 24, 2021

sdmodi commented Aug 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdmodi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdmodi commented Aug 25, 2021

sdmodi commented Aug 25, 2021

jiahuif commented Aug 25, 2021

jiahuif commented Aug 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiahuif commented Aug 30, 2021

sdmodi commented Aug 30, 2021

sdmodi commented Aug 31, 2021

basantsa1989 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 1, 2021

jiahuif commented Sep 1, 2021

sdmodi commented Sep 1, 2021

MrHohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 1, 2021

MrHohn commented Sep 1, 2021

sdmodi commented Aug 20, 2021 •

edited

Loading