-
Notifications
You must be signed in to change notification settings - Fork 535
NE-1517: Set EIP for NLB Ingress controller. #1593
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
Conversation
bc40070 to
4b14bb9
Compare
|
@miheer: This pull request references NE-1517 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. DetailsIn 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. |
9e62c06 to
45ad01d
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.
My main concerns revolve around the expectation that updating EIP allocation IDs will automatically update an existing NLB. From my brief experimentation, it seems that the system only recognises these allocations, as specified by the annotation, at the time of creation. The document implies at several points that changing EIP allocation IDs is possible and that such changes will be reconciled with the existing ELB without necessitating its recreation. It would be beneficial to conduct further tests to determine if modifications to the annotation result in immediate updates on a functioning ELB.
| This is a feature request to enhance the IngressController API to be able to support static IPs during | ||
| - Install time | ||
| - Custom NLB ingress controller creation | ||
| - Reconfiguration of the router. |
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.
As elastic IP allocations can only be added at creation time, what does reconfiguration imply recreation?
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 don't understand how the router is involved at all. Should this say "Updating the IngressController"? Andy's question then applies.
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.
Updating the IngressController is accurate.
|
|
||
| - As an administrator, I want to provision EIPs and use them with an NLB IngressController. | ||
| - As a user or admin, I want to ensure EIPs are used with NLB on default router at install time. | ||
| - As a user, I want to reconfigure default router to use EIPs. |
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.
As per my previous comment: reconfiguration would have to be destruction followed by creation as elastic allocation IDs need to be done at creation time.
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.
Yes the workflow has changed. We will recreate the LB.
| eip-allocations: eipalloc-<redacted>,eipalloc-<redacted>,eipalloc-<redacted>,eipalloc-<redacted>,eipalloc-<redacted> | ||
| subnets: | ||
| - subnet-1 | ||
| - subnet-2 | ||
| - subnet-3 |
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 proposed design, will there be a check to ensure the count of EIP allocation IDs matches the number of public subnets? Without this, one might only discover that the ingress controller cannot deploy after much progress in the installation process...
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.
One option is we can add a field to mention the number of public subnets. Then using CEL at API level we could try to compare the number of eip-allocations they provided and the the number of public subnets
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.
Or I think we may put this as a Non-Goal for this EP.
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.
Andy said "in the installation process". Validation to ensure installation succeeds can be done by the installer.
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 added this point in the Open Questions
| # Please edit the object below. Lines beginning with a '#' will be ignored, | ||
| # and an empty file will abort the edit. If an error occurs while saving this file will be | ||
| # reopened with the relevant failures. | ||
| # |
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.
Nit: Maybe remove the noise?
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.
yes
| aws: | ||
| type: NLB | ||
| networkLoadBalancer: | ||
| eip-allocations: eipalloc-<redacted>,eipalloc-<redacted>,eipalloc-<redacted>,eipalloc-<redacted>,eipalloc-<redacted> |
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.
Could we update the example to use sequential numbers for the EIP allocation IDs? This would make it clearer. Suggesting: "eipalloc-12345, eipalloc-12346, eipalloc-12347, eipalloc-12348, eipalloc-12349". Ditto for the other usage where we use .
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.
yes
| - The API field for `eip-allocations` field in the Ingress Controller CR needs to be set as follows | ||
| in the file `operator/v1/types_ingress.go`: | ||
| ```go | ||
| // AWSNetworkLoadBalancerParameters holds configuration parameters for an | ||
| // AWS Network load balancer. | ||
| type AWSNetworkLoadBalancerParameters struct { | ||
| EIPAllocations EIPAllocations `json:"eip-allocations,omitempty"` | ||
| } | ||
|
|
||
| type EIPAllocations string | ||
|
|
||
| ``` |
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.
This appears to be duplication. Aren't the types also described above?
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.
We can do something like this
// AWSNetworkLoadBalancerParameters holds configuration parameters for an
// AWS Network load balancer.
type AWSNetworkLoadBalancerParameters struct {
EIPAllocations configv1.EIPAllocations `json:"eip-allocations,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.
In operator/v1/types_ingress.go we already have a type ProviderLoadBalancerParameters with an AWSLoadBalancerParameters. Is that where this is supposed to go? https://github.com/openshift/api/blob/master/operator/v1/types_ingress.go#L473
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.
not sure what you mean please check openshift/api@261093f#diff-891d2cdcb01603ef2a2c1bfb2f6ef32cf2acf8b1471871a5a6e27308d50050f9R641
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.
Okay, my confusion was because the struct was empty to begin with. I thought you were adding a new struct type. Btw, your api PR shows this as a slice:
type AWSNetworkLoadBalancerParameters struct {
EIPAllocations []configv1.EIPAllocations `json:"eipAllocations,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.
OK so I hope your question is answered.
| The admin sets the `eip-allocations` in the Ingress Controller CR. Then same process | ||
| as per point 1.c.ii and 1.c.iii is followed by the ingress operator. | ||
|
|
||
| #### 3. Update the existing EIP for the IngressController: |
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.
As previously mentioned, will changes to EIP allocations actually be reconciled after creation?
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.
@frobware we need to recreate the LB service.
| #### 3. Update the existing EIP for the IngressController: | ||
|
|
||
| When the admin changes the eip-allocations field in the Ingress Controller CR, the watch on the | ||
| `eip-allocations` field by the ingress operator scales a new deployment for the Ingress Controller CR. |
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.
Oh, so if you change the EIP allocations then you get a "brand new" ingresscontroller? That might be surprising as it will drop all traffic. Seems subtle.
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.
@frobware I need to test again. It looks like if the service is updated with the annotation the kcm should update the eips. https://github.com/brooksgarrett/kubernetes/blob/72dd54ec4856ccb7bd2b08258d95f3d0b932fb11/pkg/cloudprovider/providers/aws/aws.go#L4157
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.
It looks like https://github.com/openshift/enhancements/compare/45ad01df72b72174f9347fcb54179af4661cbd19..1137a8acba5a4c72ae496c34fa93b97dd18d3990 resolved this question by removing "scales a new deployment" and instead describing the workflow for recreating the service.
| in the Ingress Controller CR and then test if the Ingress Controller | ||
| with service type load balancer with the annotation `service.beta.kubernetes.io/aws-load-balancer-eip-allocations` | ||
| has value of `eip-allocations` field from the Ingress Controller CR was updated. | ||
|
|
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.
Is there a plan for testing when the EIP allocations are plumbed in the install-config.yaml?
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 will be unit tests in the installer.
|
|
||
| ## Upgrade / Downgrade Strategy | ||
|
|
||
| During upgrade, user should be able to populate the eip_allocations_field via installer |
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 EIP allocations are only reconciled at creation time can this work? The cluster is upgrading which implies it has a deployed ingresscontroller.
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.
Yes you are right. During the upgrade we won't have this option for populating the field. So, the cluster admin will have to set the eip-allocations field once the cluster is installed.
| - As an administrator, I want to provision EIPs and use them with an NLB IngressController. | ||
| - As a user or admin, I want to ensure EIPs are used with NLB on default router at install time. | ||
| - As a user, I want to reconfigure default router to use EIPs. | ||
| - As a user of OpenShift on AWS (ROSA), I want to use static IPs (and therefore AWS Elastic IPs) so that |
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.
These roles are ambiguous. Is the administrator or user in all cases the cluster administrator? If there are multiple roles, can you define them?
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.
OK. From my understanding we shall have only one user i.e the cluster-admin as he is the one who is responsible for creating routers.
| ### User Stories | ||
|
|
||
| - As an administrator, I want to provision EIPs and use them with an NLB IngressController. | ||
| - As a user or admin, I want to ensure EIPs are used with NLB on default router at install time. | ||
| - As a user, I want to reconfigure default router to use EIPs. | ||
| - As a user of OpenShift on AWS (ROSA), I want to use static IPs (and therefore AWS Elastic IPs) so that | ||
| I can configure appropriate firewall rules. | ||
| I want the default AWS Load Balancer that they use (NLB) for their router to use these EIPs. |
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.
Another one:
- As a cluster-admin who has purchased a block of public IPv4 addresses, I want to use these IP addresses, so that I can avoid Amazon's new charge for public IP addresses.
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.
thanks!
|
| - "" | ||
| --- | ||
|
|
||
| # Set AWS EIP For NLB Ingress Controller |
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.
Please define EIP and NLB at least once in this proposal.
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.
Yes added under "Proposal"
| ### Goals | ||
| - Users are able to use EIP for a NLB Ingress Controller. | ||
| - Users are able to create a NLB default Ingress Controller during install time with the EIPs specified during install. | ||
| - Any existing ingress controller should be able to be reconfigured to use EIPs. |
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.
As mentioned earlier, "reconfigure" should really be "update with restart", right?
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.
yes
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 see that https://github.com/openshift/enhancements/compare/1137a8acba5a4c72ae496c34fa93b97dd18d3990..79d098963268d49c7e4b7d64ffe1e25878c2b6c0 implemented this suggestion.
To me, "restart" implies that the router pod has to be restarted. How about this?
- The ELB of an existing IngressController should be able to be reconfigured to use specific EIPs by updating the IngressController CR and recreating the ELB. 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.
added with slight changes.
| In this enhancement we would be adding API fields in the installer and the ingress controller CRD | ||
| to set the AWS EIP for the NLB Ingress Controller. The cluster ingress operator will then create | ||
| the ingress controller and get the EIP assigned to the service LoadBalancer with the annotation | ||
| `service.beta.kubernetes.io/aws-load-balancer-eip-allocations` |
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.
Can you add a note about why the annotation is in kubernetes.io rather than openshift.io?
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.
ok
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 like this was addressed in https://github.com/openshift/enhancements/compare/1137a8acba5a4c72ae496c34fa93b97dd18d3990..79d098963268d49c7e4b7d64ffe1e25878c2b6c0. However, that change uses service.beta.kubernetes.io/aws-load-balancer-eip-allocations as the target of a link, which will not work. Better to use https://kubernetes.io/docs/reference/labels-annotations-taints/#service-beta-kubernetes-io-aws-load-balancer-eip-allocations as a documentation reference.
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.
yes done
| cluster. A cluster administrator also has the rights to modify the cluster level components. | ||
|
|
||
| #### Set EIP using an installer: | ||
| 1. Cluster administrator creates adds the following in the install-config.yaml |
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.
| 1. Cluster administrator creates adds the following in the install-config.yaml | |
| 1. Cluster administrator creates or adds the following in the install-config.yaml |
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.
updated in a different way
| 1. Shall we support updating EIP in the default Classic `IngressController` after installation ? | ||
| Please refer to [section](#updating-eip-in-the-default-classic-ingresscontroller-after-installation). |
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.
| 1. Shall we support updating EIP in the default Classic `IngressController` after installation ? | |
| Please refer to [section](#updating-eip-in-the-default-classic-ingresscontroller-after-installation). | |
| 1. Shall we support updating EIP in the default Classic `IngressController` after installation ? | |
| Yes. Please refer to [section](#updating-eip-in-the-default-classic-ingresscontroller-after-installation). |
| The load balancer would be created with no eipAllocations and the service annotation `service.beta.kubernetes.io/aws-load-balancer-eip-allocations` will be removed. | ||
| For more check [this](#cluster-admin-wants-operator-to-automatically-effectuate-the-load-balancer-service-for-updated-ingress-controller-eipallocations-) | ||
| AWS will randomly assign the NLB's IP addresses from the IP address pools available. |
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.
Rearrange the sentences:
| The load balancer would be created with no eipAllocations and the service annotation `service.beta.kubernetes.io/aws-load-balancer-eip-allocations` will be removed. | |
| For more check [this](#cluster-admin-wants-operator-to-automatically-effectuate-the-load-balancer-service-for-updated-ingress-controller-eipallocations-) | |
| AWS will randomly assign the NLB's IP addresses from the IP address pools available. | |
| The load balancer would be created with no eipAllocations and the service annotation `service.beta.kubernetes.io/aws-load-balancer-eip-allocations` will be removed. AWS will randomly assign the NLB's IP addresses from the IP address pools available. | |
| For more details, see [this](#cluster-admin-wants-operator-to-automatically-effectuate-the-load-balancer-service-for-updated-ingress-controller-eipallocations-) | |
| 5. Can you assign `eipAllocations` to an `IngressController` with scope `Internal` ? | ||
|
|
||
| No, eipAllocations can be provided only for an `IngressController` with scope `External`. | ||
| The default IC is set to [External](https://github.com/openshift/cluster-ingress-operator/blob/master/pkg/operator/operator.go#L476) |
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.
This link isn't correct, it doesn't point to scope.
| However, we will validate the `IngressController` CR at API using CEL to check if `eipAllocations` are provided only when `scope` is | ||
| set to `External`. |
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.
| However, we will validate the `IngressController` CR at API using CEL to check if `eipAllocations` are provided only when `scope` is | |
| set to `External`. | |
| The `IngressController` CR cannot be saved if `eipAllocations` are provided and `scope` is not | |
| set to `External`. |
| 3. **Future Possibilities**: Should the AWS CCM eventually supports reliably modifying subnets without deleting the | ||
| LB-type service, we can then easily adapt the Ingress Operator to immediately effectuate the `eipAllocations` and eliminate | ||
| the need for a cluster admin to delete the LB-type service. |
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.
This is not a drawback, and isn't related to immutability. It is really its own Alternative.
| 3. **Future Possibilities**: Should the AWS CCM eventually supports reliably modifying subnets without deleting the | |
| LB-type service, we can then easily adapt the Ingress Operator to immediately effectuate the `eipAllocations` and eliminate | |
| the need for a cluster admin to delete the LB-type service. | |
| ### Future Possibility based on changes to AWS CCM. | |
| Should the AWS CCM eventually supports reliably modifying subnets without deleting the | |
| LB-type service, we can then easily adapt the Ingress Operator to immediately effectuate the `eipAllocations` and eliminate the need for a cluster admin to delete the LB-type service. |
|
/remove-lifecycle-rotten Hi @miheer, I have completed a final review on this EP. It's mostly nits, but some areas need bigger changes. Afaik none of my suggestions would need to be propagated to the implementation. If/when you address my suggestions, can you please either put a 👍🏼 on each, or explain why you didn't accept the suggestion. That will make it easier for me to add the lgtm. |
|
/remove-lifecycle rotten |
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
|
/remove-lifecycle stale |
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
|
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
|
Rotten enhancement proposals close after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Reopen the proposal by commenting /close |
|
@openshift-bot: Closed this PR. DetailsIn 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 kubernetes-sigs/prow repository. |
|
/reopen |
|
@miheer: This pull request references NE-1517 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.19.0" version, but no target version was set. DetailsIn 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. |
|
@candita: Reopened this PR. DetailsIn 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 kubernetes-sigs/prow repository. |
|
Team decided to go ahead with this change, disregarding the final commentary that was not incorporated. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: candita The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@miheer: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
This EP describes how to set EIP for NLB ingress controller.
Epic link: https://issues.redhat.com/browse/NE-1274