OCPBUGS-30833: Added a check for loadBalancerClass for built-in load balancing service.

[jdhirst]

Fixes #3136

Which issue(s) this PR addresses: #3136

Closes #3136

This is my idea for an option to implement the checking of loadBalancerClass in the internal load balancing service. It would avoid conflicts with metallb or other load balancers being used.

I specified "microshift-internal" as the name of the internal balancer service, but that could be anything really, was just the first thing that I thought of.

Let me know what you think 😄

[openshift-ci]

Hi @jdhirst. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[pacevedom]

/assign @pacevedom

[pacevedom]

Thanks for the PR! I left one small comment.

[pacevedom]

While the controller is too open now (thanks for catching that!), this might be a bit too restrictive. This controller acts not just on MicroShift-owned resources, but also on any LoadBalancer service owned by applications.
By requiring the loadBalancerClass to match an expected value, MicroShift would be imposing that need to already deployed services and they would fail to pick up IPs again. This would happen in an upgrade, for example. Also, requiring this value in application's manifests would break compatibility with OpenShift and would require specific configuration for apps to run on MicroShift.
Instead of checking a specific value of the loadBalancerClass, I would check for emptiness instead.
From the docs:

If you specify .spec.loadBalancerClass, it is assumed that a load balancer implementation that matches the specified class is watching for Services. Any default load balancer implementation (for example, the one provided by the cloud provider) will ignore Services that have this field set.

I would drop the second part of the new check and that should work in all cases.

[jdhirst]

Thanks for reviewing my request! Yep, that makes perfect sense! I updated the PR with only the check for nil now 😄

[dhellmann]

/retitle OCPBUGS-30833: Added a check for loadBalancerClass for built-in load balancing service.

[openshift-ci-robot]

@jdhirst: This pull request references Jira Issue OCPBUGS-30833, which is invalid:

• expected the bug to target the "4.16.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Fixes #3136

Which issue(s) this PR addresses: #3136

Closes #3136

This is my idea for an option to implement the checking of loadBalancerClass in the internal load balancing service. It would avoid conflicts with metallb or other load balancers being used.

I specified "microshift-internal" as the name of the internal balancer service, but that could be anything really, was just the first thing that I thought of.

Let me know what you think 😄

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 openshift-eng/jira-lifecycle-plugin repository.

[dhellmann]

/jira refresh

[openshift-ci-robot]

@dhellmann: This pull request references Jira Issue OCPBUGS-30833, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.16.0) matches configured target version for branch (4.16.0)
• bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jogeo

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

[pacevedom]

/ok-to-test

[pacevedom]

Verified to be working by QE.
/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jdhirst, pacevedom

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [pacevedom]

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

[asood-rh]

/ocpbugs cc-qa

[asood-rh]

/label qe-approved

[openshift-ci-robot]

@jdhirst: This pull request references Jira Issue OCPBUGS-30833, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.16.0) matches configured target version for branch (4.16.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jogeo

In response to this:

Fixes #3136

Which issue(s) this PR addresses: #3136

Closes #3136

This is my idea for an option to implement the checking of loadBalancerClass in the internal load balancing service. It would avoid conflicts with metallb or other load balancers being used.

I specified "microshift-internal" as the name of the internal balancer service, but that could be anything really, was just the first thing that I thought of.

Let me know what you think 😄

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 openshift-eng/jira-lifecycle-plugin repository.

[asood-rh]

Details of the pre-merge testing can be found in JIRA ticket for the bug.

[openshift-ci]

@jdhirst: all tests passed!

Full PR test history. Your PR dashboard.

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.

[openshift-ci-robot]

@jdhirst: Jira Issue OCPBUGS-30833: All pull requests linked via external trackers have merged:

• openshift/microshift#3142

Jira Issue OCPBUGS-30833 has been moved to the MODIFIED state.

In response to this:

Fixes #3136

Which issue(s) this PR addresses: #3136

Closes #3136

This is my idea for an option to implement the checking of loadBalancerClass in the internal load balancing service. It would avoid conflicts with metallb or other load balancers being used.

I specified "microshift-internal" as the name of the internal balancer service, but that could be anything really, was just the first thing that I thought of.

Let me know what you think 😄

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 openshift-eng/jira-lifecycle-plugin repository.