-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
CORS-2902: capi/aws: add ext-LB as CAPA secondary LB #8149
Conversation
@r4f4: This pull request references CORS-2902 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. |
/uncc @AnnaZivkovic @andfasano |
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.
Hey Rafael. Some open questions, otherwise LGTM
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.
Suggestions in the rule names.
Update: addressed review comments. |
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.
@r4f4 I added this rule to the deployment considering the limitations in when syncing the rules for the SG shared in both LBs
Because it is always created. The ext-LB, when needed, will be added as a secondary LB.
Name: ptr.To(clusterID.InfraID + "-ext"), | ||
LoadBalancerType: capa.LoadBalancerTypeNLB, | ||
Scheme: &capa.ELBSchemeInternetFacing, | ||
CrossZoneLoadBalancing: true, |
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 need to enable cross zone load balancing on the primary as well, but we could do that in a follow up pr if preferred
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 we enable it only if there is more than 1 subnet setup?
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 terraform we always enable it. I'm going to enable in both and we can review that decision in a follow-up.
/approve |
/assign @vincepri |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon 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 |
CAPA does not apply rules specified only in the secondary load balancer. Add the rule to the primary LB instead, since both share the same security group.
For parity with our terraform configs.
/test e2e-aws-capi-ovn |
@r4f4: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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 kubernetes/test-infra repository. |
/test altinfra-e2e-aws-capi-ovn |
/retest-required |
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.
/lgtm
@r4f4: The following test 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/test-infra repository. I understand the commands that are listed here. |
/label acknowledge-critical-fixes-only |
|
@r4f4: Overrode contexts on behalf of r4f4: ci/prow/altinfra-e2e-aws-capi-ovn 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 kubernetes/test-infra repository. |
6b4523d
into
openshift:master
This PR makes the internal LB be the primary CAPA load balancer and then adds the external LB as a secondary one when using public endpoints.