Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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. #1148

Merged
merged 1 commit into from Oct 19, 2022

Conversation

miheer
Copy link
Contributor

@miheer miheer commented Jun 9, 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-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2022
@openshift-ci openshift-ci bot requested review from jsafrane and mfojtik June 9, 2022 06:24
@miheer miheer force-pushed the nlb-field branch 4 times, most recently from 1263516 to 7fb3b75 Compare June 9, 2022 08:58
@miheer miheer changed the title WIP: 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. 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. Jun 9, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2022
@miheer miheer force-pushed the nlb-field branch 2 times, most recently from 1016fac to 6617347 Compare June 15, 2022 10:25
@barbacbd
Copy link

/cc barbacbd

@openshift-ci openshift-ci bot requested a review from barbacbd June 16, 2022 12:11

#### Destroying cluster

- After destroying the cluster the NLB resources created in the AWS cluster will be deleted.

Choose a reason for hiding this comment

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

This isn't a change correct? The NLB resources should already be destroyed unless this is a difference resource. The Machine API creates an NLB for the cluster and those resources should be destroyed.

enhancements/installer/aws-load-balancer-type.md Outdated Show resolved Hide resolved
enhancements/installer/aws-load-balancer-type.md Outdated Show resolved Hide resolved
- "@frobware"
- "@sdodson"
approvers:
- "@wking"
Copy link
Member

@wking wking Jun 16, 2022

Choose a reason for hiding this comment

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

You probably want an installer approver here instead of me. Actually, probably one of the aws-approvers.

@miheer miheer force-pushed the nlb-field branch 3 times, most recently from 0f07ec6 to e15af82 Compare June 22, 2022 06:26
Comment on lines 135 to 219
For the either infra config cluster object or ingress config cluster object we will need to add api for AwsLBType 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.

