Add Private Link support#6274
Conversation
Co-authored-by: Nikola Prokopić <nikola@prokopic.rs> Signed-off-by: Robin Ketelbuters <robin.k@giantswarm.io>
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @robinkb! |
k8s-ci-robot
added
size/XXL
|
Hi @robinkb. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
/ok-to-test Thanks for reviving this :) FYI stale bot closed the issue #3400, probably a good idea to check with CAPZ folks if the issue should be reopened. |
k8s-ci-robot
added
ok-to-test
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6274 +/- ##
==========================================
- Coverage 43.88% 43.64% -0.24%
==========================================
Files 289 293 +4
Lines 25351 26077 +726
==========================================
+ Hits 11125 11382 +257
- Misses 13448 13904 +456
- Partials 778 791 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/assign |
|
Hi @robinkb, thanks for your hard work on this PR! Apologies if this wasn't clear in our docs, but we would prefer any new services added to use ASO instead of Azure SDK. We're trying to migrate away from Azure SDK and still have a goal of deprecating it and going ASO-only. Would you be able to investigate refactoring this PR to use ASO? I know it's a big ask, so if not we can pick it up as a follow-up. |
|
The vast, vast majority of the work was done by Nikola, so all credit to him! ❤️ I just updated the patch. Can you point me towards some example implementations based on ASO? That will help us make an estimate. Our time is a bit tight right now though, so I doubt that we will have the bandwidth to refactor the PR shortly. In any case, it will be helpful to have a reference for future contributions that we plan to make. |
|
No worries! I want to actually correct what I said earlier. I think it's fine to merge this as an Azure SDK service since we haven't been prioritizing migrating everything to ASO anyways. I'll give this a proper review and get back to you, thanks for your patience! |
willie-yao
left a comment
willie-yao
left a comment
There was a problem hiding this comment.
Thanks again for your great work! I left a few comments and will do another pass once they're addressed.
| // Check allowed subscriptions | ||
| if !compareStringPointerSlicesUnordered( | ||
| wanted.Properties.Visibility.Subscriptions, | ||
| existing.Properties.Visibility.Subscriptions) { | ||
| return false | ||
| } | ||
|
|
||
| // Check auto-approved subscriptions | ||
| if !compareStringPointerSlicesUnordered( | ||
| wanted.Properties.AutoApproval.Subscriptions, | ||
| existing.Properties.AutoApproval.Subscriptions) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
I noticed that constructParameters only populates Properties.Visibility when len(s.AllowedSubscriptions) > 0 and Properties.AutoApproval when len(s.AutoApprovedSubscriptions) > 0. Both API fields are documented as optional. The comparison here dereferences both unconditionally, so any spec that omits the subscription lists panics on the second reconcile. Can you add a nil check for both fields?
| // First we get all private links to API server load balancer. | ||
| // Other load balancers (ControlPlaneOutboundLB and NodeOutboundLB) are outbound, so we cannot create private links | ||
| // for those. | ||
| privateLinks := s.AzureCluster.Spec.NetworkSpec.APIServerLB.PrivateLinks |
There was a problem hiding this comment.
I think you'll need to add a nil check for Spec.NetworkSpec.APIServerLB here as well. For externally managed control planes, Spec.NetworkSpec.APIServerLB will be set to nil as seen here" https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/internal/api/v1beta1/azurecluster_default.go#L74
You can do something like what you had on line 457:
if apiServerLB := s.AzureCluster.Spec.NetworkSpec.APIServerLB; apiServerLB != nil { ... }| FrontendIPsCount *int32 `json:"frontendIPsCount,omitempty"` | ||
| // PrivateLinks to the load balancer (max 8 private links). | ||
| // +optional | ||
| PrivateLinks []PrivateLink `json:"privateLinks,omitempty"` |
There was a problem hiding this comment.
Since PrivateLinks is only for internal LBs, I think it would help to have the webhook reject this field being set for node/control plane outbound lbs. You can maybe add something like this to validateNodeOutboundLB and validateControlPlaneOutboundLB:
if len(lb.PrivateLinks) > 0 {
allErrs = append(allErrs, field.Forbidden(
fldPath.Child("privateLinks"),
"privateLinks are only supported on the API server load balancer",
))
}
| var subnetCIDRs []string | ||
| for _, subnet := range subnets { | ||
| subnetCIDRs = append(subnetCIDRs, subnet.CIDRBlocks...) | ||
| } |
There was a problem hiding this comment.
Getting the CIDRs from subnet.CIDRBlocks would get them from every cluster subnet, so a NAT IP config that targets subnet A with a static IP from subnet B's CIDR is accepted. I think we should instead look up the subnet referenced by natIPConfig.Subnet first and validate the IP only against that subnet's CIDRs.
| // PrivateLinkNATIPConfiguration specifies NAT IP configuration for the private link. | ||
| type PrivateLinkNATIPConfiguration struct { | ||
| // AllocationMethod specifies how the private link NAT IPs are allocated: "Static" or "Dynamic". | ||
| AllocationMethod string `json:"allocationMethod"` |
There was a problem hiding this comment.
Shouldn't this use the PrivateLinkNATIPAllocationMethod type defined below instead of a string?
| pl.NATIPConfigurations[j].Subnet, | ||
| fmt.Sprintf("NATIPConfiguration must use existing subnet (subnet %s not specified in AzureCluster resource)", natIPConfig.Subnet))) | ||
| } | ||
| if natIPConfig.AllocationMethod == "Static" { |
There was a problem hiding this comment.
| if natIPConfig.AllocationMethod == "Static" { | |
| if natIPConfig.AllocationMethod == infrav1.NATIPAllocationMethodStatic { |
| // NATIPConfiguration defines the NAT IP configuration for the private link service. | ||
| type NATIPConfiguration struct { | ||
| // AllocationMethod can be Static or Dynamic. | ||
| AllocationMethod string |
There was a problem hiding this comment.
| AllocationMethod string | |
| AllocationMethod infrav1.PrivateLinkNATIPAllocationMethod |
Related to above comment changing AllocationMethod from type string to PrivateLinkNATIPAllocationMethod.
| for i, natIPConfiguration := range s.NATIPConfiguration { | ||
| ipAllocationMethod := armnetwork.IPAllocationMethod(natIPConfiguration.AllocationMethod) | ||
| if ipAllocationMethod != armnetwork.IPAllocationMethodDynamic && ipAllocationMethod != armnetwork.IPAllocationMethodStatic { | ||
| return armnetwork.PrivateLinkService{}, errors.Errorf("%T is not a supported armnetwork.IPAllocationMethodStatic", natIPConfiguration.AllocationMethod) |
There was a problem hiding this comment.
nit: I think %T would just print out the Go type name and not the value the user passed. Also it could be dynamic in this case.
| return armnetwork.PrivateLinkService{}, errors.Errorf("%T is not a supported armnetwork.IPAllocationMethodStatic", natIPConfiguration.AllocationMethod) | |
| return armnetwork.PrivateLinkService{}, errors.Errorf("%q is not a supported NAT IP allocation method (must be %q or %q)", natIPConfiguration.AllocationMethod, infrav1.NATIPAllocationMethodStatic, infrav1.NATIPAllocationMethodDynamic) |
| for i, pl := range lb.PrivateLinks { | ||
| if err := validatePrivateLinkName(pl.Name, fldPath.Child("privateLinks").Index(i).Child("name")); err != nil { | ||
| allErrs = append(allErrs, err) | ||
| } |
There was a problem hiding this comment.
I think you need to also add a uniqueness check for privateLinks[i].name across lb.PrivateLinks. Right now the webhook accepts two privateLinks entries with identical names.
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
From: #3747 (comment)
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 #3400
Special notes for your reviewer:
This PR was originally submitted in #3747, but unfortunately it got stalled due to time constraints. I have gone through the comments in the original PR, and applied what still seemed relevant to the revision in this PR. Special thanks to @nprokopic for development of the feature.
TODOs:
Release note: