search

IBMStockTrader / trader

UI microservice for the Stock Trader app
Apache License 2.0
24 stars 63 forks source link

Keycloak #11

Closed maxveit closed 3 years ago

maxveit commented 3 years ago
jwalcorn commented 3 years ago

Help me understand the following points a bit more here:

  1. We moved away from a multi-stage Dockerfile, which now would suggest someone has to know to do a mvn package before doing the docker build. I suppose an OpenShift Pipeline would know this, but we at least need to document (probably both in the Dockerfile and the repo's README.md) this change in what people have gotten used to when building this sample locally.
  2. We appear to have two ssl stanzas in the server.xml now. They are almost identical, except the new one has no truststore. Is the id of the stanza some magic string that means something special? This is confusing at best....
  3. It seems we're exposing the http port now rather than the https port. There's an entry in the web.xml for Trader that makes it only accept https; if you come in over http, it sends a 301 redirecting you to the https port (which could be problematic since Liberty might know that by a different value than what Kube maps it to).
  4. Does going from passthrough to edge make the whole "use the Let's Encrypt certificate" thing happen?
  5. We appear to have removed the HPA. I guess that's OK for a sample yaml, but I'll leave it in the yaml in the main umbrella helm chart and the operator that wraps it.
  6. For some reason the JWT_ISSUER env var has been removed. Its value doesn't really matter, but I think it needs to be set to something.
  7. The triggers got removed from the DeploymentConfig. I guess this is related to moving from BuildConfig/ImageStream stuff to this Tekton stuff. But how does the DeploymentConfig know now when the image has been updated? If it doesn't, is there any value in using the proprietary DeploymentConfig versus a standard Deployment?
maxveit commented 3 years ago
  1. Correct, I added it to the docker file - the readme is quite outdated as it still talks about the ICP deployment
  2. I can only see one ssl stanza - could you point out the two lines?
  3. I haven't touched this, we always exposed both at the container level (at least for the OCP deployment)
  4. correct
  5. ok
  6. We don't need it if we use Keycloak (or another proper OIDC provider), since it's discovered from the OIDC Discovery URL (.well_known)
  7. Good point, we can move this to Deployment
maxveit commented 3 years ago

Ok, there was something quirky going on with the server.xml, found the second ssl line now and removed it.