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

Environment variables for config options #217

Closed
ibnesayeed opened this issue Feb 15, 2015 · 23 comments
Closed

Environment variables for config options #217

ibnesayeed opened this issue Feb 15, 2015 · 23 comments

Comments

@ibnesayeed
Copy link
Contributor

Is there a way to set certain config option using environment variables? There are cases when it can be handy to pass some environment variables instead of making changes to the config files such as:

  • In testing or development environments we want to run the instance with different configs frequently.
  • In production environments if we want to run multiple instances on different hosts/ports and load balance then we will have to duplicate config files with very little differences.
  • When the application is deployed for the first time using .war file, there is no way to overwrite the defaults unless the config files are modified and the and the application is reloaded by restarting Tomcat.

Here is an example of some configs from the wayback.xml file:

wayback.basedir=/tmp/openwayback
wayback.url.scheme=http
wayback.url.host=localhost
wayback.url.port=8080

I understand that XML files cannot load environment variables, but when these config options are loaded in Java files, at that time respective environment variables can take precedence if present otherwise fallback to the values set in the config files. Here is a pseudo example to illustrate the usage:

wayback.url.port = env["wayback-url-port"] || conf["wayback.url.port"] || 8080

I was deploying Wayback suing Docker Container and I wished this functionality was there.

@kris-sigur
Copy link
Member

This is not directly supported. However, if you create your own WAR overlay (highly recommended) you can use this trick to read configuration from a properties file outside the webapp.

@ibnesayeed
Copy link
Contributor Author

Thanks for the link @kris-sigur, that was a good read. But I am trying to run Wayback in containers (and I have done successful attempts) and to make it easier, I was looking for a way to pass some environment variables at the time of spinning containers instead of having different config files for every possible combination of cases I might want to run it. The trick described in that WAR overlay trick just allows me to have overrides in yet another file.

I was looking for ways to do so and I found about Spring Expression Language that is supported in Spring 3 (and from the schema used in wayback.xml file, it looks like it is using Spring 3). Don't you think it should be possible to it in the XML file itself? I usually don't work on Java projects so I don't have an idea, how will it all work, but I do feel that it is possible.

@ibnesayeed
Copy link
Contributor Author

Do you think something like this will work?

<bean class="org.springframework.beans.factory.config.PropertyPlaceholderConfigurer">
  <property name="properties">
    <value>
        wayback.basedir=#{systemEnvironment['BASE_DIR']?:'/tmp/openwayback'}
        ...
      </value>
  </property>
</bean>

The intention is to capture the value of environment variable BASE_DIR and if it does not exist then fallback to the default /tmp/openwayback path.

@ibnesayeed
Copy link
Contributor Author

Above approach seems to be working in my tests. :)

@kris-sigur
Copy link
Member

I'm happy to have set you on the right path :)
I'm a little worried, though, that this makes editing the wayback.xml a bit more confusing.

@ibnesayeed
Copy link
Contributor Author

@kris-sigur, I understand that it will cause confusion when editing wayback.xml. To solve that problem, how about the following approach?

<bean class="org.springframework.beans.factory.config.PropertyPlaceholderConfigurer">
  <property name="properties">
    <value>
      <!-- Customize these basic placeholders. -->
      wayback.basedir=/tmp/openwayback
      wayback.url.scheme=http
      wayback.url.host=localhost
      wayback.url.port=8080

      <!-- Environment variable overrides (only if present). No need to customize these. -->
      wayback.basedir=#{systemEnvironment['WAYBACK_BASEDIR']?:${wayback.basedir}}
      wayback.url.scheme=#{systemEnvironment['WAYBACK_URL_SCHEME']?:${wayback.url.scheme}}
      wayback.url.host=#{systemEnvironment['WAYBACK_URL_HOST']?:${wayback.url.host}}
      wayback.url.port=#{systemEnvironment['WAYBACK_URL_PORT']?:${wayback.url.port}}

      <!-- Derived placeholders. -->
      wayback.archivedir.1=${wayback.basedir}/files1/
      wayback.archivedir.2=${wayback.basedir}/files2/
      wayback.url.prefix=${wayback.url.scheme}://${wayback.url.host}:${wayback.url.port}
    </value>
  </property>
</bean>

@machawk1
Copy link
Contributor

This approach of setting OpenWayback paths via environment variables (Pull request #220) is extremely relevant to my interests in regard to https://github.com/machawk1/wail , which relies on these paths for all of the bundled tools to communicate and have a common data source.

@kris-sigur
Copy link
Member

@ibnesayeed I like the revised approach.

@ibnesayeed
Copy link
Contributor Author

@kris-sigur, the sample code in the later approach has some syntax problems. The Spring expression interpreter is possibly not happy with the $ sign there or nested { } are an issue. If one can point out the proper way of doing it, I will make the changes in the relevant PR.

@machawk1
Copy link
Contributor

@ibnesayeed, I am unfamiliar with the Spring config syntax but shouldn't the ternary operator be

result = #{condition?${valueIfTrue}:${valueIfFalse}}

If this form applies to Spring, no value is set if the condition (e.g., systemEnvironment['WAYBACK_URL_SCHEME']) is true.

@ibnesayeed
Copy link
Contributor Author

@machawk1 this is called Elvis Operator syntax used in Groovy (and supported in Spring 3 Expressions) and it is a shorthand that can be used if valueIfTrue is same as condition. To illustrate it consider the following statement:

displayName = fullName ?: userName

It is equivalent to:

displayName = fullName ? fullName : userName

In this example displayName variable will be set to fullName if it is present, otherwise if it is blank or has a falsy value, userName will be used instead. Ruby alternative to this syntax will be something like this:

display_name = full_name || user_name

This is good way for setting fallback or default value.

@ibnesayeed
Copy link
Contributor Author

I was able to fix the syntax issue. Property ${wayback.basedir} in the Elvis expression was supposed to be in literal form in order for it to be parsed properly by the Spring Expressions. After fixing that, the code will look like this:

<bean class="org.springframework.beans.factory.config.PropertyPlaceholderConfigurer">
  <property name="properties">
    <value>
      <!-- Customize these basic placeholders. -->
      wayback.basedir=/tmp/openwayback
      wayback.url.scheme=http
      wayback.url.host=localhost
      wayback.url.port=8080

      <!-- Environment variable overrides (only if present). No need to customize these. -->
      wayback.basedir=#{ systemEnvironment['WAYBACK_BASEDIR'] ?: '${wayback.basedir}' }
      wayback.url.scheme=#{ systemEnvironment['WAYBACK_URL_SCHEME'] ?: '${wayback.url.scheme}' }
      wayback.url.host=#{ systemEnvironment['WAYBACK_URL_HOST'] ?: '${wayback.url.host}' }
      wayback.url.port=#{ systemEnvironment['WAYBACK_URL_PORT'] ?: '${wayback.url.port}' }

      <!-- Derived placeholders. -->
      wayback.archivedir.1=${wayback.basedir}/files1/
      wayback.archivedir.2=${wayback.basedir}/files2/
      wayback.url.prefix=${wayback.url.scheme}://${wayback.url.host}:${wayback.url.port}
    </value>
  </property>
</bean>

But this configuration throws an error:

SEVERE: Exception starting filter RequestFilter
org.springframework.beans.factory.BeanDefinitionStoreException: Invalid bean definition with name 'resourcefilelocationdb' defined in URL [file:/usr/local/tomcat/webapps/ROOT/WEB-INF/wayback.xml]: Circular placeholder reference 'wayback.basedir' in property definitions
    at org.springframework.beans.factory.config.PropertyPlaceholderConfigurer.processProperties(PropertyPlaceholderConfigurer.java:287)
    at org.springframework.beans.factory.config.PropertyResourceConfigurer.postProcessBeanFactory(PropertyResourceConfigurer.java:75)
    at org.springframework.context.support.AbstractApplicationContext.invokeBeanFactoryPostProcessors(AbstractApplicationContext.java:663)
    at org.springframework.context.support.AbstractApplicationContext.invokeBeanFactoryPostProcessors(AbstractApplicationContext.java:638)
    at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:407)
    at org.springframework.context.support.FileSystemXmlApplicationContext.<init>(FileSystemXmlApplicationContext.java:140)
    at org.springframework.context.support.FileSystemXmlApplicationContext.<init>(FileSystemXmlApplicationContext.java:84)
    at org.archive.wayback.util.webapp.SpringReader.readSpringConfig(SpringReader.java:59)
    at org.archive.wayback.util.webapp.RequestFilter.loadRequestMapper(RequestFilter.java:175)
    at org.archive.wayback.util.webapp.RequestFilter.init(RequestFilter.java:105)
    at org.apache.catalina.core.ApplicationFilterConfig.initFilter(ApplicationFilterConfig.java:279)
    at org.apache.catalina.core.ApplicationFilterConfig.getFilter(ApplicationFilterConfig.java:260)
    at org.apache.catalina.core.ApplicationFilterConfig.<init>(ApplicationFilterConfig.java:105)
    at org.apache.catalina.core.StandardContext.filterStart(StandardContext.java:4854)
    at org.apache.catalina.core.StandardContext.startInternal(StandardContext.java:5542)
    at org.apache.catalina.util.LifecycleBase.start(LifecycleBase.java:150)
    at org.apache.catalina.core.ContainerBase.addChildInternal(ContainerBase.java:901)
    at org.apache.catalina.core.ContainerBase.addChild(ContainerBase.java:877)
    at org.apache.catalina.core.StandardHost.addChild(StandardHost.java:649)
    at org.apache.catalina.startup.HostConfig.deployDirectory(HostConfig.java:1245)
    at org.apache.catalina.startup.HostConfig$DeployDirectory.run(HostConfig.java:1895)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)

It turns out that resourcefilelocationdb is the ID of the next bean configuration where ${wayback.basedir} property is used. The conclusion I could draw from this is that Spring is not allowing us to redefine the same properties such as wayback.basedir and wayback.url.host etc. that are first setup to have a default value and then redefined to be overridden by environment variables. Please let me know if my assumption is wrong and if there is a way to fix this. So the next possibility I tried was to have different temporary property names for basic customization and existing properties in the override section and everywhere else as follows:

<bean class="org.springframework.beans.factory.config.PropertyPlaceholderConfigurer">
  <property name="properties">
    <value>
      <!-- Customize these basic placeholders. -->
      config.wayback.basedir=/tmp/openwayback
      config.wayback.url.scheme=http
      config.wayback.url.host=localhost
      config.wayback.url.port=8080

      <!-- Environment variable overrides (only if present). No need to customize these. -->
      wayback.basedir=#{ systemEnvironment['WAYBACK_BASEDIR'] ?: '${config.wayback.basedir}' }
      wayback.url.scheme=#{ systemEnvironment['WAYBACK_URL_SCHEME'] ?: '${config.wayback.url.scheme}' }
      wayback.url.host=#{ systemEnvironment['WAYBACK_URL_HOST'] ?: '${config.wayback.url.host}' }
      wayback.url.port=#{ systemEnvironment['WAYBACK_URL_PORT'] ?: '${config.wayback.url.port}' }

      <!-- Derived placeholders. -->
      wayback.archivedir.1=${wayback.basedir}/files1/
      wayback.archivedir.2=${wayback.basedir}/files2/
      wayback.url.prefix=${wayback.url.scheme}://${wayback.url.host}:${wayback.url.port}
    </value>
  </property>
</bean>

Here I am changing wayback.basedir to config.wayback.basedir in the basic config section and it seems working. Then I thought if we are encouraging people to only modify the basic configs (unless they know what they are doing) then why not use all uppercase (constant style) names for basic config. If we do that, the config will look like this:

<bean class="org.springframework.beans.factory.config.PropertyPlaceholderConfigurer">
  <property name="properties">
    <value>
      <!-- Customize these basic placeholders. -->
      WAYBACK_BASEDIR=/tmp/openwayback
      WAYBACK_URL_SCHEME=http
      WAYBACK_URL_HOST=localhost
      WAYBACK_URL_PORT=8080

      <!-- Environment variable overrides (only if present). No need to customize these. -->
      wayback.basedir=#{ systemEnvironment['WAYBACK_BASEDIR'] ?: '${WAYBACK_BASEDIR}' }
      wayback.url.scheme=#{ systemEnvironment['WAYBACK_URL_SCHEME'] ?: '${WAYBACK_URL_SCHEME}' }
      wayback.url.host=#{ systemEnvironment['WAYBACK_URL_HOST'] ?: '${WAYBACK_URL_HOST}' }
      wayback.url.port=#{ systemEnvironment['WAYBACK_URL_PORT'] ?: '${WAYBACK_URL_PORT}' }

      <!-- Derived placeholders. -->
      wayback.archivedir.1=${wayback.basedir}/files1/
      wayback.archivedir.2=${wayback.basedir}/files2/
      wayback.url.prefix=${wayback.url.scheme}://${wayback.url.host}:${wayback.url.port}
    </value>
  </property>
</bean>

I personally like this last approach because it now has very clear "call for action" at the same time it uses the same names for config in the wayback.xml file as the corresponding environment variable names. Before I make changes in the related PR, I would like to know what others have to say on this? Also, we will have to make changes in the documentation to reflect these changes.

@anjackson
Copy link
Member

I'd probably prefer wayback.basedir.default or wayback.default.basedir but the upper case convention is okay too. How do others feel?

@machawk1
Copy link
Contributor

👍 I, too, prefer wayback.basedir.default over the all-caps variable name.

@ibnesayeed
Copy link
Contributor Author

I have pushed some changes 2c6ea46 to the related PR #220 to incorporate wayback.basedir.default style convention. I have also added a way to override wayback.url.prefix and added clear comments where needed. Unless others have something to say about the new placeholder naming, the PR should be considered completed.

@kris-sigur
Copy link
Member

@ibnesayeed One issue remains with the PR, please include an update to the release notes as specified here: https://github.com/iipc/openwayback/wiki/How-to-contribute#code

Other than that, I'm inclined to accept the PR. We do need to remember to update the how to configure wiki page to reflect this change.

@ibnesayeed
Copy link
Contributor Author

@kris-sigur, I have made the changes to the release notes file. I realized that Git is not recognizing release_notes.xml file as text. Possibly it has some encoding issue (such as UTF-16 characters) that's why Git considers it to be a binary file.

@kris-sigur
Copy link
Member

Fixed in PR #240

@ibnesayeed
Copy link
Contributor Author

@kris-sigur PR #220 is more complete than PR #240 and the earlier should be merged because it also takes care of customizing prefix independent of the host and port customization which is needed for load balancing scenario. Also, the #220 has naming convention more aligned with rest of the stuff as agreed by @anjackson and @machawk1 above.

@kris-sigur
Copy link
Member

@ibnesayeed Agreed this needs another look, will do so tomorrow

@kris-sigur
Copy link
Member

@ibnesayeed I've merged your changes by way of PR #249

@kris-sigur kris-sigur mentioned this issue May 8, 2015
@ibnesayeed
Copy link
Contributor Author

Looks good to me @kris-sigur

@ibnesayeed
Copy link
Contributor Author

We need to remember to change "How to configure" wiki page to reflect these changes near the release. I am not changing it yet, because it is not valid for the current version.

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

No branches or pull requests

4 participants