CMR-10393: Allow only one sqs client queue to subscribe to one collection for NRT (ingest) subscriptions by jmaeng72 · Pull Request #2216 · nasa/Common-Metadata-Repository

jmaeng72 · 2025-02-13T21:26:23Z

Overview

What is the feature/fix?

Problem: The ingest (NRT) subscriptions has a bug where if User1 creates a subscription to Coll1 and then User2 creates a different subscription to the same Coll1 for the same queue, it will overwrite the first subscription in the backend SNS subscription filter for that queue.

What is the Solution?

Add a verify check to make sure this does not happen

What areas of the application does this impact?

Ingest subscription

Checklist

I have updated/added unit and int tests that prove my fix is effective or that my feature works
New and existing unit and int tests pass locally and remotely
clj-kondo has been run locally and all errors corrected
I have removed unnecessary/dead code and imports in files I have changed
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have cleaned up integration tests by doing one or more of the following:
- migrated any are2 tests to are3 in files I have changed
- de-duped, consolidated, removed dead int tests
- transformed applicable int tests into unit tests
- refactored to reduce number of system state resets by updating fixtures (use-fixtures :each (ingest/reset-fixture {})) to be :once instead of :each

…sitory into CMR-10310-2

…sitory into CMR-10393

…sitory into CMR-10393-2

jmaeng72 · 2025-02-13T21:32:36Z

ingest-app/docs/api.md

+NOTE: The same SQS endpoint cannot be used for the same collection more than once. 
+For example if you, USER 1, create a subscriptioon with an SQS queue: SQS1 endpoint to filter UPDATE granule events from Collection 1, another user, USER 2, cannot create a new subscription with the same SQS1 queue endpoint to filter granules from Collection 1 again. 
+Ultimately, only ONE user can perform CRUD operations on the specific SQS endpoint used.
+


I hope this makes sense... please double check during your reviews if this is clear

Its clear to me.

codecov-commenter · 2025-02-14T00:46:09Z

Codecov Report

Attention: Patch coverage is 79.62963% with 11 lines in your changes missing coverage. Please review.

Project coverage is 58.25%. Comparing base (0733700) to head (77d5029).

Files with missing lines	Patch %	Lines
ingest-app/src/cmr/ingest/api/subscriptions.clj	71.42%	1 Missing and 5 partials ⚠️
...a-db-app/src/cmr/metadata_db/api/subscriptions.clj	0.00%	3 Missing ⚠️
ingest-app/src/cmr/ingest/config.clj	66.66%	0 Missing and 1 partial ⚠️
transmit-lib/src/cmr/transmit/metadata_db2.clj	95.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2216      +/-   ##
==========================================
+ Coverage   58.23%   58.25%   +0.01%     
==========================================
  Files        1056     1056              
  Lines       71134    71185      +51     
  Branches     2063     2068       +5     
==========================================
+ Hits        41428    41467      +39     
- Misses      27780    27784       +4     
- Partials     1926     1934       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

eereiter · 2025-02-14T10:39:27Z

ingest-app/src/cmr/ingest/api/subscriptions.clj

