fix: grant the bucket owner control to addon template artifacts by Lou1415926 · Pull Request #3485 · aws/copilot-cli

Lou1415926 · 2022-04-20T19:06:19Z

Fix #3453 by:

Grant bucket owner (which is the app account owner) full control, in addition to the object owner (the account that uploads the artifact)
Use hash for addon template artifacts
Instead of uploading addons to the root dir "stackset/svc.addons.stack.yml", upload it to "stackset/manual/addons/svc/sha.yml"
Instead of uploading env files to "manual/sha/mock.env.file", upload it to "manual/env-files/env.file/sha.env"

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

…value

efekarakus

Thank you Wanxian!!

efekarakus · 2022-04-20T20:41:32Z

+// AddonsArtifactPath prefixes the key with the addons artifact path.
+func AddonsArtifactPath(key string) string {
+	return path.Join(artifactDirName, addonDirName, key)
+}
+
+// TemplateArtifactPath prefixes the key with the template artifact path.
+func TemplateArtifactPath(key string) string {
+	return path.Join(artifactDirName, templateDirName, key)
+}


These don't seem to fit the s3 package 💭
What do you think of moving them to deploy instead?

I moved them to the template/ package. My motivation was that it might be easier for us to manage/visualize from the codebase what our artifact folder structure would be, if these path definition is put together in a single package.

In the current codebase that are scattered in cli/deploy, deploy and s3.

let me know what ya think!

efekarakus · 2022-04-20T20:45:58Z

 	reader := strings.NewReader(template)
-	url, err := cf.s3Client.Upload(bucket, fmt.Sprintf(fmtWorkloadCFNTemplateName, config.StackName(), sha256.Sum256([]byte(template))), reader)
+	artifactName := path.Join(config.StackName(), fmt.Sprintf("%x.yml", sha256.Sum256([]byte(template))))
+	url, err := cf.s3Client.Upload(bucket, s3.TemplateArtifactPath(artifactName), reader)


What do you think of a path like:

templates/<stack name>/<sha>.yml

Currently it's manual/templates/stackname/sha.yml what do ya think? The new code here doesn't change the path it's just some refactoring!

iamhopaul123 · 2022-04-21T18:02:04Z

-	reader := strings.NewReader(template)
-	url, err := in.uploader.Upload(d.resources.S3Bucket, fmt.Sprintf(deploy.AddonsCfnTemplateNameFormat, d.name), reader)
+	reader := strings.NewReader(tmpl)
+	artifactName := path.Join(d.name, fmt.Sprintf("%x.yml", sha256.Sum256([]byte(tmpl))))


I wonder if we should move sha256.Sum256 to AddonsArtifactPath. Same for TemplateArtifactPath because it seems like whenever we use them we always call sha256.Sum256

I would prefer the key being determined by the client but I'm fine either way

But do we have an assumption that all the addons artifacts should do sha256.Sum256? Same for the env file and template uploads. If so maybe we would want

// Env file upload url, err := in.uploader.Upload(d.resources.S3Bucket, template.EnvFileArtifactPath(envFilePath, content), reader) // Addons upload url, err := in.uploader.Upload(d.resources.S3Bucket, template.AddonsArtifactPath(d.name, content), reader) // Template upload url, err := cf.s3Client.Upload(bucket, template.TemplateArtifactPath(config.StackName(), content), reader)

it's ok if you don't want to change. Not a big one tho.

I agree that we should most likely always upload with sha256 :) this way we can eliminate this bug altogether for future code changes

efekarakus · 2022-04-21T17:54:00Z

 		Bucket: aws.String(bucket),
 		Key:    aws.String(key),
-	})
+		ACL: aws.String(s3.ObjectCannedACLBucketOwnerFullControl),


nit: is gofmt or a similar formatter running?

efekarakus · 2022-04-21T18:21:07Z

