NE-956: Configurable LB Source Ranges - Functional Implementation

[suleymanakbas91]

This PR provides the functional implementation for the new AllowedSourceRanges API of IngressController which is used to set LoadBalancerSourceRanges on the loadbalancer-typed services.

Epic: https://issues.redhat.com/browse/NE-555
EP: openshift/enhancements#1177
API: openshift/api#1222

• pkg/operator/controller/ingress/controller.go: Adds a new condition type EvaluationConditionsDetected.
• pkg/operator/controller/ingress/load_balancer_service.go:
  • Introduces new functions loadBalancerServiceIsProgressing to be used in computeLoadBalancerProgressingStatus and loadBalancerServiceEvaluationConditionDetected to be used in computeIngressEvaluationConditionsDetectedCondition on ingress/status.go, to compute load balancer service's Progressing and EvaluationConditionDetected conditions.
  • Sets the expected LoadBalancerSourceRanges based on the AllowedSourceRanges in desiredLoadBalancerService function.
  • Improve the loadBalancerServiceChanged function to set LoadBalancerSourceRanges and the annotation based on the expected LoadBalancerSourceRanges value.
  • Introduces loadBalancerSourceRangesAnnotationSet and loadBalancerSourceRangesMatch to help with Progressing=True and EvaluationConditionDetected=True condition calculations.
• pkg/operator/controller/ingress/load_balancer_service_test.go:
  • Adds unit-test to verify modifying LoadBalancerSourceRanges or the annotation are evaluated as service changed.
  • Adds unit-test to verify all the cases included in the table presented on the EP.
• pkg/operator/controller/ingress/status.go:
  • Introduces a new function computeAllowedSourceRanges to calculate the effective source ranges value by looking at the annotation and the LoadBalancerSourceRanges field on the service. This calculated value is then used to set AllowedSourceRanges in IngressController's status.
  • Adds a new function computeIngressEvaluationConditionsDetectedCondition to compute the new EvaluationConditionsDetected condition type.
  • Adapts the computeLoadBalancerProgressingStatus to use the new loadBalancerServiceIsProgressing function on ingress/load_balancer_service.go
• pkg/operator/controller/ingress/status_test.go:
  • Adds unit-tests to verify Progressing=True condition is calculated correctly for two cases mentioned in the EP.
  • Adds unit-tests for the new computeIngressEvaluationConditionsDetectedCondition function that verifies EvaluationConditionsDetected=True condition is calculated correctly for two cases mentioned in the EP.
• pkg/operator/controller/status/controller.go: Adds a new function computeOperatorEvaluationConditionsDetectedCondition to compute operator's new EvaluationConditionsDetected condition.
• pkg/operator/controller/status/controller_test.go: Adds unit-tests for the new computeOperatorEvaluationConditionsDetectedCondition function.
• test/e2e/all_test.go: Adds new e2e tests to parallel tests.
• test/e2e/allowed_source_ranges_test.go: Adds e2e tests to verify the functionality. What each test verifies is explained in the function comments.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[brandisher]

/retest

[brandisher]

/retest

[brandisher]

/retest

[brandisher]

Looks like the main changes are the test name and some formatting in the CRD.

/lgtm

[suleymanakbas91]

/retest

[brandisher]

/retest

[Miciah]

I think moving some of the update logic from desiredLoadBalancerService to loadBalancerServiceChanged would simplify the code a bit, and I'm curious to know what you think of that idea. Otherwise, my main concerns are that it looks like desiredLoadBalancerService sets the annotation with the empty string as the value—am I reading that correctly?—and that IngressStatusesEqual appears to have a potential nil-pointer dereference. My other comments are pretty minor things. Looks good overall!

[brandisher]

/retest

[Miciah]

Looks really good. I had a few more minor comments, but nothing that should hold up the PR. Thanks!
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [Miciah]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Miciah]

/retest

[brandisher]

/lgtm

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD 1600a0e and 2 for PR HEAD 48cd426 in total

[brandisher]

Looks like a few e2e test flakes which I don't think are related.

/test e2e-aws-operator

[brandisher]

level=error msg=Cluster operator control-plane-machine-set Available is False with UnavailableReplicas: Missing 3 available replica(s)

Doesn't look related.

/test e2e-aws-operator

[suleymanakbas91]

/retest

[suleymanakbas91]

/retest

[suleymanakbas91]

/retest

[openshift-ci]

@suleymanakbas91: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.