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.

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

Allow . in Environment Variable Names #3760

Merged
merged 3 commits into from
Jan 22, 2018
Merged

Allow . in Environment Variable Names #3760

merged 3 commits into from
Jan 22, 2018

Conversation

angrycub
Copy link
Contributor

From https://github.com/appc/spec/blob/master/spec/aci.md:

environment (list of objects, optional) represents the app's environment variables (ACE can append). The listed objects must have two key-value pairs: name and value. The name must consist solely of letters, digits, and underscores '_' as outlined in IEEE Std 1003.1-2008, 2016 Edition, with practical considerations dictating that the name may also include periods '.' and hyphens '-'. The value is an arbitrary string. These values are not evaluated in any way, and no substitutions are made.

Dotted environment variables are frequently used as a part of the Spring Boot pattern. (re: ZD-6116)

This PR specifically doesn't address the conversion of hyphens (-) due to an issue with rkt [Nomad GH # 2358].

Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

Can you also fix our environment variable parsing library? https://github.com/hashicorp/go-envparse/blob/master/envparse.go#L116-L144

I don't want to merge this until we have parity in our environment variable parsing library.

@angrycub
Copy link
Contributor Author

@schmichael, upon further reflection, based on the ask to adjust go-envparse, I ended up thinking that this might not be the best course of action. In the case of shell environment variables, dots have mixed support, tending to the unsupported. So for drivers like exec and raw_exec this type of change could allow for unreadable variables. I was able to find what appears to be the Spring Boot workaround using the SPRING_APPLICATION_JSON environment variable to work around the naming situation there.

Docker seems to happily allow the creation of a dotted environment variable via their -e parameter, so perhaps a solution is to create a strict and relaxed version of the check?

@angrycub
Copy link
Contributor Author

An additional comment I have received on this issue for further discussion.

[This process] is not a shell script and . is certainly legal for ENV vars passed to processes. Perhaps this should be handled in the driver layer and then drivers can override any default behavior. Because docker is constrained because of [other drivers'] limitations.

@schmichael
Copy link
Member

schmichael commented Jan 18, 2018

This sample job outputs (as viewed with nomad logs ...):

foo
foo.BAR
foo.bar

Meaning ${FOO.BAR} is a valid env var for dash (Debian/Ubuntu's version of /bin/sh).

Switching to /bin/bash produces the same logs.

So I'm inclined to accept this PR (if we also update the library as stated before) as it seems dots are fairly widely supported even if bash does not let you interactively set them.

angrycub added a commit to angrycub/go-envparse that referenced this pull request Jan 19, 2018
From https://github.com/appc/spec/blob/master/spec/aci.md:

> environment (list of objects, optional) represents the app's environment variables (ACE can append). The listed objects must have two key-value pairs: name and value. The name must consist solely of letters, digits, and underscores '_' as outlined in IEEE Std 1003.1-2008, 2016 Edition, with practical considerations dictating that the name may also include periods '.' and hyphens '-'. The value is an arbitrary string. These values are not evaluated in any way, and no substitutions are made.

Dotted environment variables are frequently used as a part of the Spring Boot pattern. (re: ZD-6116)

See also [Nomad PR# 3760](hashicorp/nomad#3760)
@angrycub
Copy link
Contributor Author

@schmichael - Made PR for go-envparse hashicorp/go-envparse#2

@schmichael
Copy link
Member

Mind updating go-envparse in this PR? Should be:

go get -u github.com/hashicorp/go-envparse
govendor update github.com/hashicorp/go-envparse
# git commit + push

angrycub and others added 3 commits January 22, 2018 13:59
From [https://github.com/appc/spec/blob/master/spec/aci.md](https://github.com/appc/spec/blob/master/spec/aci.md):

>environment (list of objects, optional) represents the app's environment variables (ACE can append). The listed objects must have two key-value pairs: name and value. The name must consist solely of letters, digits, and underscores '_' as outlined in IEEE Std 1003.1-2008, 2016 Edition, with practical considerations dictating that the name may also include periods '.' and hyphens '-'. The value is an arbitrary string. These values are not evaluated in any way, and no substitutions are made.

Dotted environment variables are frequently used as a part of the Spring Boot pattern. (re: ZD-6116)

This PR specifically doesn't address the conversion of hyphens (`-`) due to an issue with rkt [[Nomad GH # 2358]](#2358).
@schmichael schmichael merged commit f13fffb into hashicorp:master Jan 22, 2018
@angrycub angrycub deleted the f-allow-dot-envvar branch February 20, 2018 15:53
schmichael added a commit that referenced this pull request Apr 3, 2018
Mention the changes from #3760 in the upgrade docs as applications
expecting underscores will break.
schmichael added a commit that referenced this pull request Apr 9, 2018
Mention the changes from #3760 in the upgrade docs as applications
expecting underscores will break.
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2023
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.

None yet

2 participants