-
Notifications
You must be signed in to change notification settings - Fork 102
CMR-10393: Allow only one sqs client queue to subscribe to one collection for NRT (ingest) subscriptions #2216
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
Changes from 27 commits
e65dc51
656c73e
44eb3e9
68e37da
9d49d71
7ba4da3
c91d0fb
851c162
7102dc8
85eec19
36985aa
7c322c3
91ba9ab
11f5fb5
869dec5
2acc940
9e5b0de
2c87a04
90b9747
668f709
b574637
25a8943
53db25a
4841e94
7bf006b
c035b3c
77d5029
60c9dfc
9747f70
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,12 +11,14 @@ | |
| [cmr.common.services.errors :as errors] | ||
| [cmr.common.util :as util] | ||
| [cmr.ingest.api.core :as api-core] | ||
| [cmr.ingest.config :as ingest-config] | ||
| [cmr.ingest.services.ingest-service :as ingest] | ||
| [cmr.ingest.services.subscriptions-helper :as jobs] | ||
| [cmr.ingest.validation.validation :as v] | ||
| [cmr.transmit.access-control :as access-control] | ||
| [cmr.transmit.config :as config] | ||
| [cmr.transmit.metadata-db :as mdb] | ||
| [cmr.transmit.metadata-db2 :as metadata-db2] | ||
| [cmr.transmit.search :as search] | ||
| [cmr.transmit.urs :as urs]) | ||
| (:import | ||
|
|
@@ -83,14 +85,43 @@ | |
| [subscription-concept] | ||
| (let [method (:Method subscription-concept) | ||
| endpoint (:EndPoint subscription-concept) | ||
| default-url-validator (UrlValidator. UrlValidator/ALLOW_LOCAL_URLS)] | ||
| curr-env (ingest-config/app-environment) | ||
| url-validator (if (= curr-env "local") | ||
| (UrlValidator. UrlValidator/ALLOW_LOCAL_URLS) | ||
| (UrlValidator.))] | ||
|
|
||
| (if (= method "ingest") | ||
| (if-not (or (some? (re-matches #"arn:aws:sqs:.*" endpoint)) (.isValid default-url-validator endpoint)) | ||
| (if-not (or (some? (re-matches #"arn:aws:sqs:.*" endpoint)) | ||
| (.isValid url-validator endpoint)) | ||
| (errors/throw-service-error | ||
| :bad-request | ||
| "Subscription creation failed - Method was ingest, but the endpoint given was not valid SQS ARN or HTTP/S URL. | ||
| If it is a URL, make sure to give the full URL path like so: https://www.google.com."))))) | ||
| "Subscription creation failed - Method was ingest, but the endpoint given was not valid SQS ARN or HTTP/S URL." | ||
| "If it is a URL, make sure to give the full URL path like so: https://www.google.com. We do not accept local URLs."))))) | ||
|
|
||
| (defn- check-endpoint-queue-for-collection-not-already-exist | ||
| "Validates that the subscription with the same collection and same SQS endpoint does not already exist. | ||
| Throws error if the same collection and same SQS endpoint already exists because creating duplicate collection | ||
| with same SQS endpoint from a different user is not allowed." | ||
| [context subscription-concept] | ||
| (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)))) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we matching this specific URL? I though the issue was only with SQS?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not directed at this specific PR, but are there/do we need guards against localhost subscription URLs?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you explain further? You mean, do we need to distinguish between localhost URL and localhost SQS?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated with new blocker for this |
||
| (let [collection-concept-id (:CollectionConceptId subscription-concept) | ||
| cache-content (metadata-db2/get-subscription-cache-content context collection-concept-id)] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| (if cache-content | ||
| ;; cache-content format expected is something like: {:Mode {:Delete [sqs1 sqs2], :New [url1], :Update [url1]}} | ||
| (let [mode-map (get cache-content :Mode)] | ||
| ;; check if any of the endpoints in each mode type is equal to the curr sqs endpoint, if so throw the error | ||
| (doseq [modetoendpointset mode-map] | ||
| (if (some #{curr-endpoint} (val modetoendpointset)) | ||
| (errors/throw-service-error | ||
| :conflict | ||
| (format (str "The collection [%s] has already subscribed to the given sqs arn by another user. " | ||
| "Each Near Real Time subscription to a collection must have a unique sqs arn endpoint." | ||
| "You cannot have the same SQS queue subscribed to the same collection by multiple users," | ||
| "only one user can crate/update/delete subscription to the same end client queue to the same collection.") | ||
| (util/html-escape collection-concept-id))))))))))) | ||
|
|
||
| (defn- check-subscription-limit | ||
| "Given the configuration for subscription limit, this valdiates that the user has no more than | ||
|
|
@@ -163,6 +194,7 @@ | |
| (check-duplicate-subscription request-context concept parsed) | ||
| (check-subscription-limit request-context concept parsed) | ||
| (check-subscriber-collection-permission request-context parsed) | ||
| (check-endpoint-queue-for-collection-not-already-exist request-context parsed) | ||
| (let [concept-with-user-id (api-core/set-user-id concept | ||
| request-context | ||
| headers) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| (ns cmr.transmit.test.metadata-db2-test | ||
| "These tests will check the functions in cmr.transmit.metadata-db2." | ||
| (:require | ||
| [cheshire.core :as json] | ||
| [clojure.test :refer [deftest is]] | ||
| [clj-http.client :as client] | ||
| [cmr.transmit.metadata-db2 :as mdb2])) | ||
|
|
||
| (deftest get-subscription-cache-content | ||
| (let [context {:system :metadata-db} | ||
| coll-concept-id "C123-PROV1" | ||
| response {:status 200 | ||
| :headers {}, | ||
| :body (json/encode {:Mode {:New ["url1"] | ||
| :Update ["url2"]}}) | ||
| :request-time 6, | ||
| :trace-redirects []} | ||
| expected-content-cache {:Mode {:New ["url1"] | ||
| :Update ["url2"]}}] | ||
| ;; successful response returns the json decoded body | ||
| (with-redefs [client/get (fn [request-url params] response)] | ||
| (is (= expected-content-cache (mdb2/get-subscription-cache-content context coll-concept-id)))) | ||
| ;; unsuccessful response throws an error | ||
| (with-redefs [client/get (fn [request-url params] {:status 500})] | ||
| (is (thrown? Exception (mdb2/get-subscription-cache-content context coll-concept-id)))))) |
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.
I hope this makes sense... please double check during your reviews if this is clear
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.
Its clear to me.