Document origin configuration and improve user guide. #106

dvlato · 2018-03-08T18:38:15Z

Changes to the user guide:

-Included a section regarding the origin configuration.
-Status codes extracted to their own page.
-Rest of sections reviewed.

…es in the distribution.

mikkokar

Some feedback from the sick bed 🤒. Overall good work but asking to clarify few details, etc. Apologies for the brief comments. I am using a mobile interface.

mikkokar · 2018-03-09T14:00:10Z

docs/user-guide/basic-usage.md

-The build system requires Apache Maven. The Styx team uses Maven version 3.2.1
-for automated continuous integration builds. On Mac OSX, a version installed 
-by HomeBrew is satisfactory.
+Running Styx requires Java 1.8. It can be run with 1.8.0_45 or later versions. Earlier maintenance


Why is the remark about maven being removed?

The rest of the instructions were considering that we were downloading an already compiled binary, but here we were explaining how to build the app instead of how to run it.

Probably we can also add instructions to build from source code.

Ok. Makes sense.

mikkokar · 2018-03-09T14:10:36Z

docs/user-guide/basic-usage.md

@@ -90,7 +86,7 @@ configuration.
 By default, the logback file is loaded from *$STYX_HOME/conf/logback.xml*. You can specify an alternative
 logging configuration file using {-l} or {--logback} command line options:


There are some old formatting isssues with the page, such as -l and —logback above. Please address them too now as you are changing this file. Need to have a consistent formatting for this and the other pages.

mikkokar · 2018-03-09T14:14:50Z

docs/user-guide/configure-load-balancing.md

@@ -9,25 +9,26 @@ Styx supports three load balancing strategies:
 Styx also provides a mechanism to bypass the load balancer and force
 the origin at source.

+##Load balancing strategies


This heading is not correctly rendered.

Oh thanks! It's rendering fine inside IntelliJ, let me see if I can fix that...

Hi. This is still not rendering correctly. Please address this.

mikkokar · 2018-03-09T14:30:22Z

docs/user-guide/configure-origins.md

