Skip to content
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

[release-1.27] feat: support sharing IP address across services by public IP name #4271

Merged
merged 1 commit into from
Jul 14, 2023
Merged

[release-1.27] feat: support sharing IP address across services by public IP name #4271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
64 changes: 55 additions & 9 deletions pkg/provider/azure_loadbalancer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,7 +887,7 @@ func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service,

// For the services with loadBalancerIP set, an existing public IP is required, primary
// or secondary, or a public IP not found error would be reported.
pip, err := az.findMatchedPIPByLoadBalancerIP(service, loadBalancerIP, pipResourceGroup)
pip, err := az.findMatchedPIP(loadBalancerIP, "", pipResourceGroup)
if err != nil {
return "", false, err
}
Expand All @@ -899,20 +899,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) {
pips, err := az.listPIP(pipResourceGroup, azcache.CacheReadTypeDefault)
if err != nil {
return nil, fmt.Errorf("findMatchedPIPByLoadBalancerIP: failed to listPIP: %w", err)
}

pip, err := getExpectedPIPFromListByIPAddress(pips, loadBalancerIP)
if loadBalancerIP != "" {
pip, err = az.findMatchedPIPByLoadBalancerIP(&pips, loadBalancerIP, pipResourceGroup)
if err != nil {
return nil, err
}
return pip, nil
}

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

func (az *Cloud) findMatchedPIPByName(pips *[]network.PublicIPAddress, pipName, pipResourceGroup string) (*network.PublicIPAddress, error) {
for _, pip := range *pips {
if strings.EqualFold(pointer.StringDeref(pip.Name, ""), pipName) {
return &pip, nil
}
}

pipList, err := az.listPIP(pipResourceGroup, azcache.CacheReadTypeForceRefresh)
if err != nil {
return nil, fmt.Errorf("findMatchedPIPByName: failed to listPIP force refresh: %w", err)
}
for _, pip := range pipList {
if strings.EqualFold(pointer.StringDeref(pip.Name, ""), pipName) {
return &pip, nil
}
}

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) {
pip, err := getExpectedPIPFromListByIPAddress(*pips, loadBalancerIP)
if err != nil {
pips, err = az.listPIP(pipResourceGroup, azcache.CacheReadTypeForceRefresh)
pipList, err := az.listPIP(pipResourceGroup, azcache.CacheReadTypeForceRefresh)
if err != nil {
return nil, fmt.Errorf("findMatchedPIPByLoadBalancerIP: failed to listPIP force refresh: %w", err)
}

pip, err = getExpectedPIPFromListByIPAddress(pips, loadBalancerIP)
pip, err = getExpectedPIPFromListByIPAddress(pipList, loadBalancerIP)
if err != nil {
return nil, fmt.Errorf("findMatchedPIPByLoadBalancerIP: cannot find public IP with IP address %s in resource group %s", loadBalancerIP, pipResourceGroup)
}
Expand Down Expand Up @@ -3588,7 +3626,7 @@ func serviceOwnsPublicIP(service *v1.Service, pip *network.PublicIPAddress, clus

// if there is no service tag on the pip, it is user-created pip
if serviceTag == "" {
return isServiceLoadBalancerIPMatchesPIP(service, pip, isIPv6), true
return isServiceSelectPIP(service, pip, isIPv6), true
}

// if there is service tag on the pip, it is system-created pip
Expand All @@ -3604,18 +3642,26 @@ func serviceOwnsPublicIP(service *v1.Service, pip *network.PublicIPAddress, clus
}

// if the service is not included in the tags of the system-created pip, check the ip address
// this could happen for secondary services
return isServiceLoadBalancerIPMatchesPIP(service, pip, isIPv6), false
// or pip name, this could happen for secondary services
return isServiceSelectPIP(service, pip, isIPv6), false
}

// if the pip has no tags, it should be user-created
return isServiceLoadBalancerIPMatchesPIP(service, pip, isIPv6), true
return isServiceSelectPIP(service, pip, isIPv6), true
}

func isServiceLoadBalancerIPMatchesPIP(service *v1.Service, pip *network.PublicIPAddress, isIPV6 bool) bool {
return strings.EqualFold(pointer.StringDeref(pip.IPAddress, ""), getServiceLoadBalancerIP(service, isIPV6))
}

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

func isServiceSelectPIP(service *v1.Service, pip *network.PublicIPAddress, isIPV6 bool) bool {
return isServiceLoadBalancerIPMatchesPIP(service, pip, isIPV6) || isServicePIPNameMatchesPIP(service, pip, isIPV6)
}

func isSVCNameInPIPTag(tag, svcName string) bool {
svcNames := parsePIPServiceTag(&tag)

Expand Down
137 changes: 131 additions & 6 deletions pkg/provider/azure_loadbalancer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,7 @@ func TestServiceOwnsPublicIP(t *testing.T) {
clusterName string
serviceName string
serviceLBIP string
serviceLBName string
expectedOwns bool
expectedUserAssignedPIP bool
}{
Expand All @@ -933,6 +934,7 @@ func TestServiceOwnsPublicIP(t *testing.T) {
{
desc: "false should be returned when service name tag doesn't match",
pip: &network.PublicIPAddress{
Name: pointer.String("pip1"),
Tags: map[string]*string{
consts.ServiceTagKey: pointer.String("default/nginx"),
},
Expand Down Expand Up @@ -975,6 +977,7 @@ func TestServiceOwnsPublicIP(t *testing.T) {
{
desc: "false should be returned when cluster name matches while service name doesn't match",
pip: &network.PublicIPAddress{
Name: pointer.String("pip1"),
Tags: map[string]*string{
consts.ServiceTagKey: pointer.String("default/web"),
consts.ClusterNameKey: pointer.String("kubernetes"),
Expand Down Expand Up @@ -1005,6 +1008,7 @@ func TestServiceOwnsPublicIP(t *testing.T) {
{
desc: "false should be returned when the tag is empty and load balancer IP does not match",
pip: &network.PublicIPAddress{
Name: pointer.String("pip1"),
Tags: map[string]*string{
consts.ServiceTagKey: pointer.String(""),
consts.ClusterNameKey: pointer.String("kubernetes"),
Expand Down Expand Up @@ -1036,6 +1040,7 @@ func TestServiceOwnsPublicIP(t *testing.T) {
{
desc: "false should be returned if there is not a match among a multi-service tag",
pip: &network.PublicIPAddress{
Name: pointer.String("pip1"),
Tags: map[string]*string{
consts.ServiceTagKey: pointer.String("default/nginx1,default/nginx2"),
consts.ClusterNameKey: pointer.String("kubernetes"),
Expand Down Expand Up @@ -1092,6 +1097,45 @@ func TestServiceOwnsPublicIP(t *testing.T) {
expectedOwns: true,
expectedUserAssignedPIP: true,
},
{
desc: "should be true if the pip name matches",
pip: &network.PublicIPAddress{
Name: pointer.String("pip1"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: pointer.String("1.2.3.4"),
},
},
serviceLBName: "pip1",
expectedOwns: true,
expectedUserAssignedPIP: true,
},
{
desc: "should be true if the pip with tag matches the pip name",
pip: &network.PublicIPAddress{
Name: pointer.String("pip1"),
Tags: map[string]*string{},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: pointer.String("1.2.3.4"),
},
},
serviceLBName: "pip1",
expectedOwns: true,
expectedUserAssignedPIP: true,
},
{
desc: "should be true if the pip with service tag matches the pip name",
pip: &network.PublicIPAddress{
Name: pointer.String("pip1"),
Tags: map[string]*string{
consts.ServiceTagKey: pointer.String("default/web"),
},
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: pointer.String("1.2.3.4"),
},
},
serviceLBName: "pip1",
expectedOwns: true,
},
}

for i, c := range tests {
Expand All @@ -1100,6 +1144,13 @@ func TestServiceOwnsPublicIP(t *testing.T) {
if c.serviceLBIP != "" {
setServiceLoadBalancerIP(&service, c.serviceLBIP)
}
if c.serviceLBName != "" {
if service.ObjectMeta.Annotations == nil {
service.ObjectMeta.Annotations = map[string]string{consts.ServiceAnnotationPIPNameDualStack[false]: "pip1"}
} else {
service.ObjectMeta.Annotations[consts.ServiceAnnotationPIPNameDualStack[false]] = "pip1"
}
}
owns, isUserAssignedPIP := serviceOwnsPublicIP(&service, c.pip, c.clusterName)
assert.Equal(t, c.expectedOwns, owns, "TestCase[%d]: %s", i, c.desc)
assert.Equal(t, c.expectedUserAssignedPIP, isUserAssignedPIP, "TestCase[%d]: %s", i, c.desc)
Expand Down Expand Up @@ -2217,6 +2268,7 @@ func TestFindMatchedPIPByLoadBalancerIP(t *testing.T) {
testCases := []struct {
desc string
pips []network.PublicIPAddress
pipsSecondTime []network.PublicIPAddress
shouldRefreshCache bool
expectedPIP *network.PublicIPAddress
expectedError bool
Expand All @@ -2234,29 +2286,102 @@ func TestFindMatchedPIPByLoadBalancerIP(t *testing.T) {
},
{
desc: "findMatchedPIPByLoadBalancerIP should refresh cache if no matched ip is found",
pips: []network.PublicIPAddress{testPIP},
pipsSecondTime: []network.PublicIPAddress{testPIP},
shouldRefreshCache: true,
expectedPIP: &testPIP,
},
}
for _, test := range testCases {
t.Run(test.desc, func(t *testing.T) {
az := GetTestCloud(ctrl)
service := getTestService("test1", v1.ProtocolTCP, nil, false, 80)
setServiceLoadBalancerIP(&service, "1.2.3.4")

mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
if test.shouldRefreshCache {
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return([]network.PublicIPAddress{}, nil)
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.pipsSecondTime, nil)
}
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.pips, nil)
pip, err := az.findMatchedPIPByLoadBalancerIP(&service, "1.2.3.4", "rg")
pip, err := az.findMatchedPIPByLoadBalancerIP(&test.pips, "1.2.3.4", "rg")
assert.Equal(t, test.expectedPIP, pip)
assert.Equal(t, test.expectedError, err != nil)
})
}
}

func TestFindMatchedPIP(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

testPIP := network.PublicIPAddress{
Name: pointer.String("pipName"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: pointer.String("1.2.3.4"),
},
}

for _, tc := range []struct {
description string
pips []network.PublicIPAddress
pipsSecondTime []network.PublicIPAddress
pipName string
loadBalancerIP string
shouldRefreshCache bool
listError *retry.Error
listErrorSecondTime *retry.Error
expectedPIP *network.PublicIPAddress
expectedError error
}{
{
description: "should ignore pipName if loadBalancerIP is specified",
pips: []network.PublicIPAddress{testPIP},
pipsSecondTime: []network.PublicIPAddress{testPIP},
shouldRefreshCache: true,
loadBalancerIP: "2.3.4.5",
pipName: "pipName",
expectedError: errors.New("findMatchedPIPByLoadBalancerIP: cannot find public IP with IP address 2.3.4.5 in resource group rg"),
},
{
description: "should report an error if failed to list pip",
listError: retry.NewError(false, errors.New("list error")),
expectedError: errors.New("findMatchedPIPByLoadBalancerIP: failed to listPIP: Retriable: false, RetryAfter: 0s, HTTPStatusCode: 0, RawError: list error"),
},
{
description: "should refresh the cache if failed to search by name",
pips: []network.PublicIPAddress{},
pipsSecondTime: []network.PublicIPAddress{testPIP},
shouldRefreshCache: true,
pipName: "pipName",
expectedPIP: &testPIP,
},
{
description: "should return the expected pip by name",
pips: []network.PublicIPAddress{testPIP},
pipName: "pipName",
expectedPIP: &testPIP,
},
{
description: "should report an error if failed to list pip second time",
pips: []network.PublicIPAddress{},
listErrorSecondTime: retry.NewError(false, errors.New("list error")),
shouldRefreshCache: true,
expectedError: errors.New("findMatchedPIPByName: failed to listPIP force refresh: Retriable: false, RetryAfter: 0s, HTTPStatusCode: 0, RawError: list error"),
},
} {
t.Run(tc.description, func(t *testing.T) {
az := GetTestCloud(ctrl)
mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(tc.pips, tc.listError)
if tc.shouldRefreshCache {
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(tc.pipsSecondTime, tc.listErrorSecondTime)
}

pip, err := az.findMatchedPIP(tc.loadBalancerIP, tc.pipName, "rg")
assert.Equal(t, tc.expectedPIP, pip)
if tc.expectedError != nil {
assert.Equal(t, tc.expectedError.Error(), err.Error())
}
})
}
}

func TestDeterminePublicIPName(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down