feat(grafana): remove minimal config option, route to Grafana using existing route and service

[andrewazores]

Based on #118
Depends on #118
Fixes #114

To test:
Install chart on OpenShift with helm install cryostat --set core.route.enabled=true ./charts/cryostat, then follow the notes that are printed to set the additional environment variables dependent upon the route host. Wait for the deployment to become ready, then visit the route. Cryostat should be running with no error notifications about Grafana or archives. Create a localhost:0 custom target, then start a flight recording on Cryostat. You should be able to archive the active recording, generate reports on the active or archived recording, and view the active recording in Grafana. Viewing archived recordings in Grafana is not yet implemented by the Cryostat 3 server but the configuration here should allow that seamlessly once we have that reimplemented.

Test along with cryostatio/cryostat#315 / cryostatio/cryostat#324 to get more of the UI features working.

We might want to keep the minimal deployment option (no jfr-datasource and no Grafana dashboard), but I'm leaning toward removing it. Originally this was put in place so that the deployment footprint was only the Cryostat container itself, but nowadays the smallest footprint is Cryostat + database + object storage + auth proxy, and the Grafana dashboard layout has been updated quite a bit to include several more powerful/useful data visualizations, so IMO it's worth making it a standard feature.

[andrewazores]

While I'm at it in this PR I will also tackle simplifying the service/ingress/route setup, port forwarding, etc., now that all of the external client traffic is (intended to be) routed through the auth proxy on a single port. This should be a nice cleanup and remove a lot of the post-install steps that we display to users.

[andrewazores]

@ebaron @tthvo do you have any examples of other networking configurations using the core.ingress.* and core.service.type parameters? So far I have just tested with core.route.enabled=[true|false].

[tthvo]

Hi, I usually do the following installations:

1. With kind and podman:

   helm install cryostat-test ./charts/cryostat/

   Then, I follow the release notes to port-forward the Cluster-IP service.

2. With kind and docker:

   helm install cryostat-test ./charts/cryostat/ \
   --set core.service.type=NodePort \
   --set grafana.service.type=NodePort

   Then, the app is accessed with the container IP.

3. Currently, I am working with AWS EKS so I usually do the following:

   helm install cryostat-test ./charts/cryostat/ \
   --set core.ingress.enabled=true \
   --set core.ingress.className=alb \
   --set core.ingress.annotations."alb\.ingress\.kubernetes\.io/scheme"=internet-facing \
   --set core.ingress.annotations."alb\.ingress\.kubernetes\.io/target-type"=ip \
   --set core.ingress.hosts[0].paths[0].pathType="Prefix" \
   --set core.ingress.hosts[0].paths[0].path="/" \
   --set grafana.ingress.enabled=true \
   --set grafana.ingress.className=alb \
   --set grafana.ingress.annotations."alb\.ingress\.kubernetes\.io/scheme"=internet-facing \
   --set grafana.ingress.annotations."alb\.ingress\.kubernetes\.io/target-type"=ip \
   --set grafana.ingress.hosts[0].paths[0].pathType="Prefix" \
   --set grafana.ingress.hosts[0].paths[0].path="/"

   Though, I have to wait for the load balancer to be ready and manually patch the deployment with ingress hostnames.

I haven't tried installing with TLS tho :((

[andrewazores]

I haven't tried installing with TLS

I haven't testing this yet either - I think it'll look different again once we have a config switch for deploying openshift-oauth-proxy, so I think it makes sense to hold off on TLS changes until that lands and then tackle it all at once.

[tthvo]

Ah make sense to me! There is also another service case, related to service configurations.

With service type LoadBalancer:

helm install cryostat-with-svc ./charts/cryostat/ \
--set core.service.type=LoadBalancer \
--set grafana.service.type=LoadBalancer

Since its relying cloud providers, likely there are additional annotations required on the Service. For example, with AWS, I have been manually patching the Services after installing the chart:

service.beta.kubernetes.io/aws-load-balancer-nlb-target-type: ip
service.beta.kubernetes.io/aws-load-balancer-scheme: internet-facing
service.beta.kubernetes.io/aws-load-balancer-type: external

Do you think it would be good to allow setting those on chart values? Currently, we can only set Service's port and type.

Maybe, not this PR scope though.

[andrewazores]

I think that's a good feature request, ability to add labels and annotations on created resources like Services. Probably for another follow-up PR.

[andrewazores]

@mwangggg @aali309 @tthvo review please - I filed a separate issue for the enhancement to allow adding additional metadata for things like external load balancers.

[mwangggg]

https://github.com/andrewazores/cryostat-helm/blob/05352d8aeb009c0e654479846b119896843628da/charts/cryostat/templates/tests/test-connection.yaml#L20-L22 .Values.minimal and .Values.grafana.service.port were removed

[mwangggg]

https://github.com/andrewazores/cryostat-helm/blob/05352d8aeb009c0e654479846b119896843628da/charts/cryostat/README.md?plain=1#L102 should oauth2Proxy.securityContext (capital P)

[andrewazores]

https://github.com/andrewazores/cryostat-helm/blob/05352d8aeb009c0e654479846b119896843628da/charts/cryostat/templates/tests/test-connection.yaml#L20-L22 .Values.minimal and .Values.grafana.service.port were removed

.Values.grafana.service.port is still used in service.yaml

[andrewazores]

I fixed up the tests, though I found that the test expects the Cryostat server's /health response to have {"cryostatVersion": "v3.0.0-dev"} to match the chart's own AppVersion, whereas the server actually responds with its own pom.xml version: {"cryostatVersion": "v3.0.0-snapshot"}. This causes the test to fail. I think there's a reason why the versioning scheme suffixes are different, so should the version comparison in the test be relaxed? Or is this only expected to pass on release versions where there is no suffix?

[ebaron]

I fixed up the tests, though I found that the test expects the Cryostat server's /health response to have {"cryostatVersion": "v3.0.0-dev"} to match the chart's own AppVersion, whereas the server actually responds with its own pom.xml version: {"cryostatVersion": "v3.0.0-snapshot"}. This causes the test to fail. I think there's a reason why the versioning scheme suffixes are different, so should the version comparison in the test be relaxed? Or is this only expected to pass on release versions where there is no suffix?

I think the test could be relaxed for pre-release builds. Maybe if the appVersion is a semver pre-release, then ignore the suffix when comparing?

[ebaron]

I think we might be missing some configuration for LoadBalancer service. Viewing in Grafana brought me to localhost:3000.

[andrewazores]

I'll take a look, thanks.

[andrewazores]

Should be fixed now in the latest commit.

[ebaron]

I think we need the same fix for the NodePort type too. Getting localhost:3000 for viewing in Grafana.

[ebaron]

Looks good!