Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make node outbound LB configurable #1229

Merged
merged 4 commits into from
Apr 8, 2021
Merged

Make node outbound LB configurable #1229

merged 4 commits into from
Apr 8, 2021

Conversation

shysank
Copy link
Contributor

@shysank shysank commented Mar 11, 2021 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:

Adds a new field LoadBalancerNodeOutboundIPs in AzureClusterSpec -> NetworkSpec that specifies the number of frontend ips to be created for node outbound load balancer.

Adds a separate section for node outbound LB configuration. This includes configuration to:

  • Control the number of frontend ips created as part of this LB
  • Disable node outbound LB for private clusters

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #541
Also fixes #1159 partially.

Special notes for your reviewer:

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Support multiple frontend IPs for node outbound lb

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 11, 2021
@k8s-ci-robot k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 11, 2021
@shysank shysank force-pushed the feature/541 branch 2 times, most recently from 0618a01 to 4f53b3b Compare March 11, 2021 14:27
@CecileRobertMichon
Copy link
Contributor

/assign @jackfrancis @mboersma

@k8s-ci-robot
Copy link
Contributor

@CecileRobertMichon: GitHub didn't allow me to assign the following users: jackfrancis.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @jackfrancis @mboersma

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.

jackfrancis
jackfrancis reviewed Mar 11, 2021
View reviewed changes
azure/scope/cluster.go Outdated Show resolved Hide resolved
jackfrancis
jackfrancis reviewed Mar 11, 2021
View reviewed changes
azure/scope/cluster.go Outdated Show resolved Hide resolved
@shysank shysank force-pushed the feature/541 branch 2 times, most recently from 7453ada to f757980 Compare March 16, 2021 04:30
@nader-ziada
Copy link
Contributor

/test pull-cluster-api-provider-azure-e2e

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 17, 2021
@shysank
Copy link
Contributor Author

shysank commented Mar 18, 2021

/retest

@shysank shysank force-pushed the feature/541 branch 2 times, most recently from 2b7508b to 8e2e541 Compare March 18, 2021 17:16
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2021
@shysank
Copy link
Contributor Author

shysank commented Mar 24, 2021

/retest

@shysank
Copy link
Contributor Author

shysank commented Mar 25, 2021

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2021
@CecileRobertMichon CecileRobertMichon mentioned this pull request Mar 30, 2021
Closed
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2021
@shysank
Copy link
Contributor Author

shysank commented Apr 6, 2021

/retest

CecileRobertMichon
CecileRobertMichon reviewed Apr 6, 2021
View reviewed changes
docs/book/src/topics/node-outbound-lb.md Outdated Show resolved Hide resolved
docs/book/src/topics/node-outbound-lb.md Outdated Show resolved Hide resolved
docs/book/src/topics/node-outbound-lb.md Show resolved Hide resolved
docs/book/src/topics/node-outbound-lb.md Show resolved Hide resolved
api/v1alpha4/azurecluster_default.go Show resolved Hide resolved
@shysank shysank changed the title Support multiple frontend IPs for node outbound lb Make node outbound LB configurable Apr 6, 2021
@shysank
Copy link
Contributor Author

shysank commented Apr 7, 2021

/test pull-cluster-api-provider-azure-e2e

CecileRobertMichon
CecileRobertMichon reviewed Apr 7, 2021
View reviewed changes
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/lgtm

great work @shysank, thanks for tackling this

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2021
@nader-ziada
Copy link
Contributor

experimented with this PR for a little bit and all looks good

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nader-ziada

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2021
@k8s-ci-robot k8s-ci-robot merged commit 7fab35e into kubernetes-sigs:master Apr 8, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.5.0 milestone Apr 8, 2021
@CecileRobertMichon CecileRobertMichon mentioned this pull request Apr 8, 2021
Merged
3 tasks
@CecileRobertMichon CecileRobertMichon mentioned this pull request Jul 6, 2021
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CecileRobertMichon CecileRobertMichon CecileRobertMichon left review comments

@nader-ziada nader-ziada nader-ziada left review comments

@jackfrancis jackfrancis jackfrancis left review comments

@cpanato cpanato Awaiting requested review from cpanato

Assignees

@mboersma mboersma

@nader-ziada nader-ziada

@CecileRobertMichon CecileRobertMichon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v0.5.0
6 participants
@shysank @CecileRobertMichon @k8s-ci-robot @nader-ziada @jackfrancis @mboersma