From cfdad70de9d625e2ac6817cb9bcea1868a06257c Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Thu, 23 Jul 2020 09:26:25 -0700 Subject: [PATCH 1/2] Delete ILB FR in case of changes to port/proto. --- .../gce/gce_loadbalancer_internal.go | 29 ++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) 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 ef285b308ec..cf35e1a3426 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 @@ -173,12 +173,6 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v return nil, err } - bsDescription := makeBackendServiceDescription(nm, sharedBackend) - err = g.ensureInternalBackendService(backendServiceName, bsDescription, svc.Spec.SessionAffinity, scheme, protocol, igLinks, hc.SelfLink) - if err != nil { - return nil, err - } - fwdRuleDescription := &forwardingRuleDescription{ServiceName: nm.String()} fwdRuleDescriptionString, err := fwdRuleDescription.marshal() if err != nil { @@ -200,10 +194,31 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v if options.AllowGlobalAccess { newFwdRule.AllowGlobalAccess = options.AllowGlobalAccess } - if err := g.ensureInternalForwardingRule(existingFwdRule, newFwdRule); err != nil { + + fwdRuleDeleted := false + if existingFwdRule != nil && !forwardingRulesEqual(existingFwdRule, newFwdRule) { + // Delete existing forwarding rule before making changes to the backend service. For example - changing protocol + // of backend service without first deleting forwarding rule will throw an error since the linked forwarding + // rule would show the old protocol. + klog.V(2).Infof("ensureInternalLoadBalancer(%v): deleting existing forwarding rule with IP address %v", loadBalancerName, existingFwdRule.IPAddress) + if err = ignoreNotFound(g.DeleteRegionForwardingRule(loadBalancerName, g.region)); err != nil { + return nil, err + } + fwdRuleDeleted = true + } + + bsDescription := makeBackendServiceDescription(nm, sharedBackend) + err = g.ensureInternalBackendService(backendServiceName, bsDescription, svc.Spec.SessionAffinity, scheme, protocol, igLinks, hc.SelfLink) + if err != nil { return nil, err } + if fwdRuleDeleted || existingFwdRule == nil { + if err := g.ensureInternalForwardingRule(existingFwdRule, newFwdRule); err != nil { + return nil, err + } + } + // Delete the previous internal load balancer resources if necessary if existingBackendService != nil { g.clearPreviousInternalResources(svc, loadBalancerName, existingBackendService, backendServiceName, hcName) From 3fc6100eb56fa368df99a1e4747bf488f4ea8844 Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Thu, 23 Jul 2020 13:55:31 -0700 Subject: [PATCH 2/2] unit test --- .../gce/gce_loadbalancer_internal_test.go | 65 ++++++++++++++++++- 1 file changed, 63 insertions(+), 2 deletions(-) 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 f3b2d902ce3..be9560a22d6 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 @@ -32,11 +32,11 @@ import ( "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock" "google.golang.org/api/compute/v1" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" - "k8s.io/cloud-provider" + cloudprovider "k8s.io/cloud-provider" servicehelper "k8s.io/cloud-provider/service/helpers" ) @@ -1638,3 +1638,64 @@ func TestEnsureLoadBalancerPartialDelete(t *testing.T) { require.NoError(t, err) assert.False(t, exists) } + +func TestEnsureInternalLoadBalancerModifyProtocol(t *testing.T) { + t.Parallel() + + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) + require.NoError(t, err) + c := gce.c.(*cloud.MockGCE) + c.MockRegionBackendServices.UpdateHook = func(ctx context.Context, key *meta.Key, be *compute.BackendService, m *cloud.MockRegionBackendServices) error { + // Same key can be used since FR will have the same name. + fr, err := c.MockForwardingRules.Get(ctx, key) + if err != nil && !isNotFound(err) { + return err + } + if fr != nil && fr.IPProtocol != be.Protocol { + return fmt.Errorf("Protocol mismatch between Forwarding Rule value %q and Backend service value %q", fr.IPProtocol, be.Protocol) + } + return mock.UpdateRegionBackendServiceHook(ctx, key, be, m) + } + nodeNames := []string{"test-node-1"} + nodes, err := createAndInsertNodes(gce, nodeNames, vals.ZoneName) + require.NoError(t, err) + svc := fakeLoadbalancerService(string(LBTypeInternal)) + svc, err = gce.client.CoreV1().Services(svc.Namespace).Create(context.TODO(), svc, metav1.CreateOptions{}) + require.NoError(t, err) + lbName := gce.GetLoadBalancerName(context.TODO(), "", svc) + status, err := createInternalLoadBalancer(gce, svc, nil, nodeNames, vals.ClusterName, vals.ClusterID, vals.ZoneName) + if err != nil { + t.Errorf("Unexpected error %v", err) + } + assert.NotEmpty(t, status.Ingress) + fwdRule, err := gce.GetRegionForwardingRule(lbName, gce.region) + if err != nil { + t.Errorf("gce.GetRegionForwardingRule(%q, %q) = %v, want nil", lbName, gce.region, err) + } + if fwdRule.IPProtocol != "TCP" { + t.Errorf("Unexpected protocol value %s, expected TCP", fwdRule.IPProtocol) + } + + // change the protocol to UDP + svc.Spec.Ports[0].Protocol = v1.ProtocolUDP + 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.GetRegionForwardingRule(lbName, gce.region) + if err != nil { + t.Errorf("gce.GetRegionForwardingRule(%q, %q) = %v, want nil", lbName, gce.region, err) + } + if fwdRule.IPProtocol != "UDP" { + t.Errorf("Unexpected protocol value %s, expected UDP", fwdRule.IPProtocol) + } + + // 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) +}