Modify the logic to discover corresponding errors.

This commit is contained in:
t-qini 2019-08-12 11:17:03 +08:00
parent 034f96c909
commit 3facb631d4
3 changed files with 112 additions and 30 deletions

View File

@ -25,6 +25,7 @@ import (
"strings"
v1 "k8s.io/api/core/v1"
utilerrors "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/sets"
cloudprovider "k8s.io/cloud-provider"
servicehelpers "k8s.io/cloud-provider/service/helpers"
@ -72,8 +73,8 @@ const (
// to specify the resource group of load balancer objects that are not in the same resource group as the cluster.
ServiceAnnotationLoadBalancerResourceGroup = "service.beta.kubernetes.io/azure-load-balancer-resource-group"
// ServiceAnnotationLoadBalancerPIPName specifies the pip that will be applied to load balancer
ServiceAnnotationLoadBalancerPIPName = "service.beta.kubernetes.io/azure-load-balancer-pip-name"
// ServiceAnnotationPIPName specifies the pip that will be applied to load balancer
ServiceAnnotationPIPName = "service.beta.kubernetes.io/azure-pip-name"
// ServiceAnnotationAllowedServiceTag is the annotation used on the service
// to specify a list of allowed service tags separated by comma
@ -396,9 +397,6 @@ func (az *Cloud) getServiceLoadBalancerStatus(service *v1.Service, lb *network.L
if err != nil {
return nil, fmt.Errorf("get(%s): lb(%s) - failed to get LB PublicIPAddress Name from ID(%s)", serviceName, *lb.Name, *pipID)
}
if name, found := service.Annotations[ServiceAnnotationLoadBalancerPIPName]; found && name != "" {
pipName = name
}
pip, existsPip, err := az.getPublicIPAddress(az.getPublicIPAddressResourceGroup(service), pipName)
if err != nil {
return nil, err
@ -416,30 +414,32 @@ func (az *Cloud) getServiceLoadBalancerStatus(service *v1.Service, lb *network.L
return nil, nil
}
func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service) (string, error) {
if name, found := service.Annotations[ServiceAnnotationLoadBalancerPIPName]; found && name != "" {
return name, nil
func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service) (string, bool, error) {
var shouldPIPExisted bool
if name, found := service.Annotations[ServiceAnnotationPIPName]; found && name != "" {
shouldPIPExisted = true
return name, shouldPIPExisted, nil
}
loadBalancerIP := service.Spec.LoadBalancerIP
if len(loadBalancerIP) == 0 {
return az.getPublicIPName(clusterName, service), nil
return az.getPublicIPName(clusterName, service), shouldPIPExisted, nil
}
pipResourceGroup := az.getPublicIPAddressResourceGroup(service)
pips, err := az.ListPIP(service, pipResourceGroup)
if err != nil {
return "", err
return "", shouldPIPExisted, err
}
for _, pip := range pips {
if pip.PublicIPAddressPropertiesFormat.IPAddress != nil &&
*pip.PublicIPAddressPropertiesFormat.IPAddress == loadBalancerIP {
return *pip.Name, nil
return *pip.Name, shouldPIPExisted, nil
}
}
return "", fmt.Errorf("user supplied IP Address %s was not found in resource group %s", loadBalancerIP, pipResourceGroup)
return "", shouldPIPExisted, fmt.Errorf("user supplied IP Address %s was not found in resource group %s", loadBalancerIP, pipResourceGroup)
}
func flipServiceInternalAnnotation(service *v1.Service) *v1.Service {
@ -486,7 +486,7 @@ func (az *Cloud) findServiceIPAddress(ctx context.Context, clusterName string, s
return lbStatus.Ingress[0].IP, nil
}
func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domainNameLabel, clusterName string) (*network.PublicIPAddress, error) {
func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domainNameLabel, clusterName string, shouldPIPExisted bool) (*network.PublicIPAddress, error) {
pipResourceGroup := az.getPublicIPAddressResourceGroup(service)
pip, existsPip, err := az.getPublicIPAddress(pipResourceGroup, pipName)
if err != nil {
@ -497,6 +497,11 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai
}
serviceName := getServiceName(service)
if shouldPIPExisted {
return nil, fmt.Errorf("PublicIP from annotation azure-pip-name=%s for service %s doesn't exist", pipName, serviceName)
}
pip.Name = to.StringPtr(pipName)
pip.Location = to.StringPtr(az.Location)
pip.PublicIPAddressPropertiesFormat = &network.PublicIPAddressPropertiesFormat{
@ -592,7 +597,7 @@ func (az *Cloud) isFrontendIPChanged(clusterName string, config network.Frontend
if loadBalancerIP == "" {
return false, nil
}
pipName, err := az.determinePublicIPName(clusterName, service)
pipName, _, err := az.determinePublicIPName(clusterName, service)
if err != nil {
return false, err
}
@ -730,12 +735,12 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service,
fipConfigurationProperties = &configProperties
} else {
pipName, err := az.determinePublicIPName(clusterName, service)
pipName, shouldPIPExisted, err := az.determinePublicIPName(clusterName, service)
if err != nil {
return nil, err
}
domainNameLabel := getPublicIPDomainNameLabel(service)
pip, err := az.ensurePublicIPExists(service, pipName, domainNameLabel, clusterName)
pip, err := az.ensurePublicIPExists(service, pipName, domainNameLabel, clusterName, shouldPIPExisted)
if err != nil {
return nil, err
}
@ -1363,8 +1368,9 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa
var lb *network.LoadBalancer
var desiredPipName string
var err error
var shouldPIPExisted bool
if !isInternal && wantLb {
desiredPipName, err = az.determinePublicIPName(clusterName, service)
desiredPipName, shouldPIPExisted, err = az.determinePublicIPName(clusterName, service)
if err != nil {
return nil, err
}
@ -1385,32 +1391,45 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa
return nil, err
}
var found bool
var pipsToBeDeleted []*network.PublicIPAddress
for i := range pips {
pip := pips[i]
pipName := *pip.Name
if serviceOwnsPublicIP(&pip, clusterName, serviceName) {
// We need to process for pips belong to this service
pipName := *pip.Name
if wantLb && !isInternal && pipName == desiredPipName {
// This is the only case we should preserve the
// Public ip resource with match service tag
found = true
} else {
klog.V(2).Infof("reconcilePublicIP for service(%s): pip(%s) - deleting", serviceName, pipName)
err := az.safeDeletePublicIP(service, pipResourceGroup, &pip, lb)
if err != nil {
klog.Errorf("safeDeletePublicIP(%s) failed with error: %v", pipName, err)
return nil, err
}
klog.V(2).Infof("reconcilePublicIP for service(%s): pip(%s) - finished", serviceName, pipName)
pipsToBeDeleted = append(pipsToBeDeleted, &pip)
}
} else if wantLb && !isInternal && pipName == desiredPipName {
found = true
}
}
if !isInternal && shouldPIPExisted && !found && wantLb {
return nil, fmt.Errorf("reconcilePublicIP for service(%s): pip(%s) not found", serviceName, desiredPipName)
}
var deleteFuncs []func() error
for _, pip := range pipsToBeDeleted {
pipCopy := *pip
deleteFuncs = append(deleteFuncs, func() error {
klog.V(2).Infof("reconcilePublicIP for service(%s): pip(%s) - deleting", serviceName, *pip.Name)
return az.safeDeletePublicIP(service, pipResourceGroup, &pipCopy, lb)
})
}
errs := utilerrors.AggregateGoroutines(deleteFuncs...)
if errs != nil {
return nil, utilerrors.Flatten(errs)
}
if !isInternal && wantLb {
// Confirm desired public ip resource exists
var pip *network.PublicIPAddress
domainNameLabel := getPublicIPDomainNameLabel(service)
if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel, clusterName); err != nil {
if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel, clusterName, shouldPIPExisted); err != nil {
return nil, err
}
return pip, nil

View File

@ -932,7 +932,7 @@ func TestDeterminePublicIPName(t *testing.T) {
t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err)
}
}
ip, err := az.determinePublicIPName("testCluster", &service)
ip, _, err := az.determinePublicIPName("testCluster", &service)
assert.Equal(t, test.expectedIP, ip, "TestCase[%d]: %s", i, test.desc)
assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s", i, test.desc)
}
@ -1713,6 +1713,7 @@ func TestReconcilePublicIP(t *testing.T) {
testCases := []struct {
desc string
wantLb bool
annotations map[string]string
existingPIPs []network.PublicIPAddress
expectedID string
expectedPIP *network.PublicIPAddress
@ -1743,11 +1744,73 @@ func TestReconcilePublicIP(t *testing.T) {
expectedID: "/subscriptions/subscription/resourceGroups/rg/providers/" +
"Microsoft.Network/publicIPAddresses/testCluster-atest1",
},
{
desc: "reconcilePublicIP shall report error if the given PIP name doesn't exist in the resource group",
wantLb: true,
annotations: map[string]string{ServiceAnnotationPIPName: "testPIP"},
existingPIPs: []network.PublicIPAddress{
{
Name: to.StringPtr("pip1"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
},
{
Name: to.StringPtr("pip2"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
},
},
expectedError: true,
},
{
desc: "reconcilePublicIP shall delete unwanted PIP when given the name of desired PIP",
wantLb: true,
annotations: map[string]string{ServiceAnnotationPIPName: "testPIP"},
existingPIPs: []network.PublicIPAddress{
{
Name: to.StringPtr("pip1"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
},
{
Name: to.StringPtr("pip2"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
},
{
Name: to.StringPtr("testPIP"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
},
},
expectedPIP: &network.PublicIPAddress{
ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"),
Name: to.StringPtr("testPIP"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
},
},
{
desc: "reconcilePublicIP shall find the PIP by given name and shall not delete the PIP which is not owned by service",
wantLb: true,
annotations: map[string]string{ServiceAnnotationPIPName: "testPIP"},
existingPIPs: []network.PublicIPAddress{
{
Name: to.StringPtr("pip1"),
},
{
Name: to.StringPtr("pip2"),
Tags: map[string]*string{"service": to.StringPtr("default/test1")},
},
{
Name: to.StringPtr("testPIP"),
},
},
expectedPIP: &network.PublicIPAddress{
ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"),
Name: to.StringPtr("testPIP"),
},
},
}
for i, test := range testCases {
az := getTestCloud()
service := getTestService("test1", v1.ProtocolTCP, nil, 80)
service.Annotations = test.annotations
for _, pip := range test.existingPIPs {
_, err := az.PublicIPAddressesClient.CreateOrUpdate(context.TODO(), "rg", to.String(pip.Name), pip)
if err != nil {
@ -1797,7 +1860,7 @@ func TestEnsurePublicIPExists(t *testing.T) {
t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err)
}
}
pip, err := az.ensurePublicIPExists(&service, "pip1", "", "")
pip, err := az.ensurePublicIPExists(&service, "pip1", "", "", false)
if test.expectedID != "" {
assert.Equal(t, test.expectedID, to.String(pip.ID), "TestCase[%d]: %s", i, test.desc)
} else {

View File

@ -28,7 +28,7 @@ import (
"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2018-08-01/network"
"github.com/Azure/go-autorest/autorest/to"
"k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/types"
utilerrors "k8s.io/apimachinery/pkg/util/errors"
cloudprovider "k8s.io/cloud-provider"