+// AddonsArtifactPath prefixes the key with the addons artifact path.
+func AddonsArtifactPath(key string) string {


I wonder if we should have a new package for all of these? so that it reads a bit better template.MkdirSHA256 is not obvious that it's for s3.

Let me know what you think of this layout:

// Package artifactpath holds functions to generate the S3 object path for artifacts. package template/artifactpath // Addons returns the path to store addon artifact files. func Addons(key string) string // CFNTemplate returns the path to store cloudformation templates. func CFNTemplate(key string) string // SHA256Func returns a function that will namespace a key with the sha256 hash of its content. func SHA256Func(content []byte) func(string) string { return func(key string) string { return path.Join(fmt.Sprintf("%x", sha256.Sum256(content)), key) } } // Chain applies the path functions sequentially to nest the key. func Chain(key string, fn ...func(string) string) string

This would be the client's experience:

artifactpath.Chain(key, artifactpath.SHA256Func(content), artifactpath.CFNTemplate)

Since we always want to add SHA256, I've inlined it into the function -> chaining was rendered unnecessary when SHA256 is inlined. Let me know what you think!

efekarakus · 2022-04-21T23:01:43Z

+
+// Addons returns the path to store addon artifact files.
+// Example: manual/addons/key/sha.yml.
+func AddonsWithSHA256(key string, content []byte) string {


nit: It should be fine to drop the WithSHA256 suffix now that we do it for everything

Lou1415926 added 5 commits April 20, 2022 10:45

upload s3 with bucket owner full access acl

73b48f4

run go mod tidy

18a8302

temporary and need to be removed: give new env stack param a default …

0853967

…value

use sha name for addon, and upload addons to manual/addons/svc/sha.yml

e8367c3

refactor: centralize s3 artifact path management into s3 pkg

c4a65c7

Lou1415926 requested a review from a team as a code owner April 20, 2022 19:06

Lou1415926 requested review from iamhopaul123 and removed request for a team April 20, 2022 19:06

amazon-ecs-cli-v2-pr-manager requested a review from huanjani April 20, 2022 19:06

efekarakus self-requested a review April 20, 2022 19:13

remove temp

de3cb0a

efekarakus reviewed Apr 20, 2022

View reviewed changes

Lou1415926 added 2 commits April 21, 2022 10:08

addr fb: inline acl bucket owner full control

8b9b63e

addr fb: move func from s3 to template

60a5e54

iamhopaul123 reviewed Apr 21, 2022

View reviewed changes

Lou1415926 added 2 commits April 21, 2022 11:15

addr fb: move sha

fb7001e

rename func specifying sha

904a65e

efekarakus reviewed Apr 21, 2022

View reviewed changes

iamhopaul123 approved these changes Apr 21, 2022

View reviewed changes

iamhopaul123 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Apr 21, 2022

Lou1415926 added 2 commits April 21, 2022 14:53

addr fb: add sha256 to all path; refactor

dc04d18

remove yaml from envfile

239c931

Lou1415926 force-pushed the fix/addons/s3-permission branch from d83668f to 4429ad0 Compare April 21, 2022 22:05

add env file path with .env

a57db70

Lou1415926 force-pushed the fix/addons/s3-permission branch from 4429ad0 to a57db70 Compare April 21, 2022 22:10

add license

d52e9b9

efekarakus approved these changes Apr 21, 2022

View reviewed changes

remove sha256 from func name

8c7c3c4

Lou1415926 removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Apr 29, 2022

Lou1415926 changed the title ~~fix: grant the bucket owner control to addon template artifact~~ fix: grant the bucket owner control to addon template artifacts Apr 29, 2022

Merge branch 'mainline' into fix/addons/s3-permission

c2fe773

mergify Bot merged commit 0cfb5c3 into aws:mainline Apr 29, 2022

		// AddonsArtifactPath prefixes the key with the addons artifact path.
		func AddonsArtifactPath(key string) string {

Conversation

Lou1415926 commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efekarakus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lou1415926 Apr 21, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

iamhopaul123 Apr 21, 2022 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Lou1415926 commented Apr 20, 2022 •

edited

Loading

Lou1415926 Apr 21, 2022 •

edited

Loading

iamhopaul123 Apr 21, 2022 •

edited

Loading