From 48ea969e3c16f580da871447c9b795e2216b60a4 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 30 Oct 2024 22:58:14 +0900 Subject: [PATCH] Clone manifests not to modify original manifests Signed-off-by: t-kikuc --- pkg/app/piped/driftdetector/ecs/detector.go | 37 ++++++++++++------- .../piped/driftdetector/ecs/detector_test.go | 14 +++++-- 2 files changed, 35 insertions(+), 16 deletions(-) diff --git a/pkg/app/piped/driftdetector/ecs/detector.go b/pkg/app/piped/driftdetector/ecs/detector.go index a83309d940..cfc1ad0471 100644 --- a/pkg/app/piped/driftdetector/ecs/detector.go +++ b/pkg/app/piped/driftdetector/ecs/detector.go @@ -209,11 +209,11 @@ func (d *detector) checkApplication(ctx context.Context, app *model.Application, d.logger.Info(fmt.Sprintf("application %s has live ecs definition files", app.Id)) // Ignore some fields whech are not necessary or unable to detect diff. - ignoreParameters(liveManifests, headManifests) + live, head := ignoreParameters(liveManifests, headManifests) result, err := provider.Diff( - liveManifests, - headManifests, + live, + head, diff.WithEquateEmpty(), diff.WithIgnoreAddingMapKeys(), diff.WithCompareNumberAndNumericString(), @@ -237,8 +237,8 @@ func (d *detector) checkApplication(ctx context.Context, app *model.Application, // TODO: Maybe we should check diff of following fields when not set in the head manifests in some way. Currently they are ignored: // - service.PlatformVersion // - service.RoleArn -func ignoreParameters(liveManifests provider.ECSManifests, headManifests provider.ECSManifests) { - liveService := liveManifests.ServiceDefinition +func ignoreParameters(liveManifests provider.ECSManifests, headManifests provider.ECSManifests) (live, head provider.ECSManifests) { + liveService := *liveManifests.ServiceDefinition liveService.CreatedAt = nil liveService.CreatedBy = nil liveService.Events = nil @@ -251,9 +251,10 @@ func ignoreParameters(liveManifests provider.ECSManifests, headManifests provide liveService.TaskDefinition = nil // TODO: Find a way to compare the task definition if possible. liveService.TaskSets = nil - liveTask := liveManifests.TaskDefinition - // When liveTask does not exist, e.g. right after the service is created. - if liveTask != nil { + var liveTask types.TaskDefinition + if liveManifests.TaskDefinition != nil { + // When liveTask does not exist, e.g. right after the service is created. + liveTask = *liveManifests.TaskDefinition liveTask.RegisteredAt = nil liveTask.RegisteredBy = nil liveTask.RequiresAttributes = nil @@ -267,7 +268,7 @@ func ignoreParameters(liveManifests provider.ECSManifests, headManifests provide } } - headService := headManifests.ServiceDefinition + headService := *headManifests.ServiceDefinition if headService.PlatformVersion == nil { // The LATEST platform version is used by default if PlatformVersion is not specified. // See https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_CreateService.html#ECS-CreateService-request-platformVersion. @@ -279,37 +280,43 @@ func ignoreParameters(liveManifests provider.ECSManifests, headManifests provide headService.RoleArn = liveService.RoleArn } if headService.NetworkConfiguration != nil && headService.NetworkConfiguration.AwsvpcConfiguration != nil { - awsvpcCfg := headService.NetworkConfiguration.AwsvpcConfiguration + awsvpcCfg := *headService.NetworkConfiguration.AwsvpcConfiguration + awsvpcCfg.Subnets = slices.Clone(awsvpcCfg.Subnets) slices.Sort(awsvpcCfg.Subnets) if len(awsvpcCfg.AssignPublicIp) == 0 { // AssignPublicIp is DISABLED by default. // See https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_AwsVpcConfiguration.html#ECS-Type-AwsVpcConfiguration-assignPublicIp. awsvpcCfg.AssignPublicIp = types.AssignPublicIpDisabled } + headService.NetworkConfiguration = &types.NetworkConfiguration{AwsvpcConfiguration: &awsvpcCfg} } // Sort the subnets of the live service as well if liveService.NetworkConfiguration != nil && liveService.NetworkConfiguration.AwsvpcConfiguration != nil { - awsvpcCfg := liveService.NetworkConfiguration.AwsvpcConfiguration + awsvpcCfg := *liveService.NetworkConfiguration.AwsvpcConfiguration + awsvpcCfg.Subnets = slices.Clone(awsvpcCfg.Subnets) slices.Sort(awsvpcCfg.Subnets) + liveService.NetworkConfiguration = &types.NetworkConfiguration{AwsvpcConfiguration: &awsvpcCfg} } // TODO: In order to check diff of the tags, we need to add pipecd-managed tags and sort. liveService.Tags = nil headService.Tags = nil - headTask := headManifests.TaskDefinition + headTask := *headManifests.TaskDefinition headTask.Status = types.TaskDefinitionStatusActive // If livestate's status is not ACTIVE, we should re-deploy a new task definition. - if liveTask != nil { + if liveManifests.TaskDefinition != nil { headTask.Compatibilities = liveTask.Compatibilities // Users can specify Compatibilities in a task definition file, but it is not used when registering a task definition. } + headTask.ContainerDefinitions = slices.Clone(headManifests.TaskDefinition.ContainerDefinitions) for i := range headTask.ContainerDefinitions { cd := &headTask.ContainerDefinitions[i] if cd.Essential == nil { // Essential is true by default. See https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_ContainerDefinition.html#ECS-Type-ContainerDefinition-es. cd.Essential = aws.Bool(true) } + cd.PortMappings = slices.Clone(cd.PortMappings) for j := range cd.PortMappings { pm := &cd.PortMappings[j] if len(pm.Protocol) == 0 { @@ -319,6 +326,10 @@ func ignoreParameters(liveManifests provider.ECSManifests, headManifests provide pm.HostPort = nil // We ignore diff of HostPort because it has several default values. } } + + live = provider.ECSManifests{ServiceDefinition: &liveService, TaskDefinition: &liveTask} + head = provider.ECSManifests{ServiceDefinition: &headService, TaskDefinition: &headTask} + return live, head } func (d *detector) loadConfigs(app *model.Application, repo git.Repo, headCommit git.Commit) (provider.ECSManifests, error) { diff --git a/pkg/app/piped/driftdetector/ecs/detector_test.go b/pkg/app/piped/driftdetector/ecs/detector_test.go index e068c151f8..b7f7a37bfd 100644 --- a/pkg/app/piped/driftdetector/ecs/detector_test.go +++ b/pkg/app/piped/driftdetector/ecs/detector_test.go @@ -162,10 +162,11 @@ func TestIgnoreParameters(t *testing.T) { }, } - ignoreParameters(livestate, headManifest) + ignoredLive, ignoredHead := ignoreParameters(livestate, headManifest) + result, err := provider.Diff( - livestate, - headManifest, + ignoredLive, + ignoredHead, diff.WithEquateEmpty(), diff.WithIgnoreAddingMapKeys(), diff.WithCompareNumberAndNumericString(), @@ -173,6 +174,13 @@ func TestIgnoreParameters(t *testing.T) { assert.NoError(t, err) assert.Equal(t, false, result.Diff.HasDiff()) + + // Check if the original manifests are not modified. + assert.Equal(t, []string{"1_test-subnet", "0_test-subnet"}, headManifest.ServiceDefinition.NetworkConfiguration.AwsvpcConfiguration.Subnets) + assert.Equal(t, []string{"1_test-sg", "0_test-sg"}, livestate.ServiceDefinition.NetworkConfiguration.AwsvpcConfiguration.SecurityGroups) + assert.Equal(t, 0, len(headManifest.TaskDefinition.Status)) + assert.Nil(t, headManifest.TaskDefinition.ContainerDefinitions[1].Essential) + assert.Equal(t, 0, len(headManifest.TaskDefinition.ContainerDefinitions[0].PortMappings[0].Protocol)) } func TestIgnoreAutoScalingDiff(t *testing.T) {