Do not allow to change env for Radix DNS alias (#1019)

* Do not allow to change env for Radix DNS alias * Validate DNS alias appName. Cleanup * Updated chart version
equinor · Dec 28, 2023 · bd911cb · bd911cb
1 parent a785a91
commit bd911cb
Show file tree

Hide file tree

Showing 6 changed files with 117 additions and 46 deletions.
diff --git a/charts/radix-operator/Chart.yaml b/charts/radix-operator/Chart.yaml
@@ -1,7 +1,7 @@
 apiVersion: v2
 name: radix-operator
-version: 1.27.2
-appVersion: 1.47.2
+version: 1.27.3
+appVersion: 1.47.3
 kubeVersion: ">=1.24.0"
 description: Radix Operator
 keywords:

diff --git a/pkg/apis/applicationconfig/dns_alias.go b/pkg/apis/applicationconfig/dns_alias.go
@@ -2,6 +2,7 @@ package applicationconfig
 
 import (
 	stderrors "errors"
+	"fmt"
 	"strings"
 
 	"github.com/equinor/radix-operator/pkg/apis/kube"
@@ -15,7 +16,10 @@ func (app *ApplicationConfig) syncDNSAliases() error {
 	if err != nil {
 		return err
 	}
-	aliasesToCreate, aliasesToUpdate, aliasesToDelete := app.getDNSAliasesToSync(existingAliases)
+	aliasesToCreate, aliasesToUpdate, aliasesToDelete, err := app.getDNSAliasesToSync(existingAliases)
+	if err != nil {
+		return err
+	}
 	var errs []error
 	// first - delete
 	for _, dnsAlias := range aliasesToDelete {
@@ -38,28 +42,36 @@ func (app *ApplicationConfig) syncDNSAliases() error {
 	return stderrors.Join(errs...)
 }
 
-func (app *ApplicationConfig) getDNSAliasesToSync(existingAliases map[string]*radixv1.RadixDNSAlias) ([]*radixv1.RadixDNSAlias, []*radixv1.RadixDNSAlias, []*radixv1.RadixDNSAlias) {
+func (app *ApplicationConfig) getDNSAliasesToSync(existingAliases map[string]*radixv1.RadixDNSAlias) ([]*radixv1.RadixDNSAlias, []*radixv1.RadixDNSAlias, []*radixv1.RadixDNSAlias, error) {
 	var aliasesToCreate, aliasesToUpdate, aliasesToDelete []*radixv1.RadixDNSAlias
 	processedAliases := make(map[string]any)
 	appName := app.registration.Name
+	var errs []error
 	for _, dnsAlias := range app.config.Spec.DNSAlias {
 		if existingAlias, exists := existingAliases[dnsAlias.Alias]; exists {
-			updatingRadixDNSAlias := existingAlias.DeepCopy()
-			updatingRadixDNSAlias.Spec.AppName = appName
-			updatingRadixDNSAlias.Spec.Environment = dnsAlias.Environment
-			updatingRadixDNSAlias.Spec.Component = dnsAlias.Component
-			aliasesToUpdate = append(aliasesToUpdate, updatingRadixDNSAlias)
-			processedAliases[dnsAlias.Alias] = true
-			continue
+			if existingAlias.Spec.AppName != appName {
+				errs = append(errs, fmt.Errorf("DNS Alias %s of the application %s is used by another application", appName, dnsAlias.Alias))
+				continue
+			}
+			if existingAlias.Spec.Environment == dnsAlias.Environment {
+				updatingRadixDNSAlias := existingAlias.DeepCopy()
+				updatingRadixDNSAlias.Spec.Component = dnsAlias.Component
+				aliasesToUpdate = append(aliasesToUpdate, updatingRadixDNSAlias)
+				processedAliases[dnsAlias.Alias] = true
+				continue
+			}
 		}
 		aliasesToCreate = append(aliasesToCreate, app.buildRadixDNSAlias(appName, dnsAlias)) // new alias or an alias with changed environment or component
 	}
+	if len(errs) > 0 {
+		return nil, nil, nil, stderrors.Join(errs...)
+	}
 	for aliasName, dnsAlias := range existingAliases {
 		if _, ok := processedAliases[aliasName]; !ok && strings.EqualFold(dnsAlias.Spec.AppName, appName) {
 			aliasesToDelete = append(aliasesToDelete, dnsAlias)
 		}
 	}
-	return aliasesToCreate, aliasesToUpdate, aliasesToDelete
+	return aliasesToCreate, aliasesToUpdate, aliasesToDelete, nil
 }
 
 func (app *ApplicationConfig) buildRadixDNSAlias(appName string, dnsAlias radixv1.DNSAlias) *radixv1.RadixDNSAlias {

diff --git a/pkg/apis/applicationconfig/dns_alias_test.go b/pkg/apis/applicationconfig/dns_alias_test.go
@@ -2,6 +2,7 @@ package applicationconfig_test
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"testing"
 
@@ -283,3 +284,88 @@ func Test_DNSAliases_CreateUpdateDelete(t *testing.T) {
 		})
 	}
 }
+
+func Test_DNSAliases_ValidDNSAliases(t *testing.T) {
+	const (
+		appName1   = "any-app1"
+		appName2   = "any-app2"
+		env1       = "env1"
+		component1 = "server1"
+		component2 = "server2"
+		branch1    = "branch1"
+		portA      = "port-a"
+		port8080   = 8080
+		port9090   = 9090
+		alias1     = "alias1"
+		alias2     = "alias2"
+	)
+	var testScenarios = []struct {
+		name                    string
+		applicationBuilder      utils.ApplicationBuilder
+		existingRadixDNSAliases map[string]commonTest.DNSAlias
+		expectedError           error
+	}{
+		{
+			name: "exist aliases with different appName, failed",
+			applicationBuilder: utils.ARadixApplication().WithAppName(appName1).WithEnvironment(env1, branch1).
+				WithDNSAlias(radixv1.DNSAlias{Alias: alias1, Environment: env1, Component: component1}).
+				WithComponent(utils.NewApplicationComponentBuilder().WithName(component1).WithPort(portA, port8080).WithPublicPort(portA)),
+			existingRadixDNSAliases: map[string]commonTest.DNSAlias{
+				alias1: {AppName: appName2, Environment: env1, Component: component1, Port: port8080},
+			},
+			expectedError: errors.New("failed to process DNS aliases: DNS Alias any-app1 of the application alias1 is used by another application"),
+		},
+		{
+			name: "exist aliases with same component and port, no error",
+			applicationBuilder: utils.ARadixApplication().WithAppName(appName1).WithEnvironment(env1, branch1).
+				WithDNSAlias(radixv1.DNSAlias{Alias: alias1, Environment: env1, Component: component1}).
+				WithComponent(utils.NewApplicationComponentBuilder().WithName(component1).WithPort(portA, port8080).WithPublicPort(portA)),
+			existingRadixDNSAliases: map[string]commonTest.DNSAlias{
+				alias1: {AppName: appName1, Environment: env1, Component: component1, Port: port8080},
+			},
+			expectedError: nil,
+		},
+		{
+			name: "exist different alias with different name, no error",
+			applicationBuilder: utils.ARadixApplication().WithAppName(appName1).WithEnvironment(env1, branch1).
+				WithDNSAlias(radixv1.DNSAlias{Alias: alias1, Environment: env1, Component: component1}).
+				WithComponent(utils.NewApplicationComponentBuilder().WithName(component1).WithPort(portA, port8080).WithPublicPort(portA)),
+			existingRadixDNSAliases: map[string]commonTest.DNSAlias{
+				alias2: {AppName: appName2, Environment: env1, Component: component1, Port: port8080},
+			},
+			expectedError: nil,
+		},
+		{
+			name: "exist aliases with different component and port, no error",
+			applicationBuilder: utils.ARadixApplication().WithAppName(appName1).WithEnvironment(env1, branch1).
+				WithDNSAlias(radixv1.DNSAlias{Alias: alias1, Environment: env1, Component: component1}).
+				WithComponent(utils.NewApplicationComponentBuilder().WithName(component1).WithPort(portA, port8080).WithPublicPort(portA)),
+			existingRadixDNSAliases: map[string]commonTest.DNSAlias{
+				alias1: {AppName: appName1, Environment: env1, Component: component2, Port: port9090},
+			},
+			expectedError: nil,
+		},
+		{
+			name: "no existing aliases, no error",
+			applicationBuilder: utils.ARadixApplication().WithAppName(appName1).WithEnvironment(env1, branch1).
+				WithDNSAlias(radixv1.DNSAlias{Alias: alias1, Environment: env1, Component: component2}).
+				WithComponent(utils.NewApplicationComponentBuilder().WithName(component1).WithPort(portA, port8080).WithPublicPort(portA)),
+			expectedError: nil,
+		},
+	}
+
+	for _, ts := range testScenarios {
+		t.Run(ts.name, func(t *testing.T) {
+			tu, kubeClient, kubeUtil, radixClient := setupTest(t)
+
+			require.NoError(t, commonTest.RegisterRadixDNSAliases(radixClient, ts.existingRadixDNSAliases), "create existing RadixDNSAlias")
+			err := applyApplicationWithSync(tu, kubeClient, kubeUtil, radixClient, ts.applicationBuilder)
+			if ts.expectedError != nil {
+				require.Error(t, err)
+				require.EqualError(t, err, ts.expectedError.Error())
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
diff --git a/pkg/apis/dnsalias/rbac.go b/pkg/apis/dnsalias/rbac.go
@@ -122,10 +122,7 @@ func (s *syncer) getClusterRoleResourceNames() ([]string, error) {
 	if err != nil {
 		return nil, err
 	}
-	return slice.Reduce(dnsAliases, []string{}, func(acc []string, dnsAlias *radixv1.RadixDNSAlias) []string {
-		acc = append(acc, dnsAlias.GetName())
-		return acc
-	}), nil
+	return slice.Map(dnsAliases, func(dnsAlias *radixv1.RadixDNSAlias) string { return dnsAlias.GetName() }), nil
 }
 
 func (s *syncer) deleteRbac() error {
@@ -144,25 +141,17 @@ func (s *syncer) deleteRbacByName(roleName string) error {
 		return s.deleteClusterRoleAndBinding(roleName)
 	}
 	dnsAliasName := s.radixDNSAlias.GetName()
-	clusterRole.Rules[0].ResourceNames = getRoleResourceNamesWithout(dnsAliasName, clusterRole)
+	clusterRole.Rules[0].ResourceNames = slices.DeleteFunc(clusterRole.Rules[0].ResourceNames,
+		func(resourceName string) bool { return resourceName == dnsAliasName })
 	if len(clusterRole.Rules[0].ResourceNames) == 0 {
 		return s.deleteClusterRoleAndBinding(roleName)
 	}
 
 	clusterRole.ObjectMeta.OwnerReferences = slices.DeleteFunc(clusterRole.ObjectMeta.OwnerReferences,
-		func(o metav1.OwnerReference) bool { return o.Name == dnsAliasName })
+		func(ownerReference metav1.OwnerReference) bool { return ownerReference.Name == dnsAliasName })
 	return s.kubeUtil.ApplyClusterRole(clusterRole)
 }
 
-func getRoleResourceNamesWithout(dnsAliasName string, role *rbacv1.ClusterRole) []string {
-	return slice.Reduce(role.Rules[0].ResourceNames, []string{}, func(acc []string, resourceName string) []string {
-		if resourceName != dnsAliasName {
-			acc = append(acc, resourceName)
-		}
-		return acc
-	})
-}
-
 func (s *syncer) deleteClusterRoleAndBinding(roleName string) error {
 	if err := s.kubeUtil.DeleteClusterRoleBinding(roleName); err != nil {
 		return err

diff --git a/radix-operator/dnsalias/controller.go b/radix-operator/dnsalias/controller.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"reflect"
 
+	radixutils "github.com/equinor/radix-common/utils"
 	"github.com/equinor/radix-operator/pkg/apis/metrics"
 	radixv1 "github.com/equinor/radix-operator/pkg/apis/radix/v1"
 	radixlabels "github.com/equinor/radix-operator/pkg/apis/utils/labels"
@@ -19,7 +20,6 @@ import (
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/client-go/util/workqueue"
-	"k8s.io/utils/strings/slices"
 )
 
 const (
@@ -75,8 +75,8 @@ func addEventHandlersForRadixRegistrations(radixInformerFactory informers.Shared
 			oldRR := oldObj.(*radixv1.RadixRegistration)
 			newRR := newObj.(*radixv1.RadixRegistration)
 			if oldRR.GetResourceVersion() == newRR.GetResourceVersion() &&
-				slices.Equal(oldRR.Spec.AdGroups, newRR.Spec.AdGroups) &&
-				slices.Equal(oldRR.Spec.ReaderAdGroups, newRR.Spec.ReaderAdGroups) {
+				radixutils.ArrayEqualElements(oldRR.Spec.AdGroups, newRR.Spec.AdGroups) &&
+				radixutils.ArrayEqualElements(oldRR.Spec.ReaderAdGroups, newRR.Spec.ReaderAdGroups) {
 				return // updating RadixDeployment has the same resource version. Do nothing.
 			}
 			enqueueRadixDNSAliasesForRadixRegistration(controller, radixClient, newRR)

diff --git a/radix-operator/dnsalias/controller_test.go b/radix-operator/dnsalias/controller_test.go
@@ -46,10 +46,8 @@ func (s *controllerTestSuite) Test_RadixDNSAliasEvents() {
 
 	const (
 		appName1       = "any-app1"
-		appName2       = "any-app2"
 		aliasName      = "alias-alias-1"
 		envName1       = "env1"
-		envName2       = "env2"
 		componentName1 = "server1"
 		componentName2 = "server2"
 		dnsZone        = "dev.radix.equinor.com"
@@ -70,20 +68,6 @@ func (s *controllerTestSuite) Test_RadixDNSAliasEvents() {
 	s.Require().NoError(err)
 	s.WaitForSynced("Sync should be called on add RadixDNSAlias")
 
-	// Updating the RadixDNSAlias with appName should trigger a sync
-	s.Handler.EXPECT().Sync("", aliasName, s.EventRecorder).DoAndReturn(s.SyncedChannelCallback()).Times(1)
-	alias.Spec.AppName = appName2
-	alias, err = s.RadixClient.RadixV1().RadixDNSAliases().Update(context.TODO(), alias, metav1.UpdateOptions{})
-	s.Require().NoError(err)
-	s.WaitForSynced("Sync should be called on update appName in the RadixDNSAlias")
-
-	// Updating the RadixDNSAlias with environment should trigger a sync
-	s.Handler.EXPECT().Sync("", aliasName, s.EventRecorder).DoAndReturn(s.SyncedChannelCallback()).Times(1)
-	alias.Spec.Environment = envName2
-	alias, err = s.RadixClient.RadixV1().RadixDNSAliases().Update(context.TODO(), alias, metav1.UpdateOptions{})
-	s.Require().NoError(err)
-	s.WaitForSynced("Sync should be called on update environment in the RadixDNSAlias")
-
 	// Updating the RadixDNSAlias with component should trigger a sync
 	s.Handler.EXPECT().Sync("", aliasName, s.EventRecorder).DoAndReturn(s.SyncedChannelCallback()).Times(1)
 	alias.Spec.Component = componentName2