includes server header in output generated from waiter #542

shamsimam · 2018-12-27T00:43:10Z

Changes proposed in this PR

includes server name in output generated from waiter

Why are we making these changes?

Adding the Server header allows clients to differentiate between responses originating in Waiter and the backend.

Before:

$ curl -I -s -H"x-waiter-cpus: 1" -H"x-waiter-mem: 1024" -H"x-waiter-cmd: foo-bar" -H"x-waiter-version: fv" -H"x-waiter-timeout: 10000" http://localhost:9091/foo-bar 
HTTP/1.1 503 Service Unavailable
Content-Type: text/plain
Transfer-Encoding: chunked
...

After (note the extra server header):

$ curl -I -s -H"x-waiter-cpus: 1" -H"x-waiter-mem: 1024" -H"x-waiter-cmd: foo-bar" -H"x-waiter-version: fv" -H"x-waiter-timeout: 10000" http://localhost:9091/foo-bar                                      
HTTP/1.1 503 Service Unavailable
Content-Type: text/plain
Server: waiter/099b03d
Transfer-Encoding: chunked
...

scrosby · 2018-12-27T16:34:52Z

waiter/src/waiter/util/utils.clj

 (defn generate-secret-word
  [src-id dest-id processed-passwords]
  (let [password (second (first processed-passwords))
        secret-word (digest/md5 (str src-id ":" dest-id ":" password))]
    (log/debug "generate-secret-word" [src-id dest-id] "->" secret-word)
    secret-word))

+(let [server-name-atom (atom "waiter")]


Why wrap this at all? Why not just have the bare atom? Less code, and I can do a 'find uses' of the atom and see all gets and sets, globally. Now, I'd need to search for the atom, then for these function uses.

The intent is to restrict the global access to this atom. Only the two functions inside the let block can access this atom directly.

Almost everything in clojure is global, by design. What we lose by hiding it behind these accessors is
#1: We have 9 lines of code; 10% of the lines in this patch and 33% of the new code added in this patch.
#2: We can no longer track accesses to the server name atom so easily, with, e.g., the IDE. We have to now trace indirectly via the finding the references to these two functions (versus searching for the one atom.)

The atom is still mutable, except instead of doing (reset! ...) its now (reset-server-name-atom!) and (get-current-server-name ...) instead of (deref).

Almost everything in Clojure is global, but you would need to go through many hoops to access this atom in the let block.

The pattern of restricting access to the atom is not new to the code, e.g.

waiter/waiter/src/waiter/util/utils.clj

Line 401 in f6bf706

(let [messages (atom {})]

By restricting access to the atom behind a function (current-server-name) we have hidden away the implementation of how the server name is retrieved. This gives us the flexibility to change the implementation without impacting the callers of this method (e.g. what if we later chose to retrieve the server name from an environment variable).

By introducing reset-server-name-atom! it is now easy to find any code paths that mutate the atom. If we had the raw atom, we would have to find its usages and then introspect the code for uses of swap! or reset! on it.

Good find; I'd make the same claim there; the code could be more direct. (That said I'd advocate renaming that atom as i18l-messages. The current name is far too generic. :) )

Responding to the points:

Is the concern about flexibility on how the server name is retrieved a concrete or hypothetical concern? Could you elaborate?

With a handful of accesses, the IDE search makes it easy to identify the 3 places accessing the variable, so finding which of them sets it is pretty easy (the one without the @)

My reasoning above assumes we continue to use an IDE that can find all the references to the atom in 3 seconds via control-click, that we have <5 references to it, and being so simple and easily found, changing the code is straightforward.

Right now, I see a way to simply the patch by removing a third of the added lines of code, but if you feel strongly that the value described above outweighs the added complexity, I'll agree to disagree and accept the patch. :)

The flexibility is, currently, a hypothetical concern. But I would not be surprised if we needed to change the implementation in the future. E.g. in the case of messages chose to dynamically reload the messages from a properties file or some other store.

"Handful of accesses" is the current state, but may not be as the code evolves.

I feel like this is a scenario ideal for the use of mount. We want to initialize some state (we generate from the settings) and then let it be freely read without worry of it being mutated. When we can control the mutation to only the initialization, there is no harm in having everyone else access it directly to read. I am mimicking that by restricting access to the raw atom and instead requiring invocation of a helper function to read the state.

I'd very much appreciate if you accept the patch, we can have a more general discussion on how these should be handled and perform a more complete refactoring on the codebase for such uses of atom once we reach a consensus.

I'm fine with approving it, but apparently I don't have commit permissions to waiter to commit it.

scrosby · 2018-12-27T16:39:04Z

waiter/test/waiter/core_test.clj

@@ -434,7 +434,9 @@
        (let [request {:request-method :delete, :uri (str "/apps/" service-id), :authorization/user user}
              {:keys [body headers status]} (async/<!! ((ring-handler-factory waiter-request?-fn handlers) request))]
          (is (= 200 status))
-          (is (= {"content-type" "application/json"} headers))
+          (is (= {"content-type" "application/json"


These look like standard headers, repeated about 40 times.

Why not:

(def expected-error-response-headers {....})
Then reference that dictionary throughout?

Introduced a new variable to hold the expected headers and left the assertions in the tests.

scrosby · 2018-12-27T16:41:17Z

waiter/test/waiter/scaling_test.clj

@@ -166,7 +166,10 @@
                        {:basic-authentication {:src-router-id src-router-id} :route-params {:service-id test-service-id}}))
                    {:keys [body headers status]} (async/<!! response-chan)]
                (is (= 200 status))
-                (is (= {"content-type" "application/json", "x-cid" correlation-id} headers))
+                (is (= {"content-type" "application/json"


Could factor this out in a similar way.
(defn verify-response-headers
[headers expected-corrolation-id]
(is ....))

Introduced a new variable to hold the expected headers and left the assertions in the tests.

scrosby · 2018-12-27T16:41:44Z

waiter/test/waiter/util/utils_test.clj

@@ -118,7 +118,9 @@
    (testing "should convert empty map"
      (let [{:keys [body headers status]} (clj->json-response {})]
        (is (= 200 status))
-        (is (= {"content-type" "application/json"} headers))
+        (is (= {"content-type" "application/json"


Ditto here.

Introduced a new variable to hold the expected headers and left the assertions in the tests.

shamsimam · 2018-12-27T18:01:11Z

@scrosby ready for another round :)

includes server name in output generated from waiter

099b03d

shamsimam self-assigned this Dec 27, 2018

scrosby reviewed Dec 27, 2018

View reviewed changes

extracts response headers into a variable

f6bf706

shamsimam force-pushed the response-header-server branch from 420af5c to f6bf706 Compare December 27, 2018 17:22

scrosby merged commit 4b9eee0 into master Dec 28, 2018

shamsimam deleted the response-header-server branch January 29, 2019 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

includes server header in output generated from waiter #542

includes server header in output generated from waiter #542

shamsimam commented Dec 27, 2018

scrosby Dec 27, 2018

shamsimam Dec 27, 2018 •

edited

Loading

scrosby Dec 27, 2018

shamsimam Dec 27, 2018

scrosby Dec 28, 2018

shamsimam Dec 28, 2018

scrosby Dec 28, 2018 •

edited

Loading

scrosby Dec 27, 2018

shamsimam Dec 27, 2018

scrosby Dec 27, 2018

shamsimam Dec 27, 2018

scrosby Dec 27, 2018

shamsimam Dec 27, 2018

shamsimam commented Dec 27, 2018

includes server header in output generated from waiter #542

includes server header in output generated from waiter #542

Conversation

shamsimam commented Dec 27, 2018

Changes proposed in this PR

Why are we making these changes?

Choose a reason for hiding this comment

shamsimam Dec 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scrosby Dec 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamsimam commented Dec 27, 2018

shamsimam Dec 27, 2018 •

edited

Loading

scrosby Dec 28, 2018 •

edited

Loading