Skip to content

🐛 Fix nodes outbound connectivity with SLB #674

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
Merged
k8s-ci-robot merged 1 commit into from
Jun 5, 2020
Merged

🐛 Fix nodes outbound connectivity with SLB #674

k8s-ci-robot merged 1 commit into from
Jun 5, 2020
+522 −226

Conversation

@CecileRobertMichon
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon commented Jun 3, 2020 •
edited
Loading

What this PR does / why we need it: This implements outbound connectivity for all nodes as described in https://github.com/kubernetes-sigs/cloud-provider-azure/tree/master/docs/services#standard-loadbalancer. For each Azure cluster, a new node outbound public IP and public Load Balancer will be created. The Load Balancer's name is the cluster name so that cloud provider can detect the LB exists and reuse the same LB for exposing services. This LB will be created with a backend pool and each node machine (all VMs that are not control-planes) will be added to this backend pool on its creation. The LB will also have outbound rules defined to allow UDP and TCP outbound traffic from all nodes, as described in Azure docs.

See also:
https://docs.microsoft.com/en-us/azure/load-balancer/load-balancer-outbound-connections

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Partially addresses #648

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Fix nodes outbound connectivity with Standard Load Balancer

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 3, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 3, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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

@k8s-ci-robot k8s-ci-robot requested review from juan-lee and serbrech June 3, 2020 17:59
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 3, 2020
@CecileRobertMichon CecileRobertMichon force-pushed the provider-slb branch 2 times, most recently from 6ac2b43 to ab26509 Compare June 3, 2020 18:34
CecileRobertMichon
CecileRobertMichon commented Jun 3, 2020
View reviewed changes
cloud/services/publicloadbalancers/publicloadbalancers.go
type Spec struct {
Name string
PublicIPName string
Role string
Copy link
Contributor Author

@CecileRobertMichon CecileRobertMichon Jun 3, 2020

Choose a reason for hiding this comment

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

@jsturtevant please review if you can as these changes will affect #646 (positively, I hope)

Copy link
Contributor

@jsturtevant jsturtevant Jun 3, 2020

Choose a reason for hiding this comment

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

Looks like a nice improvement 👍

jsturtevant
jsturtevant reviewed Jun 3, 2020
View reviewed changes
@CecileRobertMichon CecileRobertMichon mentioned this pull request Jun 3, 2020
Closed
devigned
devigned approved these changes Jun 3, 2020
View reviewed changes
Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

Looks good.

Left a general comment about resource names we might want to think about.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 3, 2020 •
edited
Loading

@CecileRobertMichon: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cluster-api-provider-azure-apidiff 244677a link /test pull-cluster-api-provider-azure-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@CecileRobertMichon
Copy link
Contributor Author

CecileRobertMichon commented Jun 3, 2020

/hold

I found a potential bug, investigating...

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2020
@CecileRobertMichon
Copy link
Contributor Author

CecileRobertMichon commented Jun 4, 2020

ok so the bug is that cloud-provider can't find the VMSS instances to add them to the backend pool because host name != instance name, not related to this PR (it's already in master). I will work on a fix separately, this can go in as-is for now since it fixes the other issue.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 4, 2020
@CecileRobertMichon
Copy link
Contributor Author

CecileRobertMichon commented Jun 4, 2020

The apidiff failure is due to:

sigs.k8s.io/cluster-api-provider-azure/cloud/services/scalesets
  Compatible changes:
  - Service.PublicLoadBalancersClient: added
  - Spec.PublicLoadBalancerName: added
sigs.k8s.io/cluster-api-provider-azure/cloud/services/networkinterfaces
  Compatible changes:
  - Spec.MachineRole: added
sigs.k8s.io/cluster-api-provider-azure/api/v1alpha3
  Incompatible changes:
  - APIServerRoleTagValue: removed
  - BastionRoleTagValue: removed
  - CommonRoleTagValue: removed
  - PrivateRoleTagValue: removed
  - PublicRoleTagValue: removed
  Compatible changes:
  - APIServerRole: added
  - BastionRole: added
  - CommonRole: added
  - NodeOutboundRole: added
  - PrivateRole: added
  - PublicRole: added
sigs.k8s.io/cluster-api-provider-azure/cloud/services/publicloadbalancers
  Compatible changes:
  - Spec.Role: added
sigs.k8s.io/cluster-api-provider-azure/cloud
  Compatible changes:
  - GenerateNodeOutboundIPName: added

No API breaking changes.

@CecileRobertMichon
Copy link
Contributor Author

CecileRobertMichon commented Jun 4, 2020

fixed the other issue in #680

devigned
devigned approved these changes Jun 5, 2020
View reviewed changes
Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3944724 into kubernetes-sigs:master Jun 5, 2020
@k8s-ci-robot k8s-ci-robot added this to the v0.5 milestone Jun 5, 2020
@CecileRobertMichon CecileRobertMichon mentioned this pull request Jun 18, 2020
Closed
@CecileRobertMichon CecileRobertMichon deleted the provider-slb branch March 19, 2021 17:55
willie-yao pushed a commit to willie-yao/cluster-api-provider-azure that referenced this pull request Jul 11, 2025
@DannyBrito
improve mcr check error message (kubernetes-sigs#674)
0647121
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jackfrancis jackfrancis jackfrancis left review comments

@juan-lee juan-lee Awaiting requested review from juan-lee

@serbrech serbrech Awaiting requested review from serbrech

+2 more reviewers

@jsturtevant jsturtevant jsturtevant left review comments

@devigned devigned devigned approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@devigned devigned

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. 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.4.x

Development

Successfully merging this pull request may close these issues.

5 participants

@CecileRobertMichon @k8s-ci-robot @devigned @jsturtevant @jackfrancis