LBType field for AWS in infra cluster object. #1209

miheer · 2022-06-22T05:44:09Z

In the ingress config cluster object we will need to add api for LBType for AWS which will store and persist the information of the desired LB type mentioned by the installer config API so that the ingress operator will refer the LB type from this object whenever someone accidentally deletes the ingress controller. Currently, the default LB type i.e CLB/ELB gets set if you delete the ingress controller object even when the desired the LB type for default ingress controller was set to NLB as ingress operator does not have a way to check the LB type as we don't store it.

Enhancement Proposal - openshift/enhancements#1148

openshift-ci · 2022-06-22T05:44:16Z

Hello @miheer! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard lgtm and approved labels this repository requires either:

bugzilla/valid-bug - applied if your PR references a valid bugzilla bug

OR

qe-approved, docs-approved, and px-approved - these labels can be applied by anyone in the openshift org via the /label command.

Who should apply these qe/docs/px labels?

For a no-Feature-Freeze team who is merging a feature before code freeze, they need to get those labels applied to their api repo PR by the appropriate teams (i.e. qe, docs, px)
For a Feature Freeze (traditional) team who is merging a feature before FF, they can self-apply the labels (via /label commands), they are basically irrelevant for those teams
For a Feature Freeze team who is merging a feature after FF, the PR should be rejected barring an exception

gcs278 · 2022-07-20T17:17:54Z

/assign @rfredette

miheer · 2022-07-25T14:13:50Z

@rfredette PTAL. I need to get this merged ASAP as the epic has high priority. Thanks in advance!

rfredette · 2022-07-26T20:26:28Z

/lgtm

JoelSpeed · 2022-07-28T14:22:58Z

config/v1/types_infrastructure.go

 	ServiceEndpoints []AWSServiceEndpoint `json:"serviceEndpoints,omitempty"`
+
+	// LBType allows user to set a load balancer type.
+	// Allowed values are nlb or elb


Allowed values must be PascalCase to follow conventions, NLB and ELB

JoelSpeed · 2022-07-28T14:24:25Z

config/v1/types_infrastructure.go

+	// LBType allows user to set a load balancer type.
+	// Allowed values are nlb or elb
+	// +optional
+	LBType string `json:"lbType,omitempty"`


You should create an enum rather than a plain string, ie a typed string, which has some exported constants with the possible values,

Given you're adding some load balancer configuration, is there any change the user may want to be able to configure any of the load balancer options, ELB or NLB at some point in the future?

I think this probably wants to end up as a discriminated union with configuration for both types of load balancer

JoelSpeed · 2022-07-28T14:24:43Z

config/v1/types_infrastructure.go

+	// LBType allows user to set a load balancer type.
+	// Allowed values are nlb or elb
+	// +optional
+	LBType string `json:"lbType,omitempty"`


Why do you need this in the status as well as the spec?

miheer · 2022-07-29T03:58:58Z

@JoelSpeed PTAL. I have made the changes you asked for.

JoelSpeed · 2022-08-01T09:09:41Z

config/v1/types_infrastructure.go

+
+	// LBType allows user to set a load balancer type.
+	// Allowed values are NLB or Classic
+	// +kubebuilder:validation:Enum:=NLB;Classic


For backwards compatibility you need to allow an empty value too

Suggested change

// +kubebuilder:validation:Enum:=NLB;Classic

// +kubebuilder:validation:Enum:=NLB;Classic;""

JoelSpeed · 2022-08-01T09:10:27Z

config/v1/types_infrastructure.go

 	// +optional
 	ServiceEndpoints []AWSServiceEndpoint `json:"serviceEndpoints,omitempty"`
+
+	// LBType allows user to set a load balancer type.


You need to explain what happens when this field isn't set. You also need to explain what would happen if the field is changed (if that is allowed) while the cluster is running.

Reading the enhancement, this seems it's the default but can be overriden right? That will need to be explained in this godoc

Hi @JoelSpeed ! I have added the description for what happens when this value is set and not set.

@JoelSpeed PTAL. I have added the description as to what will happen when the value is set and not set.

// When this field is set the default ingresscontroller will get created using the specified LBType. // If this field is not set then the default ingress controller of LBType Classic will be created.

JoelSpeed · 2022-08-03T14:16:35Z

config/v1/types_infrastructure.go

