Repository: kubernetes-sigs/cloud-provider-azure
Pull request #487 (2021-01-22 12:28:59)

feat: Add service annotation `ServiceAnnotationDenyAllExpectSourceRanges`
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Please label this pull request according to what type of issue you are addressing, especially if this is a release targeted pull request. For reference on required PR/issue labels, read here:
https://git.k8s.io/community/contributors/devel/release.md#issue-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://git.k8s.io/community/contributors/devel/testing.md
4. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
5. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
6. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**
> Uncomment only one ` /kind <>` line, hit enter to put that in a new line, and remove leading whitespaces from that line:
>
> /kind api-change
> /kind bug
> /kind cleanup
> /kind design
> /kind documentation
> /kind failing-test
> /kind feature
> /kind flake
/kind feature

**What this PR does / why we need it**:
Add service annotation `ServiceAnnotationDenyAllExpectSourceRanges`.

**Which issue(s) this PR fixes**:
<!-- 
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes https://github.com/kubernetes-sigs/cloud-provider-azure/issues/374

**Special notes for your reviewer**:
1. This annotation only works when the `service.Spec.LoadBalancerSourceRanges` is set.
2. It creates a security rule with the name `a<service-uid>-<service-protocol>-<port>-deny_all`, which denies all traffic to the service.
3. The priority of the deny rule is lower than the allow rule.

**Release note**:
```
feat: Add service annotation `ServiceAnnotationDenyAllExpectSourceRanges`
```



Patch:
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
index 480382b37..f9b6ecdda 100644
--- a/pkg/provider/azure_loadbalancer.go
+++ b/pkg/provider/azure_loadbalancer.go
@@ -90,6 +90,10 @@ const (
 	// Refer https://docs.microsoft.com/en-us/azure/virtual-network/security-overview#service-tags for all supported service tags.
 	ServiceAnnotationAllowedServiceTag = "service.beta.kubernetes.io/azure-allowed-service-tags"
 
+	// ServiceAnnotationDenyAllExceptLoadBalancerSourceRanges  denies all traffic to the load balancer except those
+	// within the service.Spec.LoadBalancerSourceRanges. Ref: https://github.com/kubernetes-sigs/cloud-provider-azure/issues/374.
+	ServiceAnnotationDenyAllExceptLoadBalancerSourceRanges = "service.beta.kubernetes.io/azure-deny-all-except-load-balancer-source-ranges"
+
 	// ServiceAnnotationLoadBalancerIdleTimeout is the annotation used on the service
 	// to specify the idle timeout for connections on the load balancer in minutes.
 	ServiceAnnotationLoadBalancerIdleTimeout = "service.beta.kubernetes.io/azure-load-balancer-tcp-idle-timeout"
@@ -1909,6 +1913,34 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service,
 				}
 			}
 		}
+
+		shouldAddDenyRule := false
+		if len(sourceRanges) > 0 {
+			if v, ok := service.Annotations[ServiceAnnotationDenyAllExceptLoadBalancerSourceRanges]; ok && strings.EqualFold(v, "true") {
+				shouldAddDenyRule = true
+			}
+		}
+		if shouldAddDenyRule {
+			for _, port := range ports {
+				_, securityProto, _, err := getProtocolsFromKubernetesProtocol(port.Protocol)
+				if err != nil {
+					return nil, err
+				}
+				securityRuleName := az.getSecurityRuleName(service, port, "deny_all")
+				expectedSecurityRules = append(expectedSecurityRules, network.SecurityRule{
+					Name: to.StringPtr(securityRuleName),
+					SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
+						Protocol:                 *securityProto,
+						SourcePortRange:          to.StringPtr("*"),
+						DestinationPortRange:     to.StringPtr(strconv.Itoa(int(port.Port))),
+						SourceAddressPrefix:      to.StringPtr("*"),
+						DestinationAddressPrefix: to.StringPtr(destinationIPAddress),
+						Access:                   network.SecurityRuleAccessDeny,
+						Direction:                network.SecurityRuleDirectionInbound,
+					},
+				})
+			}
+		}
 	}
 
 	for _, r := range expectedSecurityRules {


Test patch:
diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
index 2ede7ff27..74762418f 100644
--- a/pkg/provider/azure_loadbalancer_test.go
+++ b/pkg/provider/azure_loadbalancer_test.go
@@ -2444,6 +2444,61 @@ func TestReconcileSecurityGroup(t *testing.T) {
 	}
 }
 
+func TestReconcileSecurityGroupLoadBalancerSourceRanges(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	az := GetTestCloud(ctrl)
+	service := getTestService("test1", v1.ProtocolTCP, map[string]string{ServiceAnnotationDenyAllExceptLoadBalancerSourceRanges: "true"}, false, 80)
+	service.Spec.LoadBalancerSourceRanges = []string{"1.2.3.4/32"}
+	existingSg := network.SecurityGroup{
+		Name: to.StringPtr("nsg"),
+		SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{
+			SecurityRules: &[]network.SecurityRule{},
+		},
+	}
+	lbIP := to.StringPtr("1.1.1.1")
+	expectedSg := network.SecurityGroup{
+		Name: to.StringPtr("nsg"),
+		SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{
+			SecurityRules: &[]network.SecurityRule{
+				{
+					Name: to.StringPtr("atest1-TCP-80-1.2.3.4_32"),
+					SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
+						Protocol:                 network.SecurityRuleProtocol("Tcp"),
+						SourcePortRange:          to.StringPtr("*"),
+						SourceAddressPrefix:      to.StringPtr("1.2.3.4/32"),
+						DestinationPortRange:     to.StringPtr("80"),
+						DestinationAddressPrefix: to.StringPtr("1.1.1.1"),
+						Access:                   network.SecurityRuleAccess("Allow"),
+						Priority:                 to.Int32Ptr(500),
+						Direction:                network.SecurityRuleDirection("Inbound"),
+					},
+				},
+				{
+					Name: to.StringPtr("atest1-TCP-80-deny_all"),
+					SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
+						Protocol:                 network.SecurityRuleProtocol("Tcp"),
+						SourcePortRange:          to.StringPtr("*"),
+						SourceAddressPrefix:      to.StringPtr("*"),
+						DestinationPortRange:     to.StringPtr("80"),
+						DestinationAddressPrefix: to.StringPtr("1.1.1.1"),
+						Access:                   network.SecurityRuleAccess("Deny"),
+						Priority:                 to.Int32Ptr(501),
+						Direction:                network.SecurityRuleDirection("Inbound"),
+					},
+				},
+			},
+		},
+	}
+	mockSGClient := az.SecurityGroupsClient.(*mocksecuritygroupclient.MockInterface)
+	mockSGClient.EXPECT().Get(gomock.Any(), az.ResourceGroup, gomock.Any(), gomock.Any()).Return(existingSg, nil)
+	mockSGClient.EXPECT().CreateOrUpdate(gomock.Any(), az.ResourceGroup, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
+	sg, err := az.reconcileSecurityGroup("testCluster", &service, lbIP, true)
+	assert.NoError(t, err)
+	assert.Equal(t, expectedSg, *sg)
+}
+
 func TestSafeDeletePublicIP(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()