[WIP] OCPBUGS-43692: Fix LB Type Defaulting with ProviderParameters Set

[gcs278]

WIP: Found more potential nil pointer dereferences.

In the case that alreadyAdmitted was true and the IngressController's spec had providerParameters set, setDefaultProviderParameters incorrectly forced the LB type to Classic.

This fix removes alreadyAdmitted as a parameter from setDefaultProviderParameters and completely prevents it from being invoked when alreadyAdmitted, preventing the update of defaulted values after an IngressController has been admitted.

Previously, setDefaultProviderParameters always ensured that specLB.ProviderParameters.AWS was initialized. Now that the function is conditionally invoked, an additional nil check has been added to handle cases when it's not called.

An unit test has been added for this scenario to prevent regression.

[openshift-ci-robot]

@gcs278: This pull request references Jira Issue OCPBUGS-43692, which is invalid:

• expected the bug to target the "4.18.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:

Most of the logic in setDefaultProviderParameters didn't execute if alreadyAdmitted is true. However, in the case alreadyAdmitted was true and the IngressController's spec had providerParameters set, setDefaultProviderParameters incorrectly forced the LB type to Classic.

There is no clear advantage to running
setDefaultProviderParameters when alreadyAdmitted is true. This fix removes alreadyAdmitted as a parameter from
setDefaultProviderParameters and instead prevents it from being invoked entirely when alreadyAdmitted is true.

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.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from gcs278. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[openshift-ci-robot]

@gcs278: This pull request references Jira Issue OCPBUGS-43692, 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.18.0) matches configured target version for branch (4.18.0)
• bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @lihongan

In response to this:

Most of the logic in setDefaultProviderParameters didn't execute if alreadyAdmitted is true. However, in the case alreadyAdmitted was true and the IngressController's spec had providerParameters set, setDefaultProviderParameters incorrectly forced the LB type to Classic.

There is no clear advantage to running setDefaultProviderParameters when alreadyAdmitted is true. This fix removes alreadyAdmitted as a parameter from setDefaultProviderParameters and instead prevents it from being invoked entirely when alreadyAdmitted is true.

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.

[openshift-ci-robot]

@gcs278: This pull request references Jira Issue OCPBUGS-43692, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @lihongan

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

In response to this:

In the case that alreadyAdmitted was true and the IngressController's spec had providerParameters set, setDefaultProviderParameters incorrectly forced the LB type to Classic.

This fix relocates the alreadyAdmitted check to the logic responsible for mutating the LB type, rather than applying it to the logic that determines the defaultLBType value.

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.

[openshift-ci-robot]

@gcs278: This pull request references Jira Issue OCPBUGS-43692, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @melvinjoseph86

In response to this:

WIP: Still working through issues.

In the case that alreadyAdmitted was true and the IngressController's spec had providerParameters set, setDefaultProviderParameters incorrectly forced the LB type to Classic.

This fix relocates the alreadyAdmitted check to the logic responsible for mutating the LB type, rather than applying it to the logic that determines the defaultLBType value.

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.

[gcs278]

I've spun my wheels a bit on a couple different ways to approach this problem. All are valid, but it depends on what approach we'd like to take with setDefaultProviderParameters. I'll document here for thoughts.

Option 1: Don't Invoke setDefaultProviderParameters when alreadyAdmitted=True

• Previously, when alreadyAdmitted was True, setDefaultProviderParameters didn't update any default values (as intended), but it did initialize empty structs in effectiveStrategy.loadBalancer
  • Later code in setDefaultPublishingStrategy depends on these initializations
• Initially I went this route, but then realized I created a couple of nil deference exceptions
• Seems like a change in the purpose of setDefaultProviderParameters now that it doesn't always ensure defaults.
• Solution: master...gcs278:cluster-ingress-operator:OCPBUGS-43692-lb-type-defaulting-providerParms_condition

Option 2: Fix setDefaultProviderParameters to not update LB Type when alreadyAdmitted=True

• The alreadyAdmitted check could be moved to here so that setDefaultProviderParameters never changes the LB Type after admission
  • However, the issue is that setDefaultProviderParameters still initializes lbs.ProviderParameters.AWS, and AWS.type is required, and the default value for AWS.type is "".
  • So, it's a bit confusing to have lbs.ProviderParameters.AWS.type as "" (empty string) in our code, which then needs to be protected against later in setDefaultPublishingStrategy, because it will try to update the status LB type to "" which causes an error. "" is not a valid AWS.type value.
  • I think our code should try to make sure our values are valid in the API (i.e. consistency with API) so future developers don't get confused.
• Solution: master...gcs278:cluster-ingress-operator:OCPBUGS-43692-lb-type-defaulting-providerParms_donotupdatelbtype

Option 3: Pass current LB Status into setDefaultProviderParameters and set effective LB Type to status when alreadyAdmitted=True

• The problem with Option 2, is that after admission LB Type needs to be whatever is configured in status, but in setDefaultProviderParameters we don't know the current value to place in AWS.type.
• This option solves that by passing in the LB status, so that setDefaultProviderParameters can use the current status values when the IC is already admitted.
• Solution: master...gcs278:cluster-ingress-operator:OCPBUGS-43692-lb-type-defaulting-providerParms_passinlbsstatus

I think I lean towards option 3 as the most "elegant" solution.

[openshift-ci]

@gcs278: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-operator-techpreview	82e4b34	link	false	/test e2e-aws-operator-techpreview
ci/prow/e2e-aws-ovn-single-node	82e4b34	link	false	/test e2e-aws-ovn-single-node
ci/prow/e2e-hypershift	82e4b34	link	true	/test e2e-hypershift
ci/prow/e2e-aws-ovn-serial	82e4b34	link	true	/test e2e-aws-ovn-serial
ci/prow/e2e-azure-ovn	82e4b34	link	false	/test e2e-azure-ovn
ci/prow/e2e-aws-ovn-techpreview	82e4b34	link	false	/test e2e-aws-ovn-techpreview
ci/prow/e2e-aws-ovn-upgrade	82e4b34	link	true	/test e2e-aws-ovn-upgrade
ci/prow/e2e-aws-ovn	82e4b34	link	true	/test e2e-aws-ovn
ci/prow/e2e-aws-gatewayapi	82e4b34	link	false	/test e2e-aws-gatewayapi
ci/prow/images	82e4b34	link	true	/test images
ci/prow/e2e-azure-operator	82e4b34	link	true	/test e2e-azure-operator
ci/prow/e2e-aws-operator	82e4b34	link	true	/test e2e-aws-operator

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