Merge pull request #284 from agau4779/expose-negs

Add annotation for exposing NEGs

pkg/annotations/service.go

@@ -41,6 +41,16 @@ const (
 	// 3. Adding this annotation on ingress.
 	NetworkEndpointGroupAlphaAnnotation = "alpha.cloud.google.com/load-balancer-neg"
 
+	// ExposeNEGAnnotationKey is the annotation key to specify standalone NEGs associated
+	// with the service. This should be a valid JSON string, as defined in
+	// ExposeNegAnnotation.
+	// example: {"80":{},"443":{}}
+	ExposeNEGAnnotationKey = "cloud.google.com/neg"
+
+	// NEGStatusKey is the annotation key whose value is the status of the NEGs
+	// on the Service, and is applied by the NEG Controller.
+	NEGStatusKey = "cloud.google.com/neg-status"
+
 	// BackendConfigKey is a stringified JSON with two fields:
 	// - "ports": a map of port names or port numbers to backendConfig names
 	// - "default": denotes the default backendConfig name for all ports except
@@ -58,6 +68,19 @@ const (
 	ProtocolHTTP2 AppProtocol = "HTTP2"
 )
 
+// ExposeNegAnnotation is the format of the annotation associated with the
+// ExposeNEGAnnotationKey key, and maps ServicePort to attributes of the NEG that should be
+// associated with the ServicePort. ServicePorts in this map will be NEG-enabled.
+type ExposeNegAnnotation map[int32]NegAttributes
+
+// NegAttributes houses the attributes of the NEGs that are associated with the
+// service. Future extensions to the Expose NEGs annotation should be added here.
+type NegAttributes struct {
+	// Note - in the future, this will be used for custom naming of NEGs.
+	// Currently has no effect.
+	Name string `json:"name,omitempty"`
+}
+
 // AppProtocol describes the service protocol.
 type AppProtocol string
 
@@ -73,7 +96,7 @@ func FromService(obj *v1.Service) *Service {
 
 // ApplicationProtocols returns a map of port (name or number) to the protocol
 // on the port.
-func (svc Service) ApplicationProtocols() (map[string]AppProtocol, error) {
+func (svc *Service) ApplicationProtocols() (map[string]AppProtocol, error) {
 	val, ok := svc.v[ServiceApplicationProtocolKey]
 	if !ok {
 		return map[string]AppProtocol{}, nil
@@ -98,8 +121,9 @@ func (svc Service) ApplicationProtocols() (map[string]AppProtocol, error) {
 	return portToProtos, err
 }
 
-// NEGEnabled is true if the service uses NEGs.
-func (svc Service) NEGEnabled() bool {
+// NEGEnabledForIngress returns true if the annotation is to be applied on
+// Ingress-referenced ports
+func (svc *Service) NEGEnabledForIngress() bool {
 	v, ok := svc.v[NetworkEndpointGroupAlphaAnnotation]
 	return ok && v == "true"
 }
@@ -110,13 +134,49 @@ var (
 	ErrBackendConfigAnnotationMissing = errors.New("annotation is missing")
 )
 
+// NEGExposed is true if the service exposes NEGs
+func (svc *Service) NEGExposed() bool {
+	if !flags.F.Features.NEGExposed {
+		return false
+	}
+
+	v, ok := svc.v[ExposeNEGAnnotationKey]
+	return ok && len(v) > 0
+}
+
+var (
+	ErrExposeNegAnnotationMissing = errors.New("No NEG ServicePorts specified")
+	ErrExposeNegAnnotationInvalid = errors.New("Expose NEG annotation is invalid")
+)
+
+// ExposeNegAnnotation returns the value of the Expose NEG annotation key
+func (svc *Service) ExposeNegAnnotation() (ExposeNegAnnotation, error) {
+	annotation, ok := svc.v[ExposeNEGAnnotationKey]
+	if !ok {
+		return nil, ErrExposeNegAnnotationMissing
+	}
+
+	// TODO: add link to Expose NEG documentation when complete
+	var exposedNegPortMap ExposeNegAnnotation
+	if err := json.Unmarshal([]byte(annotation), &exposedNegPortMap); err != nil {
+		return nil, ErrExposeNegAnnotationInvalid
+	}
+
+	return exposedNegPortMap, nil
+}
+
+// NEGEnabled is true if the service uses NEGs.
+func (svc *Service) NEGEnabled() bool {
+	return svc.NEGExposed() || svc.NEGEnabledForIngress()
+}
+
 type BackendConfigs struct {
 	Default string            `json:"default,omitempty"`
 	Ports   map[string]string `json:"ports,omitempty"`
 }
 
 // GetBackendConfigs returns BackendConfigs for the service.
-func (svc Service) GetBackendConfigs() (*BackendConfigs, error) {
+func (svc *Service) GetBackendConfigs() (*BackendConfigs, error) {
 	val, ok := svc.v[BackendConfigKey]
 	if !ok {
 		return nil, ErrBackendConfigAnnotationMissing

pkg/annotations/service_test.go

@@ -25,12 +25,77 @@ import (
 	"k8s.io/ingress-gce/pkg/flags"
 )
 
+func TestNEGService(t *testing.T) {
+	for _, tc := range []struct {
+		svc     *v1.Service
+		neg     bool
+		ingress bool
+		exposed bool
+	}{
+		{
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						NetworkEndpointGroupAlphaAnnotation: "true",
+					},
+				},
+			},
+			neg:     true,
+			ingress: true,
+			exposed: false,
+		},
+		{
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						ExposeNEGAnnotationKey: `"{"80":{}}"`,
+					},
+				},
+			},
+			neg:     true,
+			ingress: false,
+			exposed: true,
+		},
+		{
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						NetworkEndpointGroupAlphaAnnotation: "true",
+						ExposeNEGAnnotationKey:              `"{"80":{}}"`,
+					},
+				},
+			},
+			neg:     true,
+			ingress: true,
+			exposed: true,
+		},
+		{
+			svc:     &v1.Service{},
+			neg:     false,
+			ingress: false,
+			exposed: false,
+		},
+	} {
+		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)
+		}
+
+		if ing := svc.NEGEnabledForIngress(); ing != tc.ingress {
+			t.Errorf("for service %+v; svc.NEGEnabledForIngress() = %v; want %v", tc.svc, ing, tc.ingress)
+		}
+
+		if exposed := svc.NEGExposed(); exposed != tc.exposed {
+			t.Errorf("for service %+v; svc.NEGExposed() = %v; want %v", tc.svc, exposed, tc.exposed)
+		}
+	}
+}
+
 func TestService(t *testing.T) {
 	for _, tc := range []struct {
 		svc             *v1.Service
 		appProtocolsErr bool
 		appProtocols    map[string]AppProtocol
-		neg             bool
 		http2           bool
 	}{
 		{
@@ -69,17 +134,6 @@ func TestService(t *testing.T) {
 			appProtocols: map[string]AppProtocol{"443": "HTTP2"},
 			http2:        true,
 		},
-		{
-			svc: &v1.Service{
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: map[string]string{
-						NetworkEndpointGroupAlphaAnnotation: "true",
-					},
-				},
-			},
-			appProtocols: map[string]AppProtocol{},
-			neg:          true,
-		},
 		{
 			svc: &v1.Service{
 				ObjectMeta: metav1.ObjectMeta{
@@ -113,9 +167,6 @@ func TestService(t *testing.T) {
 		if err != nil || !reflect.DeepEqual(ap, tc.appProtocols) {
 			t.Errorf("for service %+v; svc.ApplicationProtocols() = %v, %v; want %v, nil", tc.svc, ap, err, tc.appProtocols)
 		}
-		if b := svc.NEGEnabled(); b != tc.neg {
-			t.Errorf("for service %+v; svc.NEGEnabled() = %v; want %v", tc.svc, b, tc.neg)
-		}
 	}
 }
 
@@ -210,3 +261,63 @@ func TestBackendConfigs(t *testing.T) {
 		}
 	}
 }
+
+func TestExposeNegAnnotation(t *testing.T) {
+	testcases := []struct {
+		desc        string
+		annotation  string
+		expected    ExposeNegAnnotation
+		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:   ExposeNegAnnotation{80: NegAttributes{}, 443: NegAttributes{}},
+			annotation: `{"80":{},"443":{}}`,
+		},
+
+		{
+			desc:       "NEGServicePort takes the union of known ports and ports referenced in the annotation",
+			annotation: `{"80":{}}`,
+			expected:   ExposeNegAnnotation{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[ExposeNEGAnnotationKey] = tc.annotation
+			}
+
+			svc := FromService(service)
+			exposeNegStruct, err := svc.ExposeNegAnnotation()
+
+			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)
+			}
+
+			if tc.expectedErr != nil && err != tc.expectedErr {
+				t.Errorf("Expected NEGServicePorts to return a %v error, got: %v", tc.expectedErr, err)
+			}
+		})
+	}
+}

