Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GCE: Fix lowercase value and alpha-missing annotation for ILB #50470

Merged
merged 1 commit into from
Aug 11, 2017
Merged

GCE: Fix lowercase value and alpha-missing annotation for ILB #50470

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 19 additions & 7 deletions pkg/cloudprovider/providers/gce/gce_annotations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ limitations under the License.

package gce

import "k8s.io/api/core/v1"
import (
"github.com/golang/glog"
"k8s.io/api/core/v1"
)

type LoadBalancerType string

Expand All @@ -26,12 +29,16 @@ const (
// Currently, only "internal" is supported.
ServiceAnnotationLoadBalancerType = "cloud.google.com/load-balancer-type"

LBTypeInternal LoadBalancerType = "internal"
LBTypeInternal LoadBalancerType = "Internal"
// Deprecating the lowercase spelling of Internal.
deprecatedTypeInternalLowerCase LoadBalancerType = "internal"

// ServiceAnnotationInternalBackendShare is annotated on a service with "true" when users
// want to share GCP Backend Services for a set of internal load balancers.
// ALPHA feature - this may be removed in a future release.
ServiceAnnotationILBBackendShare = "cloud.google.com/load-balancer-backend-share"
ServiceAnnotationILBBackendShare = "alpha.cloud.google.com/load-balancer-backend-share"
// This annotation did not correctly specify "alpha", so both annotations will be checked.
deprecatedServiceAnnotationILBBackendShare = "cloud.google.com/load-balancer-backend-share"
)

// GetLoadBalancerAnnotationType returns the type of GCP load balancer which should be assembled.
Expand All @@ -48,8 +55,8 @@ func GetLoadBalancerAnnotationType(service *v1.Service) (LoadBalancerType, bool)
}

switch v {
case LBTypeInternal:
return v, true
case LBTypeInternal, deprecatedTypeInternalLowerCase:
return LBTypeInternal, true
default:
return v, false
}
Expand All @@ -58,8 +65,13 @@ func GetLoadBalancerAnnotationType(service *v1.Service) (LoadBalancerType, bool)
// GetLoadBalancerAnnotationBackendShare returns whether this service's backend service should be
// shared with other load balancers. Health checks and the healthcheck firewall will be shared regardless.
func GetLoadBalancerAnnotationBackendShare(service *v1.Service) bool {
l, exists := service.Annotations[ServiceAnnotationILBBackendShare]
if exists && l == "true" {
if l, exists := service.Annotations[ServiceAnnotationILBBackendShare]; exists && l == "true" {
return true
}

// Check for deprecated annotation key
if l, exists := service.Annotations[deprecatedServiceAnnotationILBBackendShare]; exists && l == "true" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

report a warning event?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a helper function, so the best we could do is glog a warning. However, I haven't seen that for other deprecated annotations:

kubernetes/pkg/api/service/util.go

Lines 73 to 95 in e7b1814

// RequestsOnlyLocalTraffic checks if service requests OnlyLocal traffic.
func RequestsOnlyLocalTraffic(service *api.Service) bool {
if service.Spec.Type != api.ServiceTypeLoadBalancer &&
service.Spec.Type != api.ServiceTypeNodePort {
return false
}
// First check the beta annotation and then the first class field. This is so that
// existing Services continue to work till the user decides to transition to the
// first class field.
if l, ok := service.Annotations[api.BetaAnnotationExternalTraffic]; ok {
switch l {
case api.AnnotationValueExternalTrafficLocal:
return true
case api.AnnotationValueExternalTrafficGlobal:
return false
default:
glog.Errorf("Invalid value for annotation %v: %v", api.BetaAnnotationExternalTraffic, l)
return false
}
}
return service.Spec.ExternalTrafficPolicy == api.ServiceExternalTrafficPolicyTypeLocal
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For generic k8s object fei/annotaion, I think we will throw errors during validation.

But since this is a GCP sepecific annotation, I am not sure how we expose it.

Log it at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

glog.Warningf("Annotation %q is deprecated and replaced with an alpha-specific key: %q", deprecatedServiceAnnotationILBBackendShare, ServiceAnnotationILBBackendShare)
return true
}

Expand Down