Skip to content

Add annotation for cross zone lb - #1971

Closed
enxebre wants to merge 1 commit into
openshift:mainfrom
enxebre:cross-zone-lb
Closed

enxebre wants to merge 1 commit into
openshift:mainfrom
enxebre:cross-zone-lb

Conversation

@enxebre

@enxebre enxebre commented Dec 20, 2022

Copy link
Copy Markdown
Member

What this PR does / why we need it:
HypershiftAWSLoadBalancerCrossZoneLoadBalancingEnabled applies https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.1/guide/service/annotations/ service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled in the load balancer services of the hosted control plane endpoints. AWS Private link requires endpoint and service endpoints to exist in the same underlying zone. To ensure that requirement is satisfied in Regions with more than 3 zones, managed services create subnets in all of them. That and having this enabled in the load balancers would make the private link communication to always succeed.

ref https://issues.redhat.com/browse/HOSTEDCP-670

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

@enxebre

enxebre commented Dec 20, 2022

Copy link
Copy Markdown
Member Author

/hold

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2022
@openshift-ci
openshift-ci Bot requested review from csrwng and sjenning December 20, 2022 15:13
@openshift-ci

openshift-ci Bot commented Dec 20, 2022

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 20, 2022
HypershiftAWSLoadBalancerCrossZoneLoadBalancingEnabled applies https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.1/guide/service/annotations/
service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled in the load balancer services of the hosted control plane endpoints.
AWS Private link requires endpoint and service endpoints to exist in the same underlying zone. To ensure that requirement is satisfied in Regions with more than 3 zones, managed services create subnets in all of them. That and having this enabled in the load balancers would make the private link communication to always succeed.
apiServerService := manifests.KubeAPIServerService(hcp.Namespace)
if _, err := createOrUpdate(ctx, r.Client, apiServerService, func() error {
return kas.ReconcileService(apiServerService, serviceStrategy, p.OwnerReference, p.APIServerPort, p.AllowedCIDRBlocks, util.IsPublicHCP(hcp))
return kas.ReconcileService(apiServerService, serviceStrategy, p.OwnerReference, p.APIServerPort, p.AllowedCIDRBlocks, util.IsPublicHCP(hcp), hcp)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like passing the whole hcp down. Can we just pass a bool like CrossZoneLoadBalancingEnabled?

@sjenning

Copy link
Copy Markdown
Contributor

Is there any reason we wouldn't want to set this all the time?

@openshift-ci

openshift-ci Bot commented Dec 21, 2022

Copy link
Copy Markdown
Contributor

@enxebre: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws 7ccab01 link true /test e2e-aws
ci/prow/e2e-ibmcloud-roks 7ccab01 link false /test e2e-ibmcloud-roks
ci/prow/e2e-ibmcloud-iks 7ccab01 link false /test e2e-ibmcloud-iks

Full PR test history. Your PR dashboard.

Details

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.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2023
@openshift-merge-robot

Copy link
Copy Markdown
Contributor

@enxebre: PR needs rebase.

Details

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.

@enxebre

enxebre commented Jan 9, 2023

Copy link
Copy Markdown
Member Author

close by #2004

@enxebre enxebre closed this Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants