From 85e0457d63c7fbd0651b25a76940b2fe31d1d221 Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Mon, 2 Sep 2019 23:54:09 -0700 Subject: [PATCH] Support specifying a custom subnet for ILB ip unit test for verifying custom subnet config. simplified ip address logic. Modified init code to always initialize eventRecorder. removed extra import from automerge with master. --- .../k8s.io/legacy-cloud-providers/gce/gce.go | 8 +- .../legacy-cloud-providers/gce/gce_alpha.go | 3 + .../gce/gce_annotations.go | 15 ++++ .../gce/gce_loadbalancer_internal.go | 59 +++++++++---- .../gce/gce_loadbalancer_internal_test.go | 86 +++++++++++++++++++ 5 files changed, 148 insertions(+), 23 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce.go index 1de777909c0..70f46dc3a40 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce.go @@ -607,11 +607,9 @@ func (g *Cloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder, g.clientBuilder = clientBuilder g.client = clientBuilder.ClientOrDie("cloud-provider") - if g.OnXPN() { - g.eventBroadcaster = record.NewBroadcaster() - g.eventBroadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: g.client.CoreV1().Events("")}) - g.eventRecorder = g.eventBroadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: "g-cloudprovider"}) - } + g.eventBroadcaster = record.NewBroadcaster() + g.eventBroadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: g.client.CoreV1().Events("")}) + g.eventRecorder = g.eventBroadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: "g-cloudprovider"}) go g.watchClusterID(stop) } diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go index 70b84d35b11..8cccd66a2ca 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go @@ -27,6 +27,9 @@ const ( // AlphaFeatureILBSubsets allows InternalLoadBalancer services to include a subset // of cluster nodes as backends instead of all nodes. AlphaFeatureILBSubsets = "ILBSubsets" + // AlphaFeatureILBCustomSubnet allows InternalLoadBalancer services to specify a + // network subnet to allocate ip addresses from. + AlphaFeatureILBCustomSubnet = "ILBCustomSubnet" ) // AlphaFeatureGate contains a mapping of alpha features to whether they are enabled diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_annotations.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_annotations.go index c2a3602f714..2280f26d917 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_annotations.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_annotations.go @@ -58,6 +58,11 @@ const ( // created in. ServiceAnnotationILBAllowGlobalAccess = "networking.gke.io/internal-load-balancer-allow-global-access" + // ServiceAnnotationILBSubnet is annotated on a service with the name of the subnetwork + // the ILB IP Address should be assigned from. By default, this is the subnetwork that the + // cluster is created in. + ServiceAnnotationILBSubnet = "networking.gke.io/internal-load-balancer-subnet" + // NetworkTierAnnotationKey is annotated on a Service object to indicate which // network tier a GCP LB should use. The valid values are "Standard" and // "Premium" (default). @@ -132,6 +137,8 @@ func GetServiceNetworkTier(service *v1.Service) (cloud.NetworkTier, error) { type ILBOptions struct { // AllowGlobalAccess Indicates whether global access is allowed for the LoadBalancer AllowGlobalAccess bool + // SubnetName indicates which subnet the LoadBalancer VIPs should be assigned from + SubnetName string } // GetLoadBalancerAnnotationAllowGlobalAccess returns if global access is enabled @@ -139,3 +146,11 @@ type ILBOptions struct { func GetLoadBalancerAnnotationAllowGlobalAccess(service *v1.Service) bool { return service.Annotations[ServiceAnnotationILBAllowGlobalAccess] == "true" } + +// GetLoadBalancerAnnotationSubnet returns the configured subnet to assign LoadBalancer IP from. +func GetLoadBalancerAnnotationSubnet(service *v1.Service) string { + if val, exists := service.Annotations[ServiceAnnotationILBSubnet]; exists { + return val + } + return "" +} diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go index ad91b707d22..1cac0ec9e0d 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go @@ -58,6 +58,12 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v g.eventRecorder.Event(svc, v1.EventTypeWarning, "ILBOptionsIgnored", "Internal LoadBalancer options are not supported with Legacy Networks.") options = ILBOptions{} } + if !g.AlphaFeatureGate.Enabled(AlphaFeatureILBCustomSubnet) { + if options.SubnetName != "" { + g.eventRecorder.Event(svc, v1.EventTypeWarning, "ILBCustomSubnetOptionIgnored", "Internal LoadBalancer CustomSubnet options ignored as the feature gate is disabled.") + options.SubnetName = "" + } + } loadBalancerName := g.GetLoadBalancerName(context.TODO(), clusterName, svc) sharedBackend := shareBackendService(svc) @@ -98,23 +104,32 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v return nil, err } + subnetworkURL := g.SubnetworkURL() + if g.AlphaFeatureGate.Enabled(AlphaFeatureILBCustomSubnet) { + // If this feature is enabled, changes to subnet annotation will be + // picked up and reflected in the forwarding rule. + // Removing the annotation will set the forwarding rule to use the default subnet. + if options.SubnetName != "" { + subnetworkURL = gceSubnetworkURL("", g.networkProjectID, g.region, options.SubnetName) + } + } else { + // TODO(84885) remove this once ILBCustomSubnet goes beta. + if existingFwdRule != nil && existingFwdRule.Subnetwork != "" { + // If the ILB already exists, continue using the subnet that it's already using. + // This is to support existing ILBs that were setup using the wrong subnet - https://github.com/kubernetes/kubernetes/pull/57861 + subnetworkURL = existingFwdRule.Subnetwork + } + } // Determine IP which will be used for this LB. If no forwarding rule has been established // or specified in the Service spec, then requestedIP = "". - requestedIP := determineRequestedIP(svc, existingFwdRule) - ipToUse := requestedIP + ipToUse := ilbIPToUse(svc, existingFwdRule, subnetworkURL) - // If the ILB already exists, continue using the subnet that it's already using. - // This is to support existing ILBs that were setup using the wrong subnet. - subnetworkURL := g.SubnetworkURL() - if existingFwdRule != nil && existingFwdRule.Subnetwork != "" { - // external LBs have an empty Subnetwork field. - subnetworkURL = existingFwdRule.Subnetwork - } + klog.V(2).Infof("ensureInternalLoadBalancer(%v): Using subnet %s for LoadBalancer IP %s", loadBalancerName, options.SubnetName, ipToUse) var addrMgr *addressManager // If the network is not a legacy network, use the address manager if !g.IsLegacyNetwork() { - addrMgr = newAddressManager(g, nm.String(), g.Region(), subnetworkURL, loadBalancerName, requestedIP, cloud.SchemeInternal) + addrMgr = newAddressManager(g, nm.String(), g.Region(), subnetworkURL, loadBalancerName, ipToUse, cloud.SchemeInternal) ipToUse, err = addrMgr.HoldAddress() if err != nil { return nil, err @@ -758,20 +773,27 @@ func getNameFromLink(link string) string { return fields[len(fields)-1] } -func determineRequestedIP(svc *v1.Service, fwdRule *compute.ForwardingRule) string { +// ilbIPToUse determines which IP address needs to be used in the ForwardingRule. If an IP has been +// specified by the user, that is used. If there is an existing ForwardingRule, the ip address from +// that is reused. In case a subnetwork change is requested, the existing ForwardingRule IP is ignored. +func ilbIPToUse(svc *v1.Service, fwdRule *compute.ForwardingRule, requestedSubnet string) string { if svc.Spec.LoadBalancerIP != "" { return svc.Spec.LoadBalancerIP } - - if fwdRule != nil { - return fwdRule.IPAddress + if fwdRule == nil { + return "" } - - return "" + if requestedSubnet != fwdRule.Subnetwork { + // reset ip address since subnet is being changed. + return "" + } + return fwdRule.IPAddress } func getILBOptions(svc *v1.Service) ILBOptions { - return ILBOptions{AllowGlobalAccess: GetLoadBalancerAnnotationAllowGlobalAccess(svc)} + return ILBOptions{AllowGlobalAccess: GetLoadBalancerAnnotationAllowGlobalAccess(svc), + SubnetName: GetLoadBalancerAnnotationSubnet(svc), + } } // forwardingRuleComposite is a composite type encapsulating both the GA and Beta ForwardingRules. @@ -800,7 +822,8 @@ func (f *forwardingRuleComposite) Equal(other *forwardingRuleComposite) bool { f.lbScheme == other.lbScheme && equalStringSets(f.ports, other.ports) && f.backendService == other.backendService && - f.allowGlobalAccess == other.allowGlobalAccess + f.allowGlobalAccess == other.allowGlobalAccess && + f.subnetwork == other.subnetwork } // toForwardingRuleComposite converts a compute beta or GA ForwardingRule into the composite type diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go index 17e8a342c20..0db7128bc89 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go @@ -1309,3 +1309,89 @@ func TestForwardingRuleCompositeEqual(t *testing.T) { t.Errorf("Expected frcGA and frcBeta rules to be unequal, got true") } } + +func TestEnsureInternalLoadBalancerCustomSubnet(t *testing.T) { + t.Parallel() + + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) + require.NoError(t, err) + gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{AlphaFeatureILBCustomSubnet}) + + nodeNames := []string{"test-node-1"} + nodes, err := createAndInsertNodes(gce, nodeNames, vals.ZoneName) + require.NoError(t, err) + svc := fakeLoadbalancerService(string(LBTypeInternal)) + status, err := createInternalLoadBalancer(gce, svc, nil, nodeNames, vals.ClusterName, vals.ClusterID, vals.ZoneName) + lbName := gce.GetLoadBalancerName(context.TODO(), "", svc) + + if err != nil { + t.Errorf("Unexpected error %v", err) + } + assert.NotEmpty(t, status.Ingress) + fwdRule, err := gce.GetBetaRegionForwardingRule(lbName, gce.region) + if err != nil || fwdRule == nil { + t.Errorf("Unexpected error %v", err) + } + if fwdRule.Subnetwork != "" { + t.Errorf("Unexpected subnet value %s in ILB ForwardingRule", fwdRule.Subnetwork) + } + + // Change service to include the global access annotation and request static ip + requestedIP := "4.5.6.7" + svc.Annotations[ServiceAnnotationILBSubnet] = "test-subnet" + svc.Spec.LoadBalancerIP = requestedIP + status, err = gce.EnsureLoadBalancer(context.Background(), vals.ClusterName, svc, nodes) + if err != nil { + t.Errorf("Unexpected error %v", err) + } + assert.NotEmpty(t, status.Ingress) + if status.Ingress[0].IP != requestedIP { + t.Errorf("Reserved IP %s not propagated, Got %s", requestedIP, status.Ingress[0].IP) + } + fwdRule, err = gce.GetBetaRegionForwardingRule(lbName, gce.region) + if err != nil || fwdRule == nil { + t.Errorf("Unexpected error %v", err) + } + if !strings.HasSuffix(fwdRule.Subnetwork, "test-subnet") { + t.Errorf("Unexpected subnet value %s in ILB ForwardingRule.", fwdRule.Subnetwork) + } + + // Change to a different subnet + svc.Annotations[ServiceAnnotationILBSubnet] = "another-subnet" + status, err = gce.EnsureLoadBalancer(context.Background(), vals.ClusterName, svc, nodes) + if err != nil { + t.Errorf("Unexpected error %v", err) + } + assert.NotEmpty(t, status.Ingress) + if status.Ingress[0].IP != requestedIP { + t.Errorf("Reserved IP %s not propagated, Got %s", requestedIP, status.Ingress[0].IP) + } + fwdRule, err = gce.GetBetaRegionForwardingRule(lbName, gce.region) + if err != nil || fwdRule == nil { + t.Errorf("Unexpected error %v", err) + } + if !strings.HasSuffix(fwdRule.Subnetwork, "another-subnet") { + t.Errorf("Unexpected subnet value %s in ILB ForwardingRule.", fwdRule.Subnetwork) + } + // remove the annotation - ILB should revert to default subnet. + delete(svc.Annotations, ServiceAnnotationILBSubnet) + status, err = gce.EnsureLoadBalancer(context.Background(), vals.ClusterName, svc, nodes) + if err != nil { + t.Errorf("Unexpected error %v", err) + } + assert.NotEmpty(t, status.Ingress) + fwdRule, err = gce.GetBetaRegionForwardingRule(lbName, gce.region) + if err != nil { + t.Errorf("Unexpected error %v", err) + } + if fwdRule.Subnetwork != "" { + t.Errorf("Unexpected subnet value %s in ILB ForwardingRule.", fwdRule.Subnetwork) + } + // Delete the service + err = gce.EnsureLoadBalancerDeleted(context.Background(), vals.ClusterName, svc) + if err != nil { + t.Errorf("Unexpected error %v", err) + } + assertInternalLbResourcesDeleted(t, gce, svc, vals, true) +}