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

Improve configuration expansion #1759

Closed
BrynCooke opened this issue Sep 13, 2022 · 0 comments · Fixed by #1763
Closed

Improve configuration expansion #1759

BrynCooke opened this issue Sep 13, 2022 · 0 comments · Fixed by #1763
Assignees
@BrynCooke

Comments

@BrynCooke
Copy link
Contributor

BrynCooke commented Sep 13, 2022 •
edited
Loading

Currently there are some issues with configuration environment variable expansion:

  • Failed expansions are silently skipped
  • Non standard expansion syntax is used, : vs :-
  • No way to lock down env variables that can be expanded.
  • No way to extend expansion in the future.

Proposal is to:

  • Make env. a prefix for any environment variable expansion. Thus we can add things like file. later.
  • Use :- for defaulting.
  • Add the ability to customize the lookup of env variables. (Will not be part of the our public supported API yet)
@BrynCooke BrynCooke mentioned this issue Sep 13, 2022
Merged
@BrynCooke BrynCooke linked a pull request Sep 13, 2022 that will close this issue
Environment expansion enhancements #1763
Merged
BrynCooke added a commit that referenced this issue Sep 13, 2022
@BrynCooke
Environment expansion enhancements (#1763)
4cb425d 
### Environment variable expansion enhancements
([#1759](#1759))

* Environment expansions must be prefixed with `env.`. This will allow
us to add other expansion types in future.
* Change defaulting token from `:` to `:-`. For example:

  `${env.USER_NAME:Nandor}` => `${env.USER_NAME:-Nandor}`
* Failed expansions result in an error.
  
Previously expansions that failed due to missing environment variables
were silently skipped. Now they result in a configuration error. Add a
default if optional expansion is needed.

### Environment variable expansion prefixing
([#1759](#1759))

The environment variable: `APOLLO_ROUTER_CONFIG_ENV_PREFIX` can be used
to prefix environment variable lookups during configuration expansion.
This may be useful for security. This feature is undocumented and
unsupported and may change at any time.

For example: 

`APOLLO_ROUTER_CONFIG_ENV_PREFIX=MY_PREFIX`

Would cause:
`${env.FOO}` to be mapped to `${env.MY_PREFIX_FOO}` when expansion is
performed.



<!--
First, 🌠 thank you 🌠 for considering a contribution to Apollo!

Some of this information is also included in the /CONTRIBUTING.md file
at the
root of this repository.  We suggest you read it!

  https://github.com/apollographql/router/blob/HEAD/CONTRIBUTING.md

Here are some important details to keep in mind:

* ⏰ Your time is important
To save your precious time, if the contribution you are making will
take more than an hour, please make sure it has been discussed in an
        issue first. This is especially true for feature requests!

* 💡 Features
Feature requests can be created and discussed within a GitHub Issue.
Be sure to search for existing feature requests (and related issues!)
prior to opening a new request. If an existing issue covers the need,
please upvote that issue by using the 👍 emote, rather than opening a
        new issue.

* 🕷 Bug fixes
These can be created and discussed in this repository. When fixing a
bug,
please _try_ to add a test which verifies the fix. If you cannot, you
should
still submit the PR but we may still ask you (and help you!) to create a
test.

* 📖 Contribution guidelines
Follow https://github.com/apollographql/router/blob/HEAD/CONTRIBUTING.md
when submitting a pull request. Make sure existing tests still pass, and
add
        tests for all new behavior.

* ✏️ Explain your pull request
Describe the big picture of your changes here to communicate to what
        your pull request is meant to accomplish. Provide 🔗 links 🔗 to
associated issues! Documentation in the docs/ directory should be
updated
        as necessary.  Finally, a /CHANGELOG.md entry should be added.

We hope you will find this to be a positive experience! Contribution can
be
intimidating and we hope to alleviate that pain as much as possible.
Without
following these guidelines, you may be missing context that can help you
succeed
with your contribution, which is why we encourage discussion first.
Ultimately,
there is no guarantee that we will be able to merge your pull-request,
but by
following these guidelines we can try to avoid disappointment.

-->

Co-authored-by: bryn <bryn@apollographql.com>
BrynCooke pushed a commit that referenced this issue Sep 14, 2022
Otel configuration updated to use expansion
279bbd6 
File and env access in configuration now use the generic expansion mechanism introduced in [#1759](#1759.

```yaml
      grpc:
        key:
          file: "foo.txt"
        ca:
          file: "bar.txt"
        cert:
          file: "baz.txt"
```

Becomes:
```yaml
      grpc:
        key: "${file.foo.txt}"
        ca: "${file.bar.txt}"
        cert: "${file.baz.txt}"
```
or
```yaml
      grpc:
        key: "${env.FOO}"
        ca: "${env.BAR}"
        cert: "${env.BAZ}"
```
@BrynCooke BrynCooke mentioned this issue Sep 14, 2022
Merged
BrynCooke pushed a commit that referenced this issue Sep 14, 2022
Otel configuration updated to use expansion
b4170b1 
File and env access in configuration now use the generic expansion mechanism introduced in [#1759](#1759.

```yaml
      grpc:
        key:
          file: "foo.txt"
        ca:
          file: "bar.txt"
        cert:
          file: "baz.txt"
```

Becomes:
```yaml
      grpc:
        key: "${file.foo.txt}"
        ca: "${file.bar.txt}"
        cert: "${file.baz.txt}"
```
or
```yaml
      grpc:
        key: "${env.FOO}"
        ca: "${env.BAR}"
        cert: "${env.BAZ}"
```
BrynCooke pushed a commit that referenced this issue Sep 14, 2022
Otel configuration updated to use expansion
f47e392 
File and env access in configuration now use the generic expansion mechanism introduced in [#1759](#1759.

```yaml
      grpc:
        key:
          file: "foo.txt"
        ca:
          file: "bar.txt"
        cert:
          file: "baz.txt"
```

Becomes:
```yaml
      grpc:
        key: "${file.foo.txt}"
        ca: "${file.bar.txt}"
        cert: "${file.baz.txt}"
```
or
```yaml
      grpc:
        key: "${env.FOO}"
        ca: "${env.BAR}"
        cert: "${env.BAZ}"
```
BrynCooke pushed a commit that referenced this issue Sep 14, 2022
Otel configuration updated to use expansion
78dcbd3 
File and env access in configuration now use the generic expansion mechanism introduced in [#1759](#1759.

```yaml
      grpc:
        key:
          file: "foo.txt"
        ca:
          file: "bar.txt"
        cert:
          file: "baz.txt"
```

Becomes:
```yaml
      grpc:
        key: "${file.foo.txt}"
        ca: "${file.bar.txt}"
        cert: "${file.baz.txt}"
```
or
```yaml
      grpc:
        key: "${env.FOO}"
        ca: "${env.BAR}"
        cert: "${env.BAZ}"
```
BrynCooke added a commit that referenced this issue Sep 14, 2022
@BrynCooke @o0Ignition0o
Otel configuration updated to use expansion (#1774)
d13a9de 
Fixes #1772

File and env access in configuration now use the generic expansion
mechanism introduced in
[#1759](#1759).

```yaml
      grpc:
        key:
          file: "foo.txt"
        ca:
          file: "bar.txt"
        cert:
          file: "baz.txt"
```

Becomes:
```yaml
      grpc:
        key: "${file.foo.txt}"
        ca: "${file.bar.txt}"
        cert: "${file.baz.txt}"
```
or
```yaml
      grpc:
        key: "${env.FOO}"
        ca: "${env.BAR}"
        cert: "${env.BAZ}"
```

Co-authored-by: bryn <bryn@apollographql.com>
Co-authored-by: Jeremy Lempereur <jeremy.lempereur@gmail.com>
@abernix abernix mentioned this issue Sep 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@BrynCooke BrynCooke

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Environment expansion enhancements apollographql/router
1 participant
@BrynCooke