Skip to content
This repository has been archived by the owner on Mar 23, 2019. It is now read-only.

Pass process envvars into conductor/container envs #711

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Pass process envvars into conductor/container envs #711

wants to merge 1 commit into from

Conversation

shalomb
Copy link

@shalomb shalomb commented Aug 21, 2017

  • Allow for env. vars. set in the shell/parent process to be usable
    inside service.environment, vars-files and defaults sections.
  • Addresses Issue #703
ISSUE TYPE
  • Feature Pull Request
  • New Module Pull Request
  • Bugfix Pull Request
  • Docs Pull Request
SUMMARY

Allow for environmental variables set in ansible-containers's parent process to be used in

  • Setting docker ENV vars for a container (via service.environment)
  • Variables in roles and playbooks in the ansible-container workflow.
$ cat container.yml
---

version: "2"

defaults:
  https_proxy: '$http_proxy'

settings:
  conductor:
    base: debian:latest

services:
  web:
    from: busybox:latest
    ports:
      - "8080:80"
    environment:
      - 'http_proxy=$http_proxy'
      - 'https_proxy={{ https_proxy }}'

    command: ['/bin/sh', '-lc', 'while :; do sleep 10; { date; env; } >> /tmp/container.log; done']

registries: {}

# before
$ HTTP_PROXY='http://example.org:3821' ansible-container --devel run --production && docker exec -it simplebusybox_web_1 env | grep -i http

# after
$ HTTP_PROXY='http://example.org:3821' ansible-container --devel run --production && docker exec -it simplebusybox_web_1 env | grep -i http
...
https_proxy=http://example.org:3821
http_proxy=http://example.org:3821

$ grep -i http devel.yml
https_proxy: 'http://smart.example.org:8080'

$ http_proxy='http://example.org:3821' ansible-container --devel --vars-files devel.yml run --production && docker exec -it simplebusybox_web_1 env | grep -i http
...
https_proxy=http://smart.example.org:8080
http_proxy=http://example.org:3821

* Allow for env. vars. set in the shell/parent process to be usable
  inside service.environment, vars-files and defaults sections.
* Addresses [Issue #703](#703)
@shalomb
Copy link
Author

shalomb commented Aug 21, 2017

It's not apparent immediately why the checks failed -- I'll try and work out why when I get some time.

@chouseknecht
Copy link
Contributor

@shalomb

I have a concern about this. And, I think my concern is highlighted by your use case. Your example variables, https_proxy and http_proxy, are most likely only relevant and useful during build. I suspect that the proxy needs to be set in order to download OS packages, Python modules, etc. during build.

At runtime, however, the user may not want these variables set, or may want them set to a different value. But because they were set on the service container during build, they get baked into the image. See the specific code here. We either need to go about this differently, or maybe we should not automatically add environment vars to the image metadata.

@mvk
Copy link

mvk commented Jan 28, 2018

@chouseknecht you are correct. should is the main theme:

Each 'phase' (build, run, etc.) may need different env, even inside the same organization/team.
So, "defaults" would be a good place to put 'common' envs.

Then maybe additional attribute 'phases' or 'steps' should handle each step's env, e.g.:

defaults:
  environment:
    VAR1: 'val1'
steps:
  build:
    environment:
      VAR1: 'val2'

YaML gets ... ugly. but the above would say that build will override VAR1, and the rest - would use the default...

WDYT?

@shalomb
Copy link
Author

shalomb commented Jan 29, 2018

@chouseknecht, @mvk - Sorry, this skipped my attention and it's been a while since I worked on ansible-container so forgive me if I'm a bit rusty on the details around our use-case.

I should just add some background - Ultimately, our use case revolves around doing CI/CD on the same openstack-based cloud platform where tenants can only access webservices in other tenants or on the internet via HTTP_PROXY/HTTPS_PROXY/etc envvars being set - we mostly (but not always) do require these vars to be the same across build and dev/test/deploy phases. But depending on which tenant containers get placed in, we'd like to be able to change these for a phase by passing them in from the CI/CD environment (gitlab in our case, possibly some other scheduler in the future).

We really struggled with ansible-container getting this story to work. I recall

  1. Being surprised at being unable to set envvars for the build stage. This appears to be fixed now.
  2. Not being able to pass in envvars via the process environment/command line. Without this capability, we have to mangle the container.yml for the environment we are scheduled in or manage settings in smaller --vars-files - as in our case, the HTTP/HTTPS_PROXY env-vars can vary from environment to environment and neither option is clean for us as we (will) operate at some scale.

We would really like to solve #2 - I guess that's the primary motivation for this PR. (e.g. being able to do this VAR1=foo ansible-container build ... or even VAR1=bar ansible-container run)

As for the coincidental, I wasn't aware that vars get baked in to the image - I can see why it would be done but to me that sounds off (if you ask for our opinion - we would like for build time info to creep into a container image - so for this alone, I would vote for not automatically setting envvars to the image metadata).

@mvk
Copy link

mvk commented Jan 29, 2018

@shalomb I understand the idea, and the motivation. it's cool.
I disagree with the implementation :)
Will try to pull this from your fork, and reorg it a bit differently.
Some points:

  • environment should be (IMHO) a dict, not a list.
  • there should not be expansion of vars, but there should be full support for ansible jinja2 filters, i.e. in this case I'd want to see this:
$ cat container.yml
---

version: "2"

defaults:
  https_proxy: '{{lookup("env", "http_proxy")}}'

settings:
  conductor:
    base: debian:latest

services:
  web:
    from: busybox:latest
    ports:
      - "8080:80"
    environment:
      http_proxy: '{{lookup("env", "http_proxy")}}'
      https_proxy: '{{https_proxy}}'
    command: ['/bin/sh', '-lc', 'while :; do sleep 10; { date; env; } >> /tmp/container.log; done']

registries: {}

@mvk
Copy link

mvk commented Jan 29, 2018

@shalomb about #2 in general, I think if the --vars-files are working properly, It would be preferrable and safer. and it can be easily automated as well, you don't have to run env vars.
you probably will drive calls to ansible-container either by shell script or another something-something that allows you to pass params.
Object to Yaml is very easy to convert. so just create your custom vars file.

IS your concern about sensitive data you intend to expose as env variable? then use secrets.

@shalomb
Copy link
Author

shalomb commented Jan 29, 2018

@mvk - I think if ansible-container can support full jinja2 templating for container.yml - we have our problem solved :). We'd favour that over anything else as it's consistent with ansible. I like this idea best.

--vars-files work somewhat OK for us (we're not worried too much about secrets) .. but we feel it's kinda hacky for our use-cases, as we need at least one of these for every environment and maintain congruity with the same information in gitlab's dynamic environments. The maintenance of these files inside the git repo would trigger all sorts of jobs - we now have these --vars-files generated on the fly but per occam's razor, losing the intermediate step and have ansible-container pass envvars down into the containers would be simpler/nicer. But equally, we could lose all this hacky stuff if jinja templating was supported, I like that idea.

@mvk
Copy link

mvk commented Jan 29, 2018

@shalomb clearly you were thinking about your specific use case 👍
the concern from the ansible-container project angle, IMHO (I'm just chirping in after 2d of playing with it)
is the multi-tenant deployments, when a tenant may attempt to accidentally (or on purpose) either use incorrect value for env variable, or to extract values of "other people's" variables.

This is why env is isolated & stanitized, and you are supposed to specify it.

Maybe as a workaround, wrap your calls to ansible-container with scripts that would pick up the explicitly specified list of env variables, and generate a vars file out of them.

I'll try to see what's the reason why ansible-container does not utilize the power of ansible templating.... maybe there is just lack of resources/time.

@j00bar
Copy link
Contributor

j00bar commented Jan 29, 2018

Hey folks -

Thank you for your work and interest on this. Your use case makes sense, and I'm grateful for the effort to write and contribute code. However, I don't believe your approach is the right path forward. A couple of notes that might be of use:

  1. You can already set environment variables for your conductor in settings -> conductor -> environment.
  2. Setting environment variables at the container level during build has an unfortunate side-effect: those environment variables are burned into the committed images. The Docker engine does not offer a way to "unset" environment variables or to "unmount" a volume. It's easily one of the most requested features of their engine and for several years, they've declined to support it. See: Reset properties inherited from parent image moby/moby#3465.
  3. IMHO, the proper place to set environment variables during build is in Ansible by including them within the playbook itself - this sets them for build without burning them into the resulting image. Given the playbook is generated by Ansible Container, this isn't something you can currently tweak. But I think this would roll in nicely to Added support for override command directive when conductor is building image #719 - where the goal is to allow build_overrides much as we allow dev_overrides. But environment vars would have to be popped out and supplied to the generated playbook here: https://github.com/ansible/ansible-container/blob/cc5bdfe/container/utils/__init__.py#L241

Does that make sense? Have I understood your use case and constraints correctly? Thanks!

@mvk
Copy link

mvk commented Jan 29, 2018

@j00bar you are covering my use case, and I will find out how to skin this hairless cat in weird environments with scripting, or vars-files generating.
Now, I need to handle #866 and then we can pull roles off intranet git server (without initially pulling them running ansible-galaxy on docker host.

Offtopic Q: why is full ansible-compat templating not available/used in this project?

@j00bar
Copy link
Contributor

j00bar commented Jan 29, 2018

Please note my comment on #866 about providing SSH keys to Galaxy.

The container.yml is largely templated, with few exceptions, and those exceptions exist mostly for variable precedence reasons. What are you finding isn't templated?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants