mdsol / mauth-client-ruby

Mauth client in Ruby
MIT License
4 stars 0 forks source link

mauth-client should not require a yaml file #14

Open johngluckmdsol opened 6 years ago

johngluckmdsol commented 6 years ago

When running the executable file, mauth-client currently requires that configuration file exists in a given place, but this makes the app more painful to containerize.

Instead, we should additionally allow the mauth configuration to pass through from the environment, so that if the required variables are present in the environment, the client executable should not look for the file

jfeltesse-mdsol commented 6 years ago

You have a point but considering most (all?) our gems are working using that "get config from env, persist in file, have app read file" paradigm I'm not sure how worthy it would be to support this for mauth-client only. It looks to me it's a kind of "all-or-none" deal and to start a global effort to have our gems support that alternative config style we need to have product agree this is needed and worth prioritize.

Also, discussing with @ssteeg-mdsol, the thing is that the dice_bag approach we're using now does more than just read the env variables.

1) For instance we do this in the newrelic config file:

log_level: "<%= configured.new_relic_debug == 'true' ? 'debug' : 'info' %>"
# ...
stack_trace_threshold: <%= configured.new_relic_debug == 'true' ? '0.1' : '0.5' %>

Without this "post processing" you'd have to provide a single env variable for everything and set every single value. Also, with a config file we can provide sane defaults. To put this in perspective, for newrelic only we'd have 7 env variables to provide instead of the currently required 2.

2) dice_bag provides some utilities such as !, ensure_is_private_key and generate_private_key. Having the gems we make read directly from the env means every gem would need to replicate those, as needed. Or we'd need some utility gem that all the gems would depend on.

By the way, right now those config files templates are checked in the repositories and re-generated from env at build time, effectively acting as reading from env + extra defaults/behaviors that can be overridden as needed. Can you provide a concrete example of the pain points you're experiencing regarding containerization so we're on the same page?

cc @mdsol/team-10

johngluckmdsol commented 6 years ago

@jfeltesse-mdsol

Here is a quote from the 12 factor site - "Apps sometimes store config as constants in the code. This is a violation of twelve-factor, which requires strict separation of config from code. Config varies substantially across deploys, code does not." But effectively, dicebag is making us store constants in code because it is writing out to yaml which our application in turn specifically searches for and doesn't work if it's not there.

In order to containerize our applications as they are, best practice would be to have an entrypoint, which would would ideally simply be starting the service. In our case, this is impossible because we need configure our app before starting the service and we ideally we should not do so at build time. So for harmony, we are having to write shell scripts for every single app that does the follow

  1. run bundle exec rake config
  2. start rails

This going to be painful. I'm happy to do this for the base applications, but if it's going to be a lot of work to do it wrong, why not spend the effort to do it right?

You are absolutely right. It will take work to move the organization out of this bad habit. That's not justification for leaving it as is. If it's broke, fix it.

Copying @benton, @mdsol/team-10

I disagree that we all need product to agree to prioritize. This is an architecture issue. We can do this as we go along, slowly encouraging people to shift to this model.

Furthermore, having the application look in the environment first doesn't mean you can't still use dicebag. It simply means that you have the option not to.

Re: gems that do this better - https://github.com/markround/tiller

jfeltesse-mdsol commented 6 years ago

Let me start from the bottom with your example of tiller.

As far as I can see, tiller does exactly the same as dice_bag, in more convoluted and less flexible way, and the only edge it has is it allows you to specify a command to run afterwards so you don't have to write a script such as the one you describe. This feature feels wrong to me though, I'd like my app to be running on their own, not as a child process of some config utility.

I think a better example for our gems to take inspiration from is the newrelic gem which works as you request mauth-client does, the diagram in the docs is self-explanatory.

Let me be clear on one thing, I am not opposed to the idea itself, I'm questioning how we can use that model while keeping the nice things that dice_bag-based configuration provides. See my list in my previous comment, how do you suggest we deal with those?

To take the ! utility for instance, right now it blows up at build time which is good because no need to go all the way to try to run the app to then realize things are blowing up. We need to come up with a utility gem to do those sanity checks and provide other helpers I suppose but that's another gem to develop, test and maintain. Ideas welcome.

I disagree that we all need product to agree to prioritize. This is an architecture issue. We can do this as we go along, slowly encouraging people to shift to this model.

What I mean by "it's an all-or-nothing" thing is that even if tomorrow mauth-client supported that new model, you'd still need to use that script of yours to account for all the other gems that do not support it yet. Sure we have to start somewhere but wouldn't it be better if all the gems needing to support that model could be done in the same push? That's why I'm talking about priority mgmt, we're not talking about a half-day gig here.

johngluckmdsol commented 6 years ago

It will never be done in the same push. You certainly must realize that. I chose to pick on mauth-client-ruby first because it's the closest to working. It will work when embedded in an app. It's just that there are specific lines in the bin exec that prevent it from working the same as when it's embedded. I'm willing to guess almost every one of our apps is doing it wrong. I'm starting to gather a coalition of the willing. If you don't want to be part of it, that's fine. I'll argue it with you later.

I chose Tiller as an example. There are other apps that do this as well. NewRelic config is fine.

But yes, what I'm talking about is dynamic configuration.

By your logic I shouldn't clean my toilet if my bathroom's dirty

I understand that we lose logic if we replace dicebag. However, think about that. Logic is the application. You've just admitted to having application logic in the mechanism you use for configuration. If that isn't a violation of 12 Factor, then I don't know what is

jfeltesse-mdsol commented 6 years ago

If you don't want to be part of it, that's fine. I'll argue it with you later. By your logic I shouldn't clean my toilet if my bathroom's dirty

I think you're getting a bit intense so I'll drop from this discussion.

johngluckmdsol commented 6 years ago

I sent you a personal note. I am not intending this conversation to be personal, I am just attempting to use a bit of humor to lighten the mood and apparently that backfired.

I would however, like you to address not the logistics of releasing the fix but the nature of the fix. Do you agree, details aside, this should be fixed?

ssteeg-mdsol commented 6 years ago

@johngluckmdsol, a couple of questions:

johngluckmdsol commented 6 years ago

@ssteeg-mdsol Re: Tiller. It's just an example. There are other applications that do this. Tiller takes the extra step of exposing any variable it encounters to the environment, so the application can still get them from the environment. Dicebag is not dynamic configuration because I can't change the configuration of my application without re-writing my configuration and restarting it.

Re: logic to ensure config - This usage is clearly a violation of 12 factor. This usage of dicebag disguises application logic as configuration. You should be resolving problems in your configuration in your application but the approach we've taken assumes an operational step of sorting out your configuration before you start your application.

But yes, all I'm asking is to replace the specific lines you mention

benton commented 6 years ago

Just chiming in to supply some info about how we process app configuration, including secrets, in our Docker / ECS pipeline.

These values must be in-memory for our apps to function correctly, but they could be "injected" at (at least) four different points in the deployment process:

  1. At Build time. We chose not to include app config during the time that app dependencies are being gathered. In fact, we go to some length to avoid this, even though some secretes are often required for the Build. This does provide security benefits, but frankly, we do it mainly so that we can create all related Release images from the same Build, which has a very large efficiency and safety impact. In other words, Build images were kept config-free mostly for pragmatic reasons related to the validation / promotion workflow.

  2. At Release time. We chose (created, actually) this point in the process because we wanted to ensure that we generate immutable, ready-to-run binary artifact(s) that can be examined, tested, escrowed, etc. – before ever being deployed. This is super-useful for debugging problems, because you can pull an image to your local environment for inspection. Configuration values are saved into the Posix environment because of 12-factor principles; writing them to the filesystem is entirely optional. We could have insisted that all apps read their config directly from the environment as a prerequisite for moving to ECS. But that would have locked out a large group of apps, pending a lot of non-customer-benefit work. Or worse, it would have caused teams to wrap their slow-running Release command into the app startup process (we already see this mistake sometimes).

  3. At Deploy time. We could generate those ready-to-run artifacts at deploy time, but in addition to the reasons given above, we are striving to minimize deploy times.

  4. At Run time. Some apps are starting to move to "dynamic configuration" - the ability to (re)load part or all the config values after startup. When combined with a secrets service (like Vault or similar), this can provide valuable security and management benefits, but obviously, it requires team-by-team buy-in. I know that this is the direction that the security folks want to move in long-term, because they want to make all secrets centralized and ephemeral, but we're not there yet.

Finally, I'd like to point out a little-used feature of the ECS workflow that allows you to validate your configuration at the time you Publish it. You can include a config_schema key in your .12factor/app_dscription.json file, whose value is a JSON schema document.

"config_schema": {
  "type": "object",
  "properties": {
    "DATABASE_USERNAME": { "type": "string" },
    "GOOGLE_CLIENT_ID": { "type": "string" },

Then, when you're using the Config tab of Medistrano's ECS workflow, you can select a Build ID (would probably be nicer as the Build's Git Reference), you can choose to validate your entire configuration set using the schema for that Build.

config-validation-in-medistrano

This feature does not provide the flexibility of Dicebag, with its ability to use Ruby directly from ERB templates - but it's platform-independent, and uses a well-accepted, well-documented, machine-parseable standard.

johngluckmdsol commented 6 years ago

So I have been able to bypass the client in application code by using an initializer that contains the following code

config['mauth_baseurl'] = ENV['MAUTH_URL']
config['mauth_api_version'] = ENV['MAUTH_API_VERSION'] || 'v1'
config['app_uuid'] = ENV['MAUTH_APP_UUID']
config['private_key'] = ENV['PRIVATE_KEY']

MAUTH_CONF = MAuth::Client.default_config('mauth_client' => config)

However, the cli won't let you do this because of this. https://github.com/mdsol/mauth-client-ruby/blob/master/exe/mauth-client#L70

I'm simply asking for a change that would check the appropriate ENV vars before failing

cabbott commented 6 years ago

@johngluckmdsol do you have a working branch you'd like to use as the basis of a PR?

johngluckmdsol commented 6 years ago

@cabbott, no I don't. The above is harmony code. I don't have an immediate need to make the CLI client work because I'm instantiating it in my application from the gem, which is the problem that I "solved" above. I'm not entirely satisfied with the above as a pattern everyone can use because it ignores other configuration options in the client.

I would start in the executable, not in the client as @ssteeg-mdsol mentions, so I'm inclined not to submit a PR until I better understand what would be acceptable to the team.

Perhaps we should focus first on modifying dicebag so it adds all variables to the environment and then change this code to check just for the env vars instead of the file.

I'll schedule a call so we can discuss