feat: Pull Through Cache for ECR #15
Conversation
WalkthroughThe pull request introduces enhancements to the ECR (Elastic Container Registry) component by adding support for pull-through cache rules. This includes a new variable Changes
Sequence DiagramsequenceDiagram
participant User as User/Module
participant ECR as AWS ECR
participant Secrets as AWS Secrets Manager
User->>ECR: Configure pull-through cache rules
User->>Secrets: Optionally provide cache credentials
ECR->>Secrets: Retrieve credentials if specified
ECR-->>User: Create pull-through cache rules
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟠 Require terratestWaiting checks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/variables.tf (1)
62-69: Enhance variable definition with validation and examplesAdd validation and improve the description with examples to make the variable more user-friendly.
Apply this enhancement:
variable "pull_through_cache_rules" { type = map(object({ registry = string secret = optional(string, "") })) - description = "Map of pull through cache rules to configure" + description = <<-EOT + Map of pull through cache rules to configure. Each rule requires: + - registry: The upstream registry URL (e.g., public.ecr.aws) + - secret: (Optional) The name of the AWS Secrets Manager secret containing credentials + + Example: + { + "upstream-cache" = { + registry = "public.ecr.aws" + secret = "ecr/upstream/credentials" + } + } + EOT default = {} + + validation { + condition = alltrue([ + for k, v in var.pull_through_cache_rules : + can(regex("^[a-zA-Z0-9][a-zA-Z0-9-]*\\.dkr\\.ecr\\.[a-zA-Z0-9][a-zA-Z0-9-]*\\.amazonaws\\.com$", v.registry)) + ]) + error_message = "All registry values must be valid ECR registry URLs" + } }src/README.md (1)
90-95: Add usage example for pull-through cache configurationWhile the documentation correctly lists the new resource and input parameter, it would be helpful to add an example of how to configure pull-through cache rules in the usage section.
Add this example to the usage section:
read_only_account_role_map: corp: ["*"] dev: ["*"] prod: ["*"] stage: ["*"] + pull_through_cache_rules: + "upstream-cache": + registry: "public.ecr.aws" + secret: "ecr/upstream/credentials" # Optional: AWS Secrets Manager secret nameAlso applies to: 122-122
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignore(1 hunks)src/README.md(2 hunks)src/main.tf(3 hunks)src/outputs.tf(1 hunks)src/variables.tf(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (4)
src/main.tf (2)
43-51: Add validation for secret existenceThe current implementation doesn't validate if the secret exists before attempting to use it. This could lead to runtime errors if a secret is misconfigured.
Add error handling by using a
lifecycleblock withprecondition:data "aws_secretsmanager_secret" "cache_credentials" { for_each = local.enabled ? { for key, rule in var.pull_through_cache_rules : key => rule.secret if length(rule.secret) > 0 } : {} name = each.value + + lifecycle { + precondition { + condition = can(data.aws_secretsmanager_secret.cache_credentials[each.key]) + error_message = "Secret ${each.value} does not exist in AWS Secrets Manager" + } + } }
53-59: Add validation for registry URLsThe implementation should validate the upstream registry URL format to prevent configuration errors.
Add validation using a
lifecycleblock:resource "aws_ecr_pull_through_cache_rule" "this" { for_each = local.enabled ? var.pull_through_cache_rules : {} ecr_repository_prefix = each.key upstream_registry_url = each.value.registry credential_arn = length(each.value.secret) > 0 ? data.aws_secretsmanager_secret.cache_credentials[each.key].arn : null + + lifecycle { + precondition { + condition = can(regex("^[a-zA-Z0-9][a-zA-Z0-9-]*\\.dkr\\.ecr\\.[a-zA-Z0-9][a-zA-Z0-9-]*\\.amazonaws\\.com$", each.value.registry)) + error_message = "Invalid upstream registry URL format. Must be a valid ECR registry URL." + } + } }src/outputs.tf (1)
17-27: LGTM! Syntax update improves readabilityThe change from
.*to[*]syntax is a good modernization that aligns with current Terraform best practices..gitignore (1)
39-41: LGTM! Well-documented gitignore entryThe addition is properly commented and explains the rationale for including the account-map directory.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/README.md (3)
63-63: Fix article usage before "AWS"Change "a AWS" to "an AWS" to follow proper English grammar rules.
-To configure a pull through cache rule, you can use the `pull_through_cache_rules` input. Each rule requires a AWS Secrets Manager secret, by name, to be provided. +To configure a pull through cache rule, you can use the `pull_through_cache_rules` input. Each rule requires an AWS Secrets Manager secret, by name, to be provided.🧰 Tools
🪛 LanguageTool
[misspelling] ~63-~63: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ..._cache_rules` input. Each rule requires a AWS Secrets Manager secret, by name, to...(EN_A_VS_AN)
65-76: Enhance the configuration example with additional detailsWhile the example is clear, it would be more helpful to include:
- Comments explaining each field
- Multiple registry examples (e.g., GitHub Container Registry)
- Secret format requirements
Here's an enhanced example:
```yaml components: terraform: ecr: vars: enabled: true ... pull_through_cache_rules: + # Docker Hub configuration dockerhub: + # Docker Hub registry endpoint registry: "registry-1.docker.io" + # Secret containing {"username": "user", "password": "pass"} secret: "ecr-pullthroughcache/dockerhub" + # GitHub Container Registry configuration + ghcr: + registry: "ghcr.io" + secret: "ecr-pullthroughcache/github"--- `139-139`: **Enhance the input parameter description** The current description could be more detailed to help users understand the parameter better. ```diff -| <a name="input_pull_through_cache_rules"></a> [pull_through_cache_rules](#input_pull_through_cache_rules) | Map of pull through cache rules to configure | <pre>map(object({<br/> registry = string<br/> secret = optional(string, "")<br/> }))</pre> | `{}` | no | +| <a name="input_pull_through_cache_rules"></a> [pull_through_cache_rules](#input_pull_through_cache_rules) | Map of pull through cache rules to configure. Each rule requires a registry endpoint (e.g., 'registry-1.docker.io' for Docker Hub) and optionally a secret name from AWS Secrets Manager containing registry credentials in {"username": "user", "password": "pass"} format. | <pre>map(object({<br/> registry = string<br/> secret = optional(string, "")<br/> }))</pre> | `{}` | no |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/README.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
src/README.md
[misspelling] ~63-~63: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ..._cache_rules` input. Each rule requires a AWS Secrets Manager secret, by name, to...
(EN_A_VS_AN)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (1)
src/README.md (1)
107-107: LGTM!The resource documentation is well-formatted and includes a helpful link to the Terraform registry.
|
These changes were released in v1.536.0. |
2 participants
what
tflintfailureswhy
references
Examples
Enable pull through caching with ECR as such:
ecrcomponentSummary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
Chores
.gitignoreentry foraccount-map/directory.