Refactor nsg access control for k8s event testing

pkg/provider/azure_loadbalancer.go

@@ -2929,32 +2929,13 @@ func (az *Cloud) reconcileSecurityGroup(
 		var opts []loadbalancer.AccessControlOption
 		if !wantLb {
 			// When deleting LB, we don't need to validate the annotation
-			opts = append(opts, loadbalancer.SkipAnnotationValidation())
+			opts = append(opts, loadbalancer.WithEventEmitter(az.Event))
 		}
 		accessControl, err = loadbalancer.NewAccessControl(logger, service, sg, opts...)
 		if err != nil {
 			logger.Error(err, "Failed to parse access control configuration for service")
 			return nil, err
 		}
-		// - use both annotation `service.beta.kubernetes.io/azure-allowed-service-tags` and `spec.loadBalancerSourceRanges`
-		//   WARNING: This issue has been around for a while, and we shouldn’t mess with the existing settings.
-		if len(accessControl.SourceRanges) > 0 && len(accessControl.AllowedServiceTags) > 0 {
-			// Suggesting to use aks custom annotation instead of spec.loadBalancerSourceRanges
-			logger.V(2).Info(
-				"Service is using both of spec.loadBalancerSourceRanges and annotation service.beta.kubernetes.io/azure-allowed-service-tags",
-			)
-			az.Event(service, v1.EventTypeWarning, "ConflictConfiguration", fmt.Sprintf(
-				"Please use annotation %s instead of spec.loadBalancerSourceRanges while using %s annotation at the same time.",
-				consts.ServiceAnnotationAllowedIPRanges, consts.ServiceAnnotationAllowedServiceTags,
-			))
-		}
-
-		if len(accessControl.InvalidRanges) > 0 {
-			az.Event(service, v1.EventTypeWarning, "InvalidConfiguration", fmt.Sprintf(
-				"Found invalid LoadBalancerSourceRanges %v, ignoring and adding a default DenyAll rule in security group.",
-				accessControl.InvalidRanges,
-			))
-		}
 	}
 
 	var (

pkg/provider/loadbalancer/accesscontrol.go

@@ -46,24 +46,24 @@ type AccessControl struct {
 	// immutable pre-compute states.
 	SourceRanges                           []netip.Prefix
 	AllowedIPRanges                        []netip.Prefix
-	InvalidRanges                          []string
 	AllowedServiceTags                     []string
+	invalidRanges                          []string
 	securityRuleDestinationPortsByProtocol map[armnetwork.SecurityRuleProtocol][]int32
 }
 
 type accessControlOptions struct {
-	SkipAnnotationValidation bool
+	EventEmitter K8sEventEmitter
 }
 
 var defaultAccessControlOptions = accessControlOptions{
-	SkipAnnotationValidation: false,
+	EventEmitter: noopEventEmitter,
 }
 
 type AccessControlOption func(*accessControlOptions)
 
-func SkipAnnotationValidation() AccessControlOption {
+func WithEventEmitter(emitter K8sEventEmitter) AccessControlOption {
 	return func(o *accessControlOptions) {
-		o.SkipAnnotationValidation = true
+		o.EventEmitter = emitter
 	}
 }
 
@@ -74,24 +74,28 @@ func NewAccessControl(logger logr.Logger, svc *v1.Service, sg *armnetwork.Securi
 	for _, opt := range opts {
 		opt(&options)
 	}
+	eventEmitter := options.EventEmitter
 
 	sgHelper, err := securitygroup.NewSecurityGroupHelper(logger, sg)
 	if err != nil {
 		logger.Error(err, "Failed to initialize RuleHelper")
 		return nil, err
 	}
 	sourceRanges, invalidSourceRanges, err := SourceRanges(svc)
-	if err != nil && !options.SkipAnnotationValidation {
+	if err != nil {
 		logger.Error(err, "Failed to parse SourceRange configuration")
+
+		// Backward compatibility: no error but emit a warning event.
+		eventEmitter(svc, v1.EventTypeWarning, "InvalidSourceRanges", EventMessageOfInvalidSourceRanges(invalidSourceRanges))
 	}
 	allowedIPRanges, invalidAllowedIPRanges, err := AllowedIPRanges(svc)
-	if err != nil && !options.SkipAnnotationValidation {
+	if err != nil {
 		logger.Error(err, "Failed to parse AllowedIPRanges configuration")
+
+		// Backward compatibility: no error but emit a warning event.
+		eventEmitter(svc, v1.EventTypeWarning, "InvalidAllowedIPRanges", EventMessageOfInvalidAllowedIPRanges(invalidAllowedIPRanges))
 	}
-	allowedServiceTags, err := AllowedServiceTags(svc)
-	if err != nil && !options.SkipAnnotationValidation {
-		logger.Error(err, "Failed to parse AllowedServiceTags configuration")
-	}
+	allowedServiceTags := AllowedServiceTags(svc)
 	securityRuleDestinationPortsByProtocol, err := SecurityRuleDestinationPortsByProtocol(svc)
 	if err != nil {
 		logger.Error(err, "Failed to parse service spec.Ports")
@@ -102,14 +106,23 @@ func NewAccessControl(logger logr.Logger, svc *v1.Service, sg *armnetwork.Securi
 		return nil, ErrSetBothLoadBalancerSourceRangesAndAllowedIPRanges
 	}
 
+	if len(sourceRanges) > 0 && len(allowedServiceTags) > 0 {
+		logger.Info(
+			"Service is using both of spec.loadBalancerSourceRanges and annotation service.beta.kubernetes.io/azure-allowed-service-tags",
+		)
+		// Backward compatibility: emit a warning event.
+		// It won't work as expected if both are used at the same time.
+		eventEmitter(svc, v1.EventTypeWarning, "ConflictConfiguration", EventMessageOfConflictLoadBalancerSourceRangesAndAllowedIPRanges())
+	}
+
 	return &AccessControl{
 		logger:                                 logger,
 		svc:                                    svc,
 		sgHelper:                               sgHelper,
 		SourceRanges:                           sourceRanges,
 		AllowedIPRanges:                        allowedIPRanges,
 		AllowedServiceTags:                     allowedServiceTags,
-		InvalidRanges:                          append(invalidSourceRanges, invalidAllowedIPRanges...),
+		invalidRanges:                          append(invalidSourceRanges, invalidAllowedIPRanges...),
 		securityRuleDestinationPortsByProtocol: securityRuleDestinationPortsByProtocol,
 	}, nil
 }
@@ -128,7 +141,7 @@ func (ac *AccessControl) IsAllowFromInternet() bool {
 	if len(ac.AllowedIPRanges) > 0 && !iputil.IsPrefixesAllowAll(ac.AllowedIPRanges) {
 		return false
 	}
-	if len(ac.InvalidRanges) > 0 {
+	if len(ac.invalidRanges) > 0 {
 		return false
 	}
 	if !IsInternal(ac.svc) {
@@ -144,7 +157,7 @@ func (ac *AccessControl) DenyAllExceptSourceRanges() bool {
 	var (
 		annotationEnabled      = strings.EqualFold(ac.svc.Annotations[consts.ServiceAnnotationDenyAllExceptLoadBalancerSourceRanges], "true")
 		sourceRangeSpecified   = len(ac.SourceRanges) > 0 || len(ac.AllowedIPRanges) > 0
-		invalidRangesSpecified = len(ac.InvalidRanges) > 0
+		invalidRangesSpecified = len(ac.invalidRanges) > 0
 	)
 	return (annotationEnabled && sourceRangeSpecified) || invalidRangesSpecified
 }

pkg/provider/loadbalancer/accesscontrol_test.go

@@ -24,6 +24,7 @@ import (
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v6"
 	"github.com/stretchr/testify/assert"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/utils/ptr"
 
 	"sigs.k8s.io/cloud-provider-azure/internal/testutil"
@@ -135,7 +136,7 @@ func TestNewAccessControl(t *testing.T) {
 		sg      = azureFx.SecurityGroup().WithRules(azureFx.NoiseSecurityRules()).Build()
 	)
 
-	t.Run("it should return error if using both spec.LoadBalancerSourceRanges and service annotation service.beta.kubernetes.io/azure-allowed-ip-ranges", func(t *testing.T) {
+	t.Run("it should return error if using spec.LoadBalancerSourceRanges and azure-allowed-ip-ranges", func(t *testing.T) {
 		svc := k8sFx.Service().
 			WithLoadBalancerSourceRanges("20.0.0.1/32").
 			WithAllowedIPRanges("10.0.0.1/32").
@@ -144,6 +145,64 @@ func TestNewAccessControl(t *testing.T) {
 		_, err := NewAccessControl(log.Noop(), &svc, sg)
 		assert.ErrorIs(t, err, ErrSetBothLoadBalancerSourceRangesAndAllowedIPRanges)
 	})
+
+	t.Run("it should emit warning event if invalid spec.LoadBalancerSourceRanges", func(t *testing.T) {
+		svc := k8sFx.Service().
+			WithLoadBalancerSourceRanges("foo", "10.0.0.1/32", "bar").
+			Build()
+
+		called := 0
+		eventEmitter := func(obj runtime.Object, eventType, reason, message string) {
+			called++
+			assert.Equal(t, &svc, obj)
+			assert.Equal(t, v1.EventTypeWarning, eventType)
+			assert.Equal(t, "InvalidSourceRanges", reason)
+			assert.Equal(t, EventMessageOfInvalidSourceRanges([]string{"foo", "bar"}), message)
+		}
+
+		_, err := NewAccessControl(log.Noop(), &svc, sg, WithEventEmitter(eventEmitter))
+		assert.NoError(t, err)
+		assert.Equal(t, 1, called)
+	})
+
+	t.Run("it should emit warning event if invalid azure-allowed-ip-ranges", func(t *testing.T) {
+		svc := k8sFx.Service().
+			WithAllowedIPRanges("foo", "10.0.0.1/32", "bar").
+			Build()
+
+		called := 0
+		eventEmitter := func(obj runtime.Object, eventType, reason, message string) {
+			called++
+			assert.Equal(t, &svc, obj)
+			assert.Equal(t, v1.EventTypeWarning, eventType)
+			assert.Equal(t, "InvalidAllowedIPRanges", reason)
+			assert.Equal(t, EventMessageOfInvalidAllowedIPRanges([]string{"foo", "bar"}), message)
+		}
+
+		_, err := NewAccessControl(log.Noop(), &svc, sg, WithEventEmitter(eventEmitter))
+		assert.NoError(t, err)
+		assert.Equal(t, 1, called)
+	})
+
+	t.Run("it should emit warning event if using spec.LoadBalancerSourceRanges and azure-allowed-service-tags", func(t *testing.T) {
+		svc := k8sFx.Service().
+			WithLoadBalancerSourceRanges("20.0.0.1/32").
+			WithAllowedServiceTags("AKS").
+			Build()
+
+		called := 0
+		eventEmitter := func(obj runtime.Object, eventType, reason, message string) {
+			called++
+			assert.Equal(t, &svc, obj)
+			assert.Equal(t, v1.EventTypeWarning, eventType)
+			assert.Equal(t, "ConflictConfiguration", reason)
+			assert.Equal(t, EventMessageOfConflictLoadBalancerSourceRangesAndAllowedIPRanges(), message)
+		}
+
+		_, err := NewAccessControl(log.Noop(), &svc, sg, WithEventEmitter(eventEmitter))
+		assert.NoError(t, err)
+		assert.Equal(t, 1, called)
+	})
 }
 
 func TestAccessControl_DenyAllExceptSourceRanges(t *testing.T) {

pkg/provider/loadbalancer/configuration.go

@@ -35,22 +35,22 @@ func IsInternal(svc *v1.Service) bool {
 }
 
 // AllowedServiceTags returns the allowed service tags configured by user through AKS custom annotation.
-func AllowedServiceTags(svc *v1.Service) ([]string, error) {
+func AllowedServiceTags(svc *v1.Service) []string {
 	const (
 		Sep = ","
 		Key = consts.ServiceAnnotationAllowedServiceTags
 	)
 
 	value, found := svc.Annotations[Key]
 	if !found {
-		return nil, nil
+		return nil
 	}
 
 	tags := strings.Split(strings.TrimSpace(value), Sep)
 	for i := range tags {
 		tags[i] = strings.TrimSpace(tags[i])
 	}
-	return tags, nil
+	return tags
 }
 
 // AllowedIPRanges returns the allowed IP ranges configured by user through AKS custom annotations:

pkg/provider/loadbalancer/configuration_test.go

@@ -70,19 +70,18 @@ func TestIsInternal(t *testing.T) {
 
 func TestAllowedServiceTags(t *testing.T) {
 	t.Run("no annotation", func(t *testing.T) {
-		actual, err := AllowedServiceTags(&v1.Service{
+		actual := AllowedServiceTags(&v1.Service{
 			Spec: v1.ServiceSpec{
 				Type: v1.ServiceTypeLoadBalancer,
 			},
 			ObjectMeta: metav1.ObjectMeta{
 				Annotations: map[string]string{},
 			},
 		})
-		assert.NoError(t, err)
 		assert.Empty(t, actual)
 	})
 	t.Run("with 1 service tag", func(t *testing.T) {
-		actual, err := AllowedServiceTags(&v1.Service{
+		actual := AllowedServiceTags(&v1.Service{
 			Spec: v1.ServiceSpec{
 				Type: v1.ServiceTypeLoadBalancer,
 			},
@@ -92,11 +91,10 @@ func TestAllowedServiceTags(t *testing.T) {
 				},
 			},
 		})
-		assert.NoError(t, err)
 		assert.Equal(t, []string{"Microsoft.ContainerInstance/containerGroups"}, actual)
 	})
 	t.Run("with multiple service tags", func(t *testing.T) {
-		actual, err := AllowedServiceTags(&v1.Service{
+		actual := AllowedServiceTags(&v1.Service{
 			Spec: v1.ServiceSpec{
 				Type: v1.ServiceTypeLoadBalancer,
 			},
@@ -106,7 +104,6 @@ func TestAllowedServiceTags(t *testing.T) {
 				},
 			},
 		})
-		assert.NoError(t, err)
 		assert.Equal(t, []string{"Microsoft.ContainerInstance/containerGroups", "foo", "bar"}, actual)
 	})
 }

pkg/provider/loadbalancer/k8sevent.go

@@ -0,0 +1,51 @@
+/*
+Copyright 2024 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package loadbalancer
+
+import (
+	"fmt"
+
+	"sigs.k8s.io/cloud-provider-azure/pkg/consts"
+
+	"k8s.io/apimachinery/pkg/runtime"
+)
+
+type K8sEventEmitter func(obj runtime.Object, eventType, reason, message string)
+
+func noopEventEmitter(_ runtime.Object, _, _, _ string) {}
+
+func EventMessageOfInvalidSourceRanges(sourceRanges []string) string {
+	return fmt.Sprintf(
+		"Found invalid spec.LoadBalancerSourceRanges %q, ignoring and adding a default DenyAll rule in security group.",
+		sourceRanges,
+	)
+}
+
+func EventMessageOfInvalidAllowedIPRanges(allowedIPRanges []string) string {
+	return fmt.Sprintf("Found invalid %s %q, ignoring and adding a default DenyAll rule in security group.",
+		consts.ServiceAnnotationAllowedIPRanges,
+		allowedIPRanges,
+	)
+}
+
+func EventMessageOfConflictLoadBalancerSourceRangesAndAllowedIPRanges() string {
+	return fmt.Sprintf(
+		"Please use annotation %s instead of spec.loadBalancerSourceRanges while using %s annotation at the same time.",
+		consts.ServiceAnnotationAllowedIPRanges,
+		consts.ServiceAnnotationAllowedServiceTags,
+	)
+}