Added addr support to vault #76

joris-burgers · 2019-09-06T15:20:39Z

Fixes #66.

Added support for VAULT_ADDR to the config files, as well as the command
line arguments. Changed the config parsing process such that everything
is parsed to an Options datatype after which the options are merged. The
addr field is after each separate parsing verified to be correct and the
information is distributed over the UseTLs (HTTP/HTTPS), Host and Port.

Added support for VAULT_ADDR to the config files, as well as the command line arguments. Changed the config parsing process such that everything is parsed to an Options datatype after which the options are merged. The addr field is after each separate parsing verified to be correct and the information is distributed over the UseTLs (HTTP/HTTPS), Host and Port.

Hlint changes applied that seemed logical and consistent with the code style.

Fixed warnings and errors in Main.hs and Config.hs. Removed some redundant code and changed some imports.

Fixed a bug where wrong schemes were accepted with an invalid address. Added some whitespace to two error messages.

Added a number of test that test whether mismatching addresses are rejected and correct addresses are accepted.

duijf

All in all, I think this is a very good effort! Nice job so far! :)

Some things that I liked:

Thank you for thinking about tests and comments!
Being considerate about naming/the style of the rest of the code.

Since you asked me over lunch to take an especially critical look at this, I have quite the list of comments/remarks. I hope this feedback is of the kind that you were looking for. If you want me to have a look at something else in particular, then let me know.

Please feel free to:

Ask me to clarify points where I glanced over reasoning and/or details.
Tell me where you feel like a remark does not make sense/feels pointless/is not applicable/won't work/takes too much effort to address/is not important in the grand scheme of things/is scope inflation and you want to fix this later/etc. Basically: code reviews are a two way street :)

If you want to, then I'm available to go over these remarks in person.

Some general notes:

We prefer imperative commit messages over past-tense: Add VAULT_ADDR support instead of Added VAULT_ADDR support.
We indent Haskell with 2 spaces, not 4.

src/Config.hs

app/Main.hs

src/Config.hs

duijf · 2019-09-09T11:33:43Z

src/Config.hs

+      , [UnspecifiedValue "Secret file" | isNothing (oSecretFile opts)]
+      ]
+
+splitAddress :: String -> (Maybe String, String, String)


We probably already have a dependency on a package which parses URLs/URIs (either direct or transitive). Can we use that instead?

Otherwise:

Please add a Haddock. What is the goal of this function, what are some example inputs/outputs?

The return type probably benefits from a type alias/newtype/record

What happens when you give this thing some garbage? E.g. VAULT_ADDR=https://foo::8550/asdf

Why is the scheme a Maybe, but not the other tuple members? Why is the port a string?

In general, I think this kind of parsing code is tricky to get right. From looking at the code here, I can't easily reason about the correctness (especially in degerate cases). If we're not using a function from a library, then I would at least:

Add an example based unit test for this function in particular.

If we found some problems while doing that; add a property test which start with a random URL, turn it into a string, and then try to parse it with this function. I can show you how to do that if you want.

E.g. for an abitrary (Maybe String, String, String) -- (you'll need a newtype to specify the instance), generate the original String and see if parsing it with this function yields the same (Maybe String, String, String)

I could not find a function that does exactly as described (and needed), so I propose we keep this for the moment. I added Haddock documentation, including several examples, as well as a number of example tests and property tests.

src/Config.hs

test/integration/invalid_addr.sh

app/Main.hs

src/Config.hs

duijf · 2019-09-09T13:44:45Z

Oh and just noticed that there is quite some trailing whitespace here. Please configure your editor to strip trailing whitespace before saving a file :)

Added phantom types for validated and completed to options data type to ensure type safety for certain calls. Rewrote several datatypes because of this.

Added several comments, removed unnecessary code

Added and changed comments to Config.hs

Fixed several issues with Haddock comments.

Fixed several issues where the layout and naming could be improved.

Added several HSpec tests for the splitAddress function.

duijf

Thanks for the fixes! I took this for a spin on my local dev env and on a test server! This seems to work; nice job :)

I have two specific remarks. One about a test, another about the display of the error messages.

In addition, I have two high-level requests:

Can you clean up all trailing whitespace? There is still some left
We also need user facing documentation for the things in here. (But let's not do that on this PR). Can you create a new issue for documenting all these new environment variables + what happens in case the user specifies conflicting options?

duijf · 2019-09-13T11:23:11Z

test/ConfigSpec.hs

+spec :: SpecWith ()
+spec =
+    describe "Split addr" $ do
+        it "should accept http schemes "


Nit: you can add a it "should accept http schemes" $ do to save a few lines with parentheses.

duijf · 2019-09-23T09:30:51Z

test/integration/invalid_addr.sh

+  echo "not ok 1 - vaultenv didn't complete"
+fi
+
+#test if the addr is accepted when the same as the host, port and scheme (use tls)


Is this comment correct? You seem to test rejection instead of acceptance

duijf · 2019-09-23T09:50:00Z

src/Config.hs

+  show (UnknownScheme s)  = "The address " ++ s ++ " has no recognisable scheme, "
+      ++ " expected http:// or https:// at the beginning of the address."
+  show (NonNumericPort s) = "The port " ++ s ++ " is not a valid int value"
+  show (HostPortSchemeAddrMismatch useTLs host port addr) =


This error message renders up as:

[ERROR] The use TLs True , the host "localhost" and the port 8200 do not match the provided addr "http://localhost:8200"

Can we clean this up and make it a bit more user friendly?

Suggested error message:

[ERROR] Conflicting configuration values in (environment variables|config file x|CLI options): Vault Address: {} Vault Host: {} Vault Port: {} Use TLS: {} Hint: This can happen when you provide a `VAULT_ADDR` environment which conflicts with the `--port`, `--host`, and `--[no-]connect-tls` flags and when [INSERT OTHER REASONS IF THOSE EXIST].

Since we can also override these settings on the CLI and with the environment, this message would be even better if we could include "Conflicting options on the CLI" / in config file X / in the process environment

I clarified the error message, it now reads:

[ERROR] Confliciting configuration values in cli options Vault address: "http://localhost:42" Vault host: Unspecified Vault port: 1234 Use TLS: Unspecified Hint: This can happen when you provide a `addr` which conflicts with `port`, `host`, or `[no-]connect-tls` in either the environment variables or the CLI.

I changed the hint a bit, because the conflicts can also occur in the environment variables of the .env files.

duijf · 2019-09-23T09:51:12Z

src/Config.hs

+
+-- | Validates for a set of options that any provided addr is valid and that either the
+-- scheme, host and port or that any given addr matches the other provided information.
+validateCopyAddr :: Options UnValidated completed -> Either OptionsError (Options Validated completed)


Do we have test coverage for all these different code paths somewhere? Could we add some tests for this?

duijf · 2019-09-23T09:52:07Z

src/Config.hs

+-- http://localhost:80/foo/bar -> (Just "http://","localhost","80/foo/bar")
+splitAddress :: String -> (Maybe Scheme, Host, StringPort)
+splitAddress addr =
+        let


Nit: indentation settings.

duijf · 2019-09-23T09:59:02Z

test/integration/environment_inherritance.sh

@@ -1,7 +1,5 @@
 #!/bin/bash

-set -e


Why was this test changed? The commit message (a0c33fb) does not discuss any reasoning.

Was a part of this test failing hadn't we spotted it?

With or without this flag, the tests will correctly verify. The main difference is that without this flag, the tests will run properly and a failure in them will be caught by the check for their return code. If we were to have the flag, the whole script would fail and prove would not be able to properly tell which test failed, it would simply say that the entire script failed.

duijf · 2019-09-23T10:02:46Z

src/Config.hs

+data UnValidated
+
+-- | An empty set of options, nothing is specified
+emptyOptions :: Options Validated UnCompleted


Doesn't seem to be used anymore

duijf · 2019-09-23T10:10:31Z

src/Config.hs

+
+
+instance Show OptionsError where
+  show (UnspecifiedValue s) = "The option " ++ s ++ " is required but not specified"


Sort of related: when we detect mulitple errors (awesome that we detect mulitple errors 👏), we render them like:

[ERROR] [The option Command is required but not specified,The option Secret file is required but not specified]

Can we split that:

[ERROR] The option Command is required but not specified [ERROR] The option Secret file is required but not specified

This has been updated.

Updated the error messages to better display multiple errors and include extra information when there is a mismatch with the addr.

duijf

LGTM :)

Thanks for all the work you put into this! Let's write some docs next sprint and ship 🚀

joris-burgers added 6 commits September 6, 2019 17:11

HLint applied to changed Main.hs and Config.hs

21dfaab

Hlint changes applied that seemed logical and consistent with the code style.

Removed warnings and errors in Main.hs, Config.hs

bb98768

Fixed warnings and errors in Main.hs and Config.hs. Removed some redundant code and changed some imports.

Changed line length and removed not needed import

71b44fc

Fixed bug with wrong schemes, small text changes

abd4e60

Fixed a bug where wrong schemes were accepted with an invalid address. Added some whitespace to two error messages.

Added tests for address parsing

b1f1ea7

Added a number of test that test whether mismatching addresses are rejected and correct addresses are accepted.

joris-burgers requested a review from duijf September 9, 2019 10:08

joris-burgers marked this pull request as ready for review September 9, 2019 10:08

duijf suggested changes Sep 9, 2019

View reviewed changes

joris-burgers added 7 commits September 9, 2019 15:51

Add phantom types to options

de08d33

Added phantom types for validated and completed to options data type to ensure type safety for certain calls. Rewrote several datatypes because of this.

Add comments, remove not needed code

a8f2b4d

Added several comments, removed unnecessary code

Add comments to Config.hs

7202090

Added and changed comments to Config.hs

Fix Haddock errors

ada5d90

Fixed several issues with Haddock comments.

Add type signatures to local definitions.

0cb70bb

Fix layout and naming issues

6adda1e

Fixed several issues where the layout and naming could be improved.

Add tests for splitAddress function

d30f0a3

Added several HSpec tests for the splitAddress function.

joris-burgers requested a review from duijf September 10, 2019 08:49

maartenberg mentioned this pull request Sep 13, 2019

Allow overriding environment files to load #75

Closed

duijf reviewed Sep 23, 2019

View reviewed changes

joris-burgers mentioned this pull request Sep 24, 2019

Add documentation for VAULT_ADDR support #77

Closed

joris-burgers added 3 commits September 24, 2019 13:12

Clarify error messages, updated default

75cc935

Updated the error messages to better display multiple errors and include extra information when there is a mismatch with the addr.

Add new tests for validateCopyAddr

b1157a9

Fix whitespace issues

52998e6

joris-burgers requested a review from duijf September 24, 2019 11:43

duijf approved these changes Sep 26, 2019

View reviewed changes

joris-burgers merged commit 52998e6 into master Sep 26, 2019

joris-burgers deleted the feature/support_vault_addr branch September 26, 2019 09:26

duijf mentioned this pull request Dec 11, 2019

Load Vault token from file #72

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added addr support to vault #76

Added addr support to vault #76

joris-burgers commented Sep 6, 2019 •

edited by no-longer-on-githu-b

Loading

duijf left a comment

duijf Sep 9, 2019

joris-burgers Sep 10, 2019

duijf commented Sep 9, 2019

duijf left a comment

duijf Sep 13, 2019

duijf Sep 23, 2019

duijf Sep 23, 2019

joris-burgers Sep 24, 2019

duijf Sep 23, 2019

duijf Sep 23, 2019

duijf Sep 23, 2019

joris-burgers Sep 24, 2019

duijf Sep 23, 2019

duijf Sep 23, 2019

joris-burgers Sep 24, 2019

duijf left a comment



		instance Show OptionsError where
		show (UnspecifiedValue s) = "The option " ++ s ++ " is required but not specified"

Added addr support to vault #76

Added addr support to vault #76

Conversation

joris-burgers commented Sep 6, 2019 • edited by no-longer-on-githu-b Loading

duijf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duijf commented Sep 9, 2019

duijf left a 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

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

duijf left a comment

Choose a reason for hiding this comment

joris-burgers commented Sep 6, 2019 •

edited by no-longer-on-githu-b

Loading