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

Deprecate LoadBalancerIP with Servie LB IP annotation #2428

Merged
merged 1 commit into from
Oct 10, 2022
Merged

Deprecate LoadBalancerIP with Servie LB IP annotation #2428

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions pkg/consts/consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,16 @@ const (
BackoffJitterDefault = 1.0
)

// LB variables for dual-stack
var (
// Service.Spec.LoadBalancerIP has been deprecated and may be removed in a future release. Those two annotations are introduced as alternatives to set IPv4/IPv6 LoadBalancer IPs.
// Refer https://github.com/kubernetes/api/blob/3638040e4063e0f889c129220cd386497f328276/core/v1/types.go#L4459-L4468 for more details.
ServiceAnnotationLoadBalancerIPDualStack = map[bool]string{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make them two strings? It looks strange to have variables in consts file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention is to make dualstack code cleaner. Since there will be a lot of isIPv6 bool parameters for dualstack support, such map avoids if statements.

feiskyer marked this conversation as resolved.
Show resolved Hide resolved
false: "service.beta.kubernetes.io/azure-load-balancer-ipv4",
true: "service.beta.kubernetes.io/azure-load-balancer-ipv6",
}
)

// load balancer
const (
// PreConfiguredBackendPoolLoadBalancerTypesNone means that the load balancers are not pre-configured
Expand Down
50 changes: 41 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 @@ -22,6 +22,7 @@ import (
"errors"
"fmt"
"math"
"net"
"reflect"
"sort"
"strconv"
Expand All @@ -46,6 +47,36 @@ import (
"sigs.k8s.io/cloud-provider-azure/pkg/retry"
)

// getServiceLoadBalancerIP retrieves LB IP from IPv4 annotation, then IPv6 annotation, then service.Spec.LoadBalancerIP.
// TODO: Dual-stack support is not implemented.
func getServiceLoadBalancerIP(service *v1.Service) string {
if service == nil {
return ""
}

if ip, ok := service.Annotations[consts.ServiceAnnotationLoadBalancerIPDualStack[false]]; ok && ip != "" {
return ip
}
if ip, ok := service.Annotations[consts.ServiceAnnotationLoadBalancerIPDualStack[true]]; ok && ip != "" {
return ip
}

// Retrieve LB IP from service.Spec.LoadBalancerIP (will be deprecated)
return service.Spec.LoadBalancerIP
}

// setServiceLoadBalancerIP sets LB IP to a Service
func setServiceLoadBalancerIP(service *v1.Service, ip string) {
if service.Annotations == nil {
service.Annotations = map[string]string{}
}
if net.ParseIP(ip).To4() != nil {
service.Annotations[consts.ServiceAnnotationLoadBalancerIPDualStack[false]] = ip
return
}
service.Annotations[consts.ServiceAnnotationLoadBalancerIPDualStack[true]] = ip
}

// GetLoadBalancer returns whether the specified load balancer and its components exist, and
// if so, what its status is.
func (az *Cloud) GetLoadBalancer(ctx context.Context, clusterName string, service *v1.Service) (status *v1.LoadBalancerStatus, exists bool, err error) {
Expand Down Expand Up @@ -863,7 +894,7 @@ func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service,
}

pipResourceGroup := az.getPublicIPAddressResourceGroup(service)
loadBalancerIP := service.Spec.LoadBalancerIP
loadBalancerIP := getServiceLoadBalancerIP(service)

// Assume that the service without loadBalancerIP set is a primary service.
// If a secondary service doesn't set the loadBalancerIP, it is not allowed to share the IP.
Expand Down Expand Up @@ -921,14 +952,15 @@ func flipServiceInternalAnnotation(service *v1.Service) *v1.Service {
func updateServiceLoadBalancerIP(service *v1.Service, serviceIP string) *v1.Service {
copyService := service.DeepCopy()
if len(serviceIP) > 0 && copyService != nil {
copyService.Spec.LoadBalancerIP = serviceIP
setServiceLoadBalancerIP(copyService, serviceIP)
}
return copyService
}

func (az *Cloud) findServiceIPAddress(ctx context.Context, clusterName string, service *v1.Service) (string, error) {
if len(service.Spec.LoadBalancerIP) > 0 {
return service.Spec.LoadBalancerIP, nil
lbIP := getServiceLoadBalancerIP(service)
if len(lbIP) > 0 {
return lbIP, nil
}

if len(service.Status.LoadBalancer.Ingress) > 0 && len(service.Status.LoadBalancer.Ingress[0].IP) > 0 {
Expand Down Expand Up @@ -1318,7 +1350,7 @@ func (az *Cloud) isFrontendIPChanged(clusterName string, config network.Frontend
if !strings.EqualFold(to.String(config.Name), lbFrontendIPConfigName) {
return false, nil
}
loadBalancerIP := service.Spec.LoadBalancerIP
loadBalancerIP := getServiceLoadBalancerIP(service)
isInternal := requiresInternalLoadBalancer(service)
if isInternal {
// Judge subnet
Expand Down Expand Up @@ -1837,7 +1869,7 @@ func (az *Cloud) reconcileFrontendIPConfigs(clusterName string, service *v1.Serv
configProperties.PrivateIPAddressVersion = network.IPVersionIPv6
}

loadBalancerIP := service.Spec.LoadBalancerIP
loadBalancerIP := getServiceLoadBalancerIP(service)
if loadBalancerIP != "" {
configProperties.PrivateIPAllocationMethod = network.IPAllocationMethodStatic
configProperties.PrivateIPAddress = &loadBalancerIP
Expand Down Expand Up @@ -3304,7 +3336,7 @@ func getServiceTags(service *v1.Service) []string {
// The pip is user-created if and only if there is no service tags.
// The service owns the pip if:
// 1. The serviceName is included in the service tags of a system-created pip.
// 2. The service.Spec.LoadBalancerIP matches the IP address of a user-created pip.
// 2. The service LoadBalancerIP matches the IP address of a user-created pip.
func serviceOwnsPublicIP(service *v1.Service, pip *network.PublicIPAddress, clusterName string) (bool, bool) {
if service == nil || pip == nil {
klog.Warningf("serviceOwnsPublicIP: nil service or public IP")
Expand All @@ -3324,7 +3356,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 strings.EqualFold(to.String(pip.IPAddress), service.Spec.LoadBalancerIP), true
return strings.EqualFold(to.String(pip.IPAddress), getServiceLoadBalancerIP(service)), true
}

// if there is service tag on the pip, it is system-created pip
Expand All @@ -3342,7 +3374,7 @@ func serviceOwnsPublicIP(service *v1.Service, pip *network.PublicIPAddress, clus
} else {
// if the service is not included in te tags of the system-created pip, check the ip address
// this could happen for secondary services
return strings.EqualFold(to.String(pip.IPAddress), service.Spec.LoadBalancerIP), false
return strings.EqualFold(to.String(pip.IPAddress), getServiceLoadBalancerIP(service)), false
}
}

Expand Down
62 changes: 32 additions & 30 deletions pkg/provider/azure_loadbalancer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,7 +961,7 @@ func TestServiceOwnsPublicIP(t *testing.T) {
t.Run(c.desc, func(t *testing.T) {
service := getTestService(c.serviceName, v1.ProtocolTCP, nil, false, 80)
if c.serviceLBIP != "" {
service.Spec.LoadBalancerIP = c.serviceLBIP
setServiceLoadBalancerIP(&service, c.serviceLBIP)
}
owns, isUserAssignedPIP := serviceOwnsPublicIP(&service, c.pip, c.clusterName)
assert.Equal(t, c.expectedOwns, owns, "TestCase[%d]: %s", i, c.desc)
Expand Down Expand Up @@ -2008,34 +2008,36 @@ func TestIsFrontendIPChanged(t *testing.T) {
},
}

for i, test := range testCases {
az := GetTestCloud(ctrl)
mockSubnetsClient := az.SubnetsClient.(*mocksubnetclient.MockInterface)
mockSubnetsClient.EXPECT().Get(gomock.Any(), "rg", "vnet", "testSubnet", "").Return(test.existingSubnet, nil).AnyTimes()
mockSubnetsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", "vnet", "testSubnet", test.existingSubnet).Return(nil)
err := az.SubnetsClient.CreateOrUpdate(context.TODO(), "rg", "vnet", "testSubnet", test.existingSubnet)
if err != nil {
t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err)
}

mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
for _, existingPIP := range test.existingPIPs {
mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", *existingPIP.Name, gomock.Any()).Return(existingPIP, nil).AnyTimes()
err := az.PublicIPAddressesClient.CreateOrUpdate(context.TODO(), "rg", *existingPIP.Name, existingPIP)
for _, test := range testCases {
t.Run(test.desc, func(t *testing.T) {
az := GetTestCloud(ctrl)
mockSubnetsClient := az.SubnetsClient.(*mocksubnetclient.MockInterface)
mockSubnetsClient.EXPECT().Get(gomock.Any(), "rg", "vnet", "testSubnet", "").Return(test.existingSubnet, nil).AnyTimes()
mockSubnetsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", "vnet", "testSubnet", test.existingSubnet).Return(nil)
err := az.SubnetsClient.CreateOrUpdate(context.TODO(), "rg", "vnet", "testSubnet", test.existingSubnet)
if err != nil {
t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err)
t.Fatal(err)
}
}
test.service.Spec.LoadBalancerIP = test.loadBalancerIP
test.service.Annotations[consts.ServiceAnnotationLoadBalancerInternalSubnet] = test.annotations
flag, rerr := az.isFrontendIPChanged("testCluster", test.config,
&test.service, test.lbFrontendIPConfigName, &test.existingPIPs)
if rerr != nil {
fmt.Println(rerr.Error())
}
assert.Equal(t, test.expectedFlag, flag, "TestCase[%d]: %s", i, test.desc)
assert.Equal(t, test.expectedError, rerr != nil, "TestCase[%d]: %s", i, test.desc)

mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
for _, existingPIP := range test.existingPIPs {
mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", *existingPIP.Name, gomock.Any()).Return(existingPIP, nil).AnyTimes()
err := az.PublicIPAddressesClient.CreateOrUpdate(context.TODO(), "rg", *existingPIP.Name, existingPIP)
if err != nil {
t.Fatal(err)
}
}
setServiceLoadBalancerIP(&test.service, test.loadBalancerIP)
test.service.Annotations[consts.ServiceAnnotationLoadBalancerInternalSubnet] = test.annotations
flag, rerr := az.isFrontendIPChanged("testCluster", test.config,
&test.service, test.lbFrontendIPConfigName, &test.existingPIPs)
if rerr != nil {
fmt.Println(rerr.Error())
}
assert.Equal(t, test.expectedFlag, flag)
assert.Equal(t, test.expectedError, rerr != nil)
})
}
}

Expand Down Expand Up @@ -2082,7 +2084,7 @@ func TestDeterminePublicIPName(t *testing.T) {
for i, test := range testCases {
az := GetTestCloud(ctrl)
service := getTestService("test1", v1.ProtocolTCP, nil, false, 80)
service.Spec.LoadBalancerIP = test.loadBalancerIP
setServiceLoadBalancerIP(&service, test.loadBalancerIP)

mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil).MaxTimes(1)
Expand Down Expand Up @@ -3055,7 +3057,7 @@ func TestReconcileLoadBalancer(t *testing.T) {
clusterResources, expectedInterfaces, expectedVirtualMachines := getClusterResources(az, 3, 3)
setMockEnv(az, ctrl, expectedInterfaces, expectedVirtualMachines, 1)

test.service.Spec.LoadBalancerIP = "1.2.3.4"
setServiceLoadBalancerIP(&test.service, "1.2.3.4")

err := az.PublicIPAddressesClient.CreateOrUpdate(context.TODO(), "rg", "pipName", network.PublicIPAddress{
Name: to.StringPtr("pipName"),
Expand Down Expand Up @@ -4623,7 +4625,7 @@ func TestUnbindServiceFromPIP(t *testing.T) {
}
serviceName := "ns2/svc2"
service := getTestService(serviceName, v1.ProtocolTCP, nil, false, 80)
service.Spec.LoadBalancerIP = "1.2.3.4"
setServiceLoadBalancerIP(&service, "1.2.3.4")
expectedTags := []map[string]*string{
nil,
{consts.ServiceTagKey: to.StringPtr("")},
Expand Down
2 changes: 1 addition & 1 deletion pkg/provider/azure_standard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ func (az *Cloud) serviceOwnsFrontendIP(fip network.FrontendIPConfiguration, serv
return true, isPrimaryService, nil
}

loadBalancerIP := service.Spec.LoadBalancerIP
loadBalancerIP := getServiceLoadBalancerIP(service)
if loadBalancerIP == "" {
// it is a must that the secondary services set the loadBalancer IP
return false, isPrimaryService, nil
Expand Down
34 changes: 13 additions & 21 deletions pkg/provider/azure_standard_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1684,10 +1684,8 @@ func TestServiceOwnsFrontendIP(t *testing.T) {
},
service: &v1.Service{
ObjectMeta: meta.ObjectMeta{
UID: types.UID("secondary"),
},
Spec: v1.ServiceSpec{
LoadBalancerIP: "1.2.3.4",
UID: types.UID("secondary"),
Annotations: map[string]string{consts.ServiceAnnotationLoadBalancerIPDualStack[false]: "1.2.3.4"},
},
},
},
Expand All @@ -1712,10 +1710,8 @@ func TestServiceOwnsFrontendIP(t *testing.T) {
},
service: &v1.Service{
ObjectMeta: meta.ObjectMeta{
UID: types.UID("secondary"),
},
Spec: v1.ServiceSpec{
LoadBalancerIP: "4.3.2.1",
UID: types.UID("secondary"),
Annotations: map[string]string{consts.ServiceAnnotationLoadBalancerIPDualStack[false]: "4.3.2.1"},
},
},
},
Expand All @@ -1739,10 +1735,8 @@ func TestServiceOwnsFrontendIP(t *testing.T) {
},
service: &v1.Service{
ObjectMeta: meta.ObjectMeta{
UID: types.UID("secondary"),
},
Spec: v1.ServiceSpec{
LoadBalancerIP: "4.3.2.1",
UID: types.UID("secondary"),
Annotations: map[string]string{consts.ServiceAnnotationLoadBalancerIPDualStack[false]: "4.3.2.1"},
},
},
},
Expand All @@ -1766,10 +1760,8 @@ func TestServiceOwnsFrontendIP(t *testing.T) {
},
service: &v1.Service{
ObjectMeta: meta.ObjectMeta{
UID: types.UID("secondary"),
},
Spec: v1.ServiceSpec{
LoadBalancerIP: "4.3.2.1",
UID: types.UID("secondary"),
Annotations: map[string]string{consts.ServiceAnnotationLoadBalancerIPDualStack[false]: "4.3.2.1"},
},
},
isOwned: true,
Expand All @@ -1784,11 +1776,11 @@ func TestServiceOwnsFrontendIP(t *testing.T) {
},
service: &v1.Service{
ObjectMeta: meta.ObjectMeta{
UID: types.UID("secondary"),
Annotations: map[string]string{consts.ServiceAnnotationLoadBalancerInternal: "true"},
},
Spec: v1.ServiceSpec{
LoadBalancerIP: "4.3.2.1",
UID: types.UID("secondary"),
Annotations: map[string]string{
consts.ServiceAnnotationLoadBalancerInternal: "true",
consts.ServiceAnnotationLoadBalancerIPDualStack[false]: "4.3.2.1",
},
},
},
isOwned: true,
Expand Down
Loading