Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ECS] Fix unexpected diff of Plan Preview #5306

Merged
merged 1 commit into from
Nov 1, 2024
Merged

[ECS] Fix unexpected diff of Plan Preview #5306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 24 additions & 13 deletions pkg/app/piped/driftdetector/ecs/detector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,11 +209,11 @@
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)

Check warning on line 212 in pkg/app/piped/driftdetector/ecs/detector.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/piped/driftdetector/ecs/detector.go#L212 

Added line #L212 was not covered by tests

result, err := provider.Diff(
liveManifests,
headManifests,
live,
head,

Check warning on line 216 in pkg/app/piped/driftdetector/ecs/detector.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/piped/driftdetector/ecs/detector.go#L215-L216 

Added lines #L215 - L216 were not covered by tests
diff.WithEquateEmpty(),
diff.WithIgnoreAddingMapKeys(),
diff.WithCompareNumberAndNumericString(),
Expand All @@ -237,8 +237,8 @@
// 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
Expand All @@ -251,9 +251,10 @@
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
Expand All @@ -267,7 +268,7 @@
}
}

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.
Expand All @@ -279,37 +280,43 @@
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 {
Expand All @@ -319,6 +326,10 @@
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) {
Expand Down
14 changes: 11 additions & 3 deletions pkg/app/piped/driftdetector/ecs/detector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,17 +162,25 @@ 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(),
)

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) {
Expand Down
Loading