Bug 1960284: Set the "local-with-fallback" service annotation#622
Conversation
openshift-ci
bot
added
the
bugzilla/severity-urgent
|
@Miciah: This pull request references Bugzilla bug 1960284, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci
bot
added
the
bugzilla/valid-bug
openshift-ci
bot
added
the
approved
Miciah
force-pushed
the
BZ1960284-set-the-local-with-fallback-service-annotation
branch
from
d4554f6 to
912b190
Compare
|
@Miciah: This pull request references Bugzilla bug 1960284, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
|
/assign |
Miciah
force-pushed
the
BZ1960284-set-the-local-with-fallback-service-annotation
branch
from
912b190 to
dc339fe
Compare
|
|
||
| // By default, use local-with-fallback when using the "Local" external | ||
| // traffic policy. | ||
| if service.Spec.ExternalTrafficPolicy == corev1.ServiceExternalTrafficPolicyTypeLocal { |
There was a problem hiding this comment.
Should we return false if service.Spec.ExternalTrafficPolicy is not local?
As is, this logic would let a user use an unsupported config override to set the local-with-fallback annotation when service.Spec.ExternalTrafficPolicy is cluster, which is a state we shouldn't allow, right?
There was a problem hiding this comment.
I'm kind of ambivalent about allowing the user to do weird things with an unsupported config override. I'll go ahead and make the suggested change.
There was a problem hiding this comment.
Fair enough. Seems like some low-hanging fruit that we might as well patch while we can IMO.
| // loadBalancerServiceChanged checks if the current load balancer service | ||
| // matches the expected and if not returns an updated one. | ||
| func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev1.Service) { | ||
| annotationCmpOpts := []cmp.Option{ |
| if want && (!have || currentVal != expectedVal) { | ||
| updated.Annotations[annotation] = expected.Annotations[annotation] | ||
| } else if have && !want { | ||
| delete(updated.Annotations, annotation) |
There was a problem hiding this comment.
Could you add a unit test to cover this scenario?
| if want && (!have || currentVal != expectedVal) { | ||
| updated.Annotations[annotation] = expected.Annotations[annotation] | ||
| } else if have && !want { | ||
| delete(updated.Annotations, annotation) |
There was a problem hiding this comment.
Could you add a unit test for this scenario?
There was a problem hiding this comment.
Fixed. However, I had to replace the test case for adding the annotation with test cases for updating and deleting. To cover all three would make the test setup more complicated. If you want coverage for all three, I'd just as soon add a separate test. What do you think?
There was a problem hiding this comment.
I see. Current coverage looks good enough to me! Thanks.
test/e2e/operator_test.go
Outdated
| return !ok, nil | ||
| }) | ||
| if _, ok := service.Annotations[annotation]; ok { | ||
| t.Fatalf("failed to observe removal of the %q annotation on ingresscontroller %q", annotation, defaultName) |
There was a problem hiding this comment.
| t.Fatalf("failed to observe removal of the %q annotation on ingresscontroller %q", annotation, defaultName) | |
| t.Fatalf("failed to observe removal of the %q annotation on service %q", annotation, serviceName) |
test/e2e/operator_test.go
Outdated
| return !ok, nil | ||
| }) | ||
| if _, ok := service.Annotations[annotation]; ok { | ||
| t.Fatalf("failed to observe removal of the %q annotation on the %q ingresscontroller", annotation, icName) |
There was a problem hiding this comment.
| t.Fatalf("failed to observe removal of the %q annotation on the %q ingresscontroller", annotation, icName) | |
| t.Fatalf("failed to observe removal of the %q annotation on the %q service", annotation, serviceName) |
openshift-ci
bot
added
the
needs-rebase
Miciah
force-pushed
the
BZ1960284-set-the-local-with-fallback-service-annotation
branch
from
dc339fe to
a83ff0c
Compare
Set the "traffic-policy.network.alpha.openshift.io/local-with-fallback" annotation on LoadBalancer- and NodePort-type services that use the "Local" external traffic policy. This commit is related to bug 1960284. https://bugzilla.redhat.com/show_bug.cgi?id=1960284 * pkg/operator/controller/ingress/load_balancer_service.go (localWithFallbackAnnotation): New const. Define the annotation key for the "local-with-fallback" service annotation. (desiredLoadBalancerService): Set the "local-with-fallback" annotation when using the "Local" external traffic policy. (managedLoadBalancerServiceAnnotations): New variable. Define the keys for annotations that the operator manages on LoadBalancer-type services, which now include the "local-with-fallback" annotation. (loadBalancerServiceChanged): Fix the update logic to properly handle annotations with empty values using go-cmp and managedLoadBalancerServiceAnnotations. * pkg/operator/controller/ingress/load_balancer_service_test.go (TestDesiredLoadBalancerService): Verify that the "local-with-fallback" annotation is set when appropriate. (TestLoadBalancerServiceChanged): Verify that loadBalancerServiceChanged properly handles updates to the "local-with-fallback" annotation. * pkg/operator/controller/ingress/nodeport_service.go (desiredNodePortService): Set the "local-with-fallback" annotation. (managedNodePortServiceAnnotations): New variable. Define the annotation keys that the operator manages on NodePort-type services. (nodePortServiceChanged): Check if any of the annotations in managedNodePortServiceAnnotations have changed. Update managed annotations if they have changed. * pkg/operator/controller/ingress/nodeport_service_test.go (TestDesiredNodePortService): Verify that the expected annotations are set. (TestNodePortServiceChanged): Add a test case to verify that nodePortServiceChanged properly handles updates to the "local-with-fallback" annotation.
Miciah
force-pushed
the
BZ1960284-set-the-local-with-fallback-service-annotation
branch
from
a83ff0c to
3326d75
Compare
|
Rebased. |
openshift-ci
bot
removed
the
needs-rebase
Miciah
force-pushed
the
BZ1960284-set-the-local-with-fallback-service-annotation
branch
from
3326d75 to
56685eb
Compare
* pkg/operator/controller/ingress/load_balancer_service.go (desiredLoadBalancerService): Use the new shouldUseLocalWithFallback function to determine whether to set the "local-with-fallback" annotation. (shouldUseLocalWithFallback): New function. Check the service's external traffic policy and the ingresscontroller's unsupported config overrides and return a Boolean value indicating whether "local-with-fallback" should be enabled, or an error if the override could not be parsed. * pkg/operator/controller/ingress/load_balancer_service_test.go (TestShouldUseLocalWithFallback): New test. Verify that shouldUseLocalWithFallback behaves as expected. * pkg/operator/controller/ingress/nodeport_service.go (ensureNodePortService): Check the error value from desiredNodePortService. (desiredNodePortService): Add error return value. Use the new shouldUseLocalWithFallback function (which can return an error) to determine whether to set the "local-with-fallback" annotation. * pkg/operator/controller/ingress/nodeport_service_test.go (TestDesiredNodePortService): Check the error value from desiredNodePortService. * test/e2e/operator_test.go (TestLocalWithFallbackOverrideForLoadBalancerService) (TestLocalWithFallbackOverrideForNodePortService): New tests. Verify that the operator does not set the "local-with-fallback" annotation on a LoadBalancer or NodePort service if the localWithFallback unsupported config override is set to "false". Co-authored-by: candita <cholman@redhat.com>
Miciah
force-pushed
the
BZ1960284-set-the-local-with-fallback-service-annotation
branch
from
56685eb to
fcc0f4a
Compare
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah, sgreene570 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
28/289 e2e-gcp-serial tests failed, nothing that looks related to ingress. |
|
/retest |
|
/test e2e-gcp-serial |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
4 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@Miciah: All pull requests linked via external trackers have merged: Bugzilla bug 1960284 has been moved to the MODIFIED state. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
4 participants
Set the "local-with-fallback" service annotation
Set the
traffic-policy.network.alpha.openshift.io/local-with-fallbackannotation on LoadBalancer- and NodePort-type services that use theLocalexternal traffic policy.pkg/operator/controller/ingress/load_balancer_service.go(localWithFallbackAnnotation): New const. Define the annotation key for thelocal-with-fallbackservice annotation.(
desiredLoadBalancerService): Set thelocal-with-fallbackannotation when using theLocalexternal traffic policy.(
managedLoadBalancerServiceAnnotations): New variable. Define the keys for annotations that the operator manages on LoadBalancer-type services, which now include thelocal-with-fallbackannotation.(
loadBalancerServiceChanged): Fix the update logic to properly handle annotations with empty values using go-cmp andmanagedLoadBalancerServiceAnnotations.pkg/operator/controller/ingress/load_balancer_service_test.go(TestDesiredLoadBalancerService): Verify that thelocal-with-fallbackannotation is set when appropriate.(
TestLoadBalancerServiceChanged): Verify thatloadBalancerServiceChangedproperly handles updates to thelocal-with-fallbackannotation.pkg/operator/controller/ingress/nodeport_service.go(desiredNodePortService): Set thelocal-with-fallbackannotation.(
managedNodePortServiceAnnotations): New variable. Define the annotation keys that the operator manages on NodePort-type services.(
nodePortServiceChanged): Check if any of the annotations inmanagedNodePortServiceAnnotationshave changed. Update managed annotations if they have changed.pkg/operator/controller/ingress/nodeport_service_test.go(TestDesiredNodePortService): Verify that the expected annotations are set.(
TestNodePortServiceChanged): Add a test case to verify thatnodePortServiceChangedproperly handles updates to thelocal-with-fallbackannotation.Add local-with-fallback unsupported config override
pkg/operator/controller/ingress/load_balancer_service.go(desiredLoadBalancerService): Use the newshouldUseLocalWithFallbackfunction to determine whether to set thelocal-with-fallbackannotation.(
shouldUseLocalWithFallback): New function. Check the service's external traffic policy and the ingresscontroller's unsupported config overrides and return a Boolean value indicating whetherlocal-with-fallbackshould be enabled, or an error if the override could not be parsed.pkg/operator/controller/ingress/load_balancer_service_test.go(TestShouldUseLocalWithFallback): New test. Verify thatshouldUseLocalWithFallbackbehaves as expected.pkg/operator/controller/ingress/nodeport_service.go(ensureNodePortService): Check the error value fromdesiredNodePortService.(
desiredNodePortService): Add error return value. Use the newshouldUseLocalWithFallbackfunction (which can return an error) to determine whether to set thelocal-with-fallbackannotation.pkg/operator/controller/ingress/nodeport_service_test.go(TestDesiredNodePortService): Check the error value fromdesiredNodePortService.TestLocalWithFallbackOverrideForLoadBalancerService,TestLocalWithFallbackOverrideForNodePortService): New tests. Verify that the operator does not set thelocal-with-fallbackannotation on a LoadBalancer or NodePort service if thelocalWithFallbackunsupported config override is set to "false".