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.

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

Allow for use subnetworks with IPV6_Only stack type and check if cust… #2624

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion pkg/loadbalancers/l4.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ import (
)

const (
subnetInternalIPv6AccessType = "INTERNAL"
subnetInternalAccessType = "INTERNAL"

WeightedLBPodsPerNodeAllowlistMessage = "Weighted Load Balancing for L4 " +
"Internal Passthrough Load Balancers requires project allowlisting. If " +
"you need access to this feature please contact Google Cloud support team"
Expand Down
4 changes: 2 additions & 2 deletions pkg/loadbalancers/l4_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1231,7 +1231,7 @@ func TestDualStackILBBadCustomSubnet(t *testing.T) {
},
{
desc: "Should return error on external ipv6 subnet",
subnetIpv6AccessType: subnetExternalIPv6AccessType,
subnetIpv6AccessType: subnetExternalAccessType,
subnetStackType: "IPV4_IPV6",
},
}
Expand Down Expand Up @@ -2295,7 +2295,7 @@ func mustSetupILBTestHandler(t *testing.T, svc *v1.Service, nodeNames []string)
clusterSubnetName := ""
key := meta.RegionalKey(clusterSubnetName, l4.cloud.Region())
subnetToCreate := &compute.Subnetwork{
Ipv6AccessType: subnetInternalIPv6AccessType,
Ipv6AccessType: subnetInternalAccessType,
StackType: "IPV4_IPV6",
}
err := fakeGCE.Compute().(*cloud.MockGCE).Subnetworks().Insert(context.TODO(), key, subnetToCreate)
Expand Down
18 changes: 17 additions & 1 deletion pkg/loadbalancers/l4ipv6.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ func (l4 *L4) getOldIPv6ForwardingRule(existingBS *composite.BackendService) (*c

func (l4 *L4) serviceSubnetHasInternalIPv6Range() error {
subnetName := l4.subnetName()
hasIPv6SubnetRange, err := utils.SubnetHasIPv6Range(l4.cloud, subnetName, subnetInternalIPv6AccessType)
hasIPv6SubnetRange, err := utils.SubnetHasIPv6Range(l4.cloud, subnetName, subnetInternalAccessType)
if err != nil {
return err
}
Expand All @@ -215,3 +215,19 @@ func (l4 *L4) serviceSubnetHasInternalIPv6Range() error {
}
return nil
}

func (l4 *L4) serviceSubnetHasInternalIPv4Range() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it even called anywhere?

subnetName := l4.subnetName()
hasIPv6SubnetRange, err := utils.SubnetHasIPv4Range(l4.cloud, subnetName, subnetInternalAccessType)
Copy link

Choose a reason for hiding this comment

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

You have IPv6 on the left and IPv4 on the right

if err != nil {
return err
}
if !hasIPv6SubnetRange {
// We don't need to emit custom event, because errors are already emitted to the user as events.
l4.svcLogger.Info("Subnet for IPv4 Service does not have internal IPv4 ranges", "subnetName", subnetName)
return utils.NewUserError(
fmt.Errorf(
"subnet %s does not have internal IPv4 ranges, required for an internal IPv4 Service. You can specify an internal IPv4 subnet using the \"%s\" annotation on the Service", subnetName, annotations.CustomSubnetAnnotationKey))
}
return nil
}
9 changes: 9 additions & 0 deletions pkg/loadbalancers/l4netlb.go
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,15 @@ func (l4netlb *L4NetLB) EnsureFrontend(nodeNames []string, svc *corev1.Service)
}
}

// If service requires IPv6 LoadBalancer -- verify that Subnet with External IPv4 ranges is used.
Copy link

Choose a reason for hiding this comment

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

Suggested change
// If service requires IPv6 LoadBalancer -- verify that Subnet with External IPv4 ranges is used.
// If service requires IPv4 LoadBalancer -- verify that Subnet with External IPv4 ranges is used.

