-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(ec2): fixing vpc endpoint pattern for ecr and ecr docker #31434
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
Exemption Request : No changes to integration test as the fix is for isolated regions. |
test.each([ | ||
['us-isof-test-1', 'gov.ic.hci.csp'], | ||
['eu-isoe-test-1', 'uk.adc-e.cloud'], | ||
])('test vpc interface endpoint for ECR can be created correctly in isolated regions', (region : string, domain: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add tests for commercial regions, cn regions, govcloud regions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, though we already had tests for cn regions below this, added it here too.
c1cf904
to
aace418
Compare
'us-isof-': ['ecr.api', 'ecr.dkr'], | ||
'eu-isoe-': ['ecr.api', 'ecr.dkr'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we only need this two here? What about other us gov regions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two are the only ones flagged in ticket, so for now only these two. These are isolated, for US Gov region i think it starts with us-gov
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #.
Internal Tracking ticket P150271569
Reason for this change
Expected Endpoints for ECR in some isolated regions are as below
gov.ic.hci.csp.us-isof-name.ecr.api,
gov.ic.hci.csp.us-isof-name.ecr.dkr,
uk.adc-e.cloud.eu-isoe-name.ecr.api,
uk.adc-e.cloud.eu-isoe-name.ecr.dkr,
Description of changes
As discussed with the ECR Service team, endpoints for the service are being generated in reverse order of the domain suffix.
Since some of the endpoints for other services are still using
com.amazonaws
, added fix only for the partitions and service(ECR) flagged.Cannot do for cn regions on the basis of suffix as both regions have different services under exceptions.
Description of how you validated changes
Added unit test for validation of endpoint, keeping the region names as
us-isoe-test-1
as the regions are in build stage and could be confidential.No changes to integration test as the fix is for isolated regions.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license