Update jupyter-api image and add Listen_address env variable

[wg102]

fixes #39

[brendangadd]

New environment variable LISTEN_ADDRESS required due to StatCan/jupyter-apis#15.

[justbert]

LGTM!

[justbert]

@brendangadd We usually only merge in after-hours to prevent downtime of the Kubeflow Control plane. That still okay?

[justbert]

@sylus do we need to change anything else for this?

[sylus]

Thinking will merge over weekend as wont have time to debug any issues before then.

[justbert]

@sylus was gonna do it last night, but @brendangadd pointed out there may be something missing:

The new environment variable is in params.env, but I'm wondering if it also needs to be referenced somewhere else, like here: https://github.com/StatCan/kubeflow-manifest/blob/master/kustomize/jupyter-web-app/base/kustomization.yaml#L27

[brendangadd]

Reason for environment variable

The jupyter-apis image has been modified so that the listen address is configurable. StatCan/jupyter-apis#15.

• Old behaviour: listen on 0.0.0.0:5000
• New behaviour: order of precedence of -listen-address CLI arg > LISTEN_ADDRESS env var > default of 127.0.0.1:5000

This PR needs to add the new LISTEN_ADDRESS env var, set to 0.0.0.0:5000, to maintain the original (desired) behaviour in prod.

/cc @justbert @sylus

[justbert]

@sylus Can you take a look at this? I'm not familiar with the components of the DAaaS platform. My knowledge of kustomize is also lacking.

Should we load LISTEN_ADDRESS from the ConfigMap that is generated from params.env like ROK_SECRET_NAME is done or template them in like they are done for userid-header and userid-prefix?

Other changes still need to be made.

[justbert]

Brought up by @brendangadd:

kubectl kustomize and kustomize build do not behave the same! kubernetes-sigs/kustomize#2205 (edited)

[brendangadd]

I built with kustomize (kubectl kustomize throws) and the new environment variable in params.env is being added to the config map correctly. I just pushed a new commit to reference the new value in the deployment, as suggested by @justbert.

/cc @sylus @wg102

[sylus]

So the P.R. looks fine and nothing looks wrong to me. As long as Zach has everything committed properly it should be fine.

This has been testing and confirmed to work?

[sylus]

So this change brings in the new jupyter-api of which the delta is quite large?

StatCan/jupyter-apis@3c844b4...692c8f4

I guess the mitigation is if things don't work we set back the image version?

[brendangadd]

@sylus

• The manifest updates have not been tested. Not sure how to test that out properly with only the one cluster.
• I verified that the new env var exists in the kustomize-generated config map as expected, so should have the same behaviour as the ROK_SECRET_NAME.
• The fallback mechanism for setting the listen address in the Go app has been tested.
• The other jupyter-apis changes have been tested.

[sylus]

@zachomedia can i merge this? Should we update the SHA do the latest?

[zachomedia]

I feel like we should make this environment variable value the default in the Dockerfile rather than setting it on the cluster. It'll will be needed to run in Docker anyways to expose the app.

Adding a ENV LISTEN_ADDRESS 0.0.0.0:5000

Thoughts @sylus @justbert ?

[sylus]

@saffaalvi can u make this change? then can merge!

[zachomedia]

LGTM!