Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

allow exposing more environment variables #292

Merged
merged 2 commits into from
Mar 14, 2017
Merged

allow exposing more environment variables #292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
319f06b
[apicast] allow customizing main section of nginx config
mikz Mar 13, 2017
203b5b9
[apicast] automatically forward env variables
mikz Mar 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
### Changed

- Use per request configuration when cache is disabled [PR #289](https://github.com/3scale/apicast/pull/289)
- Automatically expose all environment variables starting with `APICAST_` or `THREESCALE_` to nginx [PR #292](https://github.com/3scale/apicast/pull/292)

### Added

- Backend HTTP client that uses cosockets [PR #295](https://github.com/3scale/apicast/pull/295)
- Ability to customize main section of nginx configuration (and expose more env variables) [PR #292](https://github.com/3scale/apicast/pull/292)

### Removed

Expand Down
1 change: 1 addition & 0 deletions apicast/bin/apicast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ done

main+=("daemon ${daemon};")
main+=("worker_processes ${worker_processes};")
main+=$(printenv | awk '$1 ~ /^(APICAST|THREESCALE)_/ {split($0,env,"="); print "env", env[1] ";"}')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I don't like this approach too much, for the following reasons:

  1. It makes bin/apicast the only way to start APIcast that would work natively out-of-the-box. Even though it is the preferred way to start APIcast, it might not suite some users who have some already established deployment pipelines (Chef, Puppet etc, where OpenResty is started as a service). Well, yes, you can of course add all the required env definitions in main.d, but it's easy to forget something there.

  2. This way I think we are loosing the "single source of truth". It's actually super practical to have all the supported env vars listed in one place. We do have .env and parameters doc that can help, but as you usually (rightfully) say @mikz - the docs can get out of sync (the .env already did, for example) :)
    So, it would actually be good to keep the complete list in some file, that can be extended with other files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. bin/apicast is the only way to start apicast. It was before anyway. It sets up daemon mode, worker processes and everything. It finds the correct path etc etc etc. In the future we might have to template the whole nginx config, so you can forget there is nginx now.

  2. It is error prone to add everything into the configuration. Tests can be passing but without full integration suite there is no way to actually verify it. There never was single source of truth because code evaluated in the init phase had access to all the variables (even not defined) where code executed later would get them stripped. That was super confusing and not predictable.

  3. I don't see a way to get dynamic variables working for [configuration] allow locking service configuration version #293. Unless we figure out some super complicated format like: id:version;id2:version2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Hm, OK, I thought it was just a handy way to override NGINX default values. So, as it's not the case, I think it might complicate the transition to v2/v3 for the existing customers using self-managed deployment. Ideally, APIcast would work if you just drop the contents of the repo to the default nginx conf directory and restart the process.
    But if we can't have it, I guess we'd need some kind of transition guide. @3scale/support what do you think?

  2. My point is that I as a user want to have a complete list of the env vars that APIcast "understands", so I know what to export. If it's not in the code (configuration in this case) itself, it's OK, but we just need to make sure that the doc is always in sync.

  3. OK, I was not aware of this requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mayorova want to create some checkboxes todo in the description what needs to be done for this to be merged?

  1. I don't see anywhere in the documentation that we would suggest using nginx to start by pointing to a configuration. Not sure why anyone would do that. Also not sure how many people are using v2 already (but close to 0).

  2. We can't make sure it stays in sync unless we are able to code it. Reviews fail and docs get out of sync. Proposals welcome. Tests are a good way to make sure the documentation works (and become the documentation).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. OK, not related to this PR, probably we can discuss with @3scale/support and open a separate issue if needed.

  2. Agreed. The proposal for now would just to be disciplined and document the env vars in the parameters doc as soon as they are added/changed/deleted (which from what I can tell has been done properly lately).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory we could write a ruby script that goes through the codebase, finds strings that are in all caps and compares them to the parameters doc. This could be executed as a lint.

Maybe when we have more time for tooling and we burn ourselves .


function join_by { local IFS="$1"; shift; echo "$*"; }
args=$(join_by '' "${args[@]}")
Expand Down
12 changes: 1 addition & 11 deletions apicast/conf/nginx.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,9 @@
env THREESCALE_DEPLOYMENT_ENV;
env THREESCALE_PORTAL_ENDPOINT;
env THREESCALE_CONFIG_FILE;
env APICAST_CUSTOM_CONFIG;
env APICAST_PATH_ROUTING_ENABLED;
env APICAST_SERVICES;
env REDIS_HOST;
env REDIS_PORT;
env RESOLVER;
env APICAST_MODULE;
env APICAST_RESPONSE_CODES;
env APICAST_MANAGEMENT_API;
env BACKEND_ENDPOINT_OVERRIDE;

env APICAST_CONFIGURATION_LOADER;
env APICAST_CONFIGURATION_CACHE;
include ../main.d/*.conf;

error_log /dev/null emerg;

Expand Down
Empty file added apicast/main.d/.keep
View file
Open in desktop
Empty file.