+    originsFile=${STYX_HOME}/conf/env-development/origins.yaml
+    originsFile=classpath:/conf/origins.yaml
+ ```
+


This file is a great overall addition to our documentation. Thanks for putting it together.

However the above description about originsFile is slightly incomplete.

The origins file is actually specified in the services/ section, under backendServiceRegistry, see: https://github.com/dvlato/styx/blob/5e18c02bdcf7e39e462f5c1232fe851fc3c1ed67/distribution/conf/default.yml#L59.

Note that this behaviour also depends on the advanced routing settings.

The reason the above works is because of Styx configuration overrides.

mikkokar · 2018-03-09T14:33:27Z

docs/user-guide/configure-overview.md


+Styx supports a YAML file-based configuration. The YAML file can be specified using the system property `YAML_CONFIG_LOCATION=your/file/path`.
+By default, the file is read from the classpath at `classpath:conf/environment/styx-config.yaml`.
+Additionally, all these properties can be overwritten using environment variables with the same name as the property.


This is badly outdated information. The preferred (and only?) way is to pass the configuration file as a command line argument to the startup script. Please update accordingly.

Actually, specifying the configuration file with "CONFIG_FILE" (as the quickstart explains) also work. I hadn't noticed it wasn't the same property.

Should I remove the reference to "CONFIG_FILE" from both documents?

As far as I can tell YAML_CONFIG_LOCATION is not used by Styx at all, so it should not appear in the documentation.

mikkokar · 2018-03-09T14:38:23Z

docs/user-guide/configure-tls.md

-TLS can be enabled independently for each network interface. You can
-enable it independently for proxy server port, and for each of the
-back-end services. Styx will convert between the protocols where
+TLS can be enabled independently for each network resource. You can


The usage of term “network resource” is rether confusing. What we are trying to say is that the Styx server port and each app can have their TLS settings configured separately.

Ok.
The document used to say "for each network interface" which, to me, would mean that you can bind to different network cards / ip addresses. I don't see any part of the code that allows for this. If it's some feature we have lost or I've missed it, please tell me so.

You are correct. The original use of "network interface" was also a poor choice of words. But it reads much better now for what I can see 👍

mikkokar · 2018-03-09T14:43:39Z

docs/user-guide/configure-tls.md

@@ -108,50 +107,54 @@ be specified as separate backends.

 ### Backend Application Attributes:

-  - trustAllCerts - Whether (true) or not (false) to authenticate server side.
+
+  - trustAllCerts - Turns off (true) or on (true) client-side TLS authentication. When `trustAllCerts` is false,


“Client-side TLS authentication” is potentially confusing. It can lead readers to think about TLS client authentication which is a completely different matter. Please paraphrase and remove “client side” etc from the description.

mikkokar · 2018-03-09T14:47:22Z

docs/user-guide/configure-tls.md

-  - *trustStorePath* - Styx can load the relevant secure material from Java truststore.
-    This attribute specifies a path to the keystore file containing relevant trust material.
+  - *trustStorePath* - Styx can load the list of trusted certificates from a Java JKS keystore.
+    This attribute specifies a path to the truststore file containing the trusted certificates.


By convention this file where trust material is stored is called truststore which technically is just a java keystore file. In this regard the description feels bit more confusing than it was before.

mikkokar · 2018-03-12T16:51:56Z

docs/user-guide/configure-origins.md

+ - A path in the filesystem by default.
+ - A resource in the classpath by using the prefix *classpath:*.
+
+###Examples:


This heading doesn't render correctly. I believe you need to add a whitespace character between the ### marks and the heading text.

mikkokar · 2018-03-12T16:57:31Z

docs/user-guide/configure-tls.md

-TLS can be enabled independently for each network interface. You can
-enable it independently for proxy server port, and for each of the
-back-end services. Styx will convert between the protocols where
+TLS can be enabled independently for each network resource. You can


You are correct. The original use of "network interface" was also a poor choice of words. But it reads much better now for what I can see 👍

mikkokar

Good work so far David. Few additional changes still requested.

mikkokar · 2018-03-12T17:00:00Z

docs/user-guide/configure-tls.md

@@ -108,50 +107,55 @@ be specified as separate backends.

 ### Backend Application Attributes:

-  - trustAllCerts - Whether (true) or not (false) to authenticate server side.
+
+  - *trustAllCerts* -  When `trustAllCerts` is false,origin servers need to provide a 


A missing whitespace between comma and "origin".

mikkokar · 2018-03-12T17:02:53Z

docs/user-guide/configure-tls.md

-  - *trustStorePath* - Styx can load the relevant secure material from Java truststore.
-    This attribute specifies a path to the keystore file containing relevant trust material.
+  - *trustStorePath* - Styx can load the list of trusted certificates from a Java JKS truststore.
+    This attribute specifies a path to the truststore file containing the trusted certificates.


I would remove the "file containing the trusted certificates." part. That is, just leave: "Styx can load the list of trusted certificates from a Java truststore. This attribute specifies a path to the truststore."

mikkokar · 2018-03-13T08:12:01Z

docs/user-guide/configure-overview.md

@@ -1,14 +1,17 @@
-## Configuring Styx environment
+## Configuring Styx



Hi. I believe this file has incorrect information about load balancer configurations. In particular:

loadBalancing: # Load balancer strategy type. Allowed values: "LEAST\_RESPONSE\_TIME", "ROUND\_ROBIN", and "ADAPTIVE". Defaults to "ROUND_ROBIN". strategy: "ADAPTIVE"

I believe you'd have to supply the full class name using the factory. See the load balancer config page for an example.

mikkokar · 2018-03-13T08:13:11Z

docs/user-guide/configure-connection-pooling.md

@@ -21,38 +21,42 @@ An example configuration block looks like this:
      connectionExpirationSeconds: 1000 # default value 0


-Connection pool size is given by *maxConnectionsPerHost* setting. 
-The pool initially has no established TCP connections. 
+##General settings.


Rendering issue above.

mikkokar · 2018-03-13T08:13:22Z

docs/user-guide/configure-connection-pooling.md

 This is useful when an origin host is specified as a DNS domain name, and you want to ensure that domain names are re-resolved periodically.
 If the value of the setting is non-positive, connections will not expire. 
 Connection age is checked on each incoming request, so connections may live longer than their expiration time if they do not serve any requests.

+##Connection pending settings.


Rendering issue.

a-dlatorre added 7 commits March 5, 2018 16:56

ExpediaGroup#100 Backend configuration was missing from the user guide.

cb109de

ExpediaGroup#100 Changed references in the quickstart to existing fil…

356087b

…es in the distribution.

Standard codes moved out to their own page. Additional pages revised.

623e62a

ExpediaGroup#100 Rest of documents reviewed.

2c71302

ExpediaGroup#100 Merged changes for the server error codes.

413d973

ExpediaGroup#100 Merged changes for the server error codes.

fa4d62a

ExpediaGroup#100 Small typos.

5e18c02

mikkokar suggested changes Mar 9, 2018

View reviewed changes

a-dlatorre added 4 commits March 9, 2018 16:15

Standard codes moved out to their own page. Additional pages revised.

2d6d908

ExpediaGroup#100 Documentation improvements.

a350d45

ExpediaGroup#100 Documentation improvements.

cfcedc4

ExpediaGroup#100 Documentation improvements.

f475532

mikkokar reviewed Mar 12, 2018

View reviewed changes

mikkokar suggested changes Mar 12, 2018

View reviewed changes

a-dlatorre added 2 commits March 12, 2018 18:13

ExpediaGroup#100 Minor fixes to the documentation.

ca86cc7

ExpediaGroup#100 Minor fixes to the documentation.

e912e97

mikkokar suggested changes Mar 13, 2018

View reviewed changes

ExpediaGroup#100 Minor fixes to the documentation.

681cb38

mikkokar approved these changes Mar 13, 2018

View reviewed changes

mikkokar merged commit 2613993 into ExpediaGroup:master Mar 13, 2018

mikkokar mentioned this pull request Mar 13, 2018

Styx user guide: Overarching backends/origins configuration document is missing #100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document origin configuration and improve user guide. #106

Document origin configuration and improve user guide. #106

dvlato commented Mar 8, 2018

mikkokar left a comment

mikkokar Mar 9, 2018

dvlato Mar 9, 2018

mikkokar Mar 12, 2018

mikkokar Mar 9, 2018

mikkokar Mar 9, 2018

dvlato Mar 9, 2018

mikkokar Mar 12, 2018

mikkokar Mar 9, 2018

mikkokar Mar 9, 2018

dvlato Mar 9, 2018

kvosper Mar 9, 2018

mikkokar Mar 9, 2018

dvlato Mar 9, 2018

mikkokar Mar 12, 2018

mikkokar Mar 9, 2018

mikkokar Mar 9, 2018

mikkokar Mar 12, 2018

mikkokar Mar 12, 2018

mikkokar left a comment

mikkokar Mar 12, 2018

mikkokar Mar 12, 2018

mikkokar Mar 13, 2018

mikkokar Mar 13, 2018

mikkokar Mar 13, 2018

		@@ -90,7 +86,7 @@ configuration.
		By default, the logback file is loaded from $STYX_HOME/conf/logback.xml. You can specify an alternative
		logging configuration file using {-l} or {--logback} command line options:

		@@ -1,14 +1,17 @@
		## Configuring Styx environment
		## Configuring Styx

Document origin configuration and improve user guide. #106

Document origin configuration and improve user guide. #106

Conversation

dvlato commented Mar 8, 2018

mikkokar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikkokar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment