Whirr
  1. Whirr
  2. WHIRR-347

Support provider-independent environment variables for cloud credentials

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.8.0
    • Component/s: None
    • Labels:
      None

      Description

      Adrian suggested that we support WHIRR_PROVIDER, WHIRR_IDENTITY, WHIRR_CREDENTIAL environment variables. This would allow users to set them for any cloud they are using.

      1. WHIRR-347-take-2.patch
        81 kB
        Andrei Savu
      2. WHIRR-347-take-2.patch
        77 kB
        Andrei Savu
      3. WHIRR-347.patch
        18 kB
        David Alves
      4. WHIRR-347.patch
        10 kB
        David Alves
      5. WHIRR-347.patch
        10 kB
        David Alves
      6. WHIRR-347.patch
        8 kB
        David Alves

        Activity

        Hide
        Adrian Cole added a comment -

        good idea. only thing might be to have 2 sets of env variables one for BLOBSTORE and one for COMPUTE. if these aren't set, fall back to using WHIRR_PROVIDER.. for both blobstore and compute.

        also, might want to have a look at the config file method used in pallet, which assumes a .pallet dir, or pulling these properties from maven's settings file:
        https://github.com/pallet/pallet-examples/tree/master/blank-project/

        Show
        Adrian Cole added a comment - good idea. only thing might be to have 2 sets of env variables one for BLOBSTORE and one for COMPUTE. if these aren't set, fall back to using WHIRR_PROVIDER.. for both blobstore and compute. also, might want to have a look at the config file method used in pallet, which assumes a .pallet dir, or pulling these properties from maven's settings file: https://github.com/pallet/pallet-examples/tree/master/blank-project/
        Hide
        Andrei Savu added a comment -

        This should also make the recipes more portable.

        Show
        Andrei Savu added a comment - This should also make the recipes more portable.
        Hide
        David Alves added a comment - - edited

        Here's what I did:

        there are now the following env variables:
        WHIRR_PROVIDER
        WHIRR_IDENTITY
        WHIRR_CREDENTIAL
        WHIRR_BLOBSTORE_PROVIDER
        WHIRR_BLOBSTORE_IDENTITY
        WHIRR_BLOBSTORE_CREDENTIAL

        whirr now expects properties to come from (in order of preference, ie later values will take precedence (override) previous ones):
        1 - values in whirr/conf/whirrrc
        2 - values in ~/.whirr/whirrrc
        3 - environment variables
        4 - arguments passed in the command line
        4 - properties file

        • In a typical use case the user would maintain a whirrrc file in ~/.whirr with the credential details.
        • For punctual cases where the user wants to switch providers temporarily he/she can set the environment variables (they override whirrrc)
        • For a single command in another provider args can be used as always (which override the previous methods)
        • Also as always recipes can also have the config (which overrides everything)

        if specifically set blobstore creds will follow the same procedure. if after everything is loaded blobstore creds are still missing then they are set to null (so that whirr can load the compute ones in its place, as usual)

        maven builds and unit tests pass

        Show
        David Alves added a comment - - edited Here's what I did: there are now the following env variables: WHIRR_PROVIDER WHIRR_IDENTITY WHIRR_CREDENTIAL WHIRR_BLOBSTORE_PROVIDER WHIRR_BLOBSTORE_IDENTITY WHIRR_BLOBSTORE_CREDENTIAL whirr now expects properties to come from (in order of preference, ie later values will take precedence (override) previous ones): 1 - values in whirr/conf/whirrrc 2 - values in ~/.whirr/whirrrc 3 - environment variables 4 - arguments passed in the command line 4 - properties file In a typical use case the user would maintain a whirrrc file in ~/.whirr with the credential details. For punctual cases where the user wants to switch providers temporarily he/she can set the environment variables (they override whirrrc) For a single command in another provider args can be used as always (which override the previous methods) Also as always recipes can also have the config (which overrides everything) if specifically set blobstore creds will follow the same procedure. if after everything is loaded blobstore creds are still missing then they are set to null (so that whirr can load the compute ones in its place, as usual) maven builds and unit tests pass
        Hide
        David Alves added a comment -

        forgot the cli defaults props file

        Show
        David Alves added a comment - forgot the cli defaults props file
        Hide
        Tom White added a comment -

        Command-line options override those in the properties file (see statement in http://whirr.apache.org/docs/0.7.0/configuration-guide.html), so your precedence order needs changing I think.

        In whirr-cli-defaults.properties could we have whirr.provider=$

        {env:WHIRR_PROVIDER}

        rather than whirr.provider=$

        {sys:WHIRR_PROVIDER}

        and avoid passing them as system properties from the script?

        Some tests and documentation would be good to have too. Also, how do we plan to change the recipes to use these?

        Show
        Tom White added a comment - Command-line options override those in the properties file (see statement in http://whirr.apache.org/docs/0.7.0/configuration-guide.html ), so your precedence order needs changing I think. In whirr-cli-defaults.properties could we have whirr.provider=$ {env:WHIRR_PROVIDER} rather than whirr.provider=$ {sys:WHIRR_PROVIDER} and avoid passing them as system properties from the script? Some tests and documentation would be good to have too. Also, how do we plan to change the recipes to use these?
        Hide
        David Alves added a comment - - edited

        Command-line options override those in the properties file (see statement in http://whirr.apache.org/docs/0.7.0/configuration-guide.html), so your precedence order needs changing I think.

        I did not change this, I just mentioned that order because the code suggests it (args are parsed and then the props file is added), if it was in reverse order it still is with this patch.

        In whirr-cli-defaults.properties could we have whirr.provider=${env:WHIRR_PROVIDER} rather than whirr.provider=${sys:WHIRR_PROVIDER} and avoid passing them as system properties from the script?

        I avoided using env variables on purpose. My though was that i an env variable is set it is still used, if not then the rc files are used. Just using the rc files thus avoids setting unnecessary env variables. I can change that but the overriding mechanism (env overrides rc) will then cease to work, which might not be a bad thing, what do you think?

        wrt to documentation, you mean add something to the site? will do!

        wrt to testing I thought about it but was unsure exactly what to test since all that's happening is that whirr reads a file that sets some variables if they are not present in env. Moreover unit tests already failed a lot until i got it right which gave me some confidence. any test suggestions?

        Show
        David Alves added a comment - - edited Command-line options override those in the properties file (see statement in http://whirr.apache.org/docs/0.7.0/configuration-guide.html ), so your precedence order needs changing I think. I did not change this, I just mentioned that order because the code suggests it (args are parsed and then the props file is added), if it was in reverse order it still is with this patch. In whirr-cli-defaults.properties could we have whirr.provider=${env:WHIRR_PROVIDER} rather than whirr.provider=${sys:WHIRR_PROVIDER} and avoid passing them as system properties from the script? I avoided using env variables on purpose. My though was that i an env variable is set it is still used, if not then the rc files are used. Just using the rc files thus avoids setting unnecessary env variables. I can change that but the overriding mechanism (env overrides rc) will then cease to work, which might not be a bad thing, what do you think? wrt to documentation, you mean add something to the site? will do! wrt to testing I thought about it but was unsure exactly what to test since all that's happening is that whirr reads a file that sets some variables if they are not present in env. Moreover unit tests already failed a lot until i got it right which gave me some confidence. any test suggestions?
        Hide
        Tom White added a comment -

        > My though was that i an env variable is set it is still used, if not then the rc files are used.

        Yes, I think that's the right behaviour.

        > any test suggestions?

        Could we test the precedence order (of all options except the user's ~/.whirr/whirrrc). Here are some suggestions for setting the environment variables in a unit test: http://stackoverflow.com/questions/318239/how-do-i-set-environment-variables-from-java

        Show
        Tom White added a comment - > My though was that i an env variable is set it is still used, if not then the rc files are used. Yes, I think that's the right behaviour. > any test suggestions? Could we test the precedence order (of all options except the user's ~/.whirr/whirrrc). Here are some suggestions for setting the environment variables in a unit test: http://stackoverflow.com/questions/318239/how-do-i-set-environment-variables-from-java
        Hide
        Andrei Savu added a comment -

        I would prefer that we stick to ENV variables (they are all scoped to WHIRR_ anyway + we can reduce boilerplate in bin/whirr for converting to system properties). Also I think we can have better defaults in whirrrc - aws ENV variables.

        Show
        Andrei Savu added a comment - I would prefer that we stick to ENV variables (they are all scoped to WHIRR_ anyway + we can reduce boilerplate in bin/whirr for converting to system properties). Also I think we can have better defaults in whirrrc - aws ENV variables.
        Hide
        David Alves added a comment -

        I would prefer that we stick to ENV variables (they are all scoped to WHIRR_ anyway + we can reduce boilerplate in bin/whirr for converting to system properties).

        The thing is that without (mandatorily) using env variables we can have multiple installations each with its whirrrc file, besides testing for env variables within java is hacky.
        As is it now you can still use env variables when needed, but these are transformed to script scope variables.

        Also I think we can have better defaults in whirrrc - aws ENV variables.

        What so you suggest? whirr picking up aws env variables when none other are provided? (btw this is another reason not to require whirr_ env variables as if we implement this the user would expect the properties to change when he changes the aws ones, but they don't because previous script runs already set the whirr_ ones and the script would prefer whirr_ vs aws_)

        If so I think this is a nice idea and will include it in the next patch.

        Also the next patch does a much more thorough job of testing precedence (sys vs props vs args).

        Show
        David Alves added a comment - I would prefer that we stick to ENV variables (they are all scoped to WHIRR_ anyway + we can reduce boilerplate in bin/whirr for converting to system properties). The thing is that without (mandatorily) using env variables we can have multiple installations each with its whirrrc file, besides testing for env variables within java is hacky. As is it now you can still use env variables when needed, but these are transformed to script scope variables. Also I think we can have better defaults in whirrrc - aws ENV variables. What so you suggest? whirr picking up aws env variables when none other are provided? (btw this is another reason not to require whirr_ env variables as if we implement this the user would expect the properties to change when he changes the aws ones, but they don't because previous script runs already set the whirr_ ones and the script would prefer whirr_ vs aws_) If so I think this is a nice idea and will include it in the next patch. Also the next patch does a much more thorough job of testing precedence (sys vs props vs args).
        Hide
        David Alves added a comment -

        wrt to reading the aws env properties, even though I thought it was a good idea at first now I'm not so sure. aws might not be the only provider using env props and we certainly shouldn't demonstrate preference, right? in order not to do so we should support multiple provider env properties, which will be a pain.

        Show
        David Alves added a comment - wrt to reading the aws env properties, even though I thought it was a good idea at first now I'm not so sure. aws might not be the only provider using env props and we certainly shouldn't demonstrate preference, right? in order not to do so we should support multiple provider env properties, which will be a pain.
        Hide
        David Alves added a comment -

        Corrected a couple of bugs

        Added tests to make sure the properties precedence is correct (args,props file,sys props).

        Thing not unit tested because it happens outside of java is precedence of env props vs file props but I tested this manually.

        Show
        David Alves added a comment - Corrected a couple of bugs Added tests to make sure the properties precedence is correct (args,props file,sys props). Thing not unit tested because it happens outside of java is precedence of env props vs file props but I tested this manually.
        Hide
        David Alves added a comment -

        small correction to the previous patch

        Show
        David Alves added a comment - small correction to the previous patch
        Hide
        Andrei Savu added a comment -

        Let me give it a try to rework this patch. There are a few problems. (e.g. WHIRR_BLOBSTORE_PROVIDER=$PROVIDER is wrong - ClusterSpec knows how to do this conversion if the blob store provider is unset, whirrrc is too complicated - it should contain only the credentials).

        Show
        Andrei Savu added a comment - Let me give it a try to rework this patch. There are a few problems. (e.g. WHIRR_BLOBSTORE_PROVIDER=$PROVIDER is wrong - ClusterSpec knows how to do this conversion if the blob store provider is unset, whirrrc is too complicated - it should contain only the credentials).
        Hide
        Andrei Savu added a comment -

        Here is what I was thinking about. Wdyt? I will improve things a bit more later today.

        Show
        Andrei Savu added a comment - Here is what I was thinking about. Wdyt? I will improve things a bit more later today.
        Hide
        Tom White added a comment -

        The simplification of the recipes is great! Can we drop the whirr.

        {provider,identity,credential}

        lines too since they are inherited from whirr-default.properties? Do David's tests need incorporating too?

        Nit: maybe reword "to be able to do punctual overrides on recipes" as "to be able to do one-off overrides on recipes".

        Show
        Tom White added a comment - The simplification of the recipes is great! Can we drop the whirr. {provider,identity,credential} lines too since they are inherited from whirr-default.properties? Do David's tests need incorporating too? Nit: maybe reword "to be able to do punctual overrides on recipes" as "to be able to do one-off overrides on recipes".
        Hide
        Andrei Savu added a comment -

        Thanks Tom for reviewing! I have updated the patch as requested. Porting the tests is not trivial because the override logic is only in bin/whirr (maybe we can find a way in another jIRA). I will go ahead and commit.

        Show
        Andrei Savu added a comment - Thanks Tom for reviewing! I have updated the patch as requested. Porting the tests is not trivial because the override logic is only in bin/whirr (maybe we can find a way in another jIRA). I will go ahead and commit.
        Hide
        Andrei Savu added a comment -

        Committed to trunk. I like it a lot that we removed the credentials from .properties files.

        Show
        Andrei Savu added a comment - Committed to trunk. I like it a lot that we removed the credentials from .properties files.

          People

          • Assignee:
            Andrei Savu
            Reporter:
            Tom White
          • Votes:
            2 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development