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

Added ECR pull-through configuration #148

Merged
merged 3 commits into from
Sep 11, 2024
Merged

Conversation

amarin-dspace
Copy link
Contributor

@amarin-dspace amarin-dspace commented Jun 11, 2024

As the title says, this will create ECR rule to set up pull-through cache in your private ECR registry.
It includes necessary credentals' secret, IAM policy and the pull-through rule itself.
The creation of resources is controlled through object variable.

Actual functionality from EKS cluster (ie IAM permissions, usage of images from upstream repository in cluster) is tested.

Terraform v1.9.4
on windows_amd64

  • provider registry.terraform.io/gavinbunney/kubectl v1.14.0
  • provider registry.terraform.io/hashicorp/aws v5.37.0
  • provider registry.terraform.io/hashicorp/cloudinit v2.3.4
  • provider registry.terraform.io/hashicorp/helm v2.13.2
  • provider registry.terraform.io/hashicorp/http v3.4.3
  • provider registry.terraform.io/hashicorp/kubernetes v2.30.0
  • provider registry.terraform.io/hashicorp/local v2.5.1
  • provider registry.terraform.io/hashicorp/null v3.2.2
  • provider registry.terraform.io/hashicorp/random v3.6.2
  • provider registry.terraform.io/hashicorp/time v0.11.2
  • provider registry.terraform.io/hashicorp/tls v4.0.5
  • provider registry.terraform.io/terraform-aws-modules/http v2.4.1

@amarin-dspace amarin-dspace changed the title [WIP] added ECR pull-through configuration Added ECR pull-through configuration Jun 13, 2024
ecr.tf Outdated Show resolved Hide resolved
ecr.tf Show resolved Hide resolved
variables.tf Outdated
}

variable "registry_credentials" {
type = map(string)
Copy link
Contributor

@lukabudak lukabudak Jun 14, 2024

Choose a reason for hiding this comment

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

Why don't you be more specific and inside the map use an object instead of random combination of string key value pairs?
I take it we'll always expect username and accessToken for registry_credentials.
Therefore, it'd be in our best interest to use a defined object like so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this particular registry, this could be considered an overkill, since it does not require any authentication at all.
Generally, yes, it's always username and accessToken, but there's discussion below with SimonG to have this hardcoded instead, for this particular registry...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

If registry_credentials variable is agreed upon, I would urge that you map an object to the proposed attributes in order to prevent the injection of any unexpected/harmful key-value pairs.

Copy link
Contributor Author

@amarin-dspace amarin-dspace Jun 17, 2024

Choose a reason for hiding this comment

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

Agreements can be re-agreed 😈
In any case, this was copied from upstream, I understand there is possibility for end user to inject unwanted kv pair(s), but what you propose is just a measure to prevent end-user shooting themselves in the foot (they should do it simply, c/p what is already there and put correct values).
Waiting for response from SimonG (below; it's connected to your input as well, because he wants it hard-coded).

variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
lukabudak
lukabudak previously approved these changes Sep 6, 2024
Copy link
Contributor

@lukabudak lukabudak left a comment

Choose a reason for hiding this comment

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

LGTM
@sgrdn could you also leave a review

ecr.tf Outdated Show resolved Hide resolved
@vradicevicds vradicevicds force-pushed the cot/ecr-pullthrough branch 2 times, most recently from cbe3c30 to 332ca4b Compare September 10, 2024 09:03
@vradicevicds vradicevicds merged commit d03380d into main Sep 11, 2024
@vradicevicds vradicevicds deleted the cot/ecr-pullthrough branch September 11, 2024 11:50
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