Use the server config instead of env variables for the API port and the server hostname (#488)

* Get public hostname from config file

* Tidy up

* Respond to Review Comments

At some point down the line I royally screwed up my git history and
reverted a bunch of changes.  I'm not sure what or when, so I `git reset --hard 1cd28e4` and tried to redo my
past responses to comments from there.

* Export SB_PUBLIC_IP in cmd.sh

* respond to review comments

* Review response

src/server_manager/install_scripts/install_server.sh

@@ -251,19 +251,17 @@ function write_config() {
   if [[ $FLAGS_KEYS_PORT != 0 ]]; then
     config+=("\"portForNewAccessKeys\":$FLAGS_KEYS_PORT")
   fi
-  if [[ ${#config[@]} > 0 ]]; then
-    echo "{"$(join , "${config[@]}")"}" > $STATE_DIR/shadowbox_server_config.json
-  fi
+  config+=$(printf '"hostname": "%q"' ${PUBLIC_HOSTNAME})
+  echo "{"$(join , "${config[@]}")"}" > $STATE_DIR/shadowbox_server_config.json
 }
 
 function start_shadowbox() {
-  # TODO(fortuna): Write PUBLIC_HOSTNAME and API_PORT to config file,
+  # TODO(fortuna): Write API_PORT to config file,
   # rather than pass in the environment.
   declare -a docker_shadowbox_flags=(
     --name shadowbox --restart=always --net=host
     -v "${STATE_DIR}:${STATE_DIR}"
     -e "SB_STATE_DIR=${STATE_DIR}"
-    -e "SB_PUBLIC_IP=${PUBLIC_HOSTNAME}"
     -e "SB_API_PORT=${API_PORT}"
     -e "SB_API_PREFIX=${SB_API_PREFIX}"
     -e "SB_CERTIFICATE_FILE=${SB_CERTIFICATE_FILE}"
@@ -330,6 +328,7 @@ function add_api_url_to_config() {
 }
 
 function check_firewall() {
+  # TODO(cohenjon) This is incorrect if access keys are using more than one port.
   local readonly ACCESS_KEY_PORT=$(curl --insecure -s ${LOCAL_API_URL}/access-keys | 
       docker exec -i shadowbox node -e '
           const fs = require("fs");

src/shadowbox/README.md

@@ -165,7 +165,18 @@ yarn shadowbox_docker_build && docker tag quay.io/outline/shadowbox $USER/shadow
 
 If you need to test an unsigned image (e.g. your dev one):
 ```
-DOCKER_CONTENT_TRUST=0 SHADOWBOX_IMAGE=$USER/shadowbox yarn do shadowbox/integration_test/run
+DOCKER_CONTENT_TRUST=0 SB_IMAGE=$USER/shadowbox yarn do shadowbox/integration_test/run
 ```
 
 You can add tags if you need different versions in different clients.
+
+### Testing Changes to the Server Config
+
+If your change includes new fields in the server config which are needed at server
+start-up time, then you mey need to remove the pre-existing test config:
+
+```
+rm /tmp/outline/persisted-state/shadowbox_server_config.json
+```
+
+This will warn about deleting a write-protected file, which is okay to ignore.  You will then need to hand-edit the JSON string in src/shadowbox/docker/run_action.sh.

src/shadowbox/docker/run_action.sh

@@ -17,13 +17,17 @@
 do_action shadowbox/docker/build
 
 readonly OUTLINE_DIR=/tmp/outline
-mkdir -p $OUTLINE_DIR && touch "$OUTLINE_DIR/config.json"
+readonly HOST_STATE_DIR=$OUTLINE_DIR/persisted-state
+readonly CONTAINER_STATE_DIR=/root/shadowbox/persisted-state
+readonly STATE_CONFIG=$HOST_STATE_DIR/shadowbox_server_config.json
+mkdir -p $HOST_STATE_DIR && touch "$HOST_STATE_DIR/shadowbox_config.json"
+[[ -e $STATE_CONFIG ]] || echo '{"hostname":"127.0.0.1"}' > $STATE_CONFIG
 source $ROOT_DIR/src/shadowbox/scripts/make_test_certificate.sh "${OUTLINE_DIR}"
 
 # TODO: mount a folder rather than individual files.
 declare -a docker_bindings=(
-  -v "$OUTLINE_DIR/config.json":/root/shadowbox/shadowbox_config.json
-  -v "$OUTLINE_DIR/stats.json":/root/shadowbox/shadowbox_stats.json
+  -v "$HOST_STATE_DIR":${CONTAINER_STATE_DIR}
+  -e "SB_STATE_DIR=${CONTAINER_STATE_DIR}"
   -v ${SB_CERTIFICATE_FILE}:${SB_CERTIFICATE_FILE}
   -v ${SB_PRIVATE_KEY_FILE}:${SB_PRIVATE_KEY_FILE}
   -e "LOG_LEVEL=${LOG_LEVEL:-debug}"
@@ -32,4 +36,6 @@ declare -a docker_bindings=(
   -e SB_PRIVATE_KEY_FILE=${SB_PRIVATE_KEY_FILE}
 )
 
-docker run --rm -it --network=host --name shadowbox "${docker_bindings[@]}" outline/shadowbox
+echo "Running image ${SB_IMAGE}"
+
+docker run --rm -it --network=host --name shadowbox "${docker_bindings[@]}" ${SB_IMAGE}

src/shadowbox/integration_test/docker-compose.yml

@@ -16,9 +16,8 @@ services:
     stop_signal: SIGKILL
 
   shadowbox:
-    image: ${SHADOWBOX_IMAGE:-outline/shadowbox}
+    image: ${SB_IMAGE:-outline/shadowbox}
     environment:
-      - SB_PUBLIC_IP=shadowbox
       - SB_API_PORT=443
       - SB_API_PREFIX=${SB_API_PREFIX}
       - LOG_LEVEL=debug
@@ -34,6 +33,7 @@ services:
     volumes:
       - ${SB_CERTIFICATE_FILE}:/root/shadowbox/test.crt
       - ${SB_PRIVATE_KEY_FILE}:/root/shadowbox/test.key
+      - ${TMP_STATE_DIR}:/root/shadowbox/persisted-state
     # The user management service doesn't quit with SIGTERM
     stop_signal: SIGKILL
 

src/shadowbox/integration_test/test.sh

@@ -74,7 +74,10 @@ function fail() {
 
 function cleanup() {
   status=$?
-  (($DEBUG != 0)) || docker-compose --project-name=integrationtest down
+  if (($DEBUG != 0)); then
+    docker-compose --project-name=integrationtest down
+    rm -r ${TMP_STATE_DIR}
+  fi
   return $status
 }
 
@@ -92,6 +95,8 @@ function cleanup() {
   # Sets everything up
   export SB_API_PREFIX=TestApiPrefix
   SB_API_URL=https://shadowbox/${SB_API_PREFIX}
+  export TMP_STATE_DIR=$(mktemp -d)
+  echo '{"hostname": "shadowbox"}' > ${TMP_STATE_DIR}/shadowbox_server_config.json
   docker-compose --project-name=integrationtest up --build -d
 
   # Wait for target to come up.

src/shadowbox/server/main.ts

@@ -65,7 +65,6 @@ async function main() {
 
   prometheus.collectDefaultMetrics({register: prometheus.register});
 
-  const proxyHostname = process.env.SB_PUBLIC_IP;
   // Default to production metrics, as some old Docker images may not have
   // SB_METRICS_URL properly set.
   const metricsCollectorUrl = process.env.SB_METRICS_URL || 'https://metrics-prod.uproxy.org';
@@ -73,16 +72,6 @@ async function main() {
     logging.warn('process.env.SB_METRICS_URL not set, using default');
   }
 
-  if (!proxyHostname) {
-    logging.error('Need to specify SB_PUBLIC_IP for invite links');
-    process.exit(1);
-  }
-
-  logging.debug(`=== Config ===`);
-  logging.debug(`SB_PUBLIC_IP: ${proxyHostname}`);
-  logging.debug(`SB_METRICS_URL: ${metricsCollectorUrl}`);
-  logging.debug(`==============`);
-
   const DEFAULT_PORT = 8081;
   const apiPortNumber = Number(process.env.SB_API_PORT || DEFAULT_PORT);
   if (isNaN(apiPortNumber)) {
@@ -94,6 +83,17 @@ async function main() {
   const serverConfig =
       server_config.readServerConfig(getPersistentFilename('shadowbox_server_config.json'));
 
+  const proxyHostname = serverConfig.data().hostname;
+  if (!proxyHostname) {
+    logging.error('Need to specify hostname in shadowbox_server_config.json');
+    process.exit(1);
+  }
+
+  logging.debug(`=== Config ===`);
+  logging.debug(`Hostname: ${proxyHostname}`);
+  logging.debug(`SB_METRICS_URL: ${metricsCollectorUrl}`);
+  logging.debug(`==============`);
+
   logging.info('Starting...');
 
   const prometheusPort = await portProvider.reserveFirstFreePort(9090);

src/shadowbox/server/server_config.ts

@@ -35,6 +35,8 @@ export interface ServerConfigJson {
   // Sliding timeframe, in hours, used to measure data usage and enforce data limits.
   dataUsageTimeframe?: DataUsageTimeframe;
   // We don't serialize the shadowbox version, this is obtained dynamically from node.
+  // Public proxy hostname.
+  hostname?: string;
 }
 
 // Serialized format for rollouts.
@@ -54,6 +56,7 @@ export function readServerConfig(filename: string): json_config.JsonConfig<Serve
     config.data().metricsEnabled = config.data().metricsEnabled || false;
     config.data().createdTimestampMs = config.data().createdTimestampMs || Date.now();
     config.data().dataUsageTimeframe = config.data().dataUsageTimeframe || {hours: 30 * 24};
+    config.data().hostname = config.data().hostname || process.env.SB_PUBLIC_IP;
     config.write();
     return config;
   } catch (error) {