Skip to content

Fix ECR deploy race condition and document developer IAM policy - #213

Merged
ramesh-maddegoda merged 1 commit into
mainfrom
lroc-prep
Aug 25, 2026
Merged

Fix ECR deploy race condition and document developer IAM policy#213
ramesh-maddegoda merged 1 commit into
mainfrom
lroc-prep

Conversation

@ramesh-maddegoda

Copy link
Copy Markdown
Contributor

🗒️ Summary

This pull request fixes a race condition that occasionally encountered when deploying to images to ECR. Also, this provides documentation of creating a suitable developer IAM policy with necessary permissions, if we do the deployment in two steps: Deploy IAM fist, deploy other resources next.

🤖 AI Assistance Disclosure

  • No AI assistance used
  • AI used for light assistance (e.g., suggestions, refactoring, documentation help, minor edits)
  • AI used for moderate content generation (AI generated some code or logic, but the developer authored or heavily revised the majority)
  • AI generated substantial portions of this code

Estimated % of code influenced by AI: 95 %

⚙️ Test Data and/or Report

Tested by completely destroying the environment and recreating the it with two steps: Deploy IAM fist, deploy other resources next.

♻️ Related Issues

Improve PDS Nucleus pipeline scalability for terabyte to petabyte-scale data ingestion
#205

🤓 Reviewer Checklist

Reviewers: Please verify the following before approving this pull request.

Documentation and PR Content

  • Documentation: README, Wiki, or inline documentation (Sphinx, Javadoc, Docstrings) have been updated to reflect these changes.
  • Issue Traceability: The PR is linked to a valid GitHub Issue
  • PR Title: The PR title is "user-friendly" clearly identifying what is being fixed or the new feature being added, that if you saw it in the Release Notes for a tool, you would be able to get the gist of what was done.

Security & Quality

  • SonarCloud: Confirmed no new High or Critical security findings.
  • Secrets Detection: Verified that the Secrets Detection scan passed and no sensitive information (keys, tokens, PII) is exposed.
  • Code Quality: Code follows organization style guidelines and best practices for the specific language (e.g., PEP 8, Google Java Style).

Testing & Validation

  • Test Accuracy: Verified that test data is accurate, representative of real-world PDS4 scenarios, and sufficient for the logic being tested.
  • Coverage: Automated tests cover new logic and edge cases.
  • Local Verification: (If applicable) Successfully built and ran the changes in a local or staging environment.

Maintenance

  • Backward Compatibility: Confirmed that these changes do not break existing downstream dependencies or API contracts (or that breaking changes are clearly documented).

Terraform (only if this PR touches a terraform/ directory)

  • Validator run: scripts/validate_terraform.py was run against the changed terraform/ tree and any Must-Have failures were resolved (or are tracked in a reviewed .tfvalidate-ignore entry, not silently bypassed).
  • Ownership boundary: Shared/singleton CDS resources (Cognito, CloudFront, org-wide IAM roles, shared security groups) were only added/changed in pdc-cds-infra, not duplicated in an application repo.
  • Module structure: modules/ stays flat and no new module is just a thin wrapper around a single resource.
  • SSM interface: Any new cross-component output is actually published under /pds/<component>/... and, if this PR is the consumer side, reads from the correct published parameter rather than another repo's Terraform state.
  • Naming intent: Resource and variable names are semantically meaningful, not just non-redundant (the validator only checks for repeated resource-type substrings).
  • Auth at runtime: Terraform/CI actually authenticates via OIDC or an assumed role when applied — not just "no static key found in this diff."
  • Least privilege: Any new or changed IAM policy attached to the Terraform execution role grants only what this change needs.

@ramesh-maddegoda
ramesh-maddegoda requested a review from a team as a code owner August 25, 2026 02:13
@sonarqubecloud

Copy link
Copy Markdown

Comment on lines +61 to +64
"Sid": "WhoAmI",
"Effect": "Allow",
"Action": "sts:GetCallerIdentity",
"Resource": "*"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AWS docs explicitly says no permision is required for GetCallerIdentity (even when it says Deny!) so this inclusion shouldn't be necessary.

"Effect": "Allow",
"Action": "iam:GetPolicy",
"Resource": "arn:aws:iam::*:policy/mcp-tenantOperator-APIG"
},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this sufficient? The data source mcp_operator_policy identifies policies by name but the AWS provider searches using the AWS API ListPolicies, which needs to enumerate them. You might need:

{
  "Sid": "FindExternalPermissionBoundaryPolicy",
  "Effect": "Allow",
  "Action": "iam:ListPolicies",
  "Resource": "*" // must be "*", but is at least read-only
},
{
  "Sid": "ReadExternalPermissionBoundaryPolicy",
  "Effect": "Allow",
  "Action": "iam:GetPolicy",
  "Resource": "arn:aws:iam::*:policy/mcp-tenantOperator-APIG"
}

@nutjob4life nutjob4life left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @ramesh-maddegoda this looks great.

Approved: ✅

I did leave a couple comments intersperesed but I don't think they're blockers for merging. Take a look and see what you think. Thanks again!

@ramesh-maddegoda

Copy link
Copy Markdown
Contributor Author

Thanks @ramesh-maddegoda this looks great.

Approved: ✅

I did leave a couple comments intersperesed but I don't think they're blockers for merging. Take a look and see what you think. Thanks again!

Thanks @nutjob4life for your review!

@ramesh-maddegoda
ramesh-maddegoda merged commit 5cbaeef into main Aug 25, 2026
2 checks passed
@ramesh-maddegoda
ramesh-maddegoda deleted the lroc-prep branch August 25, 2026 20:08
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.

2 participants