Skip to content

Commit

Permalink
fix: failed to deploy non-unique name configs with same name (#1250)
Browse files Browse the repository at this point in the history 
fix: handle non-unique name config deployment

Check if there are multiple non-unique name api configs having the same name. if yes, we need to "mark" them using an artificial parameter in order to identify them later on when we need to handle them specially during deploy.

Signed-off-by: Bernd Warmuth <[email protected]>
  • Loading branch information
@warber
warber authored Oct 31, 2023
1 parent a51d3d1 commit c7b120d
Show file tree
Hide file tree
Showing 11 changed files with 88 additions and 41 deletions.
4 changes: 4 additions & 0 deletions cmd/monaco/download/download_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,7 @@ func TestDownloadIntegrationDashboards(t *testing.T) {
Skip: false,
Parameters: map[string]parameter.Parameter{
"name": &value.ValueParameter{Value: "Non-unique dashboard-name"},
config.NonUniqueNameConfigDuplicationParameter: value.New(true),
},
Group: "default",
Environment: projectName,
Expand All @@ -538,6 +539,7 @@ func TestDownloadIntegrationDashboards(t *testing.T) {
Skip: false,
Parameters: map[string]parameter.Parameter{
"name": &value.ValueParameter{Value: "Non-unique dashboard-name"},
config.NonUniqueNameConfigDuplicationParameter: value.New(true),
},
Group: "default",
Environment: projectName,
Expand Down Expand Up @@ -621,6 +623,7 @@ func TestDownloadIntegrationAllDashboardsAreDownloadedIfFilterFFTurnedOff(t *tes
Coordinate: coordinate.Coordinate{Project: projectName, Type: dashboardApi.ID, ConfigId: "id-1"},
Skip: false,
Parameters: map[string]parameter.Parameter{
config.NonUniqueNameConfigDuplicationParameter: value.New(true),
"name": &value.ValueParameter{Value: "Non-unique dashboard-name"},
},
Group: "default",
Expand All @@ -632,6 +635,7 @@ func TestDownloadIntegrationAllDashboardsAreDownloadedIfFilterFFTurnedOff(t *tes
Coordinate: coordinate.Coordinate{Project: projectName, Type: dashboardApi.ID, ConfigId: "id-2"},
Skip: false,
Parameters: map[string]parameter.Parameter{
config.NonUniqueNameConfigDuplicationParameter: value.New(true),
"name": &value.ValueParameter{Value: "Non-unique dashboard-name"},
},
Group: "default",
Expand Down
14 changes: 7 additions & 7 deletions cmd/monaco/integrationtest/v2/non_unique_names_e2e_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,13 @@ func TestNonUniqueNameUpserts(t *testing.T) {
assert.Assert(t, len(getConfigsOfName(t, c, a, name)) == 1, "Expected single configs of name %q but found %d", name, len(existing))

// 1. if only one config of non-unique-name exist it MUST be updated
e, err := c.UpsertConfigByNonUniqueNameAndId(context.TODO(), a, monacoGeneratedUUID, name, payload)
e, err := c.UpsertConfigByNonUniqueNameAndId(context.TODO(), a, monacoGeneratedUUID, name, payload, false)
assert.NilError(t, err)
assert.Equal(t, e.Id, firstExistingObjectUUID, "expected existing single config %d to be updated, but reply UUID was", firstExistingObjectUUID, e.Id)
assert.Assert(t, len(getConfigsOfName(t, c, a, name)) == 1, "Expected single configs of name %q but found %d", name, len(existing))

// 1.1. Deploying another config of the same name is also just an update (unwanted behaviour if a project re-uses names)
e, err = c.UpsertConfigByNonUniqueNameAndId(context.TODO(), a, uuid2.GenerateUUIDFromConfigId("test_project", "other-config"), name, payload)
e, err = c.UpsertConfigByNonUniqueNameAndId(context.TODO(), a, uuid2.GenerateUUIDFromConfigId("test_project", "other-config"), name, payload, false)
assert.NilError(t, err)
assert.Equal(t, e.Id, firstExistingObjectUUID, "expected existing single config %d to be updated, but reply UUID was", firstExistingObjectUUID, e.Id)
assert.Assert(t, len(getConfigsOfName(t, c, a, name)) == 1, "Expected single configs of name %q but found %d", name, len(existing))
Expand All @@ -93,14 +93,14 @@ func TestNonUniqueNameUpserts(t *testing.T) {

// 2. if several configs of non-unique-name exist an additional config with monaco controlled UUID is created
assert.NilError(t, err)
e, err = c.UpsertConfigByNonUniqueNameAndId(context.TODO(), a, monacoGeneratedUUID, name, payload)
e, err = c.UpsertConfigByNonUniqueNameAndId(context.TODO(), a, monacoGeneratedUUID, name, payload, false)
assert.NilError(t, err)
assert.Equal(t, e.Id, monacoGeneratedUUID)
assert.Assert(t, len(getConfigsOfName(t, c, a, name)) == 3, "Expected three configs of name %q but found %d", name, len(existing))

// 3. if several configs of non-unique-name exist and one with known monaco-controlled UUID is found that MUST be updated
assert.NilError(t, err)
e, err = c.UpsertConfigByNonUniqueNameAndId(context.TODO(), a, monacoGeneratedUUID, name, payload)
e, err = c.UpsertConfigByNonUniqueNameAndId(context.TODO(), a, monacoGeneratedUUID, name, payload, false)
assert.NilError(t, err)
assert.Equal(t, e.Id, monacoGeneratedUUID)
assert.Assert(t, len(getConfigsOfName(t, c, a, name)) == 3, "Expected three configs of name %q but found %d", name, len(existing))
Expand Down Expand Up @@ -149,13 +149,13 @@ func TestNonUniqueNameUpserts_InactiveUpdateByName(t *testing.T) {
assert.Assert(t, len(getConfigsOfName(t, c, a, name)) == 1, "Expected single configs of name %q but found %d", name, len(existing))

// 1. if only one config of non-unique-name exist an additional one is still create (update feature OFF)
e, err := c.UpsertConfigByNonUniqueNameAndId(context.TODO(), a, monacoGeneratedUUID, name, payload)
e, err := c.UpsertConfigByNonUniqueNameAndId(context.TODO(), a, monacoGeneratedUUID, name, payload, false)
assert.NilError(t, err)
assert.Equal(t, e.Id, monacoGeneratedUUID, "expected existing single config %d to be updated, but reply UUID was", firstExistingObjectUUID, e.Id)
assert.Assert(t, len(getConfigsOfName(t, c, a, name)) == 2, "Expected single configs of name %q but found %d", name, len(existing))

// 2. Deploying another config of the same name is also just an update (unwanted behaviour if a project re-uses names)
e, err = c.UpsertConfigByNonUniqueNameAndId(context.TODO(), a, otherMonacoGeneratedUUID, name, payload)
e, err = c.UpsertConfigByNonUniqueNameAndId(context.TODO(), a, otherMonacoGeneratedUUID, name, payload, false)
assert.NilError(t, err)
assert.Equal(t, e.Id, otherMonacoGeneratedUUID, "expected existing single config %d to be updated, but reply UUID was", firstExistingObjectUUID, e.Id)
assert.Assert(t, len(getConfigsOfName(t, c, a, name)) == 3, "Expected single configs of name %q but found %d", name, len(existing))
Expand All @@ -166,7 +166,7 @@ func TestNonUniqueNameUpserts_InactiveUpdateByName(t *testing.T) {

// 3. if several configs of non-unique-name exist and one with known monaco-controlled UUID is found that MUST be updated
assert.NilError(t, err)
e, err = c.UpsertConfigByNonUniqueNameAndId(context.TODO(), a, monacoGeneratedUUID, name, payload)
e, err = c.UpsertConfigByNonUniqueNameAndId(context.TODO(), a, monacoGeneratedUUID, name, payload, false)
assert.NilError(t, err)
assert.Equal(t, e.Id, monacoGeneratedUUID)
assert.Assert(t, len(getConfigsOfName(t, c, a, name)) == 4, "Expected three configs of name %q but found %d", name, len(existing))
Expand Down
10 changes: 5 additions & 5 deletions pkg/client/dtclient/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ type ConfigClient interface {
// It calls the underlying GET and PUT endpoints for the API. E.g. for alerting profiles this would be:
// GET <environment-url>/api/config/v1/alertingProfiles ... to check if the config is already available
// PUT <environment-url>/api/config/v1/alertingProfiles/<id> ... with the given (or found by unique name) entity ID
UpsertConfigByNonUniqueNameAndId(ctx context.Context, a api.API, entityID string, name string, payload []byte) (entity DynatraceEntity, err error)
UpsertConfigByNonUniqueNameAndId(ctx context.Context, a api.API, entityID string, name string, payload []byte, duplicate bool) (entity DynatraceEntity, err error)

// DeleteConfigById removes a given config for a given API using its id.
// It calls the DELETE endpoint for the API. E.g. for alerting profiles this would be:
Expand Down Expand Up @@ -489,15 +489,15 @@ func (d *DynatraceClient) upsertConfigByName(ctx context.Context, api api.API, n
return d.upsertDynatraceObject(ctx, api, name, payload)
}

func (d *DynatraceClient) UpsertConfigByNonUniqueNameAndId(ctx context.Context, api api.API, entityId string, name string, payload []byte) (entity DynatraceEntity, err error) {
func (d *DynatraceClient) UpsertConfigByNonUniqueNameAndId(ctx context.Context, api api.API, entityId string, name string, payload []byte, duplicate bool) (entity DynatraceEntity, err error) {
d.limiter.ExecuteBlocking(func() {
entity, err = d.upsertConfigByNonUniqueNameAndId(ctx, api, entityId, name, payload)
entity, err = d.upsertConfigByNonUniqueNameAndId(ctx, api, entityId, name, payload, duplicate)
})
return
}

func (d *DynatraceClient) upsertConfigByNonUniqueNameAndId(ctx context.Context, api api.API, entityId string, name string, payload []byte) (entity DynatraceEntity, err error) {
return d.upsertDynatraceEntityByNonUniqueNameAndId(ctx, entityId, name, api, payload)
func (d *DynatraceClient) upsertConfigByNonUniqueNameAndId(ctx context.Context, api api.API, entityId string, name string, payload []byte, duplicate bool) (entity DynatraceEntity, err error) {
return d.upsertDynatraceEntityByNonUniqueNameAndId(ctx, entityId, name, api, payload, duplicate)
}

func (d *DynatraceClient) GetSettingById(objectId string) (res *DownloadSettingsObject, err error) {
Expand Down
5 changes: 3 additions & 2 deletions pkg/client/dtclient/config_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ func (d *DynatraceClient) upsertDynatraceEntityByNonUniqueNameAndId(
objectName string,
theApi api.API,
payload []byte,
duplicate bool,
) (DynatraceEntity, error) {
fullUrl := theApi.CreateURL(d.environmentURLClassic)
body := payload
Expand All @@ -102,8 +103,8 @@ func (d *DynatraceClient) upsertDynatraceEntityByNonUniqueNameAndId(
return entity, err
}

// Note: this logic is flawed if several configs of the same name exist in a project - they will all update the same single configuration!
if featureflags.UpdateNonUniqueByNameIfSingleOneExists().Enabled() && len(entitiesWithSameName) == 1 { // name is currently unique, update know entity
// check if we are dealing with a duplicate non-unique name configuration, if not, go ahead and update the known entity
if featureflags.UpdateNonUniqueByNameIfSingleOneExists().Enabled() && len(entitiesWithSameName) == 1 && !duplicate {
existingUuid := entitiesWithSameName[0].Id
entity, err := d.updateDynatraceObject(ctx, fullUrl, objectName, existingUuid, theApi, body)
return entity, err
Expand Down
2 changes: 1 addition & 1 deletion pkg/client/dtclient/config_client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,7 @@ func TestDeployConfigsTargetingClassicConfigNonUnique(t *testing.T) {
testApi := api.API{ID: "some-api", NonUniqueName: true, PropertyNameOfGetAllResponse: api.StandardApiPropertyNameOfGetAllResponse}

dtclient, _ := NewDynatraceClientForTesting(server.URL, server.Client(), WithRetrySettings(testRetrySettings))
got, err := dtclient.upsertDynatraceEntityByNonUniqueNameAndId(context.TODO(), generatedUuid, theConfigName, testApi, []byte("{}"))
got, err := dtclient.upsertDynatraceEntityByNonUniqueNameAndId(context.TODO(), generatedUuid, theConfigName, testApi, []byte("{}"), false)
assert.NoError(t, err)
assert.Equal(t, got.Id, tt.expectedIdToBeUpserted)
})
Expand Down
2 changes: 1 addition & 1 deletion pkg/client/dtclient/dummy_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ func (c *DummyClient) UpsertConfigByName(_ context.Context, a api.API, name stri
}, nil
}

func (c *DummyClient) UpsertConfigByNonUniqueNameAndId(_ context.Context, a api.API, entityId string, name string, data []byte) (entity DynatraceEntity, err error) {
func (c *DummyClient) UpsertConfigByNonUniqueNameAndId(_ context.Context, a api.API, entityId string, name string, data []byte, _ bool) (entity DynatraceEntity, err error) {
entries, _ := c.GetEntries(a)

var dataEntry DataEntry
Expand Down
21 changes: 21 additions & 0 deletions pkg/config/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package config

import (
"fmt"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/json"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/config/coordinate"
configErrors "github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/config/errors"
Expand Down Expand Up @@ -44,6 +45,10 @@ const (

// SkipParameter is special in that config should be deployed or not
SkipParameter = "skip"

// NonUniqueNameConfigDuplicationParameter is a special parameter set on non-unique name API configurations
// that appear multiple times in a project
NonUniqueNameConfigDuplicationParameter = "__MONACO_NUN_API_DUP__"
)

// ReservedParameterNames holds all parameter names that may not be specified by a user in a config.
Expand Down Expand Up @@ -253,3 +258,19 @@ func (c *Config) ResolveParameterValues(entities EntityLookup) (parameter.Proper

return properties, nil
}

func GetNameForConfig(c Config) (any, error) {
nameParam, exist := c.Parameters[NameParameter]
if !exist {
return nil, fmt.Errorf("configuration %s has no 'name' parameter defined", c.Coordinate)
}

switch v := nameParam.(type) {
case *valueParam.ValueParameter:
return v.ResolveValue(parameter.ResolveContext{ParameterName: NameParameter})
case *envParam.EnvironmentVariableParameter:
return v.ResolveValue(parameter.ResolveContext{ParameterName: NameParameter})
default:
return c.Parameters[NameParameter], nil
}
}
2 changes: 1 addition & 1 deletion pkg/deploy/deploy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ func TestDeployConfigsTargetingClassicConfigNonUniqueWithExistingCfgsOfSameName(
theApiName := "alerting-profile"

client := dtclient.NewMockClient(gomock.NewController(t))
client.EXPECT().UpsertConfigByNonUniqueNameAndId(gomock.Any(), gomock.Any(), gomock.Any(), theConfigName, gomock.Any())
client.EXPECT().UpsertConfigByNonUniqueNameAndId(gomock.Any(), gomock.Any(), gomock.Any(), theConfigName, gomock.Any(), false)

parameters := []parameter.NamedParameter{
{
Expand Down
17 changes: 16 additions & 1 deletion pkg/deploy/internal/classic/classic.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,5 +82,20 @@ func upsertNonUniqueNameConfig(ctx context.Context, client dtclient.ConfigClient
entityUuid = idutils.GenerateUUIDFromConfigId(projectId, configID)
}

return client.UpsertConfigByNonUniqueNameAndId(ctx, apiToDeploy, entityUuid, configName, []byte(renderedConfig))
// check if we are dealing with a non-unique name configuration that appears multiple times
// in a monaco project. if that's the case, we need to handle it differently, by setting the
// duplicate parameter accordingly
var duplicate bool
if val, exists := conf.Parameters[config.NonUniqueNameConfigDuplicationParameter]; exists {
resolvedVal, err := val.ResolveValue(parameter.ResolveContext{})
if err != nil {
return dtclient.DynatraceEntity{}, err
}
resolvedValBool, ok := resolvedVal.(bool)
if !ok {
return dtclient.DynatraceEntity{}, err
}
duplicate = resolvedValBool
}
return client.UpsertConfigByNonUniqueNameAndId(ctx, apiToDeploy, entityUuid, configName, []byte(renderedConfig), duplicate)
}
23 changes: 2 additions & 21 deletions pkg/deploy/internal/classic/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@ import (
"fmt"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/api"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/config"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/config/parameter"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/config/parameter/environment"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/config/parameter/value"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/deploy/errors"
project "github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/project/v2"
"github.com/google/go-cmp/cmp"
Expand Down Expand Up @@ -50,12 +47,12 @@ func ValidateUniqueConfigNames(projects []project.Project) error {
}

for _, c2 := range uniqueList[c.Environment][a.Api] {
n1, err := getNameForConfig(c)
n1, err := config.GetNameForConfig(c)
if err != nil {
errs = errs.Append(c.Environment, err)
return
}
n2, err := getNameForConfig(c2)
n2, err := config.GetNameForConfig(c2)
if err != nil {
errs = errs.Append(c.Environment, err)
return
Expand Down Expand Up @@ -84,19 +81,3 @@ func ValidateUniqueConfigNames(projects []project.Project) error {
}
return nil
}

func getNameForConfig(c config.Config) (any, error) {
nameParam, exist := c.Parameters[config.NameParameter]
if !exist {
return nil, fmt.Errorf("configuration %s has no 'name' parameter defined", c.Coordinate)
}

switch v := nameParam.(type) {
case *value.ValueParameter:
return v.ResolveValue(parameter.ResolveContext{ParameterName: config.NameParameter})
case *environment.EnvironmentVariableParameter:
return v.ResolveValue(parameter.ResolveContext{ParameterName: config.NameParameter})
default:
return c.Parameters[config.NameParameter], nil
}
}
29 changes: 27 additions & 2 deletions pkg/project/v2/project_loader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ import (
"github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/files"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/log"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/log/field"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/api"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/config"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/config/coordinate"
configErrors "github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/config/errors"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/config/parameter"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/config/parameter/value"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/manifest"
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/persistence/config/loader"
"github.com/spf13/afero"
Expand Down Expand Up @@ -126,14 +128,37 @@ func loadProject(fs afero.Fs, context ProjectLoaderContext, projectDefinition ma
errors = append(errors, newDuplicateConfigIdentifierError(c))
}
}

if errors != nil {
return Project{}, errors
}

// find and memoroize (non-unique-name) configurations with identical names and set a special parameter on them
// to be able to identify them later
nonUniqueNameConfigCount := make(map[string]int)
apis := api.NewAPIs()
for _, c := range configs {
if c.Type.ID() == config.ClassicApiTypeId && apis[c.Coordinate.Type].NonUniqueName {
name, err := config.GetNameForConfig(c)
if err != nil {
log.WithFields(field.Error(err), field.Coordinate(c.Coordinate)).Error("Unable to resolve name of configuration")
}
if nameStr, ok := name.(string); ok {
nonUniqueNameConfigCount[nameStr]++
}
}
}

configMap := make(ConfigsPerTypePerEnvironments)
for i, conf := range configs {
name, _ := config.GetNameForConfig(configs[i])
// set special parameter for non-unique configs that appear multiple times with the same name
// in order to be able to identify them during deployment
if nameStr, ok := name.(string); ok {
if nonUniqueNameConfigCount[nameStr] > 1 {
configs[i].Parameters[config.NonUniqueNameConfigDuplicationParameter] = value.New(true)
}
}

for _, conf := range configs {
if _, found := configMap[conf.Environment]; !found {
configMap[conf.Environment] = make(map[string][]config.Config)
}
Expand Down

0 comments on commit c7b120d

Please sign in to comment.