+
+	// LBType allows user to set a load balancer type.
+	// When this field is set the default ingresscontroller will get created using the specified LBType.
+	// If this field is not set then the default ingress controller of LBType Classic will be created.


I think we need to make a note that if this is changed while the cluster is running that the existing default ingress controller will not be updated

The ingress operator will check this value at certain interval.

JoelSpeed · 2022-08-03T14:18:38Z

config/v1/types_infrastructure.go

+	// Allowed values are NLB or Classic
+	// +kubebuilder:validation:Enum:=NLB;Classic;""
+	// +optional
+	LBType AWSLBType `json:"lbType,omitempty"`


I don't think we should abbreviate the field name here, I also wonder if we want to make a struct of options rather than just adding an individual field.

Perhaps instead we want it to look something like

ingress: loadBalancer: defaultType: NLB

Then we have the option to group other relevant options we might need in the future

miheer · 2022-08-04T00:36:50Z

@JoelSpeed based on our discussions it looks like the right place is ingress cluster object and not the infrastructure cluster object. The default ingresscontroller can have either an internal or external LB. However, The API uses a separate load balancer.
As you mentioned ->
"Should this be on the infrastructure object? Infrastructure to me implies things around the control plane, so i'd expect the internal and external control plane load balancers to be here, but, if this is a workload load balancer, maybe it's better suited somewhere else?"
over here openshift/enhancements#1148 (comment)

I think ingress cluster object seems to be the right place.

I have made the necessary changes. PTAL.

Copying @sadasu from the installler team as she had voted for this field to be added in the infrastructure object.

miheer · 2022-08-04T01:54:01Z

/retest

openshift-ci · 2022-08-30T10:46:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, JoelSpeed, miheer, rfredette

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~~ [deads2k]

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

JoelSpeed · 2022-08-30T15:31:36Z

/hold cancel

miheer · 2022-08-31T00:06:18Z

@ahardin-rh can you please docs-approved this PR ?
@lihongan can you please qe-approved this PR ?

lihongan · 2022-08-31T01:36:24Z

/label qe-approved

CFields651 · 2022-08-31T12:32:00Z

/label px-approved

ahardin-rh · 2022-08-31T16:34:11Z

/label docs-approved

openshift-ci · 2022-08-31T17:02:00Z

@miheer: 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.

In the ingress config cluster object we need to add api for AWS LBType which will store and persist the information of the desired LB type mentioned by the installer config API so that the ingress operator will refer the LB type from this object whenever someone accidentally deletes the ingress controller. Currently, the default LB type i.e CLB/ELB gets set if you delete the ingress controller object even when the desired the LB type for default ingress controller was set to NLB as ingress operator does not have a way to check the LB type as we don't store it. This commit adds LB Type for AWS mentioned in the install-config.yaml in the ingress cluster object. `data/data/install.openshift.io_installconfigs.yaml` A generated new field called lbType will be available to add load balancer type in the install-config.yaml for AWS `pkg/asset/manifests/ingress.go` The ingress cluster object will be patched with this lbType provided by user so that we have the information related to the lbType persistent which can be referred by ingress operator when the default ingress controller is deleted. `pkg/asset/manifests/ingress_test.go` Unit test to test setting of lbType in the ingress cluster object's spec and status as per the value specified in the install-config.yaml `pkg/explain/printer_test.go` A new field to test called lbType which will be available to add the load balancer type in the install-config.yaml for AWS `pkg/types/aws/platform.go` A new field called lbType will be available to add the load balancer type in the install-config.yaml for AWS Enhacement proposal - openshift/enhancements#1148 OpenShift API - openshift/api#1209 Fixes Jira ticket - https://issues.redhat.com/browse/NE-942

In the ingress config cluster object we need to add api for AWS LBType which will store and persist the information of the desired LB type mentioned by the installer config API so that the ingress operator will refer the LB type from this object whenever someone accidentally deletes the ingress controller. Currently, the default LB type i.e CLB/ELB gets set if you delete the ingress controller object even when the desired the LB type for default ingress controller was set to NLB as ingress operator does not have a way to check the LB type as we don't store it. This commit adds LB Type for AWS mentioned in the install-config.yaml in the ingress cluster object. `data/data/install.openshift.io_installconfigs.yaml` A generated new field called lbType will be available to add load balancer type in the install-config.yaml for AWS `pkg/asset/manifests/ingress.go` The ingress cluster object will be patched with this lbType provided by user so that we have the information related to the lbType persistent which can be referred by ingress operator when the default ingress controller is deleted. `pkg/asset/manifests/ingress_test.go` Unit test to test setting of lbType in the ingress cluster object's spec and status as per the value specified in the install-config.yaml `pkg/explain/printer_test.go` A new field to test called lbType which will be available to add the load balancer type in the install-config.yaml for AWS `pkg/types/aws/platform.go` A new field called lbType will be available to add the load balancer type in the install-config.yaml for AWS Enhacement proposal - openshift/enhancements#1148 OpenShift API - openshift/api#1209 Fixes Jira ticket - https://issues.redhat.com/browse/NE-942 Modified-by: Grant Spence <gspence@redhat.com>

openshift-ci bot requested review from eparis and jwforres June 22, 2022 05:44

miheer force-pushed the nlb branch from 6257016 to 52c7839 Compare June 22, 2022 05:44

miheer force-pushed the nlb branch from 52c7839 to a251c79 Compare July 4, 2022 06:32

miheer mentioned this pull request Jul 11, 2022

[AWS] Add LB Type in the infrastructure cluster object via install-config yaml openshift/installer#6074

Closed

sadasu mentioned this pull request Jul 12, 2022

This proposal describes how we would add a new field to the install-config during installation to set the load balancer type in AWS either to NLB or ELB. openshift/enhancements#1148

Merged

miheer force-pushed the nlb branch from a251c79 to a15f6bd Compare July 16, 2022 13:11

miheer force-pushed the nlb branch from a15f6bd to 21d3ef5 Compare July 17, 2022 00:58

openshift-ci bot assigned rfredette Jul 20, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2022

JoelSpeed reviewed Jul 28, 2022

View reviewed changes

miheer force-pushed the nlb branch from 21d3ef5 to a28b425 Compare July 29, 2022 03:50

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2022

miheer force-pushed the nlb branch from a28b425 to e89ff7c Compare July 29, 2022 03:58

JoelSpeed reviewed Aug 1, 2022

View reviewed changes

miheer force-pushed the nlb branch from e89ff7c to b2e72f1 Compare August 2, 2022 05:57

JoelSpeed reviewed Aug 3, 2022

View reviewed changes

miheer force-pushed the nlb branch from b2e72f1 to 60faa1e Compare August 4, 2022 00:30

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2022

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Aug 31, 2022

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Aug 31, 2022

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Aug 31, 2022

openshift-merge-robot merged commit 1858b75 into openshift:master Aug 31, 2022

Miciah mentioned this pull request Sep 26, 2022

NE-799: config/ingress: "loadbalancer" → "loadBalancer" #1306

Merged

gcs278 mentioned this pull request Oct 11, 2022

[AWS] Add LB Type in the infrastructure cluster object via install-config yaml openshift/installer#6478

Merged

gcs278 mentioned this pull request Oct 12, 2022

NE-975: Update openshift/api for updated ingress config loadBalancer fields openshift/cluster-config-operator#268

Merged

	// +kubebuilder:validation:Enum:=NLB;Classic
	// +kubebuilder:validation:Enum:=NLB;Classic;""

LBType field for AWS in infra cluster object. #1209

LBType field for AWS in infra cluster object. #1209

Uh oh!

Conversation

miheer commented Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jun 22, 2022

Uh oh!

gcs278 commented Jul 20, 2022

Uh oh!

miheer commented Jul 25, 2022

Uh oh!

rfredette commented Jul 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miheer commented Jul 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miheer Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miheer commented Aug 4, 2022

Uh oh!

miheer commented Aug 4, 2022

Uh oh!

openshift-ci bot commented Aug 30, 2022

Uh oh!

JoelSpeed commented Aug 30, 2022

Uh oh!

miheer commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lihongan commented Aug 31, 2022

Uh oh!

CFields651 commented Aug 31, 2022

Uh oh!

ahardin-rh commented Aug 31, 2022

Uh oh!

openshift-ci bot commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

miheer commented Jun 22, 2022 •

edited

Loading

miheer Aug 2, 2022 •

edited

Loading

miheer commented Aug 31, 2022 •

edited

Loading