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

New data source aws_ecr_authorization_token #12395

Conversation

edgarpoce
Copy link
Contributor

@edgarpoce edgarpoce commented Mar 15, 2020

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Summary

Add a new data source called aws_ecr_authorization_token that provides the necessary credentials to access private ECR repositories
see https://docs.aws.amazon.com/AmazonECR/latest/APIReference/API_GetAuthorizationToken.html

Reasoning for change

As a terraform user I want to manage Docker images in private ECR repositories. In order to do this I need to get the credentials of the private repository.

The end goal is to be able to build and manage remote docker images fully in Terraform.
e.g.

provider "aws" {
  region  = "us-east-1"
}

resource "aws_ecr_repository" "helloworld" {
  name = "helloworld"
}

data "aws_ecr_authorization_token" "helloworld" {
  registry_id = aws_ecr_repository.helloworld.registry_id
}

provider "docker" {
  registry_auth {
    address  = data.aws_ecr_authorization_token.helloworld.proxy_endpoint
    username = data.aws_ecr_authorization_token.helloworld.user_name
    password = data.aws_ecr_authorization_token.helloworld.password
  }
}

resource "docker_registry_image" "helloworld" {
  name = "${aws_ecr_repository.helloworld.repository_url}:1.0"
  build {
    context = "context"
  }
}

Relates OR Closes #11332

Release note for CHANGELOG:


Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSEcrDataSource_ecrAuthorizationToken'

...

@edgarpoce edgarpoce requested a review from a team March 15, 2020 03:06
@ghost ghost added needs-triage Waiting for first response or review from a maintainer. size/L Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. service/ecr Issues and PRs that pertain to the ecr service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. documentation Introduces or discusses updates to documentation. labels Mar 15, 2020
@ewbankkit
Copy link
Contributor

Verified acceptance tests:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSEcrDataSource_ecrAuthorizationToken'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 20 -run=TestAccAWSEcrDataSource_ecrAuthorizationToken -timeout 120m
=== RUN   TestAccAWSEcrDataSource_ecrAuthorizationToken
--- PASS: TestAccAWSEcrDataSource_ecrAuthorizationToken (41.01s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	41.049s

@anGie44 anGie44 self-assigned this May 21, 2020
@anGie44 anGie44 added new-data-source Introduces a new data source. and removed needs-triage Waiting for first response or review from a maintainer. labels May 21, 2020
aws/data_source_aws_ecr_authorization_token.go Outdated Show resolved Hide resolved
aws/data_source_aws_ecr_authorization_token.go Outdated Show resolved Hide resolved
aws/data_source_aws_ecr_authorization_token.go Outdated Show resolved Hide resolved
aws/data_source_aws_ecr_authorization_token.go Outdated Show resolved Hide resolved
aws/data_source_aws_ecr_authorization_token.go Outdated Show resolved Hide resolved
aws/provider.go Outdated Show resolved Hide resolved
website/aws.erb Outdated Show resolved Hide resolved
aws/data_source_aws_ecr_authorization_token.go Outdated Show resolved Hide resolved
@edgarpoce
Copy link
Contributor Author

Thanks for reviewing!, I'll implement the changes ASAP.

@edgarpoce edgarpoce requested a review from anGie44 May 25, 2020 19:15
@anGie44 anGie44 added the waiting-response Maintainers are waiting on response from community or contributor. label May 27, 2020
@edgarpoce edgarpoce requested a review from anGie44 May 28, 2020 13:37
@anGie44 anGie44 removed the waiting-response Maintainers are waiting on response from community or contributor. label May 28, 2020
Copy link
Contributor

@anGie44 anGie44 left a comment

Choose a reason for hiding this comment

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

Thanks for the latest updates @edgarpoce, LGTM 👍

Output of acceptance test:

--- PASS: TestAccAWSEcrAuthorizationTokenDataSource_basic (32.09s)

@anGie44 anGie44 added this to the v2.64.0 milestone May 28, 2020
@anGie44 anGie44 requested a review from bflad May 28, 2020 20:14
@anGie44 anGie44 removed this from the v2.64.0 milestone May 28, 2020
@anGie44 anGie44 modified the milestone: v2.66.0 Jun 11, 2020
@anGie44 anGie44 added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 11, 2020
@anGie44 anGie44 removed the waiting-response Maintainers are waiting on response from community or contributor. label Jun 12, 2020
@anGie44
Copy link
Contributor

anGie44 commented Jun 16, 2020

Hi @edgarpoce, in using this data-source in practice such that it's attributes are used as input's to the docker provider's registry_auth block, I came across a plan-time issue such that the docker provider doesn't seem to uses these attributes and instead continues to look for the auth credentials in the registry config file. this behavior of a provider config block not acknowledging data-source values, unfortunately, is a known limitation in some providers, and in retrospect not something I was aware of while reviewing. while this use case may not cover all potential uses of this data-source, I wanted to get your feedback (as well anyone following this PR or @Phat3 related GH Issue) regarding this potential unwanted behavior before we make this feature generally available to ensure the data-source can still assist in yours/others resource management workflows.

@edgarpoce
Copy link
Contributor Author

@anGie44 , If I understand the problem you are mentioning correctly I think it's bug in the Docker provider.
I contributed a PR that is scheduled for release v.2.8.0. see hashicorp/terraform-provider-docker#250
The bug in the Docker provider makes the whole deployment fail if there's a reference to another resource as part of the docker provider configuration.

@anGie44
Copy link
Contributor

anGie44 commented Jun 16, 2020

@edgarpoce thanks for pointing to the PR in the docker provider! I can now confirm that building that provider from your PR branch and using this new data-source works as I'd expect 👍

e.g.

data "aws_ecr_repository" "helloworld" {
  name = "helloworld"
}

data "aws_ecr_authorization_token" "helloworld" {
  registry_id = data.aws_ecr_repository.helloworld.registry_id
}

provider "docker" {
registry_auth {
    address  = data.aws_ecr_authorization_token.helloworld.proxy_endpoint
    username = data.aws_ecr_authorization_token.helloworld.user_name
    password = data.aws_ecr_authorization_token.helloworld.password
  }
}

data "docker_registry_image" "test" {
  name = "${data.aws_ecr_repository.helloworld.repository_url}:latest"
}

output "image_sha" {
    value = data.docker_registry_image.test.sha256_digest
 }

@anGie44 anGie44 force-pushed the feature/data_source_aws_ecr_authorization_token branch 2 times, most recently from 538ebee to 4d7afdf Compare June 18, 2020 04:04
@anGie44
Copy link
Contributor

anGie44 commented Jun 18, 2020

hi @edgarpoce! do you mind fixing the conflicts that have emerged? (I made an attempt here but found it more involved than I expected so reverted to how it was last). I'd like to get this into the next release :)

@anGie44 anGie44 removed the thinking label Jun 18, 2020
@edgarpoce
Copy link
Contributor Author

@anGie44 , the conflict is fixed now. Thanks!

@anGie44 anGie44 added this to the v2.67.0 milestone Jun 18, 2020
@anGie44 anGie44 merged commit 1206393 into hashicorp:master Jun 18, 2020
anGie44 added a commit that referenced this pull request Jun 18, 2020
@ghost
Copy link

ghost commented Jun 19, 2020

This has been released in version 2.67.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Jul 18, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Jul 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. new-data-source Introduces a new data source. provider Pertains to the provider itself, rather than any interaction with AWS. service/ecr Issues and PRs that pertain to the ecr service. size/L Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attribute to retrieve ECR login credentials
3 participants