Make annotation wrapper types internal keys private; adds unit tests

This prevents uses of raw annotation keys without proper accessor
methods.

pkg/annotations/ingress.go

@@ -18,6 +18,8 @@ package annotations
 
 import (
 	"strconv"
+
+	extensions "k8s.io/api/extensions/v1beta1"
 )
 
 const (
@@ -46,12 +48,6 @@ const (
 	// to the target proxies of the Ingress.
 	PreSharedCertKey = "ingress.gcp.kubernetes.io/pre-shared-cert"
 
-	// ServiceApplicationProtocolKey is a stringified JSON map of port names to
-	// protocol strings. Possible values are HTTP, HTTPS
-	// Example:
-	// '{"my-https-port":"HTTPS","my-http-port":"HTTP"}'
-	ServiceApplicationProtocolKey = "service.alpha.kubernetes.io/app-protocols"
-
 	// IngressClassKey picks a specific "class" for the Ingress. The controller
 	// only processes Ingresses with this annotation either unset, or set
 	// to either gceIngessClass or the empty string.
@@ -68,23 +64,21 @@ const (
 	// This is read only for users. Controller will overrite any user updates.
 	// This is only set for ingresses with ingressClass = "gce-multi-cluster"
 	InstanceGroupsAnnotationKey = "ingress.gcp.kubernetes.io/instance-groups"
-
-	// 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"
 )
 
 // Ingress represents ingress annotations.
-type Ingress map[string]string
+type Ingress struct {
+	v map[string]string
+}
+
+// FromIngress extracts the annotations from an Ingress definition.
+func FromIngress(ing *extensions.Ingress) *Ingress {
+	return &Ingress{ing.Annotations}
+}
 
 // AllowHTTP returns the allowHTTP flag. True by default.
-func (ing Ingress) AllowHTTP() bool {
-	val, ok := ing[AllowHTTPKey]
+func (ing *Ingress) AllowHTTP() bool {
+	val, ok := ing.v[AllowHTTPKey]
 	if !ok {
 		return true
 	}
@@ -96,25 +90,25 @@ func (ing Ingress) AllowHTTP() bool {
 }
 
 // UseNamedTLS returns the name of the GCE SSL certificate. Empty by default.
-func (ing Ingress) UseNamedTLS() string {
-	val, ok := ing[PreSharedCertKey]
+func (ing *Ingress) UseNamedTLS() string {
+	val, ok := ing.v[PreSharedCertKey]
 	if !ok {
 		return ""
 	}
 
 	return val
 }
 
-func (ing Ingress) StaticIPName() string {
-	val, ok := ing[StaticIPNameKey]
+func (ing *Ingress) StaticIPName() string {
+	val, ok := ing.v[StaticIPNameKey]
 	if !ok {
 		return ""
 	}
 	return val
 }
 
-func (ing Ingress) IngressClass() string {
-	val, ok := ing[IngressClassKey]
+func (ing *Ingress) IngressClass() string {
+	val, ok := ing.v[IngressClassKey]
 	if !ok {
 		return ""
 	}

pkg/annotations/ingress_test.go

@@ -0,0 +1,69 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package annotations
+
+import (
+	"testing"
+
+	extensions "k8s.io/api/extensions/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestIngress(t *testing.T) {
+	for _, tc := range []struct {
+		ing          *extensions.Ingress
+		allowHTTP    bool
+		useNamedTLS  string
+		staticIPName string
+		ingressClass string
+	}{
+		{
+			ing:       &extensions.Ingress{},
+			allowHTTP: true, // defaults to true.
+		},
+		{
+			ing: &extensions.Ingress{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						AllowHTTPKey:     "false",
+						IngressClassKey:  "gce",
+						PreSharedCertKey: "shared-cert-key",
+						StaticIPNameKey:  "1.2.3.4",
+					},
+				},
+			},
+			allowHTTP:    false,
+			useNamedTLS:  "shared-cert-key",
+			staticIPName: "1.2.3.4",
+			ingressClass: "gce",
+		},
+	} {
+		ing := FromIngress(tc.ing)
+		if x := ing.AllowHTTP(); x != tc.allowHTTP {
+			t.Errorf("ingress %+v; AllowHTTP() = %v, want %v", tc.ing, x, tc.allowHTTP)
+		}
+		if x := ing.UseNamedTLS(); x != tc.useNamedTLS {
+			t.Errorf("ingress %+v; UseNamedTLS() = %v, want %v", tc.ing, x, tc.useNamedTLS)
+		}
+		if x := ing.StaticIPName(); x != tc.staticIPName {
+			t.Errorf("ingress %+v; StaticIPName() = %v, want %v", tc.ing, x, tc.staticIPName)
+		}
+		if x := ing.IngressClass(); x != tc.ingressClass {
+			t.Errorf("ingress %+v; IngressClass() = %v, want %v", tc.ing, x, tc.ingressClass)
+		}
+	}
+}

pkg/annotations/service.go

@@ -19,9 +19,26 @@ package annotations
 import (
 	"encoding/json"
 	"fmt"
+
+	"k8s.io/api/core/v1"
 )
 
 const (
+	// ServiceApplicationProtocolKey is a stringified JSON map of port names to
+	// protocol strings. Possible values are HTTP, HTTPS
+	// Example:
+	// '{"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"
+
 	// ProtocolHTTP protocol for a service
 	ProtocolHTTP AppProtocol = "HTTP"
 	// ProtocolHTTPS protocol for a service
@@ -32,12 +49,19 @@ const (
 type AppProtocol string
 
 // Service represents Service annotations.
-type Service map[string]string
+type Service struct {
+	v map[string]string
+}
+
+// FromService extracts the annotations from an Service definition.
+func FromService(obj *v1.Service) *Service {
+	return &Service{obj.Annotations}
+}
 
 // ApplicationProtocols returns a map of port (name or number) to the protocol
 // on the port.
 func (svc Service) ApplicationProtocols() (map[string]AppProtocol, error) {
-	val, ok := svc[ServiceApplicationProtocolKey]
+	val, ok := svc.v[ServiceApplicationProtocolKey]
 	if !ok {
 		return map[string]AppProtocol{}, nil
 	}
@@ -59,6 +83,6 @@ func (svc Service) ApplicationProtocols() (map[string]AppProtocol, error) {
 
 // NEGEnabled is true if the service uses NEGs.
 func (svc Service) NEGEnabled() bool {
-	v, ok := svc[NetworkEndpointGroupAlphaAnnotation]
+	v, ok := svc.v[NetworkEndpointGroupAlphaAnnotation]
 	return ok && v == "true"
 }

pkg/annotations/service_test.go

@@ -0,0 +1,95 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package annotations
+
+import (
+	"reflect"
+	"testing"
+
+	"k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestService(t *testing.T) {
+	for _, tc := range []struct {
+		svc             *v1.Service
+		appProtocolsErr bool
+		appProtocols    map[string]AppProtocol
+		neg             bool
+	}{
+		{
+			svc:          &v1.Service{},
+			appProtocols: map[string]AppProtocol{},
+		},
+		{
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						ServiceApplicationProtocolKey: `{"80": "HTTP", "443": "HTTPS"}`,
+					},
+				},
+			},
+			appProtocols: map[string]AppProtocol{"80": "HTTP", "443": "HTTPS"},
+		},
+		{
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						NetworkEndpointGroupAlphaAnnotation: "true",
+					},
+				},
+			},
+			appProtocols: map[string]AppProtocol{},
+			neg:          true,
+		},
+		{
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						ServiceApplicationProtocolKey: `invalid`,
+					},
+				},
+			},
+			appProtocolsErr: true,
+		},
+		{
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						ServiceApplicationProtocolKey: `{"SSH": "22"}`,
+					},
+				},
+			},
+			appProtocolsErr: true,
+		},
+	} {
+		svc := FromService(tc.svc)
+		ap, err := svc.ApplicationProtocols()
+		if tc.appProtocolsErr {
+			if err == nil {
+				t.Errorf("for service %+v; svc.ApplicationProtocols() = _, %v; want _, error", tc.svc, err)
+			}
+			continue
+		}
+		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)
+		}
+	}
+}

