NE-1516: Adds a field in the cluster ingress config object and Ingress Controller to set EIP via installer and Ingress Controller #1826
Conversation
|
@miheer: This pull request references NE-1516 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
Hello @miheer! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: miheer The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@miheer: This pull request references NE-1516 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
config/v1/types_ingress.go
Outdated
| @@ -151,8 +151,22 @@ 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.
Add CEL to enforce this.
config/v1/types_ingress.go
Outdated
| // AWSNetworkLoadBalancerParameters holds configuration parameters for an | ||
| // AWS Network load balancer. | ||
| type AWSNetworkLoadBalancerParameters struct { | ||
| EIPAllocations EIPAllocations `json:"eip-allocations,omitempty"` |
There was a problem hiding this comment.
What does this do? Why is it plural, but only has a single value? Why would I want to set it?
There was a problem hiding this comment.
https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.7/guide/service/annotations/#eip-allocations
service.beta.kubernetes.io/aws-load-balancer-eip-allocations specifies a list of elastic IP address configuration for an internet-facing NLB.
So, when the eip-allocations field is set in the Ingress Controller CR which is comma separated value of type string, we set the value of the service of load balancer type with service annotation mentioned above.
Do you think we should specify this as an array and then seperate the value in the ingress controller based on the delimiter comma ?
| // network load balancer. Present only if type is NLB. | ||
| // | ||
| // +optional | ||
| NetworkLoadBalancerParameters *AWSNetworkLoadBalancerParameters `json:"networkLoadBalancer,omitempty"` |
There was a problem hiding this comment.
Add a featuregate to this field. It's done by
- add a featuregate like https://github.com/openshift/api/blob/master/config/v1/feature_gates.go#L182-L188
- add a
// +openshift:enable:FeatureGate=Examplecomment to this type - run
hack/update-payload-featuregates.sh - run
make update
This will add the schema to TechPreviewNoUpgrade (files now auto-generated). Once your automated tests are passing or QE signs off, a PR enabling by default can flip the gate to true ref
There was a problem hiding this comment.
OK. I need to try this. Thanks!
|
/assign @Miciah |
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
added
size/XXL
openshift-merge-robot
removed
the
needs-rebase
miheer
force-pushed
the
eip
branch
2 times, most recently
from
a8e5e98
to
f6536ae
Compare
config/v1/feature_gates.go
Outdated
| reportProblemsToJiraComponent("Routing"). | ||
| contactPerson("miheer"). | ||
| productScope(ocpSpecific). | ||
| enableIn(TechPreviewNoUpgrade). |
There was a problem hiding this comment.
There's now also a DevPreview you probably want to enable it on. DevPreviewNoUpgrade, TechPreviewNoUpgrade
| } | ||
|
|
||
| // AWSNetworkLoadBalancerParameters holds configuration parameters for an |
There was a problem hiding this comment.
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.
hi @deads2k ! sorry I did not get this point. Can you please explain more on this ?
config/v1/types_ingress.go
Outdated
| // AWS Network load balancer. | ||
| // +openshift:enable:FeatureGate=SetEIPForNLBIngressController | ||
| type AWSNetworkLoadBalancerParameters struct { | ||
| EIPAllocations []EIPAllocations `json:"eipAllocations,omitempty"` |
config/v1/types_ingress.go
Outdated
| } | ||
|
|
||
| // AWSNetworkLoadBalancerParameters holds configuration parameters for an | ||
| // AWS Network load balancer. | ||
| // +openshift:enable:FeatureGate=SetEIPForNLBIngressController |
There was a problem hiding this comment.
this probably more appropriately belongs on either networkLoadBalancer above or eipAllocations below. I'd probably choose the eipAllocations
miheer
force-pushed
the
eip
branch
2 times, most recently
from
7be17c6
to
24253ce
Compare
| } | ||
|
|
||
| // AWSNetworkLoadBalancerParameters holds configuration parameters for an | ||
| // 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.
@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.
@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.
They are two different APIs. So, we can't.
config/v1/types_ingress.go
Outdated
| type AWSNetworkLoadBalancerParameters struct { | ||
| // You can assign Elastic IP addresses to the Network Load Balancer by adding the following annotation. | ||
| // The number of Allocation IDs must match the number of subnets that are used for the load balancer. | ||
| // service.beta.kubernetes.io/aws-load-balancer-eip-allocations: eipalloc-xxxxxxxxxxxxxxxxx,eipalloc-yyyyyyyyyyyyyyyyy |
There was a problem hiding this comment.
Must the EIP allocation ids have eipalloc- prefix? Any other restrictions on formatting, length, or uniqueness?
There was a problem hiding this comment.
The annotation should not be mentioned at all.
config/v1/types_ingress.go
Outdated
| // https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/elastic-ip-addresses-eip.html | ||
| // https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.7/guide/service/annotations/#eip-allocations | ||
| // +openshift:enable:FeatureGate=SetEIPForNLBIngressController | ||
| EIPAllocations []EIPAllocations `json:"eipAllocations"` |
There was a problem hiding this comment.
Consider adding a maximum slice length, and enforce with CEL.
There was a problem hiding this comment.
No need for CEL; just use +kubebuilder:validation:MaxItems.
config/v1/types_ingress.go
Outdated
| // You can assign Elastic IP addresses to the Network Load Balancer by adding the following annotation. | ||
| // The number of Allocation IDs must match the number of subnets that are used for the load balancer. | ||
| // service.beta.kubernetes.io/aws-load-balancer-eip-allocations: eipalloc-xxxxxxxxxxxxxxxxx,eipalloc-yyyyyyyyyyyyyyyyy | ||
| // https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/elastic-ip-addresses-eip.html |
There was a problem hiding this comment.
I would not point people to this AWS url unless we support all of the actions described there. Just summarize the parts our users need to know.
E.g., do we support "Associate an Elastic IP address with an instance or network interface"?
There was a problem hiding this comment.
Providing a URL is fine, but only if the godoc explains why a URL is mentioned. If you tell me a URL but don't tell me why you told me that URL, I will ignore it. For example, you can say something like, "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."
There was a problem hiding this comment.
Agreed, but if this AWS url ends up in OpenShift docs, I predict it will cause more questions than it answers.
There was a problem hiding this comment.
I think it is fine to reference the AWS documentation if it is relevant. We already reference it here:
api/operator/v1/types_ingress.go
Lines 513 to 535 in 8203151
There are also references to documentation on amazon.com in the Machine API, Ingress config API, DNS config API, and Infrastructure config API, and there are references to alibabacloud.com, cloud.google.com, cloud.ibm.com, docker.com, kubernetes.io and k8s.io, microsoft.com, mozilla.com, and vmware.com documentation throughout OpenShift API definitions. It makes sense to be judicious about linking to non-Red Hat documentation, but as an integrator, we inevitably need to reference external documentation.
| // AWSNetworkLoadBalancerParameters holds configuration parameters for an | ||
| // 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 { | ||
| // You can assign Elastic IP addresses to the Network Load Balancer by adding the following annotation. |
There was a problem hiding this comment.
The whole point of this API is the the cluster-admin should not use the annotation.
config/v1/types_ingress.go
Outdated
| // The number of Allocation IDs must match the number of subnets that are used for the load balancer. | ||
| // service.beta.kubernetes.io/aws-load-balancer-eip-allocations: eipalloc-xxxxxxxxxxxxxxxxx,eipalloc-yyyyyyyyyyyyyyyyy | ||
| // https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/elastic-ip-addresses-eip.html | ||
| // https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.7/guide/service/annotations/#eip-allocations |
There was a problem hiding this comment.
The aws-load-balancer-controller documentation is irrelevant; ELBs for ingresscontrollers are handled by cloud-controller-manager and cloud-provider-aws. Please avoid linking to the aws-load-balancer-controller documentation as it has been the source of much confusion in the past.
config/v1/types_ingress.go
Outdated
| EIPAllocations []EIPAllocations `json:"eipAllocations"` | ||
| } | ||
|
|
||
| type EIPAllocations string |
There was a problem hiding this comment.
This definition is still missing godoc.
config/v1/types_ingress.go
Outdated
| @@ -151,8 +152,28 @@ 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.
Please capitalize "Network Load Balancer" correctly here and elsewhere.
| // network load balancer. Present only if type is NLB. | |
| // Network Load Balancer. This field is present only if the type field is set to NLB. |
There was a problem hiding this comment.
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.
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).
operator/v1/types_ingress.go
Outdated
| @@ -544,7 +546,7 @@ type AWSLoadBalancerParameters struct { | |||
| // network load balancer. Present only if type is NLB. | |||
| // | |||
| // +optional | |||
| NetworkLoadBalancerParameters *AWSNetworkLoadBalancerParameters `json:"networkLoadBalancer,omitempty"` | |||
| NetworkLoadBalancerParameters *AWSNetworkLoadBalancerParameters `json:"nlbParameters,omitempty"` | |||
There was a problem hiding this comment.
This API field is already in released versions of OpenShift. You cannot change the field name.
There was a problem hiding this comment.
Oh, that's my fault. I asked for this change. Sorry @miheer.
config/v1/types_ingress.go
Outdated
| // https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/elastic-ip-addresses-eip.html | ||
| // https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.7/guide/service/annotations/#eip-allocations | ||
| // +openshift:enable:FeatureGate=SetEIPForNLBIngressController | ||
| EIPAllocations []EIPAllocations `json:"eipAllocations"` |
There was a problem hiding this comment.
I think you are going to get pinged by API team on not having // +listType=<type> tag. See https://github.com/kubernetes/kube-openapi/blob/master/pkg/idl/doc.go. Atomic is generally safe bet.
@JoelSpeed can confirm.
There was a problem hiding this comment.
This is a list of strings, I assume each item in the list should be unique right? Will the list have one actor or multiple? I think we probably want this to be atomic and to enforce uniqueness using CEL
Thinking about this which caused issues before with the set listType
There was a problem hiding this comment.
I assume each item in the list should be unique right? Yes
There was a problem hiding this comment.
List type atomic on this and a CEL validation for self.all(x, self.exists_one(y, x == y))
| @@ -633,8 +635,15 @@ type AWSClassicLoadBalancerParameters struct { | |||
| } | |||
|
|
|||
| // AWSNetworkLoadBalancerParameters holds configuration parameters for an | |||
| // AWS Network load balancer. | |||
| // AWS Network load balancer. For Example: Setting AWS EIPs https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/elastic-ip-addresses-eip.html | |||
There was a problem hiding this comment.
This "for example" and including a link seems quite odd to me for this type of GO doc. We don't list examples in AWSClassicLoadBalancerParameters. It seems a bit inconsistent.
There was a problem hiding this comment.
OK but I thought we should add some details like what can be added in this type.
config/v1/types_ingress.go
Outdated
| EIPAllocations []EIPAllocations `json:"eipAllocations"` | ||
| } | ||
|
|
||
| type EIPAllocations string |
There was a problem hiding this comment.
Calling this the same name as the field type is confusing to me. I don't think this should be plural, it is a singular EIPAllocation.
| type EIPAllocations string | |
| type EIPAllocation string |
|
After rebasing with master I am getting this issue after while running
|
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
@miheer Most of the failures are because you are adding a gated field to an existing struct, thus splitting a single CRD into multiple, this is normal and expected, violations are present already. However, we do ask when folks hit this, to resolve any SSA tags for lists that are missing, you'll see a few of these in the result there. Your options are Please can you review the existing SSA tag failures and see if you can resolve those at least |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
@JoelSpeed I am getting the following error even when I have mentioned maxItems for EIPAllocations. Can you please help on this ?
|
…s Controller to set EIP via installer and Ingress Controller `config/v1/types_ingress.go` - Adds an API field `eip-allocations` in the cluster ingress config object whose value is set by the installer using install-config.yaml `operator/v1/types_ingress.go` - Adds an API field `eip-allocations` in the Ingress Controller CR object using which the Ingress Operator scales an Ingress Controller with service type balancer whose annotation `service.beta.kubernetes.io/aws-load-balancer-eip-allocations` is set by the value of the field `eip-allocations` of the Ingress Controller. Epic: https://issues.redhat.com/browse/NE-1274 Story: https://issues.redhat.com/browse/NE-1516
@JoelSpeed The errors don't seem to be related to my API. So, do you want me to make changes to different APIs ? |
|
@miheer: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
|
@miheer I created https://issues.redhat.com/browse/OCPBUGS-34906 to resolve the unrelated-to-this-PR verify-crd-schema failures. I think @JoelSpeed is amenable overriding since we are tracking this work elsewhere, but correct me if I'm wrong @JoelSpeed. |
| } | ||
|
|
||
| // EIPAllocation is a reference to available eip allocations in AWS environment. | ||
| type EIPAllocation string |
There was a problem hiding this comment.
I'd recommend reviewing some of the decisions for validation made in #1841.
For example:
// AWSSubnetID is a reference to an AWS subnet ID.
// +kubebuilder:validation:MinLength=24
// +kubebuilder:validation:MaxLength=24
// +kubebuilder:validation:Pattern=`^subnet-[0-9A-Za-z]+$`
type AWSSubnetID string
At a minimum, you need to make sure commas , won't make it into the annotation, because that will be invalid since , is used as the annotation deliminator.
Adds a field in the cluster ingress config object and Ingress Controller to set EIP via installer and Ingress Controller
config/v1/types_ingress.go- Adds an API fieldeip-allocationsin the cluster ingress config object whose value is set by the installer using install-config.yamloperator/v1/types_ingress.go- Adds an API fieldeip-allocationsin the Ingress Controller CR object using which the Ingress Operator scales an Ingress Controller with service type balancer whose annotationservice.beta.kubernetes.io/aws-load-balancer-eip-allocationsis set by the value of the fieldeip-allocationsof the Ingress Controller.Epic: https://issues.redhat.com/browse/NE-1274
Story: https://issues.redhat.com/browse/NE-1516