From a4ab574689c9b1d14915fa8589887d48bcc22ae1 Mon Sep 17 00:00:00 2001 From: Bowei Du Date: Sat, 23 Dec 2017 14:42:54 -0800 Subject: [PATCH] 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 | 42 +++++------- pkg/annotations/ingress_test.go | 69 +++++++++++++++++++ pkg/annotations/service.go | 30 +++++++- pkg/annotations/service_test.go | 95 ++++++++++++++++++++++++++ pkg/controller/controller.go | 2 +- pkg/controller/utils.go | 9 ++- pkg/networkendpointgroup/controller.go | 2 +- 7 files changed, 218 insertions(+), 31 deletions(-) create mode 100644 pkg/annotations/ingress_test.go create mode 100644 pkg/annotations/service_test.go diff --git a/pkg/annotations/ingress.go b/pkg/annotations/ingress.go index be5d54c510..484373d97f 100644 --- a/pkg/annotations/ingress.go +++ b/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,8 +90,8 @@ 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 "" } @@ -105,16 +99,16 @@ func (ing Ingress) UseNamedTLS() string { 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 "" } diff --git a/pkg/annotations/ingress_test.go b/pkg/annotations/ingress_test.go new file mode 100644 index 0000000000..82bbe50c28 --- /dev/null +++ b/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) + } + } +} diff --git a/pkg/annotations/service.go b/pkg/annotations/service.go index 616cfc7619..bc0e542704 100644 --- a/pkg/annotations/service.go +++ b/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" } diff --git a/pkg/annotations/service_test.go b/pkg/annotations/service_test.go new file mode 100644 index 0000000000..d51bc848a1 --- /dev/null +++ b/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) + } + } +} diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 0fcb949644..09c205d0f4 100644 --- a/pkg/controller/controller.go +++ b/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() == "" { diff --git a/pkg/controller/utils.go b/pkg/controller/utils.go index ebae3f8191..97bdc278aa 100644 --- a/pkg/controller/utils.go +++ b/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. diff --git a/pkg/networkendpointgroup/controller.go b/pkg/networkendpointgroup/controller.go index 3be2860bae..a535dbb2b8 100644 --- a/pkg/networkendpointgroup/controller.go +++ b/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 {