if l4netlb.enableDualStack && utils.NeedsIPv6(svc) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
if l4netlb.enableDualStack && utils.NeedsIPv6(svc) {
if l4netlb.enableDualStack && utils.NeedsIPv4(svc) {

err := l4netlb.serviceSubnetHasExternalIPv4Range()
if err != nil {
result.Error = err
return result
}
}

hcLink := l4netlb.provideHealthChecks(nodeNames, result)
if result.Error != nil {
return result
Expand Down
6 changes: 3 additions & 3 deletions pkg/loadbalancers/l4netlb_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -689,7 +689,7 @@ func TestEnsureIPv6ExternalLoadBalancerCustomSubnet(t *testing.T) {
// create custom subnet
subnetKey := meta.RegionalKey("test-subnet", l4NetLB.cloud.Region())
subnetToCreate := &ga.Subnetwork{
Ipv6AccessType: subnetExternalIPv6AccessType,
Ipv6AccessType: subnetExternalAccessType,
StackType: "IPV4_IPV6",
}
err := l4NetLB.cloud.Compute().(*cloud.MockGCE).Subnetworks().Insert(context.TODO(), subnetKey, subnetToCreate)
Expand Down Expand Up @@ -750,7 +750,7 @@ func TestDualStackNetLBBadCustomSubnet(t *testing.T) {
},
{
desc: "Should return error on internal ipv6 subnet",
subnetIpv6AccessType: subnetInternalIPv6AccessType,
subnetIpv6AccessType: subnetInternalAccessType,
subnetStackType: "IPV4_IPV6",
},
}
Expand Down Expand Up @@ -907,7 +907,7 @@ func mustSetupNetLBTestHandler(t *testing.T, svc *v1.Service, nodeNames []string
}

// Create cluster subnet. Mock GCE uses subnet with empty string name.
test.MustCreateDualStackClusterSubnet(t, l4NetLB.cloud, subnetExternalIPv6AccessType)
test.MustCreateDualStackClusterSubnet(t, l4NetLB.cloud, subnetExternalAccessType)
return l4NetLB
}

Expand Down
26 changes: 21 additions & 5 deletions pkg/loadbalancers/l4netlbipv6.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ import (
)

const (
ipv6Suffix = "-ipv6"
subnetExternalIPv6AccessType = "EXTERNAL"
ipv6Suffix = "-ipv6"
subnetExternalAccessType = "EXTERNAL"
)

// ensureIPv6Resources creates resources specific to IPv6 L4 NetLB Load Balancers:
Expand Down Expand Up @@ -195,7 +195,7 @@ func (l4netlb *L4NetLB) ipv6SubnetURL() (string, error) {
return l4netlb.cloud.SubnetworkURL(), nil
}

func (l4netlb *L4NetLB) ipv6SubnetName() string {
func (l4netlb *L4NetLB) getSubnetName() string {
// At first check custom subnet annotation.
customSubnetName := annotations.FromService(l4netlb.Service).GetExternalLoadBalancerAnnotationSubnet()
if customSubnetName != "" {
Expand All @@ -209,8 +209,8 @@ func (l4netlb *L4NetLB) ipv6SubnetName() string {
}

func (l4netlb *L4NetLB) serviceSubnetHasExternalIPv6Range() error {
subnetName := l4netlb.ipv6SubnetName()
hasIPv6SubnetRange, err := utils.SubnetHasIPv6Range(l4netlb.cloud, subnetName, subnetExternalIPv6AccessType)
subnetName := l4netlb.getSubnetName()
hasIPv6SubnetRange, err := utils.SubnetHasIPv6Range(l4netlb.cloud, subnetName, subnetExternalAccessType)
if err != nil {
return err
}
Expand All @@ -223,3 +223,19 @@ func (l4netlb *L4NetLB) serviceSubnetHasExternalIPv6Range() error {
}
return nil
}

func (l4netlb *L4NetLB) serviceSubnetHasExternalIPv4Range() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

it's a bit strange to me that function serviceSubnetHasExternalIPv4Range instead of returning bool returns error

reading something like this

err := l4netlb.serviceSubnetHasExternalIPv4Range()

makes you think that error happens when something wrong happened, not when service just doesn't have ExternalIPv4Range

though, I am not strong on this opinion :)

subnetName := l4netlb.getSubnetName()
hasIPv6SubnetRange, err := utils.SubnetHasIPv4Range(l4netlb.cloud, subnetName, subnetExternalAccessType)
Copy link

Choose a reason for hiding this comment

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

ditto: IPv6 on the left, IPv4 on the right

if err != nil {
return err
}
if !hasIPv6SubnetRange {
// We don't need to emit custom event, because errors are already emitted to the user as events.
l4netlb.svcLogger.Info("Subnet for IPv4 Service does not have external IPv6 ranges", "subnetName", subnetName)
return utils.NewUserError(
fmt.Errorf(
"subnet %s does not have external IPv4 ranges, required for an external IPv6 Service. You can specify an external IPv4 subnet using the \"%s\" annotation on the Service", subnetName, annotations.CustomSubnetAnnotationKey))
}
return nil
}
24 changes: 21 additions & 3 deletions pkg/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package utils

import (
"context"
"encoding/json"
"errors"
"fmt"
Expand Down Expand Up @@ -87,6 +88,9 @@ const (
// be removed in 1.18.
LabelAlphaNodeRoleExcludeBalancer = "alpha.service-controller.kubernetes.io/exclude-balancer"
DualStackSubnetStackType = "IPV4_IPV6"
IPv6SubnetStackType = "IPV6_ONLY"

IPv4SubnetStackType = "IPV4_ONLY"

// LabelNodeSubnet specifies the subnet name of this node.
LabelNodeSubnet = "cloud.google.com/gke-node-pool-subnet"
Expand Down Expand Up @@ -883,12 +887,26 @@ func AddIPToLBStatus(status *api_v1.LoadBalancerStatus, ips ...string) *api_v1.L
return status
}

func SubnetHasIPv6Range(cloud *gce.Cloud, subnetName, ipv6AccessType string) (bool, error) {
subnet, err := cloud.GetSubnetwork(cloud.Region(), subnetName)
func SubnetHasIPv6Range(gcecloud *gce.Cloud, subnetName, accessType string) (bool, error) {
key := meta.RegionalKey(subnetName, gcecloud.Region())
subnet, err := gcecloud.Compute().AlphaSubnetworks().Get(context.Background(), key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it only available in Alpha?

if err != nil {
return false, fmt.Errorf("failed getting subnet: %w", err)
}
stackTypeMatches := subnet.StackType == DualStackSubnetStackType || subnet.StackType == IPv6SubnetStackType
accessTypeMatches := subnet.Ipv6AccessType == accessType
return stackTypeMatches && accessTypeMatches, nil
}

func SubnetHasIPv4Range(gcecloud *gce.Cloud, subnetName, accessType string) (bool, error) {
key := meta.RegionalKey(subnetName, gcecloud.Region())
subnet, err := gcecloud.Compute().AlphaSubnetworks().Get(context.Background(), key)
if err != nil {
return false, fmt.Errorf("failed getting subnet: %w", err)
}
return subnet.StackType == DualStackSubnetStackType && subnet.Ipv6AccessType == ipv6AccessType, nil
stackTypeMatches := subnet.StackType == DualStackSubnetStackType || subnet.StackType == IPv4SubnetStackType
accessTypeMatches := subnet.Ipv6AccessType == accessType
Copy link

Choose a reason for hiding this comment

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

out of curiosity: is ipv6AccessType relevant for IPv4 subnet?

return stackTypeMatches && accessTypeMatches, nil
}

// IsUnsupportedFeatureError returns true if the error has 400 number,
Expand Down