Controller protocol should be https in controller application.conf

[chetanmeh]

Currently the controller.protocol value is being set in 2 different application.conf

1. controller/application.conf
2. test/application.conf

This can lead to ambiguous setups where the protocol value may differ for test setup depending on the order of application.conf being seen in classpath by typesafe config.

Post this change PR #3249 has started failing post rebase to latest master. On analyzing the root cause it was found that controller protocol being picked was "http" (see log and search for "protocol":"http").

As a fix either

1. the default in controller/application.conf should be set to "https"
2. or move the default config from application.conf to reference.conf in controller

[chetanmeh]

With this change the build passes. It uses option 2 i.e. move config to reference.conf

diff --git a/core/controller/src/main/resources/application.conf b/core/controller/src/main/resources/application.conf
index bbc298b1e3..446d5fed34 100644
--- a/core/controller/src/main/resources/application.conf
+++ b/core/controller/src/main/resources/application.conf
@@ -2,16 +2,6 @@
 include "logging"
 include "akka-http-version"
 
-whisk {
-  loadbalancer {
-    invoker-busy-threshold: 4
-    blackbox-fraction: 10%
-  }
-  controller {
-    protocol: http
-  }
-}
-
 # http://doc.akka.io/docs/akka-http/current/scala/http/configuration.html
 # descriptions inlined below for convenience
 akka.http {
diff --git a/core/controller/src/main/resources/reference.conf b/core/controller/src/main/resources/reference.conf
new file mode 100644
index 0000000000..ce13c1e124
--- /dev/null
+++ b/core/controller/src/main/resources/reference.conf
@@ -0,0 +1,10 @@
+
+whisk {
+  loadbalancer {
+    invoker-busy-threshold: 4
+    blackbox-fraction: 10%
+  }
+  controller {
+    protocol: http
+  }
+}

@vvraskin Does this approach looks fine? I can then create a PR for the same

[vvraskin]

So basically by moving the properties into reference.conf we assure that any application.conf will override it. Does pureconfig also work with reference.conf? if yes, I'm fine with the change.

[chetanmeh]

Yes. Pureconfig internally uses typesafe config which has a deterministic lookup order where reference.conf has the least preference.

May be we should move all config under "whisk" namespace to reference.conf. @markusthoemmes Thoughts?

[tysonnorris]

I don't think moving to reference.conf is the answer, since there is still a value explicitly set at https://github.com/apache/incubator-openwhisk/blob/master/tests/src/test/resources/application.conf.j2#L50, which should override it, assuming that file is rendered to the classpath before running the tests.

The log you mentioned that failed, on scans.gradle.com, how was it triggered exactly? Are you sure the tests application.conf.j2 was rendered as part of the build, with the config value you expect?

[tysonnorris]

If there is real ambiguity going on here (first need to verify that the generated application.conf on the classpath is as expected?), then I would suggest either:

• move everything into reference.conf (not just controller parts); one reference.conf per each of invoker+controller
• OR, stop relying on application.conf.j2 - I think this is already a suspicious practice of generating code into tests/src/test/resources/ directory

Because if we leave some bits remaining in the application.conf.j2, those will remain incorrect/inconsistent, I think, waiting for the next victim to stumble onto this.

It might be better to rely on system properties for running tests instead, via gradle.properties https://docs.gradle.org/current/userguide/build_environment.html#sec:gradle_system_properties since this is also more matching to how container deployments operate (using transformEnvironment.sh to reset CONFIG_ env vars as system properties).

[chetanmeh]

If there is real ambiguity going on here (first need to verify that the generated application.conf on the classpath is as expected?)

@tysonnorris Logically yes as the value of controller.protocol is being set to2 different value in 2 different application.conf. Its not guaranteed that application.conf present in tests would supercede any other application.conf as the order as seen by typesafe config logic would not be deterministic.

move everything into reference.conf (not just controller parts); one reference.conf per each of invoker+controller

We can safely move all properties under whisk path to reference.conf. However any other property cannot be moved as same property present in 2 reference.conf would end up in non deterministic resolution.

OR, stop relying on application.conf.j2 - I think this is already a suspicious practice of generating code into tests/src/test/resources/ directory

That may be better. This would also simplify work being done as part of #3492 by @jonpspri. However this may make running tests within IDE bit difficult as IDE test runner may not pick those properties (to be checked). Though Intellij can be configured to use Gradle Test Runner I am not sure if we loose any features with that