Merge pull request #77630 from feiskyer/cluster-name-tag

Fix public IPs issues when multiple clusters are sharing the same resource group
This commit is contained in:
Kubernetes Prow Robot 2019-05-09 00:30:47 -07:00 committed by GitHub
commit e9af72c6e9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 126 additions and 11 deletions

View File

@ -83,6 +83,11 @@ const (
// ServiceAnnotationLoadBalancerMixedProtocols is the annotation used on the service // ServiceAnnotationLoadBalancerMixedProtocols is the annotation used on the service
// to create both TCP and UDP protocols when creating load balancer rules. // to create both TCP and UDP protocols when creating load balancer rules.
ServiceAnnotationLoadBalancerMixedProtocols = "service.beta.kubernetes.io/azure-load-balancer-mixed-protocols" ServiceAnnotationLoadBalancerMixedProtocols = "service.beta.kubernetes.io/azure-load-balancer-mixed-protocols"
// serviceTagKey is the service key applied for public IP tags.
serviceTagKey = "service"
// clusterNameKey is the cluster name key applied for public IP tags.
clusterNameKey = "kubernetes-cluster-name"
) )
var ( var (
@ -465,7 +470,7 @@ func (az *Cloud) findServiceIPAddress(ctx context.Context, clusterName string, s
return lbStatus.Ingress[0].IP, nil return lbStatus.Ingress[0].IP, nil
} }
func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domainNameLabel string) (*network.PublicIPAddress, error) { func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domainNameLabel, clusterName string) (*network.PublicIPAddress, error) {
pipResourceGroup := az.getPublicIPAddressResourceGroup(service) pipResourceGroup := az.getPublicIPAddressResourceGroup(service)
pip, existsPip, err := az.getPublicIPAddress(pipResourceGroup, pipName) pip, existsPip, err := az.getPublicIPAddress(pipResourceGroup, pipName)
if err != nil { if err != nil {
@ -486,7 +491,10 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai
DomainNameLabel: &domainNameLabel, DomainNameLabel: &domainNameLabel,
} }
} }
pip.Tags = map[string]*string{"service": &serviceName} pip.Tags = map[string]*string{
serviceTagKey: &serviceName,
clusterNameKey: &clusterName,
}
if az.useStandardLoadBalancer() { if az.useStandardLoadBalancer() {
pip.Sku = &network.PublicIPAddressSku{ pip.Sku = &network.PublicIPAddressSku{
Name: network.PublicIPAddressSkuNameStandard, Name: network.PublicIPAddressSkuNameStandard,
@ -711,7 +719,7 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service,
return nil, err return nil, err
} }
domainNameLabel := getPublicIPDomainNameLabel(service) domainNameLabel := getPublicIPDomainNameLabel(service)
pip, err := az.ensurePublicIPExists(service, pipName, domainNameLabel) pip, err := az.ensurePublicIPExists(service, pipName, domainNameLabel, clusterName)
if err != nil { if err != nil {
return nil, err return nil, err
} }
@ -1344,9 +1352,7 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lb *
for i := range pips { for i := range pips {
pip := pips[i] pip := pips[i]
if pip.Tags != nil && if serviceOwnsPublicIP(&pip, clusterName, serviceName) {
(pip.Tags)["service"] != nil &&
*(pip.Tags)["service"] == serviceName {
// We need to process for pips belong to this service // We need to process for pips belong to this service
pipName := *pip.Name pipName := *pip.Name
if wantLb && !isInternal && pipName == desiredPipName { if wantLb && !isInternal && pipName == desiredPipName {
@ -1369,7 +1375,7 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lb *
// Confirm desired public ip resource exists // Confirm desired public ip resource exists
var pip *network.PublicIPAddress var pip *network.PublicIPAddress
domainNameLabel := getPublicIPDomainNameLabel(service) domainNameLabel := getPublicIPDomainNameLabel(service)
if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel); err != nil { if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel, clusterName); err != nil {
return nil, err return nil, err
} }
return pip, nil return pip, nil
@ -1612,3 +1618,24 @@ func getServiceTags(service *v1.Service) ([]string, error) {
return nil, nil return nil, nil
} }
func serviceOwnsPublicIP(pip *network.PublicIPAddress, clusterName, serviceName string) bool {
if pip != nil && pip.Tags != nil {
serviceTag := pip.Tags[serviceTagKey]
clusterTag := pip.Tags[clusterNameKey]
if serviceTag != nil && *serviceTag == serviceName {
// Backward compatible for clusters upgraded from old releases.
// In such case, only "service" tag is set.
if clusterTag == nil {
return true
}
// If cluster name tag is set, then return true if it matches.
if *clusterTag == clusterName {
return true
}
}
}
return false
}

View File

@ -368,3 +368,82 @@ func TestEnsureLoadBalancerDeleted(t *testing.T) {
assert.Equal(t, len(result), 0, "TestCase[%d]: %s", i, c.desc) assert.Equal(t, len(result), 0, "TestCase[%d]: %s", i, c.desc)
} }
} }
func TestServiceOwnsPublicIP(t *testing.T) {
tests := []struct {
desc string
pip *network.PublicIPAddress
clusterName string
serviceName string
expected bool
}{
{
desc: "false should be returned when pip is nil",
clusterName: "kubernetes",
serviceName: "nginx",
expected: false,
},
{
desc: "false should be returned when service name tag doesn't match",
pip: &network.PublicIPAddress{
Tags: map[string]*string{
serviceTagKey: to.StringPtr("nginx"),
},
},
serviceName: "web",
expected: false,
},
{
desc: "true should be returned when service name tag matches and cluster name tag is not set",
pip: &network.PublicIPAddress{
Tags: map[string]*string{
serviceTagKey: to.StringPtr("nginx"),
},
},
clusterName: "kubernetes",
serviceName: "nginx",
expected: true,
},
{
desc: "false should be returned when cluster name doesn't match",
pip: &network.PublicIPAddress{
Tags: map[string]*string{
serviceTagKey: to.StringPtr("nginx"),
clusterNameKey: to.StringPtr("kubernetes"),
},
},
clusterName: "k8s",
serviceName: "nginx",
expected: false,
},
{
desc: "false should be returned when cluster name matches while service name doesn't match",
pip: &network.PublicIPAddress{
Tags: map[string]*string{
serviceTagKey: to.StringPtr("web"),
clusterNameKey: to.StringPtr("kubernetes"),
},
},
clusterName: "kubernetes",
serviceName: "nginx",
expected: false,
},
{
desc: "true should be returned when both service name tag and cluster name match",
pip: &network.PublicIPAddress{
Tags: map[string]*string{
serviceTagKey: to.StringPtr("nginx"),
clusterNameKey: to.StringPtr("kubernetes"),
},
},
clusterName: "kubernetes",
serviceName: "nginx",
expected: true,
},
}
for i, c := range tests {
owns := serviceOwnsPublicIP(c.pip, c.clusterName, c.serviceName)
assert.Equal(t, owns, c.expected, "TestCase[%d]: %s", i, c.desc)
}
}

View File

@ -1367,14 +1367,23 @@ func validatePublicIP(t *testing.T, publicIP *network.PublicIPAddress, service *
t.Errorf("Expected publicIP resource exists, when it is not an internal service") t.Errorf("Expected publicIP resource exists, when it is not an internal service")
} }
if publicIP.Tags == nil || publicIP.Tags["service"] == nil { if publicIP.Tags == nil || publicIP.Tags[serviceTagKey] == nil {
t.Errorf("Expected publicIP resource has tags[service]") t.Errorf("Expected publicIP resource has tags[%s]", serviceTagKey)
} }
serviceName := getServiceName(service) serviceName := getServiceName(service)
if serviceName != *(publicIP.Tags["service"]) { if serviceName != *(publicIP.Tags[serviceTagKey]) {
t.Errorf("Expected publicIP resource has matching tags[service]") t.Errorf("Expected publicIP resource has matching tags[%s]", serviceTagKey)
} }
if publicIP.Tags[clusterNameKey] == nil {
t.Errorf("Expected publicIP resource has tags[%s]", clusterNameKey)
}
if *(publicIP.Tags[clusterNameKey]) != testClusterName {
t.Errorf("Expected publicIP resource has matching tags[%s]", clusterNameKey)
}
// We cannot use service.Spec.LoadBalancerIP to compare with // We cannot use service.Spec.LoadBalancerIP to compare with
// Public IP's IPAddress // Public IP's IPAddress
// Because service properties are updated outside of cloudprovider code // Because service properties are updated outside of cloudprovider code