DATA-5849: File download support #51

prasadlohakpure · 2025-09-25T21:24:47Z

What : Support for donwloading files from S3 on remote ECS fargate containers
Why: For certain use cases wherein my config files are large in size even after base64 encoding, I need a user friendly way to pass them on remote ECS container, instead of passing as part of API payload.

wiz-55ccc8b716 · 2025-09-25T21:25:21Z

Wiz Scan Summary

Scanner	Findings
Vulnerabilities	-
Sensitive Data	-

Total	-

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

Copilot

Pull Request Overview

This PR adds file download support to the ECS command execution system. It introduces functionality to download files from S3 (and potentially other sources) before executing the main container command.

Added startup script template and generation logic for file downloads
Extended ECS command context with file download configuration
Implemented container modification pattern to inject startup scripts

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
startup_script_template.sh	New shell script template for downloading files before container execution
ecs.go	Extended ECS command functionality with file download support and container modification options

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

internal/pkg/object/command/ecs/ecs.go

Copilot · 2025-09-25T21:25:31Z

internal/pkg/object/command/ecs/ecs.go

 )

+var (
+	templatePath = "internal/pkg/object/command/ecs/startup_script_template.sh"


The hardcoded template path will fail when deployed as the relative path won't exist. Consider embedding the template file using embed.FS or making the path configurable.

internal/pkg/object/command/ecs/startup_script_template.sh

Co pilot review suggestions Co-authored-by: Copilot <[email protected]>

hladush · 2025-10-01T16:27:07Z

internal/pkg/object/command/ecs/ecs.go

+type ScriptDownload struct {
+	Source      string
+	Destination string
+	IsS3        bool


let's add enum which will show location for download. If we add gsc,azure tomorrow we'll not need to add more and more variables

hladush

Please add test information

hladush · 2025-10-01T16:34:34Z

internal/pkg/object/command/ecs/startup_script_template.sh

+echo 'Starting file downloads to {{.DownloadDir}}...'
+{{if .CreateDirs}}mkdir -p {{.DownloadDir}}{{end}}
+
+if ! command -v aws &> /dev/null; then


this check should be run if file is S3

hladush · 2025-10-01T16:34:49Z

internal/pkg/object/command/ecs/startup_script_template.sh

+mkdir -p $(dirname {{.Destination}})
+{{if .IsS3}}
+echo "Downloading from S3: {{.Source}}"
+if aws s3 cp '{{.Source}}' '{{.Destination}}' --cli-read-timeout {{$.Timeout}} --cli-connect-timeout {{$.Timeout}}; then


let's move downloading from s3 in separate function

hladush · 2025-10-01T16:59:37Z

internal/pkg/object/command/ecs/ecs.go

+}
+
+// WithStartupScriptWrapper creates a container option that injects startup script for file downloads
+func (execCtx *executionContext) WithStartupScriptWrapper() ContainerOption {


nit: don't mix object definitions and functions.
Structure file in the way that on top we have object definitions and functions lower

hladush · 2025-10-01T17:01:51Z

internal/pkg/object/command/ecs/ecs.go

+	containerDefinitions := execCtx.TaskDefinitionWrapper.TaskDefinition.ContainerDefinitions
+
+	// Apply container options using the options pattern
+	containerOptions := execCtx.getDefaultContainerOptions()


you have containerOptions inside execCtx and you call applyContainerOptions on execCtx don't pass containerOptions to the method call.

hladush · 2025-10-01T17:02:48Z

internal/pkg/object/command/ecs/ecs.go

+// applyContainerOptions applies container options to container definitions
+func (execCtx *executionContext) applyContainerOptions(containerDefinitions []types.ContainerDefinition, options []ContainerOption) error {
+	for _, option := range options {
+		for i := range containerDefinitions {


nit: for _, cd := range containerDefinitions { here and after that option.ModifyContainer(cd)

hladush · 2025-10-01T17:03:33Z

internal/pkg/object/command/ecs/ecs.go

+}
+
+// ContainerModificationOption represents a generic option for modifying container definitions
+type ContainerModificationOption func(*types.ContainerDefinition) error


let's move options to separate file, it makes code more cleaner and easier for read.
WDYT?

It should be private object

hladush · 2025-10-01T17:04:48Z

internal/pkg/object/command/ecs/ecs.go

+}
+
+// ScriptTemplateData represents data for populating the startup script template
+type ScriptTemplateData struct {


try to make types private if they are not used outside of the package.

hladush · 2025-10-01T17:06:39Z

internal/pkg/object/command/ecs/ecs.go

@@ -1,11 +1,13 @@
 package ecs

 import (


the idea is really nice, and I like how you did that

hladush · 2025-10-01T17:07:15Z

internal/pkg/object/command/ecs/ecs.go

+}
+
+// getDefaultContainerOptions returns the default container options
+func (execCtx *executionContext) getDefaultContainerOptions() []ContainerOption {


it's not default container options. It's just container options

hladush · 2025-10-01T17:08:20Z

internal/pkg/object/command/ecs/ecs.go

+}
+
+// getDefaultContainerOptions returns the default container options
+func (execCtx *executionContext) getDefaultContainerOptions() []ContainerOption {


nit; let's consider to move this function to the buildExecutionContext() and after that call getOptions. don't create options during registerTaskDefinition

The major motivation for that from my point of view is that if we failed to build any option in the future we receive errors on the buildExecutionContext step, not on registrating them

hladush · 2025-10-01T17:10:57Z

internal/pkg/object/command/ecs/ecs.go


+// generateStartupScript creates a startup script for downloading files using a template
+func generateStartupScript(fileDownloads []FileDownload, config *StartupScriptConfig) (string, error) {
+	if len(fileDownloads) == 0 {


you don't need that check because you've already checked it on line 79

hladush · 2025-10-01T17:15:12Z

internal/pkg/object/command/ecs/ecs.go

+	// Parse template
+	tmpl, err := template.New("startup_script").Parse(string(templateContent))
+	if err != nil {
+		return "", fmt.Errorf("failed to parse template: %w", err)


consider to start emit this metrics like in this PR
On low leve functions we only emmit metrics on top level we add logs as well

hladush · 2025-10-01T17:17:37Z

internal/pkg/object/command/ecs/startup_script_template.sh

+  echo "Executing: $ORIGINAL_COMMAND"
+  exec "$ORIGINAL_COMMAND"
+else
+  echo "No original command found, executing: $@"


maybe we should add error here

hladush · 2025-10-01T17:19:24Z

internal/pkg/object/command/ecs/startup_script_template.sh

@@ -0,0 +1,47 @@
+#!/bin/bash


do we want to hardcode 1 script for every user of heimdall or we can add more flexibility provide predefined template but allows task, to override path to the script.
@babourine WDYT?

hladush

Add testing section in your PR. Please describe how the code was tested

hladush · 2025-10-20T17:12:27Z

plugins/ecs/README.md

+    task_count: 2
+
+    file_uploads:
+      - data: "Processing completed at 2025-10-13"


Please add to the comment that this field can be overridden in the execution

hladush · 2025-10-20T17:13:19Z

internal/pkg/object/command/ecs/container_options.go

+}
+
+// WithFileUploadScript wraps container commands with a script that downloads files from S3 to the container
+func WithFileUploadScript(fileUploads []FileUpload, localDir string) ContainerOption {


with file download script? Name doesn't match description

hladush · 2025-10-20T17:14:15Z

internal/pkg/object/command/ecs/container_options.go

+	}
+}
+
+const downloadScriptTemplate = `


the general rule is to put constants on the top of the file. You can also consider the option to move it to the separate file

hladush · 2025-10-20T17:14:45Z

internal/pkg/object/command/ecs/container_options.go

+	return script
+}
+
+// Future container options can be added here as needed


You can add it as a comment in PR, but when time comes people will figure out how to do that

hladush · 2025-10-20T17:49:22Z

plugins/ecs/README.md

+  "cluster_criteria": ["type:fargate"],
+  "context": {
+    "task_count": 1,
+    "file_uploads": [


these examples can miss lead users. They show task results, but basically the goal is to upload some files on ecs task machine

hladush · 2025-10-20T17:50:46Z

internal/pkg/object/command/ecs/ecs.go

 	MaxFailCount           int                       `yaml:"max_fail_count,omitempty" json:"max_fail_count,omitempty"` // max failures before giving up
+
+	// File upload configuration
+	FileUploads []FileUpload `yaml:"file_uploads,omitempty" json:"file_uploads,omitempty"`


make it a list of pointers

hladush · 2025-10-20T17:51:29Z

internal/pkg/object/command/ecs/ecs.go


+	// Upload files to S3 if configured
+	if err := execCtx.uploadFilesToS3(); err != nil {
+		return fmt.Errorf("failed to upload files to S3: %w", err)


metrics were added to the heimdall please start to use them

hladush · 2025-10-20T17:54:06Z

internal/pkg/object/command/ecs/ecs.go

+}
+
+// parseS3URI parses an S3 URI into bucket and key components
+func parseS3URI(s3URI string) (bucket, key string, err error) {


can you move this function to another package because this function can be reused in different places

hladush

Please add steps for testing and fix review comments

prasadlohakpure added 2 commits September 23, 2025 02:19

DATA-5849: File download support

7aac8f6

Minor changes

3661e5f

Copilot AI review requested due to automatic review settings September 25, 2025 21:24

Copilot AI reviewed Sep 25, 2025

View reviewed changes

prasadlohakpure and others added 2 commits September 26, 2025 03:01

Used embed FS

f811ade

Apply suggestions from code review

f6820bd

Co pilot review suggestions Co-authored-by: Copilot <[email protected]>

prasadlohakpure requested a review from babourine September 25, 2025 21:34

hladush reviewed Oct 1, 2025

View reviewed changes

prasadlohakpure added 3 commits October 13, 2025 22:29

Combined s3 upload in ecs plugin

4399b49

Merge remote-tracking branch 'origin/main' into DATA-5849

ff76d4c

Merge remote-tracking branch 'origin/DATA-5849' into DATA-5849

34b5434

hladush reviewed Oct 20, 2025

View reviewed changes

DATA-5849: File download support #51

Are you sure you want to change the base?

DATA-5849: File download support #51

Uh oh!

Conversation

prasadlohakpure commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wiz-55ccc8b716 bot commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Wiz Scan Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hladush left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hladush Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hladush left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hladush Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prasadlohakpure commented Sep 25, 2025 •

edited

Loading

wiz-55ccc8b716 bot commented Sep 25, 2025 •

edited

Loading

hladush Oct 1, 2025 •

edited

Loading

hladush Oct 20, 2025 •

edited

Loading