OpenLiberty / open-liberty

Open Liberty is a highly composable, fast to start, dynamic application server runtime environment
https://openliberty.io
Eclipse Public License 2.0
1.14k stars 587 forks source link

Configuration through command line arguments #1610

Closed hrstoyanov closed 5 years ago

hrstoyanov commented 6 years ago

It seems Open Liberty needs to provide yet another way for properties configuration - through the command line:

wlp/bin/server.bat -Dmy.conig=c:/temp/myconfig.properties

This can be very flexible in various Docker scenario. The details are discussed in this stackoverflow thread.

aguibert commented 6 years ago

hi @hrstoyanov, seems like a reasonable request. Currently if a user tries to do this, an error is thrown:

$ bin/server run player-service -Dfoo=bar
CWWKE0013E: Unknown option: -Dfoo=bar

@NottyCode can you think of any reason why we would want to disallow this? Seems like a simple enough change to make. (i.e. allow -D options to be passed into the server script)

aguibert commented 6 years ago

@hrstoyanov to clarify... are you suggesting that this behave exactly as -D arguments for the java command? (i.e. we set a system property with key=my.config and value=c:/temp/myconfig.properties)

Or, are you suggesting that liberty reads the c:/temp/myconfig.properties property file line by line and sets each property?

covener commented 6 years ago

suggestion: terminate our args with -- and remainder goes to java?

hrstoyanov commented 6 years ago

Andrew, I guess I was thinking about the first option, just as another way to implement Microprofile Config.

On Fri, Jan 26, 2018 at 4:00 PM, Andrew Guibert notifications@github.com wrote:

@hrstoyanov https://github.com/hrstoyanov to clarify... are you suggesting that this behave exactly as -D arguments for the java command? (i.e. we set a system property with key=my.config and value=c:/temp/myconfig.properties)

Or, are you suggesting that liberty reads the c:/temp/myconfig.properties property file line by line and sets each property?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OpenLiberty/open-liberty/issues/1610#issuecomment-360938221, or mute the thread https://github.com/notifications/unsubscribe-auth/AFFIxOhYoIzIdrQcZ4e8-rWiOiG625iwks5tOmcMgaJpZM4RezLZ .

-- / Hristo Stoyanov

aguibert commented 6 years ago

@covener We don't send in any -D options to the server script currently, so we could simply pass any -D options along to the JVM that starts Liberty. To clarify, only talking about passing in system props using -D, and not other JVM args such as -javaagent or -cp.

I believe this patch would solve the problem: https://github.com/aguibert/open-liberty/commit/30cb9f1406e86771cb7ecb5f82d728b02b6697fa

NottyCode commented 6 years ago

@hrstoyanov MicroProfile Config already reads from the system environment, so in docker environments anything set on docker run --env would be passed through. I would have thought this would be a better option than -D options which are going to be harder to set from outside the docker container.

hrstoyanov commented 6 years ago

Passing passwords through env variables is unsafe - certain PS commands can reveal them. My idea was to be able to attache docker volumes with all OL configuration to an immutable jars-only Docker image and be very flexible about it (e.g be able to try it in dev , ad-hoc production, etc without much ceremony)

hrstoyanov commented 6 years ago

Btw, while I do think providing flexible MicroProfile Configuration is a good thing, this issue is no longer a priority for me - CDI does not provide server-wide scope, so I packaged my code as JCA resource adapter and the configuration there follows different path:

https://github.com/peruncs/orientdb-jca

NottyCode commented 6 years ago

@hrstoyanov if you've moved on does that mean you don't need this anymore?

I'm kind of confused by the password comments because even setting properties on the command line as suggested here would not be immune to ps commands showing it. Also given docker tends to single process I think ps is less of a risk than in a shared O/S. Although perhaps the docker host ps can see them.

hrstoyanov commented 6 years ago

Correct - No longer an issue for me

Emily-Jiang commented 6 years ago

I was contacted by a developer trying to do a demo on Open Liberty. They would like to set up a config property when starting the server with server run -Daa=bb

In the app, aa was defined in the microprofile-config.properties with the above line, it is better than creating a file bootstrapping.property and put one line there. I think it is great to support the -D option on server start.

Thoughts?

hrstoyanov commented 6 years ago

@Emily-Jiang Although this is no longer critical for me, I think specifying MP Config parameters on the command line is very flexible in certain cases (docker runs, demos, experiments, etc). The other options: server.xml, setting ENV variables involve more ceremony and hassle.

The other reason I did not push more for this is because I see from the OL commits is that OL team is super busy these days with Java EE8 and the latest MP specs, and I did not want to distract you too much.

Emily-Jiang commented 6 years ago

Thank you @hrstoyanov for your comments! Very much appreciated!

aguibert commented 6 years ago

@NottyCode do you have any further reservations on this enhancement request? Emily, Hristo, and I think it would be a nice convenience so I've opened up PR #4197 with the non-Windows and FAT test changes.

Emily-Jiang commented 6 years ago

Thank you @aguibert!

aguibert commented 5 years ago

To summarize a discussion we had via Webex last week, we want to look at supporting two basic paths:

  1. Passing in JVM options (e.g. -Dfoo=bar or -Xmx2g)
  2. Passing in Java main method arguments (for Spring application support)

A server start might look like this: server start myServer <JVM_OPTIONS> -- <MAIN_ARGS>

And running an executable jar might look like this: java <JVM_OPTIONS> -jar myServer.jar <MAIN_ARGS>

marikaj123 commented 5 years ago

@aguibert Is this Issue an Epic or a story as it also exists in AHA?

marikaj123 commented 5 years ago

According to Mike, this is a story and not an Epic.

aguibert commented 5 years ago

FYI the JVM options work has been delivered in PR #4197

aguibert commented 5 years ago

This function has now been delivered, and JVM options can be applied at server start time like this:

$ bin/server start myServer -Dfoo=bar