-
Notifications
You must be signed in to change notification settings - Fork 439
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
[DSMS-19] Fix SNS topic format issue #2725
Conversation
BenchmarksBenchmark execution time: 2024-06-04 14:37:50 Comparing candidate commit 1a078aa in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 43 metrics, 1 unstable metrics. |
internal/datastreams/pathway.go
Outdated
@@ -24,6 +24,9 @@ func isWellFormedEdgeTag(t string) bool { | |||
return true | |||
} | |||
} | |||
if t[:i] == "topic" && strings.Contains(t, "arn") { |
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.
It might be a good idea to check that it is a valid arn (to the extent possible; if there isn't a convenient "is this an arn" function available you could get close by verifying that it matches one of the three general formats):
arn:partition:service:region:account-id:resource-id
arn:partition:service:region:account-id:resource-type/resource-id
arn:partition:service:region:account-id:resource-type:resource-id
All of them begin with "arn:" and contain at least five colons, so checking for those two qualities should avoid most false positives like "carnival" or "arnold".
Co-authored-by: Dario Castañé <[email protected]>
/merge |
/merge |
@juliannzhou I'll take care of this merge. |
@roisinlh Can you review the PR again? Thanks! |
What does this PR do?
This PR fixes the SNS topic format issue raised in the support ticket where SNS topics aren't detected as data streams. The customer traced the issue to the
nodeHash
function where it checks for correctly formatted tags and returned them.However, in the
isWellFormedEdgeTag
check, it will return false for any cases where the tag is a topic that is a full arn (such astopic:arn:aws:sns:eu-west-1:474060719897:sre-spec-api-ops-dev-slo-consumers
in the customer's case) because the condition check (j == i
) will returntrue
only if there is one colon in the edge tag.The fix will check that 1) there is a colon, 2) the tag is a topic, and that 3) the tag string contains "arn", then return
true
for this case so that full arn topic tags are not excluded as well-formed tags.Motivation
See support ticket
Reviewer's Checklist
Unsure? Have a question? Request a review!