If set it in the ingress object then the API will look as follows -
Code location for Option 1 and Option 2 will be [here](https://github.com/openshift/api/blob/master/config/v1/types_ingress.go)

Option 1:
```go
type IngressSpec struct {
// awsLBType is used to set type of the LB either to CLB/ELB or NLB for the
// default ingress controller in AWS environment.
// +optional
AwsLBType string `json:"awsLBType,omitempty"`

```

OR

Option 2:
```go
type IngressSpec struct {
// platformSpec holds desired information specific to the underlying
// infrastructure provider related to ingress routing.
PlatformSpec PlatformSpec `json:"platformSpec,omitempty"`

// AWS contains settings specific to the Amazon Web Services infrastructure provider
// related to ingress routing.
// +optional
AWS *AWSPlatformSpec `json:"aws,omitempty"`
}

// AWSPlatformSpec holds the desired state of the Amazon Web Services infrastructure provider
// related to ingress routing.
// This only includes fields that can be modified in the cluster.
type AWSPlatformSpec struct {
// LBType is used to set type of the LB either to CLB/ELB or NLB for the
// default ingress controller in AWS environment.
// +optional
LBType string `json:"lbType,omitempty"`
}

```

OR

Option 3:
If we set it in the infra then the API will look as follows -
Code will be [here](https://github.com/openshift/api/blob/63b58097a95d1b053cf038f56f89afc42524f598/config/v1/types_infrastructure.go#L351)
```go
// AWSPlatformSpec holds the desired state of the Amazon Web Services infrastructure provider.
// This only includes fields that can be modified in the cluster.
type AWSPlatformSpec struct {
// LBType is used to set type of the LB either to CLB/ELB or NLB for the
// default ingress controller in AWS environment.
// +optional
LBType string `json:"lbType,omitempty"`
}
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@barbacbd @patrickdillon @jstuever The main question regarding the API is as to where shall we add the LB type. Shall it be the infra cluster object or ingress cluster object ? In this enhancement I have proposed 3 options so which one would the installer prefer ? cc @Miciah

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 will be using infra object as it seems to have all the cloud related env. So closing this loop as I need to start implementing an urgent epic.

@miheer miheer force-pushed the nlb-field branch 4 times, most recently from 310a0fb to bcb5ab1 Compare July 4, 2022 08:17
@miheer
Copy link
Contributor Author

miheer commented Jul 7, 2022

/retest

1 similar comment
@miheer
Copy link
Contributor Author

miheer commented Jul 7, 2022

/retest

miheer added a commit to miheer/installer that referenced this pull request Jul 11, 2022
…sLBType 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 infrastructure 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/infrastructure.go`  The infrastructure 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/infrastructure_test.go` Unit test to test setting of lbType in the infrastructure 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

Jira ticket - https://issues.redhat.com/browse/NE-942
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.

Option 1:(Recommended as it seems to be having all the cloud related information.)
Copy link
Contributor

Choose a reason for hiding this comment

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

As an Installer reviewer, my vote is for Option 1 as it already seems to have Platform specific config in the the PlatformSpec and PlatformStatus types

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on openshift/installer#6074 and openshift/api#1209, it appears that the implementation is making progress with adding this field to the Infrastructure CR. Could you please update this proposal to reflect that and add Option 2 here to the "Althernatives" section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sadasu done

// Allowed values are nlb or elb
// +optional
LBType string `json:"lbType,omitempty"`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is an optional parameter, will clb/elb continue to be the default values?

Copy link
Contributor Author

@miheer miheer Jul 16, 2022

Choose a reason for hiding this comment

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

Right. If nothing is set then the ingress operator will create a clb/elb.

@Miciah
Copy link
Contributor

Miciah commented Sep 21, 2022

/assign

Copy link
Contributor

@Miciah Miciah left a comment

Choose a reason for hiding this comment

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

I have some suggested clarifications. In particular, I would like to make sure we are all in agreement regarding (and that no one is surprised by) my suggested clarifications for the goals and non-goals.

- "https://issues.redhat.com/browse/NE-942"
creation-date: 2022-06-09
last-updated: 2019-06-09
status: yet to be implemented
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
status: yet to be implemented
status: implementable

Comment on lines 57 to 60
- [ x ] Enhancement is `implementable`
- [ x ] Design details are appropriately documented from clear requirements
- [ x ] Test plan is defined
- [ x ] Graduation criteria for dev preview, tech preview, GA
Copy link
Contributor

Choose a reason for hiding this comment

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

[ x ] doesn't render as a checkbox, at least not in GitHub's rendering.

Suggested change
- [ x ] Enhancement is `implementable`
- [ x ] Design details are appropriately documented from clear requirements
- [ x ] Test plan is defined
- [ x ] Graduation criteria for dev preview, tech preview, GA
- [X] Enhancement is `implementable`
- [X] Design details are appropriately documented from clear requirements
- [X] Test plan is defined
- [X] Graduation criteria for dev preview, tech preview, GA

Comment on lines 65 to 68
The users would be able to specify the load balancer type either to elb or nlb during installation.
This will be used to deploy default ingress controller with user-specified load balancer service for OpenShift clusters
running on AWS.
Currently by default CLB/ELB is set by the cluster-ingress-operator.
Copy link
Contributor

Choose a reason for hiding this comment

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

Use consistent nomenclature: Classic and NLB. The user can already specify Classic ELB or NLB using a manifest; the key changes here with this enhancement are that now the user can specify the preference in the install-config, and the preference is stored in the cluster ingress config. Clarify that the preference affects both the default ingresscontroller as well as all other ingresscontrollers on which no preference is specified.

Suggested change
The users would be able to specify the load balancer type either to elb or nlb during installation.
This will be used to deploy default ingress controller with user-specified load balancer service for OpenShift clusters
running on AWS.
Currently by default CLB/ELB is set by the cluster-ingress-operator.
With this enhancement, the user can specify the AWS ELB type to be either Classic or NLB in the install-config, and this preference is stored in the cluster ingress config object.
This preference is used to deploy the default ingress controller with the user-specified load-balancer type for OpenShift clusters
running on AWS.
The install-time preference is also used for user-created ingress controllers on which no preference for load-balancer type is specified.
This enhancement does not change the default load-balancer type, which is Classic ELB.

The users would be able to specify the load balancer type either to elb or nlb during installation.
This will be used to deploy default ingress controller with user-specified load balancer service for OpenShift clusters
running on AWS.
Currently by default CLB/ELB is set by the cluster-ingress-operator.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this sentence mean? "Currently by default CLB/ELB is set by the cluster-ingress-operator." Are you referring to the default value for the new API field in the ingress config? Are you referring to defaulting logic that cluster-ingress-operator does if the value is empty?

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 enhancement does not change the default load-balancer type, which is Classic ELB. --Your suggestion looks good.


## Motivation

Currently the default load balancer type is set to CLB/ELB. One can set default load balancer during installation using [this](https://docs.openshift.com/container-platform/4.10/networking/configuring_ingress_cluster_traffic/configuring-ingress-cluster-traffic-aws-network-load-balancer.html#nw-aws-nlb-new-cluster_configuring-ingress-cluster-traffic-aws-network-load-balancer).
Copy link
Contributor

Choose a reason for hiding this comment

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

Links like "this" are annoying.

Suggested change
Currently the default load balancer type is set to CLB/ELB. One can set default load balancer during installation using [this](https://docs.openshift.com/container-platform/4.10/networking/configuring_ingress_cluster_traffic/configuring-ingress-cluster-traffic-aws-network-load-balancer.html#nw-aws-nlb-new-cluster_configuring-ingress-cluster-traffic-aws-network-load-balancer).
The default load-balancer type is Classic ELB. One can set the load-balancer type for the default ingress controller during installation [using custom installer manifests](https://docs.openshift.com/container-platform/4.10/networking/configuring_ingress_cluster_traffic/configuring-ingress-cluster-traffic-aws-network-load-balancer.html#nw-aws-nlb-new-cluster_configuring-ingress-cluster-traffic-aws-network-load-balancer).


### Test Plan

- Set the awsLBType field in the install-config.yaml either to nlb or elb and see if the default ingress controller gets created with the desired LB type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Set the awsLBType field in the install-config.yaml either to nlb or elb and see if the default ingress controller gets created with the desired LB type.
- Set the `lbType` field in the `install-config.yaml` either to "NLB" or "Classic" and see if the default ingress controller gets created with the desired LB type.

Comment on lines 292 to 295
- Set the awsLBType field in the install-config.yaml either to nlb or elb and see if the default ingress controller gets created with the desired LB type.
- Delete the default ingress controller CR and see if the default ingress controller will be created using the LB type mentioned during installation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these going to be E2E tests in openshift/installer or what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Manual test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW I have already written a unit test case in the installer as well.

Comment on lines 298 to 314
*This enhancement will follow standard graduation criteria.

#### Dev Preview -> Tech Preview

- Ability to utilize the enhancement end to end
- End user documentation, relative API stability
- Sufficient test coverage
- Gather feedback from users rather than just developers

#### Tech Preview -> GA

- Community Testing
- Sufficient time for feedback
- Upgrade, Downgrade and scale testing are not relevant to this enhancement

**For non-optional features moving to GA, the graduation criteria must include
end to end tests.**
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there really any stages to graduating this feature? We can probably put "N/A".

None

#### Failure Modes
None
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this would be a good place to put the possible failure you mentioned in the "Risks and Mitigations" section: 'Users need to put either "NLB" or "Classic" for lbType or omit the field or else the install-config will fail validation.'

Comment on lines 335 to 337
- Check the installer logs.
- Check the ingress operator logs.
- Check the events in the openshift-ingress and openshift-ingress-operator namespace.
Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't clear what this procedure is intended to accomplish. If the goal is to determine how the ingress operator is determining the default LB type, you could also suggest checking the cluster version or ingress operator version to make sure it is new enough to have the enhancement, using oc version or oc get clusteroperators/ingress -o yaml, and checking the LB type in the cluster ingress config, using oc get ingress.config.openshift.io/cluster -o yaml. You could also tell the user how to check the LB type that the operator specified on the service, by checking the appropriate annotations in oc -n openshift-ingress get svc/router-default -o yaml.

@miheer
Copy link
Contributor Author

miheer commented Sep 27, 2022

@Miciah as discussed for whether we shall support only default or all ingress controllers i will be sending an email to service delivery.

miheer added a commit to miheer/installer that referenced this pull request Sep 29, 2022
… 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

Jira ticket - https://issues.redhat.com/browse/NE-942
miheer added a commit to miheer/installer that referenced this pull request Sep 29, 2022
… 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

Jira ticket - https://issues.redhat.com/browse/NE-942
miheer added a commit to miheer/installer that referenced this pull request Oct 3, 2022
… 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

Jira ticket - https://issues.redhat.com/browse/NE-942
miheer added a commit to miheer/installer that referenced this pull request Oct 3, 2022
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
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2022
@miheer
Copy link
Contributor Author

miheer commented Oct 4, 2022

@Miciah PTAL

gcs278 pushed a commit to gcs278/installer that referenced this pull request Oct 11, 2022
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
gcs278 pushed a commit to gcs278/installer that referenced this pull request Oct 11, 2022
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
gcs278 pushed a commit to gcs278/installer that referenced this pull request Oct 11, 2022
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>

### Graduation Criteria

*This enhancement will follow standard graduation criteria.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean?

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 will say N/A

Comment on lines 333 to 337
If the goal is to determine how the ingress operator is determining the default LB type, you could also suggest checking the
cluster version or ingress operator version to make sure it is new enough to have the enhancement, using `oc version` or
`oc get clusteroperators/ingress -o yaml`, and checking the LB type in the cluster ingress config, using `oc get ingress.config.openshift.io/cluster -o yaml`.
You could also tell the user how to check the LB type that the operator specified on the service, by checking the appropriate
annotations in `oc -n openshift-ingress get svc/router-default -o yaml`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The wording here is confusing. It looks like you just copied and pasted my comment: #1148 (comment)

@miheer
Copy link
Contributor Author

miheer commented Oct 17, 2022

@Miciah PTAL

@miheer
Copy link
Contributor Author

miheer commented Oct 17, 2022

@Miciah PTAL

…onfig during installation to set the load balancer type in AWS either to NLB or ELB.
Comment on lines +340 to +341
- If the LBType is not correctly set in the ingress controller CR then please check the ingress operator logs.
- If the LBType is not correctly set in the service object then please check the kube-controller-manager logs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- If the LBType is not correctly set in the ingress controller CR then please check the ingress operator logs.
- If the LBType is not correctly set in the service object then please check the kube-controller-manager logs.
- If the LBType is not correctly set in the status of the ingresscontroller CR then please check the ingress operator logs: `oc -n openshift-ingress-operator logs -c ingress-operator deployments/ingress-operator`.
- If the LBType is not correctly set in the service object then please check the ingress operator logs.
- If the LBType is correctly set in the service object but the correct LB type is not provisioned then please check the kube-controller-manager logs: `oc -n openshift-kube-controller-manager logs -c kube-controller-manager -l app=kube-controller-manager --tail=-1`.

@Miciah
Copy link
Contributor

Miciah commented Oct 19, 2022

I see no major issues, none of the other reviewers have raised any concerns over my earlier suggestions, and the implementation PRs have already merged.
/approve
/lgtm

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

openshift-ci bot commented Oct 19, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah, 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:

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 19, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 19, 2022

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

@openshift-merge-robot openshift-merge-robot merged commit e635497 into openshift:master Oct 19, 2022
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants