diff --git a/pkg/annotations/service.go b/pkg/annotations/service.go index 5a20f22fb7..23d41839bc 100644 --- a/pkg/annotations/service.go +++ b/pkg/annotations/service.go @@ -92,6 +92,22 @@ type NegAttributes struct { Name string `json:"name,omitempty"` } +// NEGEnabledForIngress returns true if the annotation is to be applied on +// Ingress-referenced ports +func (n *NegAnnotation) NEGEnabledForIngress() bool { + return n.Ingress +} + +// NEGExposed is true if the service exposes NEGs +func (n *NegAnnotation) NEGExposed() bool { + return len(n.ExposedPorts) > 0 +} + +// NEGExposed is true if the service uses NEG +func (n *NegAnnotation) NEGEnabled() bool { + return n.NEGEnabledForIngress() || n.NEGExposed() +} + // AppProtocol describes the service protocol. type AppProtocol string @@ -138,59 +154,28 @@ func (svc *Service) ApplicationProtocols() (map[string]AppProtocol, error) { return portToProtos, err } -// NEGEnabledForIngress returns true if the annotation is to be applied on -// Ingress-referenced ports -func (svc *Service) NEGEnabledForIngress() bool { - annotation, err := svc.NegAnnotation() - if err != nil { - return false - } - return annotation.Ingress -} - var ( ErrBackendConfigNoneFound = errors.New("no BackendConfig's found in annotation") ErrBackendConfigInvalidJSON = errors.New("BackendConfig annotation is invalid json") ErrBackendConfigAnnotationMissing = errors.New("BackendConfig annotation is missing") + ErrNEGAnnotationInvalid = errors.New("NEG annotation is invalid.") ) -// NEGExposed is true if the service exposes NEGs -func (svc *Service) NEGExposed() bool { - if !flags.F.Features.NEGExposed { - return false - } - - annotation, err := svc.NegAnnotation() - if err != nil { - return false - } - return len(annotation.ExposedPorts) > 0 -} - -var ( - ErrExposeNegAnnotationMissing = errors.New("No NEG ServicePorts specified") - ErrExposeNegAnnotationInvalid = errors.New("Expose NEG annotation is invalid") -) - -// NegAnnotation returns the value of the NEG annotation key -func (svc *Service) NegAnnotation() (NegAnnotation, error) { +// NEGAnnotation returns true if NEG annotation is found. +// If found, it also returns NEG annotation struct. +func (svc *Service) NEGAnnotation() (bool, *NegAnnotation, error) { var res NegAnnotation annotation, ok := svc.v[NEGAnnotationKey] if !ok { - return res, ErrExposeNegAnnotationMissing + return false, nil, nil } // TODO: add link to Expose NEG documentation when complete if err := json.Unmarshal([]byte(annotation), &res); err != nil { - return res, ErrExposeNegAnnotationInvalid + return true, nil, ErrNEGAnnotationInvalid } - return res, nil -} - -// NEGEnabled is true if the service uses NEGs. -func (svc *Service) NEGEnabled() bool { - return svc.NEGEnabledForIngress() || svc.NEGExposed() + return true, &res, nil } type BackendConfigs struct { diff --git a/pkg/annotations/service_test.go b/pkg/annotations/service_test.go index 14bc0b9b56..a655ad3656 100644 --- a/pkg/annotations/service_test.go +++ b/pkg/annotations/service_test.go @@ -17,6 +17,7 @@ limitations under the License. package annotations import ( + "fmt" "reflect" "testing" @@ -25,14 +26,49 @@ import ( "k8s.io/ingress-gce/pkg/flags" ) -func TestNEGService(t *testing.T) { +func TestNEGAnnotation(t *testing.T) { for _, tc := range []struct { - svc *v1.Service - neg bool - ingress bool - exposed bool + desc string + svc *v1.Service + expectNegAnnotation *NegAnnotation + expectError error + expectFound bool + negEnabled bool + ingress bool + exposed bool }{ { + desc: "NEG annotation not specified", + svc: &v1.Service{}, + expectFound: false, + expectError: nil, + }, + { + desc: "NEG annotation is malformed", + svc: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + NEGAnnotationKey: `foo`, + }, + }, + }, + expectFound: true, + expectError: ErrNEGAnnotationInvalid, + }, + { + desc: "NEG annotation is malformed 2", + svc: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + NEGAnnotationKey: `{"exposed_ports":{80:{}}}`, + }, + }, + }, + expectFound: true, + expectError: ErrNEGAnnotationInvalid, + }, + { + desc: "NEG enabled for ingress", svc: &v1.Service{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -40,11 +76,16 @@ func TestNEGService(t *testing.T) { }, }, }, - neg: true, - ingress: true, - exposed: false, + expectFound: true, + expectNegAnnotation: &NegAnnotation{ + Ingress: true, + }, + negEnabled: true, + ingress: true, + exposed: false, }, { + desc: "NEG exposed", svc: &v1.Service{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -52,40 +93,77 @@ func TestNEGService(t *testing.T) { }, }, }, - neg: true, - ingress: false, - exposed: true, + expectFound: true, + expectNegAnnotation: &NegAnnotation{ + ExposedPorts: map[int32]NegAttributes{int32(80): {}}, + }, + negEnabled: true, + ingress: false, + exposed: true, }, { + desc: "Multiple ports exposed", svc: &v1.Service{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - NEGAnnotationKey: `{"ingress":true,"exposed_ports":{"80":{}}}`, + NEGAnnotationKey: `{"exposed_ports":{"80":{}, "443":{}}}`, }, }, }, - neg: true, - ingress: true, - exposed: true, + expectFound: true, + expectNegAnnotation: &NegAnnotation{ + ExposedPorts: map[int32]NegAttributes{int32(80): {}, int32(443): {}}, + }, + negEnabled: true, + ingress: false, + exposed: true, }, { - svc: &v1.Service{}, - neg: false, - ingress: false, - exposed: false, + desc: "Service is enabled for both ingress and exposedNEG", + svc: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + NEGAnnotationKey: `{"ingress":true,"exposed_ports":{"80":{}, "443":{}}}`, + }, + }, + }, + expectFound: true, + expectNegAnnotation: &NegAnnotation{ + Ingress: true, + ExposedPorts: map[int32]NegAttributes{int32(80): {}, int32(443): {}}, + }, + negEnabled: true, + ingress: true, + exposed: true, }, } { - svc := FromService(tc.svc) - if neg := svc.NEGEnabled(); neg != tc.neg { - t.Errorf("for service %+v; svc.NEGEnabled() = %v; want %v", tc.svc, neg, tc.neg) + found, negAnnotation, err := FromService(tc.svc).NEGAnnotation() + if fmt.Sprintf("%q", err) != fmt.Sprintf("%q", tc.expectError) { + t.Errorf("Test case %q: Expect error to be %q, but got: %q", tc.desc, tc.expectError, err) + } + + if found != tc.expectFound { + t.Errorf("Test case %q: Expect found to be %v, be got %v", tc.desc, tc.expectFound, found) + } + + if tc.expectError != nil || !tc.expectFound { + continue } - if ing := svc.NEGEnabledForIngress(); ing != tc.ingress { - t.Errorf("for service %+v; svc.NEGEnabledForIngress() = %v; want %v", tc.svc, ing, tc.ingress) + if !reflect.DeepEqual(*tc.expectNegAnnotation, *negAnnotation) { + t.Errorf("Test case %q: Expect NegAnnotation to be %v, be got %v", tc.desc, *tc.expectNegAnnotation, *negAnnotation) } - if exposed := svc.NEGExposed(); exposed != tc.exposed { - t.Errorf("for service %+v; svc.NEGExposed() = %v; want %v", tc.svc, exposed, tc.exposed) + if neg := negAnnotation.NEGEnabled(); neg != tc.negEnabled { + t.Errorf("Test case %q: Expect EnabledNEG() to be %v, be got %v", tc.desc, tc.negEnabled, neg) + } + + if ing := negAnnotation.NEGEnabledForIngress(); ing != tc.ingress { + t.Errorf("Test case %q: Expect NEGEnabledForIngress() = %v; want %v", tc.desc, tc.ingress, ing) + } + + if exposed := negAnnotation.NEGExposed(); exposed != tc.exposed { + t.Errorf("Test case %q: Expect NEGExposed() = %v; want %v", tc.desc, tc.exposed, exposed) } } } @@ -281,67 +359,3 @@ func TestBackendConfigs(t *testing.T) { } } } - -func TestNegAnnotation(t *testing.T) { - testcases := []struct { - desc string - annotation string - expected NegAnnotation - expectedErr error - }{ - { - desc: "no expose NEG annotation", - annotation: "", - expectedErr: ErrExposeNegAnnotationMissing, - }, - { - desc: "invalid expose NEG annotation", - annotation: "invalid", - expectedErr: ErrExposeNegAnnotationInvalid, - }, - { - desc: "NEG annotation references existing service ports", - expected: NegAnnotation{ - ExposedPorts: map[int32]NegAttributes{80: NegAttributes{}, 443: NegAttributes{}}, - }, - annotation: `{"exposed_ports":{"80":{},"443":{}}}`, - }, - - { - desc: "NEGServicePort takes the union of known ports and ports referenced in the annotation", - annotation: `{"exposed_ports":{"80":{}}}`, - expected: NegAnnotation{ - ExposedPorts: map[int32]NegAttributes{80: NegAttributes{}}, - }, - }, - } - - for _, tc := range testcases { - service := &v1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{}, - }, - } - - t.Run(tc.desc, func(t *testing.T) { - if len(tc.annotation) > 0 { - service.Annotations[NEGAnnotationKey] = tc.annotation - } - - svc := FromService(service) - exposeNegStruct, err := svc.NegAnnotation() - - if tc.expectedErr == nil && err != nil { - t.Errorf("ExpectedNEGServicePorts to not return an error, got: %v", err) - } - - if !reflect.DeepEqual(exposeNegStruct, tc.expected) { - t.Errorf("Expected NEGServicePorts to equal: %v; got: %v", tc.expected, exposeNegStruct.ExposedPorts) - } - - if tc.expectedErr != nil && err != tc.expectedErr { - t.Errorf("Expected NEGServicePorts to return a %v error, got: %v", tc.expectedErr, err) - } - }) - } -} diff --git a/pkg/controller/translator/translator.go b/pkg/controller/translator/translator.go index 8731c5b44b..d50b39870f 100644 --- a/pkg/controller/translator/translator.go +++ b/pkg/controller/translator/translator.go @@ -80,7 +80,11 @@ func (t *Translator) getServicePort(id utils.ServicePortID) (*utils.ServicePort, return nil, errors.ErrSvcPortNotFound{ServicePortID: id} } - negEnabled := annotations.FromService(svc).NEGEnabled() + var negEnabled bool + ok, negAnnotation, err := annotations.FromService(svc).NEGAnnotation() + if ok && err == nil { + negEnabled = negAnnotation.NEGEnabledForIngress() + } if !negEnabled && svc.Spec.Type != api_v1.ServiceTypeNodePort && svc.Spec.Type != api_v1.ServiceTypeLoadBalancer { // This is a fatal error. diff --git a/pkg/neg/controller.go b/pkg/neg/controller.go index a7a645c4eb..7444c61bf0 100644 --- a/pkg/neg/controller.go +++ b/pkg/neg/controller.go @@ -230,13 +230,17 @@ func (c *Controller) processService(key string) error { } var service *apiv1.Service - var enabled bool + var foundNEGAnnotation bool + var negAnnotation *annotations.NegAnnotation if exists { service = svc.(*apiv1.Service) - enabled = annotations.FromService(service).NEGEnabled() + foundNEGAnnotation, negAnnotation, err = annotations.FromService(service).NEGAnnotation() + if err != nil { + return err + } } - if !enabled { + if !foundNEGAnnotation || !negAnnotation.NEGEnabled() { c.manager.StopSyncer(namespace, name) // delete the annotation return c.syncNegStatusAnnotation(namespace, name, make(PortNameMap)) @@ -246,24 +250,19 @@ func (c *Controller) processService(key string) error { // map of ServicePort (int) to TargetPort svcPortMap := make(PortNameMap) - if annotations.FromService(service).NEGEnabledForIngress() { + if negAnnotation.NEGEnabledForIngress() { // Only service ports referenced by ingress are synced for NEG ings := getIngressServicesFromStore(c.ingressLister, service) svcPortMap = gatherPortMappingUsedByIngress(ings, service) } - if annotations.FromService(service).NEGExposed() { + if negAnnotation.NEGExposed() { knownPorts := make(PortNameMap) for _, sp := range service.Spec.Ports { knownPorts[sp.Port] = sp.TargetPort.String() } - annotation, err := annotations.FromService(service).NegAnnotation() - if err != nil { - return err - } - - negSvcPorts, err := NEGServicePorts(annotation, knownPorts) + negSvcPorts, err := NEGServicePorts(negAnnotation, knownPorts) if err != nil { return err } diff --git a/pkg/neg/utils.go b/pkg/neg/utils.go index 133283fc50..eb42a3f06f 100644 --- a/pkg/neg/utils.go +++ b/pkg/neg/utils.go @@ -55,10 +55,10 @@ func (p1 PortNameMap) Difference(p2 PortNameMap) PortNameMap { // knownPorts represents the known Port:TargetPort attributes of servicePorts // that already exist on the service. This function returns an error if // any of the parsed ServicePorts from the annotation is not in knownPorts. -func NEGServicePorts(ann annotations.NegAnnotation, knownPorts PortNameMap) (PortNameMap, error) { +func NEGServicePorts(ann *annotations.NegAnnotation, knownPorts PortNameMap) (PortNameMap, error) { portSet := make(PortNameMap) var errList []error - for port, _ := range ann.ExposedPorts { + for port := range ann.ExposedPorts { // TODO: also validate ServicePorts in the exposed NEG annotation via webhook if _, ok := knownPorts[port]; !ok { errList = append(errList, fmt.Errorf("port %v specified in %q doesn't exist in the service", port, annotations.NEGAnnotationKey)) diff --git a/pkg/neg/utils_test.go b/pkg/neg/utils_test.go index daf518d3cf..42721e1eb6 100644 --- a/pkg/neg/utils_test.go +++ b/pkg/neg/utils_test.go @@ -175,7 +175,7 @@ func TestNEGServicePorts(t *testing.T) { } svc := annotations.FromService(service) - exposeNegStruct, _ := svc.NegAnnotation() + _, exposeNegStruct, _ := svc.NEGAnnotation() t.Run(tc.desc, func(t *testing.T) { svcPorts, err := NEGServicePorts(exposeNegStruct, tc.knownPortMap)