-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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.
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
GCE: Fix lowercase value and alpha-missing annotation for ILB #50470
GCE: Fix lowercase value and alpha-missing annotation for ILB #50470
Conversation
} | ||
|
||
// Check for deprecated annotation key | ||
if l, exists := service.Annotations[deprecatedServiceAnnotationILBBackendShare]; exists && l == "true" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
report a warning event?
There was a problem hiding this comment.
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 | |
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
5382476
to
2aa6250
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: freehan, nicksardo Associated issue: 50426 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue |
…470-upstream-release-1.7 Automatic merge from submit-queue Automated cherry pick of #50470 Cherry pick of #50470 on release-1.7. #50470: GCE: Specify alpha in annotation key, deprecate lower case of **Release note**: ```release-note GCE: Internal load balancer is enabled with annotation value "Internal" instead of "internal". ```
What this PR does / why we need it:
Fixes #50426
Also explicitly sets an annotation as 'alpha'.
/assign @freehan @bowei
Release note: