feat: support sharing IP address acorss services by public IP name #4257

nilo19 · 2023-07-07T06:53:19Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

We used to support sharing one pip across services by setting the IP address. This PR supports sharing by pip name.

Which issue(s) this PR fixes:

Fixes #4243

Special notes for your reviewer:

Does this PR introduce a user-facing change?

feat: support sharing IP address acorss services by public IP name

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

lzhecheng · 2023-07-07T07:06:09Z

pkg/provider/azure_loadbalancer.go

@@ -821,20 +821,58 @@ func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service,
 	return "", false, fmt.Errorf("user supplied IP Address %s was not found in resource group %s", loadBalancerIP, pipResourceGroup)
 }

-func (az *Cloud) findMatchedPIPByLoadBalancerIP(service *v1.Service, loadBalancerIP, pipResourceGroup string) (*network.PublicIPAddress, error) {
+func (az *Cloud) findMatchedPIP(loadBalancerIP, pipName, pipResourceGroup string) (pip *network.PublicIPAddress, err error) {


I checked the usage and it seems findMatchedPIP finds PIP with LB IP or PIP name. How about returning error if both 2 variables are set or both unset?

We can cover it in the doc that ip will overweight the name.

Yes, doc is good for both set.
How about both empty? I think at least a warning is needed. When somehow 2 empty values are passed in, it returns an empty PIP. It will add difficulty when debugging.

They can't be both empty, we guard outside of the func.

lzhecheng · 2023-07-07T07:10:59Z

pkg/provider/azure_loadbalancer.go

+	return pip, nil
+}
+
+func (az *Cloud) findMatchedPIPByName(pips *[]network.PublicIPAddress, pipName, pipResourceGroup string) (*network.PublicIPAddress, error) {


Isn't it getPublicIPAddress()?

I extract the list call from findMatchedPIPByLoadBalancerIP so that it can reuse.

lzhecheng · 2023-07-07T07:16:11Z

pkg/provider/azure_loadbalancer.go

+	return strings.EqualFold(pointer.StringDeref(pip.Name, ""), getServicePIPName(service, isIPV6))
+}
+
+func isServiceSelectPIP(service *v1.Service, pip *network.PublicIPAddress, isIPV6 bool) bool {


As for the name, how about using own instead of select since we already have serviceOwnsFrontendIP?

We already have this function.

lzhecheng · 2023-07-07T07:20:05Z

pkg/provider/azure_loadbalancer.go

+	return nil, fmt.Errorf("findMatchedPIPByName: failed to find PIP %s in resource group %s", pipName, pipResourceGroup)
+}
+
+func (az *Cloud) findMatchedPIPByLoadBalancerIP(pips *[]network.PublicIPAddress, loadBalancerIP, pipResourceGroup string) (*network.PublicIPAddress, error) {


Since its logic is similar to getPublicIPAddress(), how about moving it to pkg/provider/azure_publicip_repo.go? We already have a large azure_loadbalancer.go

lzhecheng · 2023-07-07T07:22:01Z

pkg/provider/azure_loadbalancer.go

+	}
+
+	if pipResourceGroup != "" {
+		pip, err = az.findMatchedPIPByName(&pips, pipName, pipResourceGroup)


After pip cache code refactored, I don't think we need to put PIPs as a parameter to other methods. We can listPIP in findMatchedPIPByName. Please correct me if I'm wrong @jwtty

What will be the benefit?

lzhecheng

A few suggestions about memory leak.

lzhecheng · 2023-07-07T07:37:26Z

pkg/provider/azure_standard.go

+		fip.FrontendIPConfigurationPropertiesFormat != nil &&
+		fip.FrontendIPConfigurationPropertiesFormat.PublicIPAddress != nil {
+		if strings.EqualFold(pointer.StringDeref(pip.ID, ""), pointer.StringDeref(fip.PublicIPAddress.ID, "")) {
+			klog.V(6).Infof("publicIPOwnsFrontendIP:found secondary service %s of the frontend IP config %s", service.Name, *fip.Name)


Suggested change

klog.V(6).Infof("publicIPOwnsFrontendIP:found secondary service %s of the frontend IP config %s", service.Name, *fip.Name)

klog.V(6).Infof("publicIPOwnsFrontendIP: found secondary service %s of the frontend IP config %s", service.Name, pointer.StringDeref(fip.Name, ""))

This is not the new code, and the name and id of any azure resource cannot be empty, so we can keep this safely.

lzhecheng · 2023-07-07T07:39:22Z

pkg/provider/azure_standard.go

@@ -362,6 +362,22 @@ func (az *Cloud) serviceOwnsRule(service *v1.Service, rule string) bool {
 	return strings.HasPrefix(strings.ToUpper(rule), strings.ToUpper(prefix))
 }

+func publicIPOwnsFrontendIP(service *v1.Service, fip *network.FrontendIPConfiguration, pip *network.PublicIPAddress) bool {
+	if pip != nil &&
+		pip.ID != nil &&


Since pointer.StringDeref is used on L373, I don't think this check is needed any more?

Ditto, I personally do not intend to change existing code.

lzhecheng · 2023-07-07T07:40:17Z

pkg/provider/azure_standard.go

+		fip != nil &&
+		fip.FrontendIPConfigurationPropertiesFormat != nil &&
+		fip.FrontendIPConfigurationPropertiesFormat.PublicIPAddress != nil {
+		if strings.EqualFold(pointer.StringDeref(pip.ID, ""), pointer.StringDeref(fip.PublicIPAddress.ID, "")) {


I think we need to make sure ID is not an empty string.

feiskyer

/lgtm

k8s-ci-robot · 2023-07-11T14:12:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, nilo19

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:

~~OWNERS~~ [feiskyer,nilo19]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

feat: support sharing IP address acorss services by public IP name

9593fd2

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 7, 2023

k8s-ci-robot requested review from lzhecheng and MartinForReal July 7, 2023 06:53

lzhecheng reviewed Jul 7, 2023

View reviewed changes

chore: move functions to proper files

4dc15d5

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 7, 2023

feiskyer approved these changes Jul 11, 2023

View reviewed changes

k8s-ci-robot assigned feiskyer Jul 11, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 11, 2023

feiskyer added this to the v1.28 milestone Jul 11, 2023

k8s-ci-robot merged commit 547ff5d into kubernetes-sigs:master Jul 11, 2023
16 checks passed

nilo19 deleted the fix/pip-name-share branch July 12, 2023 01:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support sharing IP address acorss services by public IP name #4257

feat: support sharing IP address acorss services by public IP name #4257

nilo19 commented Jul 7, 2023 •

edited

lzhecheng Jul 7, 2023

nilo19 Jul 7, 2023

lzhecheng Jul 7, 2023

nilo19 Jul 7, 2023

lzhecheng Jul 7, 2023

nilo19 Jul 7, 2023

lzhecheng Jul 7, 2023

nilo19 Jul 7, 2023

lzhecheng Jul 7, 2023

nilo19 Jul 7, 2023

lzhecheng Jul 7, 2023

nilo19 Jul 7, 2023

lzhecheng left a comment

lzhecheng Jul 7, 2023

nilo19 Jul 7, 2023

lzhecheng Jul 7, 2023

nilo19 Jul 7, 2023

lzhecheng Jul 7, 2023

feiskyer left a comment

k8s-ci-robot commented Jul 11, 2023

	klog.V(6).Infof("publicIPOwnsFrontendIP:found secondary service %s of the frontend IP config %s", service.Name, *fip.Name)
	klog.V(6).Infof("publicIPOwnsFrontendIP: found secondary service %s of the frontend IP config %s", service.Name, pointer.StringDeref(fip.Name, ""))

feat: support sharing IP address acorss services by public IP name #4257

feat: support sharing IP address acorss services by public IP name #4257

Conversation

nilo19 commented Jul 7, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzhecheng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feiskyer left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 11, 2023

nilo19 commented Jul 7, 2023 •

edited