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.

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

DEVPROD-6193 Use IRSA to sign mciuploads bucket urls #8566

Merged
merged 14 commits into from
Jan 3, 2025

Conversation

ZackarySantana
Copy link
Contributor

@ZackarySantana ZackarySantana commented Dec 18, 2024

DEVPROD-6193

Description

This makes the app servers use AWS IRSA to sign mciuploads bucket urls and it also stops uploading the keys when a user uploads to mciuploads.

Testing

Deployed in staging. Added the new key in staging global document. This task uploaded a signed url, and it generates the url correctly and the database does hold the key/secret that was associated with it but the generated url is without it.

@ZackarySantana ZackarySantana self-assigned this Dec 18, 2024
@ZackarySantana ZackarySantana changed the title DEVPROD-6193 Use IRSA to sign mciuploads bucket urls and exclude the keys when uploading DEVPROD-6193 Use IRSA to sign mciuploads bucket urls and don't upload the keys Dec 18, 2024
@ZackarySantana ZackarySantana changed the title DEVPROD-6193 Use IRSA to sign mciuploads bucket urls and don't upload the keys DEVPROD-6193 Use IRSA to sign mciuploads bucket urls Dec 18, 2024
@ZackarySantana ZackarySantana requested a review from a team December 18, 2024 19:50
@ZackarySantana ZackarySantana marked this pull request as ready for review December 18, 2024 19:51
agent/command/s3_put.go Outdated Show resolved Hide resolved
catcher.ErrorfWhen(f.FileKey == "", "file key is required")

if f.Bucket != "mciuploads" {
catcher.ErrorfWhen(f.AwsKey == "", "aws key is required")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: capitalize AWS

@ablack12 ablack12 requested a review from ybrill December 19, 2024 22:07
model/artifact/artifact_file.go Outdated Show resolved Hide resolved
agent/command/s3_put.go Outdated Show resolved Hide resolved
agent/command/s3_put.go Outdated Show resolved Hide resolved
@ZackarySantana
Copy link
Contributor Author

Once approved, going to get an LGTM to add the corresponding field to production

config.go Outdated Show resolved Hide resolved
config_db.go Outdated Show resolved Hide resolved
@@ -125,7 +125,8 @@ type s3put struct {
isPatchable bool
isPatchOnly bool

bucket pail.Bucket
bucket pail.Bucket
devprodOwnedBuckets []string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we call this like, internalBuckets or something? Just for future proofing, what if we aren't "DevProd" forever haha (think "mci")

config.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ablack12 ablack12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with jonathan approval

service/templates/admin.html Outdated Show resolved Hide resolved
config_bucket.go Outdated Show resolved Hide resolved
@ZackarySantana ZackarySantana requested a review from ybrill January 3, 2025 15:21
config_bucket.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ybrill ybrill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!
Just flagging setting the internal buckets field in the db before the deploy - Sorry, just saw you mentioned this earlier.

@ZackarySantana ZackarySantana merged commit ebab67c into evergreen-ci:main Jan 3, 2025
10 checks passed
@ZackarySantana ZackarySantana deleted the DEVPROD-6193 branch January 3, 2025 16:16
ZackarySantana added a commit to ZackarySantana/evergreen that referenced this pull request Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants