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

Is the community open to a backward compatible PR that would have getConfig() reference env variables that could override values specified in XML or the database? #643

Closed
tylers-username opened this issue Apr 1, 2019 · 9 comments · Fixed by #3863

Comments

@tylers-username
Copy link

Sorry if this has been discussed before, I did not dig through Issue history.

I'm considering opening a PR that lets you override XML and DB config values with env variables. It should be non-breaking.

For example, you may have something like:

<default>
    <catalog>
        <some_nested_node>                
            <enabled>myname</enabled>
            <header>This is my header</header>
        </some_nested_node>
    </catalog>
</default>

This can be referenced today with:

echo Mage::getConfig()->getNode('catalog/some_nested_node')->header; 

# Returns: "This is my header"

With my proposed solution, you'd be able to implement an inveronment variable such as MAGE_CONFIG/CATALOG/SOME_NESTED_NODE/HEADER="My new header". Magento would then convert any MAGE_CONFIG/ variables to an XML node in order to maintain compatibility and load it last to ensure that it overrides any previously set values.

@tylers-username tylers-username changed the title Is the community open to a non-breaking PR that would have getConfig() reference env variables that could override values specified in XML or the database? Is the community open to a backward compatible PR that would have getConfig() reference env variables that could override values specified in XML or the database? Apr 1, 2019
@colinmollenhour
Copy link
Member

This is an interesting idea, but unfortunately environment variables cannot contain '/' so that is one issue... Underscore is the only special character allowed and that is already widely used in the config paths so I don't know of a general-purpose path separator.

Another option which does not require any core code changes is to dump your overrides into any xml file in app/etc/. I am guessing you're using Docker or Ansible or something so a simple way to do this is have a template file and then use envsubst or gotpl or some other template processor to insert the values into the xml file during deployment or entrypoint script. These values will be overridden by database values, though.

If you really want to try this I would suggest first implementing this in your own code as an extension/modification of some sort using an event observer and then if it turns out well offer a PR which implements the same directly in the core.

My main concern when adding this in would be the security implications. E.g. can environment variables be trusted (e.g. overriding the API endpoint url to cause sensitive information to be sent to the wrong address)? Can users get environment variable values that they couldn't expose before (e.g. database passwords)? Using a declarative approach of mapping environment variables to paths would help a lot to prevent abuse.

<config>
  <global>
    <environment>
      <default>
        <foo>
          <bar>
            <baz>MY_FOO_BAR_BAZ</baz>
...

Or:

<config>
  <global>
    <environment>
      <MY_FOO_BAR_BAZ>default/foo/bar/baz</MY_FOO_BAR_BAZ>
...

The above would be used to read the value from MY_FOO_BAR_BAZ environment variable into default/foo/bar/baz config path using Mage::getConfig()->setNode('...'). Since Magento has default, global, store and website scopes I think it will be better to directly specify which variables map to which paths.

@tylers-username
Copy link
Author

Ah, I'd seen tools such as dotenv honor slashes. I didn't realize that environments don't natively support this. I had only tested this concept with:

env MAGE_CONFIG\some\config\path=OKAY magerun dev:console

Mage::getStoreConfig('some\config\path'); # Returns "OKAY"

My main concern when adding this in would be the security implications.

I think there would have to be an exception thrown if putenv() was enabled. I'm not a security expert, so I don't know if or what can be done with $_ENV. I don't know about the "trustability" of environment variables in general, just that a number of frameworks (Laravel in PHP, Gatsby in NodeJS) implement tools such as dotenv to provide connection details.

My main use-case for this is for establishing connections. For example, currently on Platform.sh (the service behind Magento Cloud) I have to keep my /app/etc/ directory writable as a mount because I cannot generate my local.xml file during deployments until my service relationships are available (db, redis, etc) - they're not available until the read-only system is locked down. I'd rather not have the config directory writable.

I think the alternative route to this is implementing something like: https://packagist.org/packages/gracious/magento-env-config

@tmotyl
Copy link
Contributor

tmotyl commented Apr 1, 2019

Hi
Here is a good read on challenges of the env variables:
https://mattallan.me/posts/how-php-environment-variables-actually-work/

As there is a potential security risk I would say this feature should not be bundled in mage lts by default.

@Flyingmana
Copy link
Contributor

My general rule of thumb for this would be: "how is Magento2 handling this?"
Which brings me to, Magento2 allows configs to be written as php file, where you can use any ENV variables you would like.
Is that an acceptable way to go? Anyone has an Idea how to implement this? 😅

@LeeSaferite
Copy link
Contributor

@tylers-username
Copy link
Author

Are we in agreement on how this should be handled? Env vars adhering to the M2 standard?

@colinmollenhour
Copy link
Member

I suppose the mapping could be avoided by using the M2 naming convention. So then during bootstrap the env vars could be iterated and applied using setNode for any that match the pattern.

@fballiano
Copy link
Contributor

I'm closing this because there's no activity in 2 years. Feel free to reopen it when new ideas will arise, possibly with a PR.

@pquerner
Copy link
Contributor

@tylers-username This was just passed as completed via #3863

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants