Add flag for IPV6 only NEG's #2763

zasweq · 2024-12-10T02:21:46Z

This PR adds a flag for IPV6 only NEG's. If the flag is false, then the NEG Controller will still error if an IPV6 only service is configured.

The plan is to flag flip in a patch release once IPV6 NEG support is available in production (requires GCP LB support for IPV6 only NEG's).

k8s-ci-robot · 2024-12-10T02:21:56Z

Welcome @zasweq!

It looks like this is your first PR to kubernetes/ingress-gce 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-gce has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-12-10T02:21:57Z

Hi @zasweq. 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-sigs/prow repository.

gauravkghildiyal · 2024-12-10T02:25:10Z

/assign @swetharepakula
/assign @gauravkghildiyal
/uncc @aojea
/uncc @sawsa307

gauravkghildiyal

Thanks @zasweq

gauravkghildiyal · 2024-12-10T02:44:23Z

pkg/flags/flags.go

@@ -334,6 +335,7 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5
 	flag.BoolVar(&F.EnableL4ILBMixedProtocol, "enable-l4ilb-mixed-protocol", false, "Enable support for mixed protocol L4 internal load balancers.")
 	flag.BoolVar(&F.EnableL4NetLBMixedProtocol, "enable-l4netlb-mixed-protocol", false, "Enable support for mixed protocol L4 external load balancers.")
 	flag.StringVar(&F.ClusterSliceAPIGroup, "cluster-slice-api-group", "", "The API group for the ClusterSlice CRD.")
+	flag.BoolVar(&F.EnableIPV6NEG, "enable-ipv6-neg", false, "Enable support for IPV6 NEG's.")


nit: Can we use enable-ipv6-only-neg for clarity? (If we do, let's propagate the same to the variable names as well)

Ah, good point. The error message returned from config verification says only and so do the tests. Switched.

gauravkghildiyal · 2024-12-10T15:47:37Z

/ok-to-test

krzykwas · 2024-12-11T13:13:41Z

pkg/neg/controller_test.go

+		{
+			desc:              "ipv6 only service with l7 load balancer, ipFamilyPolicy is singleStack, ipv6 neg enabled",
+			serviceType:       v1.ServiceTypeClusterIP,
+			ipFamilies:        []v1.IPFamily{v1.IPv6Protocol},


I'd just like to make sure the tests cover all the cases. Sorry if my comments are off, because I may not have all the context. A customer in a single stack IPv6 cluster could create a normal Service, where the Spec lacks ipFamilies or ipFamilyPolicy. Because the cluster is single stack IPv6 and IPv6 is the only family available, the Service will only have IPv6 addresses, and so the backends, and the NEGs would be only configured with an IPv6 address. So theoretically there is a case where these fields are not set, but a NEG in the end would only have an IPv6 address. I don't know if this matters, maybe it's not possible to add coverage just with unit tests. WDYT?

I see what you're saying. For the scope of this specific unit test, which tests at the validation boundary, I think what to test here would be for an unset IP family to pass validation.

However, I wonder how to test this e2e. It would require however we determine the GKE Cluster's stack type and see if IPV6 + unset, and see if unset spins up a IPv6.

Hmm ok I'll add a test for unset ipFamilies and ipFamily policy at the unit test layer. I'm assuming that for IPV4 only it'll simply create an IPV4 NEG, just as in the case of IPV6 only cluster it's valid and we want it to create an IPV6 only NEG.

Thanks @krzykwas, I wasn't aware about the "theoretically there is a case where these fields are not set, but a NEG in the end would only have an IPv6 address" case. Good to have become aware about this.

But yes, this unit test here will not be able to exercise this check and we'll either need some kind of an integration test, or write a standard e2e test.

Ok sounds good. Let's leave this test to an integration or e2e test then.

gauravkghildiyal · 2024-12-13T22:33:43Z

/approve

(Pending LGTM from Swetha)

k8s-ci-robot · 2024-12-13T22:33:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gauravkghildiyal, zasweq

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:

~~OWNERS~~ [gauravkghildiyal]

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

swetharepakula

Thanks for making the changes!

One nit in how the test was written.

Otherwise can you squash the commits as well? Or please expand the second commit message with more context.

swetharepakula · 2024-12-13T23:02:52Z

pkg/neg/controller_test.go

@@ -1590,9 +1590,9 @@ func TestServiceIPFamilies(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
 			if tc.enableIPV6OnlyNEG {
-				flags.F.EnableIPV6NEG = true
+				flags.F.EnableIPV6OnlyNEG = true


since this is a global variable, can you store the previous value and reset it in the defer. As the tests grow using this flag, I think that will be safer in terms of test pollution. Thanks

Ah, good point. That is how we set env vars in gRPC-Go tests too. Thanks.

zasweq · 2024-12-16T00:39:29Z

Rebased and squashed all commits including those that address comments into one commit only.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 10, 2024

k8s-ci-robot requested review from aojea and sawsa307 December 10, 2024 02:21

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 10, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 10, 2024

k8s-ci-robot assigned gauravkghildiyal and swetharepakula Dec 10, 2024

k8s-ci-robot removed request for aojea and sawsa307 December 10, 2024 02:25

gauravkghildiyal reviewed Dec 10, 2024

View reviewed changes

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 Dec 10, 2024

krzykwas reviewed Dec 11, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 13, 2024

swetharepakula reviewed Dec 13, 2024

View reviewed changes

Add flag for IPV6 only NEG's

a8eed33

zasweq force-pushed the ipv6neg-flag branch from c3fd292 to a8eed33 Compare December 16, 2024 00:38

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flag for IPV6 only NEG's #2763

Add flag for IPV6 only NEG's #2763

zasweq commented Dec 10, 2024

k8s-ci-robot commented Dec 10, 2024

k8s-ci-robot commented Dec 10, 2024

gauravkghildiyal commented Dec 10, 2024

gauravkghildiyal left a comment

gauravkghildiyal Dec 10, 2024

zasweq Dec 10, 2024

gauravkghildiyal commented Dec 10, 2024

krzykwas Dec 11, 2024

zasweq Dec 12, 2024

zasweq Dec 12, 2024

gauravkghildiyal Dec 13, 2024

zasweq Dec 16, 2024

gauravkghildiyal commented Dec 13, 2024

k8s-ci-robot commented Dec 13, 2024

swetharepakula left a comment

swetharepakula Dec 13, 2024

zasweq Dec 16, 2024

zasweq commented Dec 16, 2024

Add flag for IPV6 only NEG's #2763

Are you sure you want to change the base?

Add flag for IPV6 only NEG's #2763

Conversation

zasweq commented Dec 10, 2024

k8s-ci-robot commented Dec 10, 2024

k8s-ci-robot commented Dec 10, 2024

gauravkghildiyal commented Dec 10, 2024

gauravkghildiyal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravkghildiyal commented Dec 10, 2024

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

gauravkghildiyal commented Dec 13, 2024

k8s-ci-robot commented Dec 13, 2024

swetharepakula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq commented Dec 16, 2024