pkg/controller/controller.go

@@ -449,7 +449,7 @@ func (lbc *LoadBalancerController) toRuntimeInfo(ingList extensions.IngressList)
 
 		var tls *loadbalancers.TLSCerts
 
-		annotations := annotations.Ingress(ing.ObjectMeta.Annotations)
+		annotations := annotations.FromIngress(&ing)
 		// Load the TLS cert from the API Spec if it is not specified in the annotation.
 		// TODO: enforce this with validation.
 		if annotations.UseNamedTLS() == "" {

pkg/controller/utils.go

@@ -33,14 +33,14 @@ import (
 // isGCEIngress returns true if the given Ingress either doesn't specify the
 // ingress.class annotation, or it's set to "gce".
 func isGCEIngress(ing *extensions.Ingress) bool {
-	class := annotations.Ingress(ing.ObjectMeta.Annotations).IngressClass()
+	class := annotations.FromIngress(ing).IngressClass()
 	return class == "" || class == annotations.GceIngressClass
 }
 
 // isGCEMultiClusterIngress returns true if the given Ingress has
 // ingress.class annotation set to "gce-multi-cluster".
 func isGCEMultiClusterIngress(ing *extensions.Ingress) bool {
-	class := annotations.Ingress(ing.ObjectMeta.Annotations).IngressClass()
+	class := annotations.FromIngress(ing).IngressClass()
 	return class == annotations.GceMultiIngressClass
 }
 
@@ -60,8 +60,13 @@ type ErrSvcAppProtosParsing struct {
 	origErr error
 }
 
+<<<<<<< HEAD
 func (e ErrSvcAppProtosParsing) Error() string {
 	return fmt.Sprintf("could not parse %v annotation on Service %v/%v, err: %v", annotations.ServiceApplicationProtocolKey, e.svc.Namespace, e.svc.Name, e.origErr)
+=======
+func (e errorSvcAppProtosParsing) Error() string {
+	return fmt.Sprintf("could not parse Service app protocol annotation %v/%v, err: %v", e.svc.Namespace, e.svc.Name, e.origErr)
+>>>>>>> Make annotation wrapper types internal keys private; adds unit tests
 }
 
 // compareLinks returns true if the 2 self links are equal.

pkg/networkendpointgroup/controller.go

@@ -200,7 +200,7 @@ func (c *Controller) processService(key string) error {
 	var enabled bool
 	if exists {
 		service = svc.(*apiv1.Service)
-		enabled = annotations.Service(service.GetAnnotations()).NEGEnabled()
+		enabled = annotations.FromService(service).NEGEnabled()
 	}
 
 	if !enabled {