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

support private registry in the ecs task definition #17119

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

edwardsb
Copy link
Contributor

I still need to run through testing this but its essentially ready for review.

@edwardsb edwardsb requested a review from a team as a code owner February 23, 2024 15:34
@edwardsb edwardsb force-pushed the edwardsb-repository-credentials branch from 9c71da8 to 4b68f9a Compare February 23, 2024 15:35
## Using a private container image repository

First create an AWS Secrets Manager Secret with your preferred method, for example:
```shell
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you prefer this method over creating the secretsmanager secret as a terraform resource and then recommending either a secretsmanager secret version or setting the version on the created secret via the AWS CLI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose I was conflicted about how to go about this. Since its not like a database secret that can be generated randomly, this needs to be supplied and I didn't want to have a strong opinion on how the user creates the secret (thus supplying the secret material).

Copy link
Contributor

Choose a reason for hiding this comment

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

We can go forward with this as-is. This way the secret can be created either via terraform or via the CLI. The only complication would be if a custom KMS key were used on the user-created resource. This is at least covers probably 95% of the use-case.

rfairburn
rfairburn previously approved these changes Mar 6, 2024
Copy link
Contributor

@rfairburn rfairburn left a comment

Choose a reason for hiding this comment

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

Assuming this is fully tested, we should get this in place. We have examples that use a "fleetdm/fleet:" image, and we should never recommend that without authentication for pull-rate reasons.

@valentinpezon-primo
Copy link

Hi everyone,

Can we have any update on this issue ? Is this going to be include in the next major release (4.49) ?

Thank you 🙏

@edwardsb
Copy link
Contributor Author

edwardsb commented Apr 2, 2024

I'll fix the conflicts and try to get it tested today. Sorry it fell off my radar a bit. Thanks for your patience.

@valentinpezon-primo
Copy link

I'll fix the conflicts and try to get it tested today. Sorry it fell off my radar a bit. Thanks for your patience.

All good, thanks!

@lukeheath
Copy link
Member

@rfairburn @edwardsb Should this be merged?

@edwardsb
Copy link
Contributor Author

@rfairburn @edwardsb Should this be merged?

I need to stand up an environment that uses a private registry via credentials (over IAM). I hopefully can have that done today.

@edwardsb
Copy link
Contributor Author

This is good to go. Tested with private docker image on Dockerhub.

@lukeheath
Copy link
Member

@edwardsb Looks like just a minor conflict to resolve then we can merge this in. Thanks!

cc @rfairburn

@edwardsb edwardsb force-pushed the edwardsb-repository-credentials branch from 025004b to 568a795 Compare April 22, 2024 20:14
@edwardsb edwardsb requested a review from pacamaster as a code owner April 22, 2024 20:14
@rfairburn rfairburn merged commit 1c35157 into main Apr 23, 2024
8 checks passed
@rfairburn rfairburn deleted the edwardsb-repository-credentials branch April 23, 2024 15:52
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.

4 participants