Implement host and CN resolution#6802
Implement host and CN resolution#6802dagnir merged 3 commits intofeature/master/sns-message-managerfrom
Conversation
| assertThat(paramsCaptor.getValue().region()).isEqualTo(region); | ||
| } | ||
|
|
||
| private static Stream<CommonNameTestCase> commonNameTestCases() { |
There was a problem hiding this comment.
Ported from the v1 manager tests.
| return commonName; | ||
| } | ||
|
|
||
| private String signingCertCommonNameInternal() { |
There was a problem hiding this comment.
| // opt-in regions | ||
| new CommonNameTestCase("me-south-1", "sns-signing.me-south-1.amazonaws.com"), | ||
| new CommonNameTestCase("ap-east-1", "sns-signing.ap-east-1.amazonaws.com"), | ||
| new CommonNameTestCase("me-south-1", "sns-signing.me-south-1.amazonaws.com"), |
There was a problem hiding this comment.
Nit: seems to be dupilcate test case with line 71
There was a problem hiding this comment.
Good catch! I double checked the v1 test that I copied this from and it's duplicated there too.
| // e.g. 'us-gov-west-3' would match the 'aws-us-gov' partition. | ||
| // This will return the 'aws' partition if it fails to match any partition. | ||
| PartitionMetadata partitionMetadata = PartitionMetadata.of(region); | ||
| return "sns." + partitionMetadata.dnsSuffix(); |
There was a problem hiding this comment.
If new regions is added and not updated it here (for example we forget), this would silently produce "sns.amazonaws.com". Is this expected?
There was a problem hiding this comment.
What do you mean by added?
There was a problem hiding this comment.
If a region is added in Region.regions()
There was a problem hiding this comment.
if the region is in the list, it wouldn't enter this code block, it would go through the switch statement and produce sns.amazonaws.com. That's expected behavior
There was a problem hiding this comment.
Got it. Yeh I meant to ask the behavior in the switch statement, commented on the wrong line number.
| } | ||
|
|
||
| String regionId = region.id(); | ||
|
|
There was a problem hiding this comment.
I saw there's a TODO comment in v1:
//TODO SNS team will use a consistent pattern for certificate naming. Then remove the special handling based on region
is it worth to port this TODO
There was a problem hiding this comment.
AFAIK they have implemented that pattern (same for opt-in regions) so it's not necessary.
SnsHostProvider implements the logic to determine the SNS endpoint for a given region, as well as the expected common name of a signing certificate used by SNS in that region. Both pieces of information used to ensure that the certificate we use to verify the message signature is legitimate.
dagnir
force-pushed
the
dongie/sns-host-provider
branch
from
32e8f06 to
decded3
Compare
…er' into dongie/sns-host-provider
Note: This is in codee that will be deleted.
|
dagnir
merged commit e2b9d23
into
feature/master/sns-message-manager
|
This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one. |
2 participants
Motivation and Context
SnsHostProvider implements the logic to determine the SNS endpoint for a given region, as well as the expected common name of a signing certificate used by SNS in that region.
Both pieces of information used to ensure that the certificate we use to verify the message signature is legitimate.
Modifications
Testing
New unit tests.
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License