Navigation Menu

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.

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

Add service annotation for specifying load balancer's pip with name. #81213

Merged
merged 2 commits into from Aug 13, 2019
Merged

Add service annotation for specifying load balancer's pip with name. #81213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
034f96c
Add service annotation for specifying load balancer's pip with name.
Aug 9, 2019
3facb63
Modify the logic to discover corresponding errors.
Aug 12, 2019
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
69 changes: 49 additions & 20 deletions staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go
View file
Open in desktop
Expand Up @@ -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"
Expand Down Expand Up @@ -72,6 +73,9 @@ 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"

// 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
// Refer https://docs.microsoft.com/en-us/azure/virtual-network/security-overview#service-tags for all supported service tags.
Expand Down Expand Up @@ -410,26 +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) {
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
}

feiskyer marked this conversation as resolved.
Show resolved Hide resolved
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 {
Expand Down Expand Up @@ -476,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 {
Expand All @@ -487,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{
Expand Down Expand Up @@ -582,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
}
Expand Down Expand Up @@ -720,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
}
Expand Down Expand Up @@ -1353,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
}
Expand All @@ -1375,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
Expand Down
67 changes: 65 additions & 2 deletions staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go
View file
Open in desktop
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/legacy-cloud-providers/azure/azure_vmss.go
View file
Open in desktop
Expand Up @@ -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"
Expand Down