merge Ingress NEG annotation and Expose NEG annotation

pkg/annotations/service.go

@@ -32,20 +32,15 @@ const (
 	// '{"my-https-port":"HTTPS","my-http-port":"HTTP"}'
 	ServiceApplicationProtocolKey = "service.alpha.kubernetes.io/app-protocols"
 
-	// NetworkEndpointGroupAlphaAnnotation is the annotation key to enable GCE NEG feature for ingress backend services.
-	// To enable this feature, the value of the annotation must be "true".
-	// This annotation should be specified on services that are backing ingresses.
-	// WARNING: The feature will NOT be effective in the following circumstances:
-	// 1. NEG feature is not enabled in feature gate.
-	// 2. Service is not referenced in any ingress.
-	// 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"
+	// NEGAnnotationKey is the annotation key to enable GCE NEG.
+	// The value of the annotation must be a valid JSON string in the format
+	// specified by type NegAnnotation. To enable, must have either Ingress: true
+	// or a non-empty ExposedPorts map referencing valid ServicePorts.
+	// examples:
+	// - `{"exposed_ports":{"80":{},"443":{}}}`
+	// - `{"ingress":true}`
+	// - `{"ingress": true,"exposed_ports":{"3000":{},"4000":{}}}`
+	NEGAnnotationKey = "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.
@@ -68,10 +63,23 @@ 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
+// NegAnnotation is the format of the annotation associated with the
+// NEGAnnotationKey key.
+type NegAnnotation struct {
+	// "Ingress" indicates whether to enable NEG feature for Ingress referencing
+	// the service. Each NEG correspond to a service port.
+	// NEGs will be created and managed under the following conditions:
+	// 1. Service is referenced by ingress
+	// 2. "ingress" is set to "true". Default to "false"
+	// When the above conditions are satisfied, Ingress will create a load balancer
+	//  and target corresponding NEGs as backends. Service Nodeport is not required.
+	Ingress bool `json:"ingress,omitempty"`
+	// ExposedPorts specifies the service ports to be exposed as stand-alone NEG.
+	// The exposed NEGs will be created and managed by NEG controller.
+	// ExposedPorts maps ServicePort to attributes of the NEG that should be
+	// associated with the ServicePort.
+	ExposedPorts map[int32]NegAttributes `json:"exposed_ports,omitempty"`
+}
 
 // NegAttributes houses the attributes of the NEGs that are associated with the
 // service. Future extensions to the Expose NEGs annotation should be added here.
@@ -124,8 +132,11 @@ func (svc *Service) ApplicationProtocols() (map[string]AppProtocol, error) {
 // 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"
+	annotation, err := svc.NegAnnotation()
+	if err != nil {
+		return false
+	}
+	return annotation.Ingress
 }
 
 var (
@@ -140,34 +151,37 @@ func (svc *Service) NEGExposed() bool {
 		return false
 	}
 
-	v, ok := svc.v[ExposeNEGAnnotationKey]
-	return ok && len(v) > 0
+	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")
 )
 
-// ExposeNegAnnotation returns the value of the Expose NEG annotation key
-func (svc *Service) ExposeNegAnnotation() (ExposeNegAnnotation, error) {
-	annotation, ok := svc.v[ExposeNEGAnnotationKey]
+// NegAnnotation returns the value of the  NEG annotation key
+func (svc *Service) NegAnnotation() (NegAnnotation, error) {
+	var res NegAnnotation
+	annotation, ok := svc.v[NEGAnnotationKey]
 	if !ok {
-		return nil, ErrExposeNegAnnotationMissing
+		return res, 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
+	if err := json.Unmarshal([]byte(annotation), &res); err != nil {
+		return res, ErrExposeNegAnnotationInvalid
 	}
 
-	return exposedNegPortMap, nil
+	return res, nil
 }
 
 // NEGEnabled is true if the service uses NEGs.
 func (svc *Service) NEGEnabled() bool {
-	return svc.NEGExposed() || svc.NEGEnabledForIngress()
+	return svc.NEGEnabledForIngress() || svc.NEGExposed()
 }
 
 type BackendConfigs struct {

pkg/annotations/service_test.go

@@ -36,7 +36,7 @@ func TestNEGService(t *testing.T) {
 			svc: &v1.Service{
 				ObjectMeta: metav1.ObjectMeta{
 					Annotations: map[string]string{
-						NetworkEndpointGroupAlphaAnnotation: "true",
+						NEGAnnotationKey: `{"ingress":true}`,
 					},
 				},
 			},
@@ -48,7 +48,7 @@ func TestNEGService(t *testing.T) {
 			svc: &v1.Service{
 				ObjectMeta: metav1.ObjectMeta{
 					Annotations: map[string]string{
-						ExposeNEGAnnotationKey: `"{"80":{}}"`,
+						NEGAnnotationKey: `{"exposed_ports":{"80":{}}}`,
 					},
 				},
 			},
@@ -60,8 +60,7 @@ func TestNEGService(t *testing.T) {
 			svc: &v1.Service{
 				ObjectMeta: metav1.ObjectMeta{
 					Annotations: map[string]string{
-						NetworkEndpointGroupAlphaAnnotation: "true",
-						ExposeNEGAnnotationKey:              `"{"80":{}}"`,
+						NEGAnnotationKey: `{"ingress":true,"exposed_ports":{"80":{}}}`,
 					},
 				},
 			},
@@ -262,11 +261,11 @@ func TestBackendConfigs(t *testing.T) {
 	}
 }
 
-func TestExposeNegAnnotation(t *testing.T) {
+func TestNegAnnotation(t *testing.T) {
 	testcases := []struct {
 		desc        string
 		annotation  string
-		expected    ExposeNegAnnotation
+		expected    NegAnnotation
 		expectedErr error
 	}{
 		{
@@ -280,15 +279,19 @@ func TestExposeNegAnnotation(t *testing.T) {
 			expectedErr: ErrExposeNegAnnotationInvalid,
 		},
 		{
-			desc:       "NEG annotation references existing service ports",
-			expected:   ExposeNegAnnotation{80: NegAttributes{}, 443: NegAttributes{}},
-			annotation: `{"80":{},"443":{}}`,
+			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: `{"80":{}}`,
-			expected:   ExposeNegAnnotation{80: NegAttributes{}},
+			annotation: `{"exposed_ports":{"80":{}}}`,
+			expected: NegAnnotation{
+				ExposedPorts: map[int32]NegAttributes{80: NegAttributes{}},
+			},
 		},
 	}
 
@@ -301,18 +304,18 @@ func TestExposeNegAnnotation(t *testing.T) {
 
 		t.Run(tc.desc, func(t *testing.T) {
 			if len(tc.annotation) > 0 {
-				service.Annotations[ExposeNEGAnnotationKey] = tc.annotation
+				service.Annotations[NEGAnnotationKey] = tc.annotation
 			}
 
 			svc := FromService(service)
-			exposeNegStruct, err := svc.ExposeNegAnnotation()
+			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)
+				t.Errorf("Expected NEGServicePorts to equal: %v; got: %v", tc.expected, exposeNegStruct.ExposedPorts)
 			}
 
 			if tc.expectedErr != nil && err != tc.expectedErr {

pkg/neg/controller.go

@@ -26,8 +26,10 @@ import (
 	"github.com/golang/glog"
 	apiv1 "k8s.io/api/core/v1"
 	extensions "k8s.io/api/extensions/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
+	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/scheme"
 	unversionedcore "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/tools/cache"
@@ -63,6 +65,7 @@ type Controller struct {
 	endpointSynced cache.InformerSynced
 	ingressLister  cache.Indexer
 	serviceLister  cache.Indexer
+	client         kubernetes.Interface
 
 	// serviceQueue takes service key as work item. Service key with format "namespace/name".
 	serviceQueue workqueue.RateLimitingInterface
@@ -97,6 +100,7 @@ func NewController(
 		ctx.EndpointInformer.GetIndexer())
 
 	negController := &Controller{
+		client:         ctx.KubeClient,
 		manager:        manager,
 		resyncPeriod:   resyncPeriod,
 		recorder:       recorder,
@@ -242,7 +246,7 @@ func (c *Controller) processService(key string) error {
 	if !enabled {
 		c.manager.StopSyncer(namespace, name)
 		// delete the annotation
-		return c.syncNegStatusAnnotation(namespace, name, service, make(PortNameMap))
+		return c.syncNegStatusAnnotation(namespace, name, make(PortNameMap))
 	}
 
 	glog.V(2).Infof("Syncing service %q", key)
@@ -261,7 +265,7 @@ func (c *Controller) processService(key string) error {
 			knownPorts[sp.Port] = sp.TargetPort.String()
 		}
 
-		annotation, err := annotations.FromService(service).ExposeNegAnnotation()
+		annotation, err := annotations.FromService(service).NegAnnotation()
 		if err != nil {
 			return err
 		}
@@ -274,26 +278,32 @@ func (c *Controller) processService(key string) error {
 		svcPortMap = svcPortMap.Union(negSvcPorts)
 	}
 
-	err = c.syncNegStatusAnnotation(namespace, name, service, svcPortMap)
+	err = c.syncNegStatusAnnotation(namespace, name, svcPortMap)
 	if err != nil {
 		return err
 	}
 	return c.manager.EnsureSyncers(namespace, name, svcPortMap)
 }
 
-func (c *Controller) syncNegStatusAnnotation(namespace, name string, service *apiv1.Service, portMap PortNameMap) error {
+func (c *Controller) syncNegStatusAnnotation(namespace, name string, portMap PortNameMap) error {
 	zones, err := c.zoneGetter.ListZones()
 	if err != nil {
 		return err
 	}
+	svcClient := c.client.CoreV1().Services(namespace)
+	service, err := svcClient.Get(name, metav1.GetOptions{})
+	if err != nil {
+		return err
+	}
 
 	// Remove NEG Status Annotation when no NEG is needed
 	if len(portMap) == 0 {
 		if _, ok := service.Annotations[annotations.NEGStatusKey]; ok {
 			// TODO: use PATCH to remove annotation
 			delete(service.Annotations, annotations.NEGStatusKey)
 			glog.V(2).Infof("Removing expose NEG annotation from service: %s/%s", namespace, name)
-			return c.serviceLister.Update(service)
+			_, err = svcClient.Update(service)
+			return err
 		}
 		// service doesn't have the expose NEG annotation and doesn't need update
 		return nil
@@ -316,8 +326,8 @@ func (c *Controller) syncNegStatusAnnotation(namespace, name string, service *ap
 
 	service.Annotations[annotations.NEGStatusKey] = annotation
 	glog.V(2).Infof("Updating NEG visibility annotation %q on service %s/%s.", annotation, namespace, name)
-	// TODO: use PATCH to Update Annotation
-	return c.serviceLister.Update(service)
+	_, err = svcClient.Update(service)
+	return err
 }
 
 func (c *Controller) handleErr(err error, key interface{}) {