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
Adds Load Balancer Type to IngressController API #672
Conversation
An alternative approach would be to introduce fields for platform-specific settings: // AWSLoadBalancerType is the type of load balancer to instantiate.
// +kubebuilder:validation:Enum=Classic;NLB
type AWSLoadBalancerType string
var (
AWSClassicLoadBalancer AWSLoadBalancerType = "Classic"
AWSNetworkLoadBalancer AWSLoadBalancerType = "NLB"
)
type AWSLoadBalancerParameters struct {
Type AWSLoadBalancerType `json:"type,omitempty"`
}
type LoadBalancerStrategy struct {
AWS *AWSLoadBalancerParameters `json:"type,omitempty"`
// ...
} We could add fields for other platforms as needed (or add fields with empty struct types as placeholders). This approach has the following advantages:
spec:
endpointPublishingStrategy:
type: LoadBalancerService
loadBalancer:
aws:
type: NLB instead of spec:
endpointPublishingStrategy:
type: LoadBalancerService
loadBalancer:
type: AWSNetworkLB
What do you think? |
@Miciah commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good other than the one small thing I noted.
f5059c7
to
833cbd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I have a couple questions that maybe @openshift/api-reviewers can answer.
operator/v1/types_ingress.go
Outdated
// +kubebuilder:validation:Enum=Classic;NLB | ||
type AWSLoadBalancerType string | ||
|
||
var ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be const
? We have been inconsistent in our use of var
versus const
. We could use some guidance here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so, commit 61d6d39
implements this change.
operator/v1/types_ingress.go
Outdated
// If unspecified, "Classic" is used. | ||
// | ||
// +optional | ||
Type AWSLoadBalancerType `json:"type,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this field is optional and named "type", does that leave open the option of changing it to a union discriminator in the future? I'm not sure whether or not that is something we are likely to need, but I'd like to know whether it leaves us the option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 61d6d39
makes AWSLoadBalancerParameters
a union type with the Type
field as the union discriminator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the examples I found when searching for "idiomatic golang enum declaration" use const
and not var
.
// +unionDiscriminator | ||
// +kubebuilder:validation:Required | ||
// +required | ||
Type AWSLoadBalancerType `json:"type"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make this a union discriminator now, do we need to go ahead and add classic
and nlb
fields with empty struct types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Miciah commit f2c24f2
adds classic
and nlb
fields with empty struct types.
// type. | ||
// | ||
// +optional | ||
AWS *AWSLoadBalancerParameters `json:"aws,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the cloud specific nature of this, a structure that includes something like https://github.com/openshift/api/blob/master/config/v1/types_infrastructure.go#L131 may be in order.
So
type LoadBalancerStrategy struct {
Scope
PlatformLoadBalancerParameters PlatformLoadBalancerParameters
}
type PlatformLoadBalancerParameters struct{
Type PlatformType `json:"type"`
AWS *AWSLoadBalancerParameters `json:"aws,omitempty"`
}
my apologies if you had this before and I didn't pay attention to the aws-y nature of this.
operator/v1/types_ingress.go
Outdated
// +unionDiscriminator | ||
// +kubebuilder:validation:Required | ||
// +required | ||
Type PlatformType `json:"type"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, type
needs to be allowed to be empty on non-AWS platforms, and we may want to add additional allowed values to PlatformType
in the future (because we may want to add parameters for additional platforms). Do these definitions allow for this? Probably a question for @deads2k again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d9a83e9
to
247cc76
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the spirit of #672 (comment)...
/lgtm |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danehans, deads2k, Miciah 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 |
Some cloud providers support multiple types of load balancers. This PR adds the
ProviderParameters
field to theIngressController
API type to express provider-specific load balancer settings to use when instantiating an ingress controller.Supports NE-349
/assign @Miciah @knobunc
/cc @frobware @sgreene570