Skip to content

OCPBUGS-2436: Revert "Revert "[AWS] Add LB Type in the infrastructure cluster object via install-config yaml"" and fix OCPBUGS-2436#6491

Merged
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
Miciah:revert-revert-6478-nlb-field-and-fix-OCPBUGS-2436
Oct 19, 2022
Merged

OCPBUGS-2436: Revert "Revert "[AWS] Add LB Type in the infrastructure cluster object via install-config yaml"" and fix OCPBUGS-2436#6491
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
Miciah:revert-revert-6478-nlb-field-and-fix-OCPBUGS-2436

Conversation

@Miciah
Copy link
Contributor

@Miciah Miciah commented Oct 17, 2022

This commit reverts #6490 (which reverted #6478) and incorporates #6489 to fix the issue that necessitated the initial reversion.

Revert "Revert "[AWS] Add LB Type in the infrastructure cluster object via install-config yaml""

This reverts #6490.

Fix ingress config with empty lbType on AWS

When generating the ingresses.config.openshift.io/cluster manifest on AWS, always set spec.loadBalancer.platform.aws.type even if the install-config lbType is nonempty.

Before this PR, if lbType was empty, the installer generated a manifest with the following:

  loadBalancer:
    platform:
      aws: {}
      type: AWS

Whereas loadBalancer, platform, and aws are optional fields, the type subfield of the aws field is required. As a consequence, the cluster bootstrap would fail with the following error:

    "cluster-ingress-02-config.yml": failed to create ingresses.v1.config.openshift.io/cluster -n : Ingress.config.openshift.io "cluster" is invalid: spec.loadBalancer.platform.aws.type: Required value

This PR ensures that the installer doesn't generate an invalid ingress manifest when lbType is empty.

Follow-up to #6478.

  • pkg/asset/manifests/ingress.go (generateClusterConfig): Always set spec.loadBalancer.platform.aws.type when the platform is AWS.
  • pkg/asset/manifests/ingress_test.go (TestGenerateIngerssDefaultPlacement): Update test cases to expect spec.loadBalancer.platform.aws.type to default to "Classic" on AWS.

…t via install-config yaml""

This reverts commit 98beea1.
@openshift-ci openshift-ci bot requested review from barbacbd and rna-afk October 17, 2022 14:11
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there concern with leaving ingress.spec.LoadBalancer.Platform.Type as empty "" if the install config.AWS.LBType is not configured?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the const type defined is either Classic or NLB. So "" shall not be allowed to add in the ingressconfig for the LBType.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ingress operator should handle this case without issue, but we can set an explicit default to "Classic" if that makes more sense to do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you can't leave it as "" as only two values are allowed i.e Classic and NLB. I don't think setting Classic in Ingress.Spec.Loadbalancer is needed as it is optional.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question about handling the LBType not being set was originally raised here: fbd2ea0#r971049072.

What happens when the manifest is created with LBType value is set to "Classic" or "NLB" initially on day 0 and the customer updates just the LBType field to "" day 2 keeping everything else in the manifest the same? Does the ingress controller be updated to take care of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when the manifest is created with LBType value is set to "Classic" or "NLB" initially on day 0 and the customer updates just the LBType field to "" day 2 keeping everything else in the manifest the same? Does the ingress controller be updated to take care of this?

The ingress operator preserves the LB type that was specified or defaulted when the ingresscontroller was created.

@Miciah
Copy link
Contributor Author

Miciah commented Oct 17, 2022

#6490 has merged.
/test all

@stbenjam
Copy link
Member

/payload-job periodic-ci-openshift-release-master-nightly-4.12-e2e-aws-sdn-serial

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 17, 2022

@stbenjam: trigger 1 job(s) for the /payload-(job|aggregate) command

  • periodic-ci-openshift-release-master-nightly-4.12-e2e-aws-sdn-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/dee3c720-4e3b-11ed-8442-8598813a5e97-0

@Miciah Miciah changed the title Revert "Revert "[AWS] Add LB Type in the infrastructure cluster object via install-config yaml"" and fix OCPBUGS-2436 OCPBUGS-2436: Revert "Revert "[AWS] Add LB Type in the infrastructure cluster object via install-config yaml"" and fix OCPBUGS-2436 Oct 17, 2022
@openshift-ci-robot openshift-ci-robot added jira/severity-critical Referenced Jira bug's severity is critical for the branch this PR is targeting. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Oct 17, 2022
@openshift-ci-robot
Copy link
Contributor

@Miciah: This pull request references Jira Issue OCPBUGS-2436, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.12.0) matches configured target version for branch (4.12.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (shaozhan@redhat.com), skipping review request.

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

Details

In response to this:

This commit reverts #6490 (which reverted #6478) and incorporates #6489 to fix the issue that necessitated the initial reversion.

Revert "Revert "[AWS] Add LB Type in the infrastructure cluster object via install-config yaml""

This reverts #6490.

Fix ingress config with empty lbType on AWS

When generating the ingresses.config.openshift.io/cluster manifest on AWS, don't set spec.loadBalancer unless the install-config lbType is nonempty.

Before this PR, if lbType was empty, the installer generated a manifest with the following:

 loadBalancer:
   platform:
     aws: {}
     type: AWS

Whereas loadBalancer, platform, and aws are optional fields, the type subfield of the aws field is required. As a consequence, the cluster bootstrap would fail with the following error:

   "cluster-ingress-02-config.yml": failed to create ingresses.v1.config.openshift.io/cluster -n : Ingress.config.openshift.io "cluster" is invalid: spec.loadBalancer.platform.aws.type: Required value

This PR ensures that the installer doesn't generate an invalid ingress manifest when lbType is empty.

Follow-up to #6478.

  • pkg/asset/manifests/ingress.go (generateClusterConfig): Don't set spec.loadBalancer unless we have a nonempty lbType value to put in spec.loadBalancer.platform.aws.type.

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-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Oct 17, 2022
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shall check if it set to NLB or Classic

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there validation somewhere to make sure that the value of LBType when set is either "Classic" or "NLB"? If not, I think it should be added to make sure we are not passing also some random string within this manifest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the unit tests that I added sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they are sufficient given the latest implementation creates the manifest always with LBType values "Classic/NLB".

@patrickdillon
Copy link
Contributor

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2022
expectedIngressAWSLBType: configv1.Classic,
expectedIngressPlatformType: configv1.AWSPlatformType,
},
{
Copy link
Contributor

@sadasu sadasu Oct 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had originally requested an update to unit tests to test for all input values here: fbd2ea0#r971050233.

As a result a test was added for when the LBType is set to "Classic". Should there be a test case here for the case when the install-config contains "" as LBType for the scenario when the LBType value is not spcifically set by the customer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be covered in the latest push.

* "NLB": A Network Load Balancer that makes routing decisions
at the transport layer (TCP/SSL). See the following for additional
details: https://docs.aws.amazon.com/AmazonECS/latest/developerguide/load-balancer-types.html#nlb'
type: string
Copy link
Contributor

@sadasu sadasu Oct 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Text here is a little misleading to reviewers and potentially users. It says that Valid values are "Classic" and "NLB". Some suggestions: Can we say "LBType is an optional configuration that allows the user..." ? Can we talk about the unset case before the values that it can be set to?
Then we can talk about the values that are allowed when the value is actually set. Right now, we talk about behavior when set, then unset and values when it is set.
Also, can we use this opportunity to fix the nit about double spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a generated file? I don't see how we can fix the double spaces. I can add some text in pkg/types/aws/platform.go to say that this field is optional, in which case the default is "Classic".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the godoc and ran go1.19 generate ./... to regenerate data/data/install.openshift.io_installconfigs.yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just did another push in which I removed indentation for the list items in the godoc in order to prevent the generated CRD yaml from having those double-spaces.

@gcs278
Copy link

gcs278 commented Oct 17, 2022

@Miciah here's the unit test I came up with. Feel free to modify as you see fit:

diff --git a/pkg/asset/manifests/ingress_test.go b/pkg/asset/manifests/ingress_test.go
index c0a70bb1e..faa8ae655 100644
--- a/pkg/asset/manifests/ingress_test.go
+++ b/pkg/asset/manifests/ingress_test.go
@@ -60,6 +60,7 @@ func TestGenerateIngerssDefaultPlacement(t *testing.T) {
                expectedIngressPlacement    configv1.DefaultPlacement
                expectedIngressAWSLBType    configv1.AWSLBType
                expectedIngressPlatformType configv1.PlatformType
+               expectedIngressAWSSpecNil   bool
        }{
                {
                        // AWS currently uses a load balancer even on single-node, so the
@@ -111,6 +112,14 @@ func TestGenerateIngerssDefaultPlacement(t *testing.T) {
                        expectedIngressAWSLBType:    configv1.Classic,
                        expectedIngressPlatformType: configv1.AWSPlatformType,
                },
+               {
+                       name:                      "test setting of aws lb type not configured",
+                       installConfigBuildOptions: []icOption{icBuild.forAWS()},
+                       controlPlaneTopology:      configv1.HighlyAvailableTopologyMode,
+                       infrastructureTopology:    configv1.HighlyAvailableTopologyMode,
+                       expectedIngressPlacement:  configv1.DefaultPlacementWorkers,
+                       expectedIngressAWSSpecNil: true,
+               },
                {
                        name:                      "none-platform single node with 0 or 1 day-1 workers",
                        installConfigBuildOptions: []icOption{icBuild.forNone()},
@@ -174,6 +183,9 @@ func TestGenerateIngerssDefaultPlacement(t *testing.T) {
                                return
                        }
                        assert.Equal(t, tc.expectedIngressPlacement, actualIngress.Status.DefaultPlacement)
+                       if tc.expectedIngressAWSSpecNil {
+                               assert.Nil(t, actualIngress.Spec.LoadBalancer.Platform.AWS)
+                       }
                        if len(tc.expectedIngressAWSLBType) != 0 && len(tc.expectedIngressPlatformType) != 0 {
                                assert.Equal(t, tc.expectedIngressAWSLBType, actualIngress.Spec.LoadBalancer.Platform.AWS.Type)
                                assert.Equal(t, tc.expectedIngressPlatformType, actualIngress.Spec.LoadBalancer.Platform.Type)

@Miciah Miciah force-pushed the revert-revert-6478-nlb-field-and-fix-OCPBUGS-2436 branch 2 times, most recently from afcc85b to 8e7d76b Compare October 17, 2022 18:53
@openshift-ci-robot
Copy link
Contributor

@Miciah: This pull request references Jira Issue OCPBUGS-2436, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.12.0) matches configured target version for branch (4.12.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (shaozhan@redhat.com), skipping review request.

Details

In response to this:

This commit reverts #6490 (which reverted #6478) and incorporates #6489 to fix the issue that necessitated the initial reversion.

Revert "Revert "[AWS] Add LB Type in the infrastructure cluster object via install-config yaml""

This reverts #6490.

Fix ingress config with empty lbType on AWS

When generating the ingresses.config.openshift.io/cluster manifest on AWS, always set spec.loadBalancer.platform.aws.type even if the install-config lbType is nonempty.

Before this PR, if lbType was empty, the installer generated a manifest with the following:

 loadBalancer:
   platform:
     aws: {}
     type: AWS

Whereas loadBalancer, platform, and aws are optional fields, the type subfield of the aws field is required. As a consequence, the cluster bootstrap would fail with the following error:

   "cluster-ingress-02-config.yml": failed to create ingresses.v1.config.openshift.io/cluster -n : Ingress.config.openshift.io "cluster" is invalid: spec.loadBalancer.platform.aws.type: Required value

This PR ensures that the installer doesn't generate an invalid ingress manifest when lbType is empty.

Follow-up to #6478.

  • pkg/asset/manifests/ingress.go (generateClusterConfig): Always set spec.loadBalancer.platform.aws.type when the platform is AWS.
  • pkg/asset/manifests/ingress_test.go (TestGenerateIngerssDefaultPlacement): Update test cases to expect spec.loadBalancer.platform.aws.type to default to "Classic" on AWS.

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.

@Miciah Miciah force-pushed the revert-revert-6478-nlb-field-and-fix-OCPBUGS-2436 branch 2 times, most recently from b3a79da to ebb7219 Compare October 17, 2022 19:15
When generating the ingresses.config.openshift.io/cluster manifest on AWS,
always set spec.loadBalancer.platform.aws.type even if the install-config
lbType is nonempty.

Before this commit, if lbType was empty, the installer generated a manifest
with the following:

  loadBalancer:
    platform:
      aws: {}
      type: AWS

Whereas loadBalancer, platform, and aws are optional fields, the type
subfield of the aws field is required.  As a consequence, the cluster
bootstrap would fail with the following error:

    "cluster-ingress-02-config.yml": failed to create ingresses.v1.config.openshift.io/cluster -n : Ingress.config.openshift.io "cluster" is invalid: spec.loadBalancer.platform.aws.type: Required value

This commit ensures that the installer doesn't generate an invalid ingress
manifest when lbType is empty.

Follow-up to commit 5d12adc.

This commit fixes bug OCPBUGS-2436.

https://issues.redhat.com/browse/OCPBUGS-2436

* pkg/asset/manifests/ingress.go (generateClusterConfig): Always set
spec.loadBalancer.platform.aws.type when the platform is AWS.
* pkg/asset/manifests/ingress_test.go
(TestGenerateIngerssDefaultPlacement): Update test cases to expect
spec.loadBalancer.platform.aws.type to default to "Classic" on AWS.
* pkg/types/aws/platform.go (Platform): Update godoc to make it clear that
LBType is optional and defaults to "Classic".
* pkg/explain/printer_test.go (Test_PrintFields): Update.
* data/data/install.openshift.io_installconfigs.yaml: Regenerate.
@Miciah Miciah force-pushed the revert-revert-6478-nlb-field-and-fix-OCPBUGS-2436 branch from ebb7219 to 1a71334 Compare October 17, 2022 20:08
@Miciah
Copy link
Contributor Author

Miciah commented Oct 17, 2022

Test_PrintFields required an update. The indentation for lbType is weird there, but it isn't uniquely weird.

@sadasu
Copy link
Contributor

sadasu commented Oct 17, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2022
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 045b952 and 2 for PR HEAD 1a71334 in total

@miheer
Copy link
Contributor

miheer commented Oct 18, 2022

issue does not seem related to PR so will /retest-required

@miheer
Copy link
Contributor

miheer commented Oct 18, 2022

/retest-required

1 similar comment
@miheer
Copy link
Contributor

miheer commented Oct 18, 2022

/retest-required

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 927dd43 and 1 for PR HEAD 1a71334 in total

@mfojtik
Copy link
Contributor

mfojtik commented Oct 18, 2022

@Miciah @wking this is affecting merges in different repos now as it makes e2e-serial-aws and e2e-upgrade fail permanently... can we speed up reverting this? i don't care if we use the button here :-)

cc @deads2k

@sadasu
Copy link
Contributor

sadasu commented Oct 18, 2022

@Miciah @wking this is affecting merges in different repos now as it makes e2e-serial-aws and e2e-upgrade fail permanently... can we speed up reverting this? i don't care if we use the button here :-)

cc @deads2k

@mfojtik The PR to revert has already merged #6490. So, CIs 2e-serial-aws and e2e-upgrade should be passing. This PR is adding the original changes (in #6478) back in with some fixes.

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 53766db and 0 for PR HEAD 1a71334 in total

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 18, 2022

@Miciah: 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-ovn-fips 1a71334 link false /test e2e-aws-ovn-fips
ci/prow/e2e-ibmcloud-ovn 1a71334 link false /test e2e-ibmcloud-ovn
ci/prow/okd-scos-e2e-aws-ovn 1a71334 link false /test okd-scos-e2e-aws-ovn
ci/prow/e2e-aws-ovn-imdsv2 1a71334 link false /test e2e-aws-ovn-imdsv2
ci/prow/okd-scos-e2e-aws-upgrade 1a71334 link false /test okd-scos-e2e-aws-upgrade
ci/prow/okd-e2e-aws-upgrade 1a71334 link false /test okd-e2e-aws-upgrade
ci/prow/e2e-aws-ovn-proxy 1a71334 link false /test e2e-aws-ovn-proxy
ci/prow/e2e-aws-ovn-single-node 1a71334 link false /test e2e-aws-ovn-single-node
ci/prow/e2e-aws-ovn-shared-vpc 1a71334 link false /test e2e-aws-ovn-shared-vpc
ci/prow/okd-e2e-aws-ovn 1a71334 link false /test okd-e2e-aws-ovn

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.

@openshift-ci-robot
Copy link
Contributor

/hold

Revision 1a71334 was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2022
@miheer
Copy link
Contributor

miheer commented Oct 18, 2022

/retest-required

@sadasu
Copy link
Contributor

sadasu commented Oct 18, 2022

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2022
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD d853fa5 and 2 for PR HEAD 1a71334 in total

@openshift-merge-robot openshift-merge-robot merged commit 5a9ad4c into openshift:master Oct 19, 2022
@openshift-ci-robot
Copy link
Contributor

@Miciah: All pull requests linked via external trackers have merged:

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

Details

In response to this:

This commit reverts #6490 (which reverted #6478) and incorporates #6489 to fix the issue that necessitated the initial reversion.

Revert "Revert "[AWS] Add LB Type in the infrastructure cluster object via install-config yaml""

This reverts #6490.

Fix ingress config with empty lbType on AWS

When generating the ingresses.config.openshift.io/cluster manifest on AWS, always set spec.loadBalancer.platform.aws.type even if the install-config lbType is nonempty.

Before this PR, if lbType was empty, the installer generated a manifest with the following:

 loadBalancer:
   platform:
     aws: {}
     type: AWS

Whereas loadBalancer, platform, and aws are optional fields, the type subfield of the aws field is required. As a consequence, the cluster bootstrap would fail with the following error:

   "cluster-ingress-02-config.yml": failed to create ingresses.v1.config.openshift.io/cluster -n : Ingress.config.openshift.io "cluster" is invalid: spec.loadBalancer.platform.aws.type: Required value

This PR ensures that the installer doesn't generate an invalid ingress manifest when lbType is empty.

Follow-up to #6478.

  • pkg/asset/manifests/ingress.go (generateClusterConfig): Always set spec.loadBalancer.platform.aws.type when the platform is AWS.
  • pkg/asset/manifests/ingress_test.go (TestGenerateIngerssDefaultPlacement): Update test cases to expect spec.loadBalancer.platform.aws.type to default to "Classic" on AWS.

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.

@lihongan
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. jira/severity-critical Referenced Jira bug's severity is critical for the branch this PR is targeting. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants