✨ Allow more permissive extensibility for securityRules and additional CP LoadBalancers#5525
Conversation
k8s-ci-robot
added
kind/feature
k8s-ci-robot
added
the
size/L
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5525 +/- ##
==========================================
+ Coverage 52.61% 52.91% +0.29%
==========================================
Files 272 272
Lines 29485 29515 +30
==========================================
+ Hits 15513 15617 +104
+ Misses 13165 13086 -79
- Partials 807 812 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
k8s-ci-robot
added
release-note
Danil-Grigorev
force-pushed
the
extend-security-rules
branch
from
bec7326 to
fbe11d7
Compare
|
|
||
| // GetSecurityRuleByDestination returns security group rule, which matches provided destination ports. | ||
| func (s SubnetSpec) GetSecurityRuleByDestination(ports string) *SecurityRule { | ||
| if s.SecurityGroup.SecurityRules == nil { |
There was a problem hiding this comment.
wrapping the below for loop in if s.SecurityGroup.SecurityRules != nil { is equivalent and allows us to remove the add'l return nil response (we don't need a specific nil return for this condition because the default return before exiting the function is nil)
There was a problem hiding this comment.
Should be fixed, thanks.
| } | ||
| if s.ControlPlaneSubnet().SecurityGroup.SecurityRules == nil { | ||
|
|
||
| subnet := s.ControlPlaneSubnet() |
There was a problem hiding this comment.
in this flow where are we adding any user-provided security rules? (for example if a user specifies TCP 9345)
or is that elsewhere and the purpose of this change is to filter out 22 and apiserver port if it's not included?
There was a problem hiding this comment.
This flow performs only defaulting on top of the user-provided set of rules, which may not be empty. This way having a field populated does not always need to specify all allowed ports, only additional ones or overrides.
I’m currently validating how this works in tandem with ClusterClass definitions we have. The desired goal is to permit specifying registrationMethod: control-plane-endpoint and allow RKE2 to register on the allowed port. But ideally this should follow internal name resolution, so that the traffic wouldn’t go through external load balancer.
If it doesn’t work, maybe it would still require additional LB rules.
There was a problem hiding this comment.
It seems reasonable to have the additional ports of the APIServer LB opened-up/have an opinion about.
Could you please elaborate on why the security rules should not be updated too ?
There was a problem hiding this comment.
Sorry, maybe I’m not fully understanding. I will try to give my thoughts on this:
I was attempting to describe the flow of how the rules are processed. We only apply defaults on top of what the user provides, so the user can still specify ports like TCP 9345 explicitly in security rules. We’re not overriding or stripping anything out, just adding fallback values when needed. LB rules were needed as well, since CP endpoint is external from the cluster perspective.
Current set of changes is what was required to make cluster healthy, and added e2e test displays that. My previous comment was a set of assumptions, or “paths to explore later” if it doesn’t work.
Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>
Danil-Grigorev
force-pushed
the
extend-security-rules
branch
3 times, most recently
from
452799e to
df2c355
Compare
Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>
Danil-Grigorev
force-pushed
the
extend-security-rules
branch
from
df2c355 to
cf4c84b
Compare
|
/test ? |
|
@Danil-Grigorev: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use DetailsIn 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-sigs/prow repository. |
k8s-ci-robot
added
size/XXL
Danil-Grigorev
changed the title
Danil-Grigorev
force-pushed
the
extend-security-rules
branch
4 times, most recently
from
d744062 to
0d5e30f
Compare
Danil-Grigorev
force-pushed
the
extend-security-rules
branch
from
0091001 to
34ffcbb
Compare
Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>
Danil-Grigorev
force-pushed
the
extend-security-rules
branch
from
34ffcbb to
b408363
Compare
|
/retest |
|
/test pull-cluster-api-provider-azure-e2e |
Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>
|
/test pull-cluster-api-provider-azure-e2e-optional |
Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>
|
/test pull-cluster-api-provider-azure-e2e-optional |
nawazkh
left a comment
nawazkh
left a comment
There was a problem hiding this comment.
This is great, thank you for putting this together, @Danil-Grigorev , Kudos!
/lgtm
/approve
| name string | ||
| cluster *clusterv1.Cluster | ||
| azureCluster *infrav1.AzureCluster | ||
| expectedRuleCount int |
There was a problem hiding this comment.
Thank you for reformatting this! 🙌🏼
| }) | ||
| }) | ||
|
|
||
| Context("Creating RKE2 clusters using clusterclass [OPTIONAL]", func() { |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. DetailsGit tree hash: ed5b341a8c6e354b764340bd8828be88a2b55f1b |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nawazkh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
Could you please squash your commits ? |
k8s-ci-robot
added
the
do-not-merge/hold
/label tide/merge-method-squash Sure, would that work @nawazkh, or do you want a manual squash? |
k8s-ci-robot
added
the
tide/merge-method-squash
|
We usually do a manual squash, but this should work too! |
k8s-ci-robot
removed
the
do-not-merge/hold
A change to CAPZ[0], creates an SSH rule if one is not specified in the cluster spec. Prior to this commit, we had been creating the SSH rule with installer SDK hooks, which is still somewhat necessary to add the inbound NAT rules, because we are not yet using CAPZ to provision a public load balancer. But we can use CAPZ to just create the rule, which will stop CAPZ from preventing a redundant SSH rule which we were leaking during bootstrap destroy. This change will also result in creating an SSH rule for private clusters which is fine, and something we do on other providers. 0: kubernetes-sigs/cluster-api-provider-azure#5525
A change to CAPZ[0], creates an SSH rule if one is not specified in the cluster spec. Prior to this commit, we had been creating the SSH rule with installer SDK hooks, which is still somewhat necessary to add the inbound NAT rules, because we are not yet using CAPZ to provision a public load balancer. But we can use CAPZ to just create the rule, which will stop CAPZ from preventing a redundant SSH rule which we were leaking during bootstrap destroy. This change will also result in creating an SSH rule for private clusters which is fine, and something we do on other providers. 0: kubernetes-sigs/cluster-api-provider-azure#5525
A change to CAPZ[0], creates an SSH rule if one is not specified in the cluster spec. Prior to this commit, we had been creating the SSH rule with installer SDK hooks, which is still somewhat necessary to add the inbound NAT rules, because we are not yet using CAPZ to provision a public load balancer. But we can use CAPZ to just create the rule, which will stop CAPZ from preventing a redundant SSH rule which we were leaking during bootstrap destroy. This change will also result in creating an SSH rule for private clusters which is fine, and something we do on other providers. 0: kubernetes-sigs/cluster-api-provider-azure#5525
A change to CAPZ[0], creates an SSH rule if one is not specified in the cluster spec. Prior to this commit, we had been creating the SSH rule with installer SDK hooks, which is still somewhat necessary to add the inbound NAT rules, because we are not yet using CAPZ to provision a public load balancer. But we can use CAPZ to just create the rule, which will stop CAPZ from preventing a redundant SSH rule which we were leaking during bootstrap destroy. This change will also result in creating an SSH rule for private clusters which is fine, and something we do on other providers. 0: kubernetes-sigs/cluster-api-provider-azure#5525
A change to CAPZ[0], creates an SSH rule if one is not specified in the cluster spec. Prior to this commit, we had been creating the SSH rule with installer SDK hooks, which is still somewhat necessary to add the inbound NAT rules, because we are not yet using CAPZ to provision a public load balancer. But we can use CAPZ to just create the rule, which will stop CAPZ from preventing a redundant SSH rule which we were leaking during bootstrap destroy. This change will also result in creating an SSH rule for private clusters which is fine, and something we do on other providers. 0: kubernetes-sigs/cluster-api-provider-azure#5525
6 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Implement
AdditionalControlPlaneLBPortsfield for the additional inbound control-plane load balancer port configuration, allowing external access for user specified ports on the CP machine.This change also improves UX while using
CAPZwithRKE2provider, by allowing to specify only additional security rules on theAzureClusterresource, and making existing security rule defaults forAPIServerandSSHport more permissive and based on destination port selection.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 #5511
Special notes for your reviewer:
TODOs:
Release note: