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
NE-1516: Adds a field in the cluster ingress config object and Ingress Controller to set EIP via installer and Ingress Controller #1826
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this | ||
name: "Ingress" | ||
crdName: ingresses.config.openshift.io | ||
featureGate: SetEIPForNLBIngressController | ||
tests: | ||
onCreate: | ||
- name: Should not allow to set NLB parameters when LBType is Classic. | ||
initial: | | ||
apiVersion: config.openshift.io/v1 | ||
kind: Ingress | ||
metadata: | ||
name: cluster | ||
spec: | ||
loadBalancer: | ||
platform: | ||
type: AWS | ||
aws: | ||
type: Classic | ||
nlbParameters: | ||
eipAllocations: | ||
- eipalloc-12345 | ||
- eipalloc-12346 | ||
expectedError: "Network load balancer parameters are allowed only when load balancer type is NLB." | ||
- name: Should allow to set NLB parameters when LBType is NLB. | ||
initial: | | ||
apiVersion: config.openshift.io/v1 | ||
kind: Ingress | ||
metadata: | ||
name: cluster | ||
spec: | ||
loadBalancer: | ||
platform: | ||
type: AWS | ||
aws: | ||
type: NLB | ||
nlbParameters: | ||
eipAllocations: | ||
- eipalloc-12345 | ||
- eipalloc-12346 | ||
expected: | | ||
apiVersion: config.openshift.io/v1 | ||
kind: Ingress | ||
metadata: | ||
name: cluster | ||
spec: | ||
loadBalancer: | ||
platform: | ||
type: AWS | ||
aws: | ||
type: NLB | ||
nlbParameters: | ||
eipAllocations: | ||
- eipalloc-12345 | ||
- eipalloc-12346 |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -130,6 +130,7 @@ type LoadBalancer struct { | |||||
|
||||||
// AWSIngressSpec holds the desired state of the Ingress for Amazon Web Services infrastructure provider. | ||||||
// This only includes fields that can be modified in the cluster. | ||||||
// +kubebuilder:validation:XValidation:rule="self.type != 'Classic' || !has(self.nlbParameters)",message="Network load balancer parameters are allowed only when load balancer type is NLB." | ||||||
// +union | ||||||
type AWSIngressSpec struct { | ||||||
// type allows user to set a load balancer type. | ||||||
|
@@ -151,8 +152,34 @@ type AWSIngressSpec struct { | |||||
// +kubebuilder:validation:Enum:=NLB;Classic | ||||||
// +kubebuilder:validation:Required | ||||||
Type AWSLBType `json:"type,omitempty"` | ||||||
|
||||||
// networkLoadBalancerParameters holds configuration parameters for an AWS | ||||||
// network load balancer. Present only if type is NLB. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please capitalize "Network Load Balancer" correctly here and elsewhere.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That might be a general use of the phrase "network load balancer", which I saw alot in the EP. If it is an object, it should be networkLoadBalancer, all one word, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the phrase is used in the general sense, then it needs to be defined. I know what a "load balancer" is, but I don't know what a "network load balancer" is (or why the distinction would be relevant here), so I assumed that Miheer meant "Network Load Balancer" (AKA NLB). |
||||||
// | ||||||
// +optional | ||||||
NetworkLoadBalancerParameters *AWSNetworkLoadBalancerParameters `json:"nlbParameters,omitempty"` | ||||||
} | ||||||
|
||||||
// AWSNetworkLoadBalancerParameters holds configuration parameters for an | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you link to where the list parameters is? Can they set anything? Are there any formating requirements? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hi @deads2k ! sorry I did not get this point. Can you please explain more on this ? |
||||||
// AWS Network load balancer. For Example: Setting AWS EIPs https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/elastic-ip-addresses-eip.html | ||||||
type AWSNetworkLoadBalancerParameters struct { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @miheer we already have an identical type in https://github.com/openshift/api/blob/master/operator/v1/types_ingress.go#L516. Can we choose one or the other? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Miciah let me know that we need both of these, but there may be a way to not declare them twice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are two different APIs. So, we can't. |
||||||
// You can assign Elastic IP addresses to the Network Load Balancer by adding the following annotation. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The whole point of this API is the the cluster-admin should not use the annotation. |
||||||
// The number of Allocation IDs must match the number of subnets that are used for the load balancer. | ||||||
// See https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/elastic-ip-addresses-eip.html for general | ||||||
// information about configuration, characteristics, and limitations of Elastic IP addresses. | ||||||
// The EIPs in the cluster config are only used when the operator creates the default IngressController. | ||||||
// It isn't used as a default value when eipAllocations is not specified on the IngressController. | ||||||
// +openshift:enable:FeatureGate=SetEIPForNLBIngressController | ||||||
// +optional | ||||||
// +listType=atomic | ||||||
// +kubebuilder:validation:XValidation:rule=`self.all(x, self.exists_one(y, x == y))`,message="EIP allocations cannot contain duplicates" | ||||||
// +kubebuilder:validation:MaxItems=10 | ||||||
EIPAllocations []EIPAllocation `json:"eipAllocations"` | ||||||
} | ||||||
|
||||||
// EIPAllocation is a reference to available eip allocations in AWS environment. | ||||||
type EIPAllocation string | ||||||
|
||||||
type AWSLBType string | ||||||
|
||||||
const ( | ||||||
|
Large diffs are not rendered by default.
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.
Add CEL to enforce this.
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.
done