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

Add flag for IPV6 only NEG's #2763

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add flag for IPV6 only NEG's #2763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ var F = struct {
ProviderConfigAPIGroup string
EnableL4ILBMixedProtocol bool
EnableL4NetLBMixedProtocol bool
EnableIPV6OnlyNEG bool
}{
GCERateLimitScale: 1.0,
}
Expand Down Expand Up @@ -334,6 +335,7 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5
flag.BoolVar(&F.EnableL4NetLBMixedProtocol, "enable-l4netlb-mixed-protocol", false, "Enable support for mixed protocol L4 external load balancers.")
flag.StringVar(&F.ProviderConfigAPIGroup, "provider-config-api-group", "", "The API group for the ProviderConfig CRD.")
flag.StringVar(&F.ProviderConfigNameLabelKey, "provider-config-name-label-key", "", "The label key for provider-config name, which is used to identify the provider-config of objects in multi-project mode.")
flag.BoolVar(&F.EnableIPV6OnlyNEG, "enable-ipv6-only-neg", false, "Enable support for IPV6 Only NEG's.")
}

func Validate() {
Expand Down
8 changes: 5 additions & 3 deletions pkg/neg/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,10 +559,12 @@ func (c *Controller) processService(key string) error {
}
if len(svcPortInfoMap) != 0 {
c.logger.V(2).Info("Syncing service", "service", key)
// TODO(cheungdavid): Remove this validation when single stack ipv6 endpoint is supported
if service.Spec.Type != apiv1.ServiceTypeLoadBalancer && isSingleStackIPv6Service(service) {
return fmt.Errorf("NEG is not supported for ipv6 only service (%T)", service)
if !flags.F.EnableIPV6OnlyNEG {
if service.Spec.Type != apiv1.ServiceTypeLoadBalancer && isSingleStackIPv6Service(service) {
return fmt.Errorf("NEG is not supported for ipv6 only service (%T)", service)
}
}

if err = c.syncNegStatusAnnotation(namespace, name, svcPortInfoMap); err != nil {
return err
}
Expand Down
27 changes: 22 additions & 5 deletions pkg/neg/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import (
"k8s.io/client-go/util/workqueue"
"k8s.io/cloud-provider-gcp/providers/gce"
"k8s.io/ingress-gce/pkg/annotations"
"k8s.io/ingress-gce/pkg/flags"
"k8s.io/ingress-gce/pkg/neg/metrics/metricscollector"
"k8s.io/ingress-gce/pkg/neg/syncers/labels"
negtypes "k8s.io/ingress-gce/pkg/neg/types"
Expand Down Expand Up @@ -1500,11 +1501,12 @@ func TestServiceIPFamilies(t *testing.T) {
preferDualStack := apiv1.IPFamilyPolicyPreferDualStack
requireDualStack := apiv1.IPFamilyPolicyRequireDualStack
testCases := []struct {
desc string
serviceType v1.ServiceType
ipFamilies []v1.IPFamily
ipFamilyPolicy *apiv1.IPFamilyPolicy
expectNil bool
desc string
serviceType v1.ServiceType
ipFamilies []v1.IPFamily
ipFamilyPolicy *apiv1.IPFamilyPolicy
expectNil bool
enableIPV6OnlyNEG bool
}{
{
desc: "ipv6 only service with l7 load balancer, ipFamilyPolicy is singleStack",
Expand All @@ -1513,6 +1515,14 @@ func TestServiceIPFamilies(t *testing.T) {
ipFamilyPolicy: &singleStack,
expectNil: false,
},
{
desc: "ipv6 only service with l7 load balancer, ipFamilyPolicy is singleStack, ipv6 neg enabled",
serviceType: v1.ServiceTypeClusterIP,
ipFamilies: []v1.IPFamily{v1.IPv6Protocol},
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

ipFamilyPolicy: &singleStack,
expectNil: true,
enableIPV6OnlyNEG: true,
},
{
desc: "ipv4 only service with l7 load balancer, ipFamilyPolicy is singleStack",
serviceType: v1.ServiceTypeClusterIP,
Expand Down Expand Up @@ -1579,6 +1589,13 @@ func TestServiceIPFamilies(t *testing.T) {
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
if tc.enableIPV6OnlyNEG {
origEnableIPV6OnlyNEG := flags.F.EnableIPV6OnlyNEG
flags.F.EnableIPV6OnlyNEG = true
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

defer func() {
flags.F.EnableIPV6OnlyNEG = origEnableIPV6OnlyNEG
}()
}
controller := newTestController(fake.NewSimpleClientset())
defer controller.stop()
testService := newTestService(controller, false, []int32{80})
Expand Down