pkg/backends/backends.go

@@ -542,7 +542,7 @@ func (b *Backends) Link(sp utils.ServicePort, zones []string) error {
 	if !sp.NEGEnabled {
 		return nil
 	}
-	negName := b.namer.NEG(sp.ID.Service.Namespace, sp.ID.Service.Name, sp.SvcTargetPort)
+	negName := sp.BackendName(b.namer)
 	var negs []*computealpha.NetworkEndpointGroup
 	var err error
 	for _, zone := range zones {

pkg/backends/backends_test.go

@@ -552,39 +552,39 @@ func TestBackendPoolSyncQuota(t *testing.T) {
 			true,
 			"New set of ports not including the same port",
 		},
-		// Need to fill the SvcTargetPort field on ServicePort to make sure
+		// Need to fill the TargetPort field on ServicePort to make sure
 		// NEG Backend naming is unique
 		{
 			[]utils.ServicePort{{NodePort: 8080}, {NodePort: 443}},
 			[]utils.ServicePort{
-				{NodePort: 8080, SvcTargetPort: "testport8080", NEGEnabled: true},
-				{NodePort: 443, SvcTargetPort: "testport443", NEGEnabled: true},
+				{Port: 8080, NodePort: 8080, NEGEnabled: true},
+				{Port: 443, NodePort: 443, NEGEnabled: true},
 			},
 			true,
 			"Same port converted to NEG, plus one new NEG port",
 		},
 		{
 			[]utils.ServicePort{
-				{NodePort: 80, SvcTargetPort: "testport80", NEGEnabled: true},
-				{NodePort: 90, SvcTargetPort: "testport90"},
+				{Port: 80, NodePort: 80, NEGEnabled: true},
+				{Port: 90, NodePort: 90},
 			},
 			[]utils.ServicePort{
-				{NodePort: 80, SvcTargetPort: "testport80"},
-				{NodePort: 90, SvcTargetPort: "testport90", NEGEnabled: true},
+				{Port: 80},
+				{Port: 90, NEGEnabled: true},
 			},
 			true,
 			"Mixed NEG and non-NEG ports",
 		},
 		{
 			[]utils.ServicePort{
-				{NodePort: 100, SvcTargetPort: "testport100", NEGEnabled: true},
-				{NodePort: 110, SvcTargetPort: "testport110", NEGEnabled: true},
-				{NodePort: 120, SvcTargetPort: "testport120", NEGEnabled: true},
+				{Port: 100, NodePort: 100, NEGEnabled: true},
+				{Port: 110, NodePort: 110, NEGEnabled: true},
+				{Port: 120, NodePort: 120, NEGEnabled: true},
 			},
 			[]utils.ServicePort{
-				{NodePort: 100, SvcTargetPort: "testport100"},
-				{NodePort: 110, SvcTargetPort: "testport110"},
-				{NodePort: 120, SvcTargetPort: "testport120"},
+				{Port: 100, NodePort: 100},
+				{Port: 110, NodePort: 110},
+				{Port: 120, NodePort: 120},
 			},
 			true,
 			"Same ports as NEG, then non-NEG",
@@ -854,20 +854,20 @@ func TestLinkBackendServiceToNEG(t *testing.T) {
 				Namespace: namespace,
 				Name:      name,
 			},
-			Port: intstr.FromInt(80),
 		},
-		NodePort:      30001,
-		Protocol:      annotations.ProtocolHTTP,
-		SvcTargetPort: port,
-		NEGEnabled:    true,
+		Port:       80,
+		NodePort:   30001,
+		Protocol:   annotations.ProtocolHTTP,
+		TargetPort: port,
+		NEGEnabled: true,
 	}
 	if err := bp.Ensure([]utils.ServicePort{svcPort}, nil); err != nil {
 		t.Fatalf("Failed to ensure backend service: %v", err)
 	}
 
 	for _, zone := range zones {
 		err := fakeNEG.CreateNetworkEndpointGroup(&computealpha.NetworkEndpointGroup{
-			Name: defaultNamer.NEG(namespace, name, port),
+			Name: defaultNamer.NEG(namespace, name, svcPort.Port),
 		}, zone)
 		if err != nil {
 			t.Fatalf("unexpected error: %v", err)

pkg/controller/translator/translator.go

@@ -87,10 +87,11 @@ func (t *Translator) getServicePort(id utils.ServicePortID) (*utils.ServicePort,
 		return nil, errors.ErrBadSvcType{Service: id.Service, ServiceType: svc.Spec.Type}
 	}
 	svcPort = &utils.ServicePort{
-		ID:            id,
-		NodePort:      int64(port.NodePort),
-		SvcTargetPort: port.TargetPort.String(),
-		NEGEnabled:    t.ctx.NEGEnabled && negEnabled,
+		ID:         id,
+		NodePort:   int64(port.NodePort),
+		Port:       int32(port.Port),
+		TargetPort: port.TargetPort.String(),
+		NEGEnabled: t.ctx.NEGEnabled && negEnabled,
 	}
 
 	appProtocols, err := annotations.FromService(svc).ApplicationProtocols()
@@ -279,7 +280,7 @@ func (t *Translator) GatherEndpointPorts(svcPorts []utils.ServicePort) []string
 			// For NEG backend, need to open firewall to all endpoint target ports
 			// TODO(mixia): refactor firewall syncing into a separate go routine with different trigger.
 			// With NEG, endpoint changes may cause firewall ports to be different if user specifies inconsistent backends.
-			endpointPorts := listEndpointTargetPorts(t.ctx.EndpointInformer.GetIndexer(), p.ID.Service.Namespace, p.ID.Service.Name, p.SvcTargetPort)
+			endpointPorts := listEndpointTargetPorts(t.ctx.EndpointInformer.GetIndexer(), p.ID.Service.Namespace, p.ID.Service.Name, p.TargetPort)
 			for _, ep := range endpointPorts {
 				portMap[int64(ep)] = true
 			}