Skip to content

Commit

Permalink
Deprecate LoadBalancerIP with Servie LB IP annotation
Browse files Browse the repository at this point in the history 
* Add service.beta.kubernetes.io/azure-load-balancer-ipv4
  and service.beta.kubernetes.io/azure-load-balancer-ipv6
* Add an e2e test
* Support retrieve Service CIDR in e2e framework

Signed-off-by: Zhecheng Li <[email protected]>
  • Loading branch information
@lzhecheng
lzhecheng committed Oct 4, 2022
1 parent 7bd5668 commit 4e7469c
Show file tree
Hide file tree
Showing 14 changed files with 328 additions and 156 deletions.
10 changes: 10 additions & 0 deletions pkg/consts/consts.go
View file
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 (
// There's no "service.beta.kubernetes.io/azure-load-balancer" annotation before these 2 new ones are added.
// Service.Spec.LoadBalancerIP was used and it will be deprecated.
ServiceAnnotationLoadBalancerIPDualStack = map[bool]string{
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
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
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
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
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

0 comments on commit 4e7469c

Please sign in to comment.