From bd911cbe35e450dde55b9eb52b3d828fe58d1ebf Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Thu, 28 Dec 2023 13:18:46 +0100 Subject: [PATCH] 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 --- charts/radix-operator/Chart.yaml | 4 +- pkg/apis/applicationconfig/dns_alias.go | 32 +++++--- pkg/apis/applicationconfig/dns_alias_test.go | 86 ++++++++++++++++++++ pkg/apis/dnsalias/rbac.go | 19 +---- radix-operator/dnsalias/controller.go | 6 +- radix-operator/dnsalias/controller_test.go | 16 ---- 6 files changed, 117 insertions(+), 46 deletions(-) diff --git a/charts/radix-operator/Chart.yaml b/charts/radix-operator/Chart.yaml index 5e55a817d..d28e393be 100644 --- 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 index 9cae52df9..dc68f4c2e 100644 --- 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 index 11965fc4e..2f46c9a31 100644 --- 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 index 42ae69aea..9f0b596bd 100644 --- 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 index 234e58dd7..0529aae8a 100644 --- 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 index 4c0692cdb..011fbd97c 100644 --- 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