diff --git a/pkg/annotations/service.go b/pkg/annotations/service.go index d6f7578eea..d5028b3391 100644 --- a/pkg/annotations/service.go +++ b/pkg/annotations/service.go @@ -22,6 +22,7 @@ import ( "fmt" "k8s.io/api/core/v1" "k8s.io/legacy-cloud-providers/gce" + "strings" ) const ( @@ -66,6 +67,25 @@ const ( ProtocolHTTPS AppProtocol = "HTTPS" // ProtocolHTTP2 protocol for a service ProtocolHTTP2 AppProtocol = "HTTP2" + + // ServiceStatusPrefix is the prefix used in annotations used to record + // debug information in the Service annotations. This is applicable to L4 ILB services. + ServiceStatusPrefix = "service.kubernetes.io" + // TCPForwardingRuleKey is the annotation key used by l4 controller to record + // GCP TCP forwarding rule name. + TCPForwardingRuleKey = ServiceStatusPrefix + "/tcp-forwarding-rule" + // UDPForwardingRuleKey is the annotation key used by l4 controller to record + // GCP UDP forwarding rule name. + UDPForwardingRuleKey = ServiceStatusPrefix + "/udp-forwarding-rule" + // BackendServiceKey is the annotation key used by l4 controller to record + // GCP Backend service name. + BackendServiceKey = ServiceStatusPrefix + "/backend-service" + // FirewallRuleKey is the annotation key used by l4 controller to record + // GCP Firewall rule name. + FirewallRuleKey = ServiceStatusPrefix + "/firewall-rule" + // HealthcheckKey is the annotation key used by l4 controller to record + // GCP Healthcheck name. + HealthcheckKey = ServiceStatusPrefix + "/healthcheck" ) // NegAnnotation is the format of the annotation associated with the @@ -182,24 +202,25 @@ func WantsL4ILB(service *v1.Service) (bool, string) { return false, fmt.Sprintf("Type : %s, LBType : %s", service.Spec.Type, ltype) } -// OnlyNEGStatusChanged returns true if the only annotation change between the 2 services is the NEG status annotation. -// This will be true if neg annotation was added or removed in the new service. +// OnlyStatusAnnotationsChanged returns true if the only annotation change between the 2 services is the NEG or ILB +// resources annotations. // Note : This assumes that the annotations in old and new service are different. If they are identical, this will // return true. -func OnlyNEGStatusChanged(oldService, newService *v1.Service) bool { - return onlyNEGStatusChanged(oldService, newService) && onlyNEGStatusChanged(newService, oldService) +func OnlyStatusAnnotationsChanged(oldService, newService *v1.Service) bool { + return onlyStatusAnnotationsChanged(oldService, newService) && onlyStatusAnnotationsChanged(newService, oldService) } -// onlyNEGStatusChanged returns true if the NEG Status annotation is the only extra annotation present in the new -// service but not in the old service. +// onlyStatusAnnotationsChanged returns true if the NEG Status or ILB resources annotations are the only extra +// annotations present in the new service but not in the old service. // Note : This assumes that the annotations in old and new service are different. If they are identical, this will // return true. -func onlyNEGStatusChanged(oldService, newService *v1.Service) bool { - for key, _ := range newService.ObjectMeta.Annotations { - if _, ok := oldService.ObjectMeta.Annotations[key]; !ok { - if key != NEGStatusKey { - return false +func onlyStatusAnnotationsChanged(oldService, newService *v1.Service) bool { + for key, val := range newService.ObjectMeta.Annotations { + if oldVal, ok := oldService.ObjectMeta.Annotations[key]; !ok || oldVal != val { + if key == NEGStatusKey || strings.HasPrefix(key, ServiceStatusPrefix) { + continue } + return false } } return true diff --git a/pkg/annotations/service_test.go b/pkg/annotations/service_test.go index 263a3be4cf..64a5b4fe50 100644 --- a/pkg/annotations/service_test.go +++ b/pkg/annotations/service_test.go @@ -459,3 +459,173 @@ func TestParseNegStatus(t *testing.T) { }) } } + +func TestOnlyStatusAnnotationsChanged(t *testing.T) { + for _, tc := range []struct { + desc string + service1 *v1.Service + service2 *v1.Service + expectedResult bool + }{ + { + desc: "Test add neg annotation", + service1: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service1", + }, + }, + service2: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service2", + Annotations: map[string]string{ + NEGStatusKey: `{"network_endpoint_groups":{"80":"neg-name"},"zones":["us-central1-a"]}`, + }, + }, + }, + expectedResult: true, + }, + { + desc: "Test valid diff", + service1: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service1", + Annotations: map[string]string{ + NEGStatusKey: `{"network_endpoint_groups":{"80":"neg-name"},"zones":["us-central1-a"]}`, + }, + }, + }, + service2: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service2", + Annotations: map[string]string{ + "RandomAnnotation": "abcde", + }, + }, + }, + expectedResult: false, + }, + { + desc: "Test no change", + service1: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service1", + Annotations: map[string]string{ + "RandomAnnotation": "abcde", + }, + }, + }, + service2: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service2", + Annotations: map[string]string{ + "RandomAnnotation": "abcde", + }, + }, + }, + expectedResult: true, + }, + { + desc: "Test remove NEG annotation", + service1: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service1", + Annotations: map[string]string{ + NEGStatusKey: `{"network_endpoint_groups":{"80":"neg-name"},"zones":["us-central1-a"]}`, + "RandomAnnotation": "abcde", + }, + }, + }, + service2: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service2", + Annotations: map[string]string{ + "RandomAnnotation": "abcde", + }, + }, + }, + expectedResult: true, + }, + { + desc: "Test only ILB ForwardingRule annotation diff", + service1: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service1", + Annotations: map[string]string{ + FirewallRuleKey: "rule1", + TCPForwardingRuleKey: "tcprule", + NEGStatusKey: `{"network_endpoint_groups":{"80":"neg-name"},"zones":["us-central1-a"]}`, + "RandomAnnotation": "abcde", + }, + }, + }, + service2: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service2", + Annotations: map[string]string{ + FirewallRuleKey: "rule1", + UDPForwardingRuleKey: "udprule", + NEGStatusKey: `{"network_endpoint_groups":{"80":"neg-name"},"zones":["us-central1-a"]}`, + "RandomAnnotation": "abcde", + }, + }, + }, + expectedResult: true, + }, + { + desc: "Test all status annotations removed", + service1: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service1", + Annotations: map[string]string{ + FirewallRuleKey: "rule1", + TCPForwardingRuleKey: "tcprule", + NEGStatusKey: `{"network_endpoint_groups":{"80":"neg-name"},"zones":["us-central1-a"]}`, + "RandomAnnotation": "abcde", + }, + }, + }, + service2: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service2", + Annotations: map[string]string{ + "RandomAnnotation": "abcde", + }, + }, + }, + expectedResult: true, + }, + { + desc: "Test change value of non-status annotation", + service1: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service1", + Annotations: map[string]string{ + FirewallRuleKey: "rule1", + TCPForwardingRuleKey: "tcprule", + NEGStatusKey: `{"network_endpoint_groups":{"80":"neg-name"},"zones":["us-central1-a"]}`, + "RandomAnnotation": "abcde", + }, + }, + }, + service2: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service2", + Annotations: map[string]string{ + FirewallRuleKey: "rule1", + TCPForwardingRuleKey: "tcprule", + NEGStatusKey: `{"network_endpoint_groups":{"80":"neg-name"},"zones":["us-central1-a"]}`, + "RandomAnnotation": "xyz", + }, + }, + }, + expectedResult: false, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + result := OnlyStatusAnnotationsChanged(tc.service1, tc.service2) + if result != tc.expectedResult { + t.Errorf("%s: Expected result for input %v, %v to be %v, got %v instead", tc.desc, tc.service1.Annotations, tc.service2.Annotations, tc.expectedResult, result) + } + }) + } +} diff --git a/pkg/l4/l4controller.go b/pkg/l4/l4controller.go index eae0c5af9c..aa1bbd8f31 100644 --- a/pkg/l4/l4controller.go +++ b/pkg/l4/l4controller.go @@ -17,16 +17,19 @@ limitations under the License. package l4 import ( + context2 "context" "fmt" "reflect" "sync" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" + "k8s.io/cloud-provider/service/helpers" "k8s.io/ingress-gce/pkg/annotations" "k8s.io/ingress-gce/pkg/backends" "k8s.io/ingress-gce/pkg/context" @@ -90,8 +93,10 @@ func NewController(ctx *context.ControllerContext, stopCh chan struct{}) *L4Cont curSvc := cur.(*v1.Service) svcKey := utils.ServiceKeyFunc(curSvc.Namespace, curSvc.Name) oldSvc := old.(*v1.Service) - if l4c.needsUpdate(curSvc, oldSvc) || needsDeletion(curSvc) { - klog.V(3).Infof("Service %v changed, enqueuing", svcKey) + needsUpdate := l4c.needsUpdate(oldSvc, curSvc) + needsDeletion := needsDeletion(curSvc) + if needsUpdate || needsDeletion { + klog.V(3).Infof("Service %v changed, needsUpdate %v, needsDeletion %v, enqueuing", svcKey, needsUpdate, needsDeletion) l4c.svcQueue.Enqueue(curSvc) return } @@ -153,7 +158,7 @@ func (l4c *L4Controller) processServiceCreateOrUpdate(key string, service *v1.Se } // Use the same function for both create and updates. If controller crashes and restarts, // all existing services will show up as Service Adds. - status, err := l4.EnsureInternalLoadBalancer(nodeNames, service, &serviceMetricsState) + status, annotationsMap, err := l4.EnsureInternalLoadBalancer(nodeNames, service, &serviceMetricsState) if err != nil { l4c.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "SyncLoadBalancerFailed", "Error syncing load balancer: %v", err) @@ -178,6 +183,11 @@ func (l4c *L4Controller) processServiceCreateOrUpdate(key string, service *v1.Se } l4c.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeNormal, "SyncLoadBalancerSuccessful", "Successfully ensured load balancer resources") + if err = l4c.updateAnnotations(service.Name, service.Namespace, l4.MergeAnnotations(service, annotationsMap)); err != nil { + l4c.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "SyncLoadBalancerFailed", + "Failed to update annotations for load balancer, err: %v", err) + return fmt.Errorf("failed to set resource annotations, err: %v", err) + } return nil } @@ -188,10 +198,16 @@ func (l4c *L4Controller) processServiceDeletion(key string, svc *v1.Service) err l4c.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeWarning, "DeleteLoadBalancerFailed", "Error deleting load balancer: %v", err) return err } + // Also remove any ILB annotations from the service metadata + if err := l4c.updateAnnotations(svc.Name, svc.Namespace, l4.MergeAnnotations(svc, nil)); err != nil { + l4c.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeWarning, "DeleteLoadBalancer", + "Error resetting resource annotations for load balancer: %v", err) + return fmt.Errorf("failed to reset resource annotations, err: %v", err) + } if err := common.EnsureDeleteServiceFinalizer(svc, common.ILBFinalizerV2, l4c.ctx.KubeClient); err != nil { l4c.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeWarning, "DeleteLoadBalancerFailed", "Error removing finalizer from load balancer: %v", err) - return err + return fmt.Errorf("failed to remove ILB finalizer, err: %v", err) } namespacedName := types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace} @@ -202,7 +218,7 @@ func (l4c *L4Controller) processServiceDeletion(key string, svc *v1.Service) err if err := l4c.updateServiceStatus(svc, &v1.LoadBalancerStatus{}); utils.IgnoreHTTPNotFound(err) != nil { l4c.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeWarning, "DeleteLoadBalancer", "Error reseting load balancer status to empty: %v", err) - return err + return fmt.Errorf("failed to reset ILB status, err: %v", err) } l4c.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeNormal, "DeletedLoadBalancer", "Deleted load balancer") return nil @@ -237,7 +253,16 @@ func (l4c *L4Controller) sync(key string) error { klog.V(2).Infof("Deleting ILB resources for service %s managed by L4 controller", key) return l4c.processServiceDeletion(key, svc) } - return l4c.processServiceCreateOrUpdate(key, svc) + // Check again here, to avoid time-of check, time-of-use race. A service deletion can get queued multiple times + // as annotations change and a service to be deleted can incorrectly get requeued here. This can happen if svc had + // finalizer when enqueuing, but when listing it here, the finalizer already got removed. It will skip needsDeletion + // and queue-up here. + if wantsILB, _ := annotations.WantsL4ILB(svc); wantsILB { + klog.V(2).Infof("Ensuring ILB resources for service %s managed by L4 controller", key) + return l4c.processServiceCreateOrUpdate(key, svc) + } + klog.V(3).Infof("Ignoring sync of service %s, neither delete nor ensure needed.", key) + return nil } func (l4c *L4Controller) updateServiceStatus(svc *v1.Service, newStatus *v1.LoadBalancerStatus) error { @@ -246,12 +271,35 @@ func (l4c *L4Controller) updateServiceStatus(svc *v1.Service, newStatus *v1.Load } updated := svc.DeepCopy() updated.Status.LoadBalancer = *newStatus - if _, err := utils.PatchService(l4c.ctx.KubeClient.CoreV1(), svc, updated); err != nil { + if _, err := helpers.PatchService(l4c.ctx.KubeClient.CoreV1(), svc, updated); err != nil { return err } return nil } +func (l4c *L4Controller) updateServiceMetadata(svc *v1.Service, newObjectMetadata metav1.ObjectMeta) error { + updated := svc.DeepCopy() + updated.ObjectMeta = newObjectMetadata + if _, err := helpers.PatchService(l4c.ctx.KubeClient.CoreV1(), svc, updated); err != nil { + return err + } + return nil +} + +func (l4c *L4Controller) updateAnnotations(name, namespace string, annotations map[string]string) error { + svcClient := l4c.ctx.KubeClient.CoreV1().Services(namespace) + currSvc, err := svcClient.Get(context2.TODO(), name, metav1.GetOptions{}) + if err != nil { + return err + } + if !reflect.DeepEqual(currSvc.Annotations, annotations) { + klog.V(3).Infof("Updating annotations of service %v/%v", namespace, name) + updatedObjectMeta := currSvc.ObjectMeta.DeepCopy() + updatedObjectMeta.Annotations = annotations + return l4c.updateServiceMetadata(currSvc, *updatedObjectMeta) + } + return nil +} func needsDeletion(svc *v1.Service) bool { if !common.HasGivenFinalizer(svc.ObjectMeta, common.ILBFinalizerV2) { return false @@ -265,14 +313,19 @@ func needsDeletion(svc *v1.Service) bool { // needsUpdate checks if load balancer needs to be updated due to change in attributes. func (l4c *L4Controller) needsUpdate(oldService *v1.Service, newService *v1.Service) bool { - oldSvcWantsILB, _ := annotations.WantsL4ILB(oldService) - newSvcWantsILB, _ := annotations.WantsL4ILB(newService) + oldSvcWantsILB, oldType := annotations.WantsL4ILB(oldService) + newSvcWantsILB, newType := annotations.WantsL4ILB(newService) recorder := l4c.ctx.Recorder(oldService.Namespace) if oldSvcWantsILB != newSvcWantsILB { - recorder.Eventf(newService, v1.EventTypeNormal, "Type", "%v -> %v", oldService.Spec.Type, newService.Spec.Type) + recorder.Eventf(newService, v1.EventTypeNormal, "Type", "%v -> %v", oldType, newType) return true } + if !newSvcWantsILB && !oldSvcWantsILB { + // Ignore any other changes if both the previous and new service do not need ILB. + return false + } + if !reflect.DeepEqual(oldService.Spec.LoadBalancerSourceRanges, newService.Spec.LoadBalancerSourceRanges) { recorder.Eventf(newService, v1.EventTypeNormal, "LoadBalancerSourceRanges", "%v -> %v", oldService.Spec.LoadBalancerSourceRanges, newService.Spec.LoadBalancerSourceRanges) @@ -280,10 +333,14 @@ func (l4c *L4Controller) needsUpdate(oldService *v1.Service, newService *v1.Serv } if !portsEqualForLBService(oldService, newService) || oldService.Spec.SessionAffinity != newService.Spec.SessionAffinity { + recorder.Eventf(newService, v1.EventTypeNormal, "Ports/SessionAffinity", "Ports %v, SessionAffinity %v -> Ports %v, SessionAffinity %v", + oldService.Spec.Ports, oldService.Spec.SessionAffinity, newService.Spec.Ports, newService.Spec.SessionAffinity) return true } if !reflect.DeepEqual(oldService.Spec.SessionAffinityConfig, newService.Spec.SessionAffinityConfig) { + recorder.Eventf(newService, v1.EventTypeNormal, "SessionAffinityConfig", "%v -> %v", + oldService.Spec.SessionAffinityConfig, newService.Spec.SessionAffinityConfig) return true } if oldService.Spec.LoadBalancerIP != newService.Spec.LoadBalancerIP { @@ -304,8 +361,10 @@ func (l4c *L4Controller) needsUpdate(oldService *v1.Service, newService *v1.Serv } } if !reflect.DeepEqual(oldService.Annotations, newService.Annotations) { - // Ignore update if only neg annotation changed, this is added by the neg controller. - if !annotations.OnlyNEGStatusChanged(oldService, newService) { + // Ignore update if only neg or ilb resources annotations changed, these are added by the neg/l4 controller. + if !annotations.OnlyStatusAnnotationsChanged(oldService, newService) { + recorder.Eventf(newService, v1.EventTypeNormal, "Annotations", "%v -> %v", + oldService.Annotations, newService.Annotations) return true } } diff --git a/pkg/l4/l4controller_test.go b/pkg/l4/l4controller_test.go index 7daa824743..bbd5726ae0 100644 --- a/pkg/l4/l4controller_test.go +++ b/pkg/l4/l4controller_test.go @@ -101,6 +101,27 @@ func validateSvcStatus(svc *api_v1.Service, expectStatus bool, t *testing.T) { if len(svc.Status.LoadBalancer.Ingress) > 0 && !expectStatus { t.Fatalf("Expected LoadBalancer status to be empty, Got %v", svc.Status.LoadBalancer) } + + expectedAnnotationKeys := []string{annotations.FirewallRuleKey, annotations.BackendServiceKey, annotations.HealthcheckKey, + annotations.TCPForwardingRuleKey} + + missingKeys := []string{} + for _, key := range expectedAnnotationKeys { + if _, ok := svc.Annotations[key]; !ok { + missingKeys = append(missingKeys, key) + } + } + if expectStatus { + // All annotations are expected to be present in this case + if len(missingKeys) > 0 { + t.Fatalf("Cannot find annotations %v in ILB service, Got %v", missingKeys, svc.Annotations) + } + } else { + //None of the ILB keys should be present since the ILB has been deleted. + if len(missingKeys) != len(expectedAnnotationKeys) { + t.Fatalf("Unexpected ILB annotations still present, Got %v", svc.Annotations) + } + } } // TestProcessCreateOrUpdate verifies the processing loop in L4Controller. @@ -144,7 +165,7 @@ func TestProcessCreateOrUpdate(t *testing.T) { t.Errorf("Failed to sync updated service %s, err %v", newSvc.Name, err) } - // List the service and ensure that it contains the finalizer as well as Status field. + // List the service and ensure that it doesn't contain the finalizer as well as Status field. newSvc, err = l4c.client.CoreV1().Services(newSvc.Namespace).Get(context2.TODO(), newSvc.Name, v1.GetOptions{}) if err != nil { t.Errorf("Failed to lookup service %s, err: %v", newSvc.Name, err) @@ -161,7 +182,7 @@ func TestProcessDeletion(t *testing.T) { if err != nil { t.Errorf("Failed to sync newly added service %s, err %v", newSvc.Name, err) } - // List the service and ensure that it does not contain the finalizer or the status field + // List the service and ensure that it contains the finalizer and the status field newSvc, err = l4c.client.CoreV1().Services(newSvc.Namespace).Get(context2.TODO(), newSvc.Name, v1.GetOptions{}) if err != nil { t.Errorf("Failed to lookup service %s, err: %v", newSvc.Name, err) diff --git a/pkg/loadbalancers/l4.go b/pkg/loadbalancers/l4.go index 8f5c13c2fc..8eca244a33 100644 --- a/pkg/loadbalancers/l4.go +++ b/pkg/loadbalancers/l4.go @@ -27,6 +27,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" "k8s.io/cloud-provider/service/helpers" + "k8s.io/ingress-gce/pkg/annotations" "k8s.io/ingress-gce/pkg/backends" "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/firewalls" @@ -53,6 +54,13 @@ type L4 struct { sharedResourcesLock *sync.Mutex } +var ILBResourceAnnotationKeys = []string{ + annotations.BackendServiceKey, + annotations.TCPForwardingRuleKey, + annotations.UDPForwardingRuleKey, + annotations.HealthcheckKey, + annotations.FirewallRuleKey} + // NewL4Handler creates a new L4Handler for the given L4 service. func NewL4Handler(service *corev1.Service, cloud *gce.Cloud, scope meta.KeyType, namer *namer.Namer, recorder record.EventRecorder, lock *sync.Mutex) *L4 { l := &L4{cloud: cloud, scope: scope, namer: namer, recorder: recorder, Service: service, sharedResourcesLock: lock} @@ -162,8 +170,9 @@ func (l *L4) getFRNameWithProtocol(protocol string) string { // EnsureInternalLoadBalancer ensures that all GCE resources for the given loadbalancer service have // been created. It returns a LoadBalancerStatus with the updated ForwardingRule IP address. -func (l *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service, metricsState *metrics.L4ILBServiceState) (*corev1.LoadBalancerStatus, error) { +func (l *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service, metricsState *metrics.L4ILBServiceState) (*corev1.LoadBalancerStatus, map[string]string, error) { // Use the same resource name for NEG, BackendService as well as FR, FWRule. + annotationsMap := make(map[string]string) l.Service = svc name := l.namer.VMIPNEG(l.Service.Namespace, l.Service.Name) options := getILBOptions(l.Service) @@ -184,15 +193,16 @@ func (l *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service, l.sharedResourcesLock.Unlock() } if err != nil { - return nil, err + return nil, nil, err } + annotationsMap[annotations.HealthcheckKey] = hcName _, portRanges, protocol := utils.GetPortsAndProtocol(l.Service.Spec.Ports) // ensure firewalls sourceRanges, err := helpers.GetLoadBalancerSourceRanges(l.Service) if err != nil { - return nil, err + return nil, nil, err } hcSourceRanges := gce.L4LoadBalancerSrcRanges() ensureFunc := func(name, IP string, sourceRanges, portRanges []string, proto string) error { @@ -210,13 +220,14 @@ func (l *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service, // Add firewall rule for ILB traffic to nodes err = ensureFunc(name, "", sourceRanges.StringSlice(), portRanges, string(protocol)) if err != nil { - return nil, err + return nil, nil, err } + annotationsMap[annotations.FirewallRuleKey] = name // Add firewall rule for healthchecks to nodes err = ensureFunc(hcFwName, "", hcSourceRanges, []string{strconv.Itoa(int(hcPort))}, string(corev1.ProtocolTCP)) if err != nil { - return nil, err + return nil, nil, err } // Check if protocol has changed for this service. In this case, forwarding rule should be deleted before @@ -238,14 +249,20 @@ func (l *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service, bs, err := l.backendPool.EnsureL4BackendService(name, hcLink, string(protocol), string(l.Service.Spec.SessionAffinity), string(cloud.SchemeInternal), l.NamespacedName, meta.VersionGA) if err != nil { - return nil, err + return nil, nil, err } - + annotationsMap[annotations.BackendServiceKey] = name // create fr rule - fr, err := l.ensureForwardingRule(l.GetFRName(), bs.SelfLink, options, existingFR) + frName := l.GetFRName() + fr, err := l.ensureForwardingRule(frName, bs.SelfLink, options, existingFR) if err != nil { klog.Errorf("EnsureInternalLoadBalancer: Failed to create forwarding rule - %v", err) - return nil, err + return nil, nil, err + } + if fr.IPProtocol == string(corev1.ProtocolTCP) { + annotationsMap[annotations.TCPForwardingRuleKey] = frName + } else { + annotationsMap[annotations.UDPForwardingRuleKey] = frName } metricsState.InSuccess = true @@ -260,5 +277,19 @@ func (l *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service, } klog.V(6).Infof("Internal L4 Loadbalancer for Service %s ensured, updating its state %v in metrics cache", l.NamespacedName, metricsState) - return &corev1.LoadBalancerStatus{Ingress: []corev1.LoadBalancerIngress{{IP: fr.IPAddress}}}, nil + return &corev1.LoadBalancerStatus{Ingress: []corev1.LoadBalancerIngress{{IP: fr.IPAddress}}}, annotationsMap, nil +} + +// MergeAnnotations merges the new set of ilb resource annotations with the pre-existing service annotations. +// Existing ILB resource annotation values will be replaced with the values in the new map. +func (l *L4) MergeAnnotations(svc *corev1.Service, ilbAnnotations map[string]string) map[string]string { + // Delete existing ILB annotations. + for _, key := range ILBResourceAnnotationKeys { + delete(svc.Annotations, key) + } + // merge existing annotations with the newly added annotations + for key, val := range ilbAnnotations { + svc.Annotations[key] = val + } + return svc.Annotations } diff --git a/pkg/loadbalancers/l4_test.go b/pkg/loadbalancers/l4_test.go index 399341a2d3..e5f736d103 100644 --- a/pkg/loadbalancers/l4_test.go +++ b/pkg/loadbalancers/l4_test.go @@ -36,6 +36,7 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/client-go/tools/record" servicehelper "k8s.io/cloud-provider/service/helpers" + "k8s.io/ingress-gce/pkg/annotations" "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/healthchecks" "k8s.io/ingress-gce/pkg/test" @@ -123,23 +124,23 @@ func TestEnsureInternalLoadBalancer(t *testing.T) { t.Errorf("Unexpected error when adding nodes %v", err) } - status, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } - assertInternalLbResources(t, svc, l, nodeNames) + assertInternalLbResources(t, svc, l, nodeNames, annotations) // Simulate a periodic sync - status, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } - assertInternalLbResources(t, svc, l, nodeNames) + assertInternalLbResources(t, svc, l, nodeNames, annotations) } func TestEnsureInternalLoadBalancerTypeChange(t *testing.T) { @@ -155,14 +156,14 @@ func TestEnsureInternalLoadBalancerTypeChange(t *testing.T) { if err != nil { t.Errorf("Unexpected error when adding nodes %v", err) } - status, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Unexpected error %v", err) } if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } - assertInternalLbResources(t, svc, l, nodeNames) + assertInternalLbResources(t, svc, l, nodeNames, annotations) // Now add the latest annotation and change scheme to external svc.Annotations[gce.ServiceAnnotationLoadBalancerType] = "" @@ -204,14 +205,14 @@ func TestEnsureInternalLoadBalancerWithExistingResources(t *testing.T) { if err != nil { t.Errorf("Failed to create backendservice, err %v", err) } - status, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } - assertInternalLbResources(t, svc, l, nodeNames) + assertInternalLbResources(t, svc, l, nodeNames, annotations) } // TestEnsureInternalLoadBalancerClearPreviousResources creates ILB resources with incomplete configuration and verifies @@ -285,7 +286,7 @@ func TestEnsureInternalLoadBalancerClearPreviousResources(t *testing.T) { } existingFwdRule.BackendService = existingBS.Name - if _, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}); err != nil { + if _, _, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}); err != nil { t.Errorf("Failed to ensure loadBalancer %s, err %v", lbName, err) } key.Name = frName @@ -377,14 +378,15 @@ func TestUpdateResourceLinks(t *testing.T) { if !reflect.DeepEqual(bs.HealthChecks, []string{"hc1", "hc2"}) { t.Errorf("Unexpected healthchecks in backend service - %v", bs.HealthChecks) } - if _, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}); err != nil { + _, annotations, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + if err != nil { t.Errorf("Failed to ensure loadBalancer %s, err %v", lbName, err) } if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } // verifies that the right healthcheck is present - assertInternalLbResources(t, svc, l, nodeNames) + assertInternalLbResources(t, svc, l, nodeNames, annotations) // ensure that the other healthchecks still exist. key.Name = "hc1" @@ -431,7 +433,7 @@ func TestEnsureInternalLoadBalancerHealthCheckConfigurable(t *testing.T) { t.Errorf("Failed to create fake healthcheck %s, err %v", hcName, err) } - if _, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}); err != nil { + if _, _, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}); err != nil { t.Errorf("Failed to ensure loadBalancer %s, err %v", lbName, err) } @@ -457,14 +459,14 @@ func TestEnsureInternalLoadBalancerDeleted(t *testing.T) { if err != nil { t.Errorf("Unexpected error when adding nodes %v", err) } - status, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } - assertInternalLbResources(t, svc, l, nodeNames) + assertInternalLbResources(t, svc, l, nodeNames, annotations) // Delete the loadbalancer err = l.EnsureInternalLoadBalancerDeleted(svc) @@ -487,14 +489,14 @@ func TestEnsureInternalLoadBalancerDeletedTwiceDoesNotError(t *testing.T) { if err != nil { t.Errorf("Unexpected error when adding nodes %v", err) } - status, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } - assertInternalLbResources(t, svc, l, nodeNames) + assertInternalLbResources(t, svc, l, nodeNames, annotations) // Delete the loadbalancer err = l.EnsureInternalLoadBalancerDeleted(svc) @@ -528,14 +530,14 @@ func TestEnsureInternalLoadBalancerWithSpecialHealthCheck(t *testing.T) { svc.Spec.Type = v1.ServiceTypeLoadBalancer svc.Spec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyTypeLocal - status, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } - assertInternalLbResources(t, svc, l, nodeNames) + assertInternalLbResources(t, svc, l, nodeNames, annotations) lbName := l.namer.VMIPNEG(svc.Namespace, svc.Name) key, err := composite.CreateKey(l.cloud, lbName, meta.Global) @@ -641,7 +643,7 @@ func TestEnsureInternalLoadBalancerErrors(t *testing.T) { if tc.injectMock != nil { tc.injectMock(fakeGCE.Compute().(*cloud.MockGCE)) } - status, err := l.EnsureInternalLoadBalancer(nodeNames, params.service, &metrics.L4ILBServiceState{}) + status, _, err := l.EnsureInternalLoadBalancer(nodeNames, params.service, &metrics.L4ILBServiceState{}) if err == nil { t.Errorf("Expected error when %s", desc) } @@ -709,25 +711,25 @@ func TestEnsureInternalLoadBalancerEnableGlobalAccess(t *testing.T) { t.Errorf("Unexpected error when adding nodes %v", err) } frName := l.GetFRName() - status, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } - assertInternalLbResources(t, svc, l, nodeNames) + assertInternalLbResources(t, svc, l, nodeNames, annotations) // Change service to include the global access annotation svc.Annotations[gce.ServiceAnnotationILBAllowGlobalAccess] = "true" - status, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } - assertInternalLbResources(t, svc, l, nodeNames) + assertInternalLbResources(t, svc, l, nodeNames, annotations) descString, err := utils.MakeL4ILBServiceDescription(utils.ServiceKeyFunc(svc.Namespace, svc.Name), "1.2.3.0", meta.VersionGA) if err != nil { t.Errorf("Unexpected error when creating description - %v", err) @@ -748,7 +750,7 @@ func TestEnsureInternalLoadBalancerEnableGlobalAccess(t *testing.T) { } // remove the annotation and disable global access. delete(svc.Annotations, gce.ServiceAnnotationILBAllowGlobalAccess) - status, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } @@ -767,6 +769,7 @@ func TestEnsureInternalLoadBalancerEnableGlobalAccess(t *testing.T) { if fwdRule.Description != descString { t.Errorf("Expected description %s, Got %s", descString, fwdRule.Description) } + assertInternalLbResources(t, svc, l, nodeNames, annotations) // Delete the service err = l.EnsureInternalLoadBalancerDeleted(svc) if err != nil { @@ -788,13 +791,14 @@ func TestEnsureInternalLoadBalancerCustomSubnet(t *testing.T) { if err != nil { t.Errorf("Unexpected error when adding nodes %v", err) } - status, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } + assertInternalLbResources(t, svc, l, nodeNames, annotations) frName := l.GetFRName() fwdRule, err := composite.GetForwardingRule(l.cloud, meta.RegionalKey(frName, l.cloud.Region()), meta.VersionGA) @@ -809,13 +813,14 @@ func TestEnsureInternalLoadBalancerCustomSubnet(t *testing.T) { requestedIP := "4.5.6.7" svc.Annotations[gce.ServiceAnnotationILBSubnet] = "test-subnet" svc.Spec.LoadBalancerIP = requestedIP - status, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } + assertInternalLbResources(t, svc, l, nodeNames, annotations) if status.Ingress[0].IP != requestedIP { t.Fatalf("Reserved IP %s not propagated, Got '%s'", requestedIP, status.Ingress[0].IP) } @@ -829,13 +834,14 @@ func TestEnsureInternalLoadBalancerCustomSubnet(t *testing.T) { // Change to a different subnet svc.Annotations[gce.ServiceAnnotationILBSubnet] = "another-subnet" - status, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } + assertInternalLbResources(t, svc, l, nodeNames, annotations) if status.Ingress[0].IP != requestedIP { t.Errorf("Reserved IP %s not propagated, Got %s", requestedIP, status.Ingress[0].IP) } @@ -848,10 +854,11 @@ func TestEnsureInternalLoadBalancerCustomSubnet(t *testing.T) { } // remove the annotation - ILB should revert to default subnet. delete(svc.Annotations, gce.ServiceAnnotationILBSubnet) - status, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } + assertInternalLbResources(t, svc, l, nodeNames, annotations) if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } @@ -955,13 +962,14 @@ func TestEnsureInternalLoadBalancerModifyProtocol(t *testing.T) { } frName := l.getFRNameWithProtocol("TCP") - status, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err := l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } + assertInternalLbResources(t, svc, l, nodeNames, annotations) key, err := composite.CreateKey(l.cloud, frName, meta.Regional) if err != nil { t.Errorf("Unexpected error when creating key - %v", err) @@ -975,13 +983,14 @@ func TestEnsureInternalLoadBalancerModifyProtocol(t *testing.T) { } // change the protocol to UDP svc.Spec.Ports[0].Protocol = v1.ProtocolUDP - status, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) + status, annotations, err = l.EnsureInternalLoadBalancer(nodeNames, svc, &metrics.L4ILBServiceState{}) if err != nil { t.Errorf("Failed to ensure loadBalancer, err %v", err) } if len(status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l) } + assertInternalLbResources(t, svc, l, nodeNames, annotations) // Make sure the old forwarding rule is deleted fwdRule, err = composite.GetForwardingRule(l.cloud, key, meta.VersionGA) if !utils.IsNotFoundError(err) { @@ -1006,7 +1015,7 @@ func TestEnsureInternalLoadBalancerModifyProtocol(t *testing.T) { assertInternalLbResourcesDeleted(t, svc, true, l) } -func assertInternalLbResources(t *testing.T, apiService *v1.Service, l *L4, nodeNames []string) { +func assertInternalLbResources(t *testing.T, apiService *v1.Service, l *L4, nodeNames []string, resourceAnnotations map[string]string) { // Check that Firewalls are created for the LoadBalancer and the HealthCheck sharedHC := !servicehelper.RequestsOnlyLocalTraffic(apiService) resourceName := l.namer.VMIPNEG(l.Service.Namespace, l.Service.Name) @@ -1014,11 +1023,14 @@ func assertInternalLbResources(t *testing.T, apiService *v1.Service, l *L4, node if err != nil { t.Errorf("Failed to create description for resources, err %v", err) } + _, _, proto := utils.GetPortsAndProtocol(apiService.Spec.Ports) + expectedAnnotations := make(map[string]string) hcName, hcFwName := healthchecks.HealthCheckName(sharedHC, l.namer.UID(), resourceName) fwNames := []string{ resourceName, hcFwName, } + expectedAnnotations[annotations.FirewallRuleKey] = resourceName for _, fwName := range fwNames { firewall, err := l.cloud.GetFirewall(fwName) @@ -1044,6 +1056,7 @@ func assertInternalLbResources(t *testing.T, apiService *v1.Service, l *L4, node if healthcheck.Name != hcName { t.Errorf("Unexpected name for healthcheck '%s' - expected '%s'", healthcheck.Name, hcName) } + expectedAnnotations[annotations.HealthcheckKey] = hcName // Only non-shared Healthchecks get a description. if !sharedHC && healthcheck.Description != resourceDesc { t.Errorf("Unexpected description in healthcheck - Expected %s, Got %s", healthcheck.Description, resourceDesc) @@ -1057,7 +1070,7 @@ func assertInternalLbResources(t *testing.T, apiService *v1.Service, l *L4, node if err != nil { t.Errorf("Failed to fetch backend service %s - err %v", backendServiceName, err) } - if bs.Protocol != "TCP" { + if bs.Protocol != string(proto) { t.Errorf("Unexpected protocol '%s' for backend service %v", bs.Protocol, bs) } if bs.SelfLink != backendServiceLink { @@ -1070,6 +1083,7 @@ func assertInternalLbResources(t *testing.T, apiService *v1.Service, l *L4, node t.Errorf("Unexpected healthcheck reference '%v' in backend service, expected '%s'", bs.HealthChecks, healthcheck.SelfLink) } + expectedAnnotations[annotations.BackendServiceKey] = backendServiceName // Check that ForwardingRule is created frName := l.GetFRName() fwdRule, err := composite.GetForwardingRule(l.cloud, meta.RegionalKey(frName, l.cloud.Region()), meta.VersionGA) @@ -1080,20 +1094,35 @@ func assertInternalLbResources(t *testing.T, apiService *v1.Service, l *L4, node if fwdRule.Name != frName { t.Errorf("Unexpected name for forwarding rule '%s' - expected '%s'", fwdRule.Name, frName) } - if fwdRule.IPProtocol != "TCP" { + if fwdRule.IPProtocol != string(proto) { t.Errorf("Unexpected protocol '%s' for forwarding rule %v", fwdRule.IPProtocol, fwdRule) } if fwdRule.BackendService != backendServiceLink { t.Errorf("Unexpected backend service link '%s' for forwarding rule, expected '%s'", fwdRule.BackendService, backendServiceLink) } - if fwdRule.Subnetwork != l.cloud.NetworkURL() { - t.Errorf("Unexpected subnetwork '%s' in forwarding rule, expected '%s'", - fwdRule.Subnetwork, l.cloud.NetworkURL()) + subnet := apiService.Annotations[gce.ServiceAnnotationILBSubnet] + if subnet == "" { + subnet = l.cloud.SubnetworkURL() + } else { + key.Name = subnet + subnet = cloud.SelfLink(meta.VersionGA, l.cloud.ProjectID(), "subnetworks", key) + } + if fwdRule.Subnetwork != subnet { + t.Errorf("Unexpected subnetwork %q in forwarding rule, expected %q", + fwdRule.Subnetwork, subnet) + } + if proto == v1.ProtocolTCP { + expectedAnnotations[annotations.TCPForwardingRuleKey] = frName + } else { + expectedAnnotations[annotations.UDPForwardingRuleKey] = frName } addr, err := l.cloud.GetRegionAddress(frName, l.cloud.Region()) if err == nil || addr != nil { t.Errorf("Expected error when looking up ephemeral address, got %v", addr) } + if !reflect.DeepEqual(expectedAnnotations, resourceAnnotations) { + t.Fatalf("Expected annotations %v, got %v", expectedAnnotations, resourceAnnotations) + } } func assertInternalLbResourcesDeleted(t *testing.T, apiService *v1.Service, firewallsDeleted bool, l *L4) { diff --git a/pkg/utils/patch.go b/pkg/utils/patch.go index 505e642c00..4884295405 100644 --- a/pkg/utils/patch.go +++ b/pkg/utils/patch.go @@ -14,16 +14,11 @@ limitations under the License. package utils import ( - "context" "encoding/json" "fmt" - "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/jsonmergepatch" "k8s.io/apimachinery/pkg/util/strategicpatch" - corev1 "k8s.io/client-go/kubernetes/typed/core/v1" ) // StrategicMergePatchBytes returns a patch between the old and new object using a strategic merge patch. @@ -67,38 +62,3 @@ func MergePatchBytes(old, cur interface{}) ([]byte, error) { return patchBytes, nil } - -// TODO remove these after picking up https://github.com/kubernetes/kubernetes/pull/87217 -// PatchService patches the given service's Status or ObjectMeta based on the original and -// updated ones. Change to spec will be ignored. -func PatchService(c corev1.CoreV1Interface, oldSvc, newSvc *v1.Service) (*v1.Service, error) { - // Reset spec to make sure only patch for Status or ObjectMeta. - newSvc.Spec = oldSvc.Spec - - patchBytes, err := getPatchBytes(oldSvc, newSvc) - if err != nil { - return nil, err - } - - return c.Services(oldSvc.Namespace).Patch(context.TODO(), oldSvc.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}, "status") - -} - -func getPatchBytes(oldSvc, newSvc *v1.Service) ([]byte, error) { - oldData, err := json.Marshal(oldSvc) - if err != nil { - return nil, fmt.Errorf("failed to Marshal oldData for svc %s/%s: %v", oldSvc.Namespace, oldSvc.Name, err) - } - - newData, err := json.Marshal(newSvc) - if err != nil { - return nil, fmt.Errorf("failed to Marshal newData for svc %s/%s: %v", newSvc.Namespace, newSvc.Name, err) - } - - patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, v1.Service{}) - if err != nil { - return nil, fmt.Errorf("failed to CreateTwoWayMergePatch for svc %s/%s: %v", oldSvc.Namespace, oldSvc.Name, err) - } - return patchBytes, nil - -} diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 9cf3d81f74..2ec05ee4eb 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -540,7 +540,7 @@ func GetPortRanges(ports []int) (ranges []string) { // GetPortsAndProtocol returns the list of ports, list of port ranges and the protocol given the list of k8s port info. func GetPortsAndProtocol(svcPorts []api_v1.ServicePort) (ports []string, portRanges []string, protocol api_v1.Protocol) { if len(svcPorts) == 0 { - return []string{}, []string{}, api_v1.ProtocolUDP + return []string{}, []string{}, api_v1.ProtocolTCP } // GCP doesn't support multiple protocols for a single load balancer