From 1397235ffac0c10fa16860ad22b805df919eb74a Mon Sep 17 00:00:00 2001 From: Unknown Date: Wed, 18 Oct 2017 19:52:09 +0000 Subject: [PATCH 1/3] Restrict Azure NSG rules to allow external access only to load balancer IP --- .../providers/azure/azure_loadbalancer.go | 105 +++++++++++------- .../providers/azure/azure_test.go | 10 +- 2 files changed, 71 insertions(+), 44 deletions(-) diff --git a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go index 5ad04a8f85a..5aaea20639c 100644 --- a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go +++ b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go @@ -134,42 +134,6 @@ func (az *Cloud) EnsureLoadBalancer(clusterName string, service *v1.Service, nod serviceName := getServiceName(service) glog.V(5).Infof("ensure(%s): START clusterName=%q lbName=%q", serviceName, clusterName, lbName) - az.operationPollRateLimiter.Accept() - glog.V(10).Infof("SecurityGroupsClient.Get(%q): start", az.SecurityGroupName) - sg, err := az.SecurityGroupsClient.Get(az.ResourceGroup, az.SecurityGroupName, "") - glog.V(10).Infof("SecurityGroupsClient.Get(%q): end", az.SecurityGroupName) - if err != nil { - return nil, err - } - sg, sgNeedsUpdate, err := az.reconcileSecurityGroup(sg, clusterName, service, true /* wantLb */) - if err != nil { - return nil, err - } - if sgNeedsUpdate { - glog.V(3).Infof("ensure(%s): sg(%s) - updating", serviceName, *sg.Name) - // azure-sdk-for-go introduced contraint validation which breaks the updating here if we don't set these - // to nil. This is a workaround until https://github.com/Azure/go-autorest/issues/112 is fixed - sg.SecurityGroupPropertiesFormat.NetworkInterfaces = nil - sg.SecurityGroupPropertiesFormat.Subnets = nil - az.operationPollRateLimiter.Accept() - glog.V(10).Infof("SecurityGroupsClient.CreateOrUpdate(%q): start", *sg.Name) - respChan, errChan := az.SecurityGroupsClient.CreateOrUpdate(az.ResourceGroup, *sg.Name, sg, nil) - resp := <-respChan - err := <-errChan - glog.V(10).Infof("SecurityGroupsClient.CreateOrUpdate(%q): end", *sg.Name) - if az.CloudProviderBackoff && shouldRetryAPIRequest(resp.Response, err) { - glog.V(2).Infof("ensure(%s) backing off: sg(%s) - updating", serviceName, *sg.Name) - retryErr := az.CreateOrUpdateSGWithRetry(sg) - if retryErr != nil { - glog.V(2).Infof("ensure(%s) abort backoff: sg(%s) - updating", serviceName, *sg.Name) - return nil, retryErr - } - } - if err != nil { - return nil, err - } - } - lb, existsLb, err := az.getAzureLoadBalancer(lbName) if err != nil { return nil, err @@ -257,6 +221,54 @@ func (az *Cloud) EnsureLoadBalancer(clusterName string, service *v1.Service, nod } } + var lbStatus *v1.LoadBalancerStatus + if lbIP == nil { + lbStatus, exists, err := az.GetLoadBalancer(clusterName, service) + if err != nil { + return nil, err + } + if !exists { + return nil, fmt.Errorf("ensure(%s): lb(%s) - failed to get back load balancer", serviceName, lbName) + } + lbIP = &lbStatus.Ingress[0].IP + } + + az.operationPollRateLimiter.Accept() + glog.V(10).Infof("SecurityGroupsClient.Get(%q): start", az.SecurityGroupName) + sg, err := az.SecurityGroupsClient.Get(az.ResourceGroup, az.SecurityGroupName, "") + glog.V(10).Infof("SecurityGroupsClient.Get(%q): end", az.SecurityGroupName) + if err != nil { + return nil, err + } + sg, sgNeedsUpdate, err := az.reconcileSecurityGroup(sg, clusterName, service, lbIP, true /* wantLb */) + if err != nil { + return nil, err + } + if sgNeedsUpdate { + glog.V(3).Infof("ensure(%s): sg(%s) - updating", serviceName, *sg.Name) + // azure-sdk-for-go introduced contraint validation which breaks the updating here if we don't set these + // to nil. This is a workaround until https://github.com/Azure/go-autorest/issues/112 is fixed + sg.SecurityGroupPropertiesFormat.NetworkInterfaces = nil + sg.SecurityGroupPropertiesFormat.Subnets = nil + az.operationPollRateLimiter.Accept() + glog.V(10).Infof("SecurityGroupsClient.CreateOrUpdate(%q): start", *sg.Name) + respChan, errChan := az.SecurityGroupsClient.CreateOrUpdate(az.ResourceGroup, *sg.Name, sg, nil) + resp := <-respChan + err := <-errChan + glog.V(10).Infof("SecurityGroupsClient.CreateOrUpdate(%q): end", *sg.Name) + if az.CloudProviderBackoff && shouldRetryAPIRequest(resp.Response, err) { + glog.V(2).Infof("ensure(%s) backing off: sg(%s) - updating", serviceName, *sg.Name) + retryErr := az.CreateOrUpdateSGWithRetry(sg) + if retryErr != nil { + glog.V(2).Infof("ensure(%s) abort backoff: sg(%s) - updating", serviceName, *sg.Name) + return nil, retryErr + } + } + if err != nil { + return nil, err + } + } + // Add the machines to the backend pool if they're not already lbBackendName := getBackendPoolName(clusterName) lbBackendPoolID := az.getBackendPoolID(lbName, lbBackendName) @@ -280,6 +292,10 @@ func (az *Cloud) EnsureLoadBalancer(clusterName string, service *v1.Service, nod glog.V(2).Infof("ensure(%s): lb(%s) finished", serviceName, lbName) + if lbStatus != nil { + return lbStatus, nil + } + if lbIP == nil { lbStatus, exists, err := az.GetLoadBalancer(clusterName, service) if err != nil { @@ -325,7 +341,7 @@ func (az *Cloud) EnsureLoadBalancerDeleted(clusterName string, service *v1.Servi return err } if existsSg { - reconciledSg, sgNeedsUpdate, reconcileErr := az.reconcileSecurityGroup(sg, clusterName, service, false /* wantLb */) + reconciledSg, sgNeedsUpdate, reconcileErr := az.reconcileSecurityGroup(sg, clusterName, service, nil, false /* wantLb */) if reconcileErr != nil { return reconcileErr } @@ -784,7 +800,7 @@ func (az *Cloud) reconcileLoadBalancer(lb network.LoadBalancer, fipConfiguration // This reconciles the Network Security Group similar to how the LB is reconciled. // This entails adding required, missing SecurityRules and removing stale rules. -func (az *Cloud) reconcileSecurityGroup(sg network.SecurityGroup, clusterName string, service *v1.Service, wantLb bool) (network.SecurityGroup, bool, error) { +func (az *Cloud) reconcileSecurityGroup(sg network.SecurityGroup, clusterName string, service *v1.Service, lbIP *string, wantLb bool) (network.SecurityGroup, bool, error) { serviceName := getServiceName(service) var ports []v1.ServicePort if wantLb { @@ -793,6 +809,17 @@ func (az *Cloud) reconcileSecurityGroup(sg network.SecurityGroup, clusterName st ports = []v1.ServicePort{} } + destinationIPAddress := "" + if wantLb { + if lbIP == nil { + return sg, false, fmt.Errorf("No load balancer IP for setting up security rules for service %s", service.Name) + } + destinationIPAddress = *lbIP + } + if destinationIPAddress == "" { + destinationIPAddress = "*" + } + sourceRanges, err := serviceapi.GetLoadBalancerSourceRanges(service) if err != nil { return sg, false, err @@ -824,7 +851,7 @@ func (az *Cloud) reconcileSecurityGroup(sg network.SecurityGroup, clusterName st SourcePortRange: to.StringPtr("*"), DestinationPortRange: to.StringPtr(strconv.Itoa(int(port.Port))), SourceAddressPrefix: to.StringPtr(sourceAddressPrefixes[j]), - DestinationAddressPrefix: to.StringPtr("*"), + DestinationAddressPrefix: to.StringPtr(destinationIPAddress), Access: network.SecurityRuleAccessAllow, Direction: network.SecurityRuleDirectionInbound, }, diff --git a/pkg/cloudprovider/providers/azure/azure_test.go b/pkg/cloudprovider/providers/azure/azure_test.go index e579a159fb6..4bc24260dfa 100644 --- a/pkg/cloudprovider/providers/azure/azure_test.go +++ b/pkg/cloudprovider/providers/azure/azure_test.go @@ -299,7 +299,7 @@ func TestReconcileSecurityGroupNewServiceAddsPort(t *testing.T) { sg := getTestSecurityGroup() - sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc1, true) + sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc1, to.StringPtr("13.70.140.150"), true) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -313,7 +313,7 @@ func TestReconcileSecurityGroupNewInternalServiceAddsPort(t *testing.T) { sg := getTestSecurityGroup() - sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc1, true) + sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc1, to.StringPtr("13.70.140.150"), true) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -329,7 +329,7 @@ func TestReconcileSecurityGroupRemoveService(t *testing.T) { validateSecurityGroup(t, sg, service1, service2) az := getTestCloud() - sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &service1, false) + sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &service1, nil, false) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -344,7 +344,7 @@ func TestReconcileSecurityGroupRemoveServiceRemovesPort(t *testing.T) { sg := getTestSecurityGroup(svc) svcUpdated := getTestService("servicea", v1.ProtocolTCP, 80) - sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svcUpdated, true) + sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svcUpdated, to.StringPtr("13.70.140.150"), true) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -361,7 +361,7 @@ func TestReconcileSecurityWithSourceRanges(t *testing.T) { } sg := getTestSecurityGroup(svc) - sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc, true) + sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc, to.StringPtr("13.70.140.150"), true) if err != nil { t.Errorf("Unexpected error: %q", err) } From be42a972f4fc06e96e0a4b21d69e579cb3fac32f Mon Sep 17 00:00:00 2001 From: Unknown Date: Thu, 19 Oct 2017 19:03:09 +0000 Subject: [PATCH 2/3] Use RFC1918 addresses in tests --- pkg/cloudprovider/providers/azure/azure_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cloudprovider/providers/azure/azure_test.go b/pkg/cloudprovider/providers/azure/azure_test.go index 4bc24260dfa..da9776e844b 100644 --- a/pkg/cloudprovider/providers/azure/azure_test.go +++ b/pkg/cloudprovider/providers/azure/azure_test.go @@ -299,7 +299,7 @@ func TestReconcileSecurityGroupNewServiceAddsPort(t *testing.T) { sg := getTestSecurityGroup() - sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc1, to.StringPtr("13.70.140.150"), true) + sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc1, to.StringPtr("192.168.0.0"), true) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -313,7 +313,7 @@ func TestReconcileSecurityGroupNewInternalServiceAddsPort(t *testing.T) { sg := getTestSecurityGroup() - sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc1, to.StringPtr("13.70.140.150"), true) + sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc1, to.StringPtr("192.168.0.0"), true) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -344,7 +344,7 @@ func TestReconcileSecurityGroupRemoveServiceRemovesPort(t *testing.T) { sg := getTestSecurityGroup(svc) svcUpdated := getTestService("servicea", v1.ProtocolTCP, 80) - sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svcUpdated, to.StringPtr("13.70.140.150"), true) + sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svcUpdated, to.StringPtr("192.168.0.0"), true) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -361,7 +361,7 @@ func TestReconcileSecurityWithSourceRanges(t *testing.T) { } sg := getTestSecurityGroup(svc) - sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc, to.StringPtr("13.70.140.150"), true) + sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc, to.StringPtr("192.168.0.0"), true) if err != nil { t.Errorf("Unexpected error: %q", err) } From ea1c5a4b383465f88962ad92cec549168330ddbd Mon Sep 17 00:00:00 2001 From: Ivan Towlson Date: Tue, 31 Oct 2017 14:31:53 +1300 Subject: [PATCH 3/3] Remove azure-sdk-for-go workaround that is no longer needed --- pkg/cloudprovider/providers/azure/azure_loadbalancer.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go index 5aaea20639c..bdbea788998 100644 --- a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go +++ b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go @@ -246,10 +246,6 @@ func (az *Cloud) EnsureLoadBalancer(clusterName string, service *v1.Service, nod } if sgNeedsUpdate { glog.V(3).Infof("ensure(%s): sg(%s) - updating", serviceName, *sg.Name) - // azure-sdk-for-go introduced contraint validation which breaks the updating here if we don't set these - // to nil. This is a workaround until https://github.com/Azure/go-autorest/issues/112 is fixed - sg.SecurityGroupPropertiesFormat.NetworkInterfaces = nil - sg.SecurityGroupPropertiesFormat.Subnets = nil az.operationPollRateLimiter.Accept() glog.V(10).Infof("SecurityGroupsClient.CreateOrUpdate(%q): start", *sg.Name) respChan, errChan := az.SecurityGroupsClient.CreateOrUpdate(az.ResourceGroup, *sg.Name, sg, nil) @@ -347,10 +343,6 @@ func (az *Cloud) EnsureLoadBalancerDeleted(clusterName string, service *v1.Servi } if sgNeedsUpdate { glog.V(3).Infof("delete(%s): sg(%s) - updating", serviceName, az.SecurityGroupName) - // azure-sdk-for-go introduced contraint validation which breaks the updating here if we don't set these - // to nil. This is a workaround until https://github.com/Azure/go-autorest/issues/112 is fixed - sg.SecurityGroupPropertiesFormat.NetworkInterfaces = nil - sg.SecurityGroupPropertiesFormat.Subnets = nil az.operationPollRateLimiter.Accept() glog.V(10).Infof("SecurityGroupsClient.CreateOrUpdate(%q): start", *reconciledSg.Name) respChan, errChan := az.SecurityGroupsClient.CreateOrUpdate(az.ResourceGroup, *reconciledSg.Name, reconciledSg, nil)