feat(discovery): configurations for KubeAPI discovery

[tthvo]

Related to #119
Depends on cryostatio/cryostat#325

Description of changes

• Added options to configure discovery mechanism.

  • JMX port names & numbers
  • Watch namespaces

• Updated Role to ClusterRole to create the corresponding RoleBinding in each of the watch namespace.

How to test

IMAGE_NAMESPACE=<namespace>
helm install \
  --namespace=myns --create-namespace=true \
  --set core.discovery.kubernetes.namespaces="{default,myns,"","",myns,myns}" \
  --set core.discovery.kubernetes.portNames="{jmx-jfr,my-port}" \
  --set core.discovery.kubernetes.portNumbers="{9091,9097}" \
  --set core.discovery.kubernetes.enabled=true \
  --set core.image.repository="${IMAGE_NAMESPACE}" \
  cryostat ./charts/cryostat/

[andrewazores]

LGTM, though may need some adjusting after review on cryostatio/cryostat#325 finishes.

[andrewazores]

@ebaron could you take a quick look over this? I think it looks good and it works well with the accompanying cryostatio/cryostat#325 (which was just merged).

[ebaron]

Sorry for the delay in getting to these Helm PRs. This looks good, just wondering if we should use the same kind of DisableBuiltInPort{Names,Numbers} that we settled on in the Operator's CRD: https://github.com/cryostatio/cryostat-operator/blob/b300689a645e3741ccd5468e09acf20b5b9fb4d2/api/v1beta1/cryostat_types.go#L541-L562 Then we don't need to rely on magic values like ..

[tthvo]

Sorry for the delay in getting to these Helm PRs. This looks good, just wondering if we should use the same kind of DisableBuiltInPort{Names,Numbers} that we settled on in the Operator's CRD: https://github.com/cryostatio/cryostat-operator/blob/b300689a645e3741ccd5468e09acf20b5b9fb4d2/api/v1beta1/cryostat_types.go#L541-L562

I guess if on helm side, if users want to disable built-in port {numbers,names}, they can set, for example, --set core.discovery.kubernetes.portNumbers="{}" --set core.discovery.kubernetes.portNames="{}". Would that be nicer than the bool flag? I am good either way though :D

Then we don't need to rely on magic values like ..

I think the . is used to represent current namespace, right?

[ebaron]

Sorry for the delay in getting to these Helm PRs. This looks good, just wondering if we should use the same kind of DisableBuiltInPort{Names,Numbers} that we settled on in the Operator's CRD: https://github.com/cryostatio/cryostat-operator/blob/b300689a645e3741ccd5468e09acf20b5b9fb4d2/api/v1beta1/cryostat_types.go#L541-L562

I guess if on helm side, if users want to disable built-in port {numbers,names}, they can set, for example, --set core.discovery.kubernetes.portNumbers="{}" --set core.discovery.kubernetes.portNames="{}". Would that be nicer than the bool flag? I am good either way though :D

OpenShift has a UI for customizing Helm values on install, think the Operator one but much more primitive. I think this might be hard to represent that way in those kind of UIs. Boolean values are easily rendered as checkboxes.

Then we don't need to rely on magic values like ..

I think the . is used to represent current namespace, right?

It makes sense like . is used to represent the current directory on a filesystem, but is this convention used in Kubernetes?

[tthvo]

OpenShift has a UI for customizing Helm values on install, think the Operator one but much more primitive. I think this might be hard to represent that way in those kind of UIs. Boolean values are easily rendered as checkboxes.

Ahh good to know! I didn't realize that. I will update.

It makes sense like . is used to represent the current directory on a filesystem, but is this convention used in Kubernetes?

Actually, I haven't yet come across any in k8s with this syntax. Though, this is implemented in Cryostat 3.0 tho: https://github.com/cryostatio/cryostat3/blob/1d991d1889e1d63b84f171afe913cf7cb0469aad/src/main/java/io/cryostat/discovery/KubeApiDiscovery.java#L504. @andrewazores any thoughts?

[tthvo]

OpenShift has a UI for customizing Helm values on install, think the Operator one but much more primitive. I think this might be hard to represent that way in those kind of UIs. Boolean values are easily rendered as checkboxes.

Ahh good to know! I didn't realize that. I will update.

Sorry another question. Since those fields (i.e. namespaces, portNumbers, portNames) are lists, they are rendered as below. If the users want to disable defaults, would it make sense for them to just remove that item from the list?

For example, installing cryostat with securityContext capabilities (using helm chart UI on OpenShift).

Editted: If we are gonna use flag for namespace (see below comment), I think we might as well do it for port{names,numbers}. Thought, it is somewhat nice to remove to disable :))

[tthvo]

Actually, I haven't yet come across any in k8s with this syntax. Though, this is implemented in Cryostat 3.0 tho: https://github.com/cryostatio/cryostat3/blob/1d991d1889e1d63b84f171afe913cf7cb0469aad/src/main/java/io/cryostat/discovery/KubeApiDiscovery.java#L504. @andrewazores any thoughts?

Maybe a flag: discovery.kubernetes.withInstallNamespace that would add release namespace in the watch list so that we don't have to use either . or {{ .Release.Namespace }}.

Editted: I realized now that's what you meant originally (my bad!) :D I have updated the chart with such flags similar to the operators. This way, it is closer to the operator-side, and avoids magic value .. How about now?

installNamespaceDisabled: false
builtInPortNamesDisabled: false
builtInPortNumbersDisabled: false

This does mean cryostat3 can just remove its OWN_NAMESPACE concept?

[andrewazores]

This does mean cryostat3 can just remove its OWN_NAMESPACE concept?

I'd like to keep the . implemented on the Cryostat3 container, even if it isn't used in our own Operator or Helm chart. It's a simple enough feature to carry with minimal maintenance burden, but it seems like it would be quite handy for anyone who wants to roll their own Cryostat deployments.

Otherwise, I'm good with the other changes suggested above.

[tthvo]

I'd like to keep the . implemented on the Cryostat3 container, even if it isn't used in our own Operator or Helm chart. It's a simple enough feature to carry with minimal maintenance burden, but it seems like it would be quite handy for anyone who wants to roll their own Cryostat deployments.

Sounds good to me!

[ebaron]

I think this is good. @tthvo what do you think about keeping the Role as a Role, and duplicating it in each namespace? I think so far, a namespace admin is able to install Cryostat using the Helm Chart. They may not have permissions to create a ClusterRole.

[tthvo]

I think this is good. @tthvo what do you think about keeping the Role as a Role, and duplicating it in each namespace? I think so far, a namespace admin is able to install Cryostat using the Helm Chart. They may not have permissions to create a ClusterRole.

Oh yes that makes sense! Totally overlooking this aspect :D

[tthvo]

Updated now to copy Role over to target namespaces instead of ClusterRole. Also, removed checks for magic value . in watch namespace list. How about now?

[tthvo]

Not related to this PR but one suggestion I think we might benefit from is unit/template testing for helm chart. This can help: https://github.com/helm-unittest/helm-unittest

During this PR, it took me a while to see that the --- (yaml separator) leaked into a template field (e.g. namespace: myns---) due to space trimming but chart renders fine. It would be nice to have testing to ease debugging :D

[andrewazores]

LGTM other than the one comment typo above

[ebaron]

Looks good, thanks @tthvo!