-
Notifications
You must be signed in to change notification settings - Fork 281
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.
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
Container Load Balancer - LB Rules #8218
base: master
Are you sure you want to change the base?
Changes from 4 commits
e9693c6
2a45c63
e1eb35d
29bb3c8
f52242e
f51f58c
1027221
8f489e7
f33f04e
81d5568
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ import ( | |
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/labels" | ||
utilerrors "k8s.io/apimachinery/pkg/util/errors" | ||
"k8s.io/apimachinery/pkg/util/intstr" | ||
cloudprovider "k8s.io/cloud-provider" | ||
servicehelpers "k8s.io/cloud-provider/service/helpers" | ||
"k8s.io/klog/v2" | ||
|
@@ -2800,6 +2801,39 @@ func (az *Cloud) getExpectedLBRules( | |
isIPv6 bool, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add PR desc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated the PR description |
||
) ([]*armnetwork.Probe, []*armnetwork.LoadBalancingRule, error) { | ||
var expectedRules []*armnetwork.LoadBalancingRule | ||
// If we are using Pod IP in the LB backend, we skip health probes, disable floating IP and use port.TargetPort. | ||
if az.IsLBBackendPoolTypePodIP() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please extract this as a new func to make the main loop concise? |
||
// Check for multi-IP families in the service spec (not allowed for BackendPool of type PodIP). | ||
if len(service.Spec.IPFamilies) > 1 { | ||
return nil, nil, fmt.Errorf("dual-stack services are not supported when LB backend pool type is PodIP") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the design doc it says "For dual stack, 2 services need to be created". Can you double check which one is the right behavior? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, that is right. Should I update the error string to say precisely that? |
||
} | ||
// Build rules for each service port but with floatingIP = false, no health probes. | ||
for _, port := range service.Spec.Ports { | ||
if port.TargetPort.Type == intstr.String { | ||
return nil, nil, fmt.Errorf("named targetPort is not supported when LB backend pool type is PodIP: %s", port.TargetPort.StrVal) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are we able to make port name work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since, CLB directly rewrites the Destination Port in the packet after selecting the Destination POD, I think we can't support named Port specified in https://kubernetes.io/docs/concepts/services-networking/service/#field-spec-ports |
||
} | ||
ruleName := az.getLoadBalancerRuleName(service, port.Protocol, port.Port, isIPv6) | ||
transportProto, _, _, err := getProtocolsFromKubernetesProtocol(port.Protocol) | ||
if err != nil { | ||
return nil, nil, fmt.Errorf("failed to parse transport protocol: %w", err) | ||
} | ||
props, err := az.getExpectedLoadBalancingRulePropertiesForPort(service, lbFrontendIPConfigID, lbBackendPoolID, port, transportProto) | ||
if err != nil { | ||
return nil, nil, fmt.Errorf("error generating LB rule: %w", err) | ||
} | ||
// Turn off floating IP and skip health probe attachments. | ||
props.EnableFloatingIP = ptr.To(false) | ||
props.Probe = nil | ||
props.BackendPort = &port.TargetPort.IntVal | ||
expectedRules = append(expectedRules, &armnetwork.LoadBalancingRule{ | ||
Name: &ruleName, | ||
Properties: props, | ||
}) | ||
} | ||
return nil, expectedRules, nil | ||
} | ||
|
||
// Otherwise, the existing flow remains unchanged | ||
var expectedProbes []*armnetwork.Probe | ||
|
||
// support podPresence health check when External Traffic Policy is local | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -183,6 +183,10 @@ func (az *Config) IsLBBackendPoolTypeNodeIP() bool { | |
return strings.EqualFold(az.LoadBalancerBackendPoolConfigurationType, consts.LoadBalancerBackendPoolConfigurationTypeNodeIP) | ||
} | ||
|
||
func (az *Config) IsLBBackendPoolTypePodIP() bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need a check to block podIP + non-standardV2? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If conflicted with multi-slb, need to block as well. |
||
return strings.EqualFold(az.LoadBalancerBackendPoolConfigurationType, consts.LoadBalancerBackendPoolConfigurationTypePodIP) | ||
} | ||
|
||
func (az *Config) GetPutVMSSVMBatchSize() int { | ||
return az.PutVMSSVMBatchSize | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch for the variable name here👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we decided to use this instead of loadBalancerClass?