fix and refactor on NEG annotation handling

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 {

pkg/annotations/service_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package annotations
 
 import (
+	"fmt"
 	"reflect"
 	"testing"
 
@@ -25,67 +26,144 @@ 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{
 						NEGAnnotationKey: `{"ingress":true}`,
 					},
 				},
 			},
-			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{
 						NEGAnnotationKey: `{"exposed_ports":{"80":{}}}`,
 					},
 				},
 			},
-			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)
-			}
-		})
-	}
-}

pkg/controller/translator/translator.go

@@ -79,7 +79,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.

pkg/neg/controller.go

@@ -226,13 +226,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(negtypes.PortNameMap))
@@ -242,24 +246,19 @@ func (c *Controller) processService(key string) error {
 	// map of ServicePort (int) to TargetPort
 	svcPortMap := make(negtypes.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(negtypes.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
 		}