+  (let [curr-endpoint (:EndPoint subscription-concept)]
+    (if (and (= "ingest" (:Method subscription-concept))
+             (or (some? (re-matches #"arn:aws:sqs:.*" curr-endpoint))
+                  (some? (re-matches #"http://localhost:9324.*" curr-endpoint))))


Why are we matching this specific URL? I though the issue was only with SQS?

this is the local dev SQS ARN (which doesn't start with arn:aws:sqs, but I still wanted to capture it and have it act like it was an sqs arn subscription concept endpoint)

Not directed at this specific PR, but are there/do we need guards against localhost subscription URLs?

can you explain further? You mean, do we need to distinguish between localhost URL and localhost SQS?

or are you saying that we might not need line 104 because we don't care about distinguishing between local sqs and local http endpoint?

Nah, like when someone sends in a subscription with url endpoint, do we block them/do we even need to block them from sending in a subscription that is for localhost (since that would point to our subscription processor). Again, not aimed directly at this PR, just occurred to me while looking at the localhost there

ooo I see what you are saying. I think we could. It doesn't hurt to just limit them to non-local URLs for http endpoints

updated with new blocker for this

transmit-lib/test/cmr/transmit/test/metadata_db2_test.clj

eereiter · 2025-02-14T10:43:56Z

ingest-app/src/cmr/ingest/api/subscriptions.clj

+             (or (some? (re-matches #"arn:aws:sqs:.*" curr-endpoint))
+                  (some? (re-matches #"http://localhost:9324.*" curr-endpoint))))
+      (let [collection-concept-id  (:CollectionConceptId subscription-concept)
+            cache-content (metadata-db2/get-subscription-cache-content context collection-concept-id)]


I am wondering why you created an API call into metadata-db, instead of just reading the cache directly?

Not asking you to change anything.

the cache funcs are in the metadata-db app which is typically accessed from the transmit lib that acts like a middle-man. Which is why I created a metadata-db API

ingest-app/docs/api.md

metadata-db-app/src/cmr/metadata_db/api/subscriptions.clj

…l env

transmit-lib/src/cmr/transmit/metadata_db2.clj

jceaser

I had very minor comments, must fix that one new line where the variable is below the let.

TylerHeald1

Just need to make sure the defconfig value is properly added to each environment

jmaeng72 · 2025-02-19T16:05:28Z

Yep. Just checked each env and it has the parameter with the correct env var

…sitory into CMR-10393-2

jmaeng72 added 22 commits January 29, 2025 16:16

refactor funcs, add unit tests

e65dc51

update delete subscription func and add unit tests

656c73e

updated changes to subscriptions

44eb3e9

updating cache, not working yet

68e37da

update cache structure, stabilized create, update, delete sub pathways

9d49d71

update subscripion message attributes

7ba4da3

add func comments

c91d0fb

add debug printlns

851c162

fix unit tests

7102dc8

fix unit tests

85eec19

Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…

36985aa

…sitory into CMR-10310-2

clean up

7c322c3

fix refresh sub cache endpoint

91ba9ab

add unit tests

11f5fb5

remove all println statements

869dec5

add new transmit lib method and metadata-db api for subscription

2acc940

saving work so far

9e5b0de

Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…

2c87a04

…sitory into CMR-10393

fix mode map get

90b9747

testing subscription check works

668f709

Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…

b574637

…sitory into CMR-10393-2

add unit tests

25a8943

jmaeng72 self-assigned this Feb 13, 2025

update documentation to reflect the edge case

53db25a

jmaeng72 commented Feb 13, 2025

View reviewed changes

jmaeng72 requested review from TylerHeald1, eereiter and jceaser February 13, 2025 21:44

jmaeng72 marked this pull request as draft February 13, 2025 21:45

jmaeng72 removed the request for review from jceaser February 13, 2025 21:45

jmaeng72 removed request for TylerHeald1 and eereiter February 13, 2025 21:45

eereiter reviewed Feb 14, 2025

View reviewed changes

transmit-lib/test/cmr/transmit/test/metadata_db2_test.clj Outdated Show resolved Hide resolved

eereiter reviewed Feb 14, 2025

View reviewed changes

eereiter approved these changes Feb 14, 2025

View reviewed changes

jmaeng72 marked this pull request as ready for review February 14, 2025 14:35

jmaeng72 requested review from TylerHeald1, jceaser and zimzoom February 14, 2025 14:35

TylerHeald1 reviewed Feb 14, 2025

View reviewed changes

ingest-app/docs/api.md Outdated Show resolved Hide resolved

TylerHeald1 reviewed Feb 14, 2025

View reviewed changes

metadata-db-app/src/cmr/metadata_db/api/subscriptions.clj Outdated Show resolved Hide resolved

jmaeng72 added 4 commits February 14, 2025 11:31

fix spelling errors and new line

4841e94

block subscription endpoint http url from being localhost if not loca…

7bf006b

…l env

fix spacing

c035b3c

do not allow non-local env to allow local URLs

77d5029

jceaser reviewed Feb 18, 2025

View reviewed changes

transmit-lib/src/cmr/transmit/metadata_db2.clj Outdated Show resolved Hide resolved

jceaser reviewed Feb 18, 2025

View reviewed changes

transmit-lib/src/cmr/transmit/metadata_db2.clj Show resolved Hide resolved

jceaser reviewed Feb 18, 2025

View reviewed changes

transmit-lib/src/cmr/transmit/metadata_db2.clj Outdated Show resolved Hide resolved

jceaser approved these changes Feb 18, 2025

View reviewed changes

cosmetic changes to metadata_db2 file

60c9dfc

TylerHeald1 approved these changes Feb 19, 2025

View reviewed changes

Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…

9747f70

…sitory into CMR-10393-2

jmaeng72 merged commit f17c748 into master Feb 19, 2025
6 checks passed

jmaeng72 deleted the CMR-10393-2 branch February 19, 2025 19:44

jmaeng72 restored the CMR-10393-2 branch February 20, 2025 19:17

jmaeng72 mentioned this pull request Feb 20, 2025

Revert "CMR-10393: Allow only one sqs client queue to subscribe to one collection for NRT (ingest) subscriptions" #2221

Merged

Conversation

jmaeng72 commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

What is the feature/fix?

What is the Solution?

What areas of the application does this impact?

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eereiter Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jceaser left a comment

Choose a reason for hiding this comment

Uh oh!

TylerHeald1 left a comment

Choose a reason for hiding this comment

Uh oh!

jmaeng72 commented Feb 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jmaeng72 commented Feb 13, 2025 •

edited

Loading

codecov-commenter commented Feb 14, 2025 •

edited

Loading

eereiter Feb 14, 2025 •

edited

Loading