feat(discovery): options to configure discovery port names and numbers

[tthvo]

Welcome to Cryostat! 👋

Before contributing, make sure you have:

• Read the contributing guidelines
• Linked a relevant issue which this PR resolves
• Linked any other relevant issues, PR's, or documentation, if any
• Resolved all conflicts, if any
• Rebased your branch PR on top of the latest upstream main branch
• Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
• Signed all commits: git commit -S -m "YOUR_COMMIT_MESSAGE"

---

Fixes: #714
Depends on https://github.com/cryostatio/cryostat/pull/1862

Description of the change:

• Added CRD fields for customizing discovery port names and numbers.
• Tests are a little messy though but please lmk what you think! :D

Spec

Define non-empty lists:

spec:
  targetDiscoveryOptions:
    discoveryPortNames:
      - for-jmx
      - jmx-port
    discoveryPortNumbers:
      - 9092
      - 9093

Use empty value (i.e. -):

spec:
  targetDiscoveryOptions:
    disableBuiltInPortNames: true
    disableBuiltInPortNumbers: true

Use default values:

spec:
  targetDiscoveryOptions:

OR

spec:
  targetDiscoveryOptions:
    discoveryPortNames: []
    discoveryPortNumbers: []

Motivation for the change:

See #714

How to test

This relies on Andrew's image for #714. Build or use a pre-built one here quay.io/thvo/cryostat-operator:2.5.0-k8s-discovery-3.

$ export BUNDLE_IMG=quay.io/thvo/cryostat-operator-bundle:2.5.0-k8s-discovery-3
$ make deploy_bundle

Then, either create Cryostat from console (OpenShift) or YAML with above spec (discoveryPortNames is a list of strings, and discoveryPortNumbers is a list of integers.

[github-actions]

This PR/issue depends on:

• cryostatio/cryostat#1862
  By Dependent Issues (🤖). Happy coding!

[tthvo]

For OpenShift, I am not sure how the empty list case can be achieved via the form view, but I suppose YAML view is available.

[andrewazores]

I haven't tested it, but for the names list since it is just a list of string, wouldn't the form view allow entering simply -?

For the port list it is a little annoying I guess because it makes sense to enforce that this is a list of integer on the form, which means they can't enter - - even though in the end this is going to get made into a string so it can be set as an environment variable.

[andrewazores]

Maybe we go back to using 0 for the "no ports" case?

[tthvo]

I haven't tested it, but for the names list since it is just a list of string, wouldn't the form view allow entering simply -?
For the port list it is a little annoying I guess because it makes sense to enforce that this is a list of integer on the form, which means they can't enter - - even though in the end this is going to get made into a string so it can be set as an environment variable.

(Field descriptor in this image is outdated. Missing default mentions are added in latest commit)

Right, actually both form's input allows entering -. However, for port number input, invalid items are silently removed after submission.

[andrewazores]

Maybe with that form view UI in mind it would be best to treat the variable as undefined differently from the variable as blank.

undefined -> "Use defaults (names: ["jfr-jmx"], ports: [9091])
'' -> "Use empty (names: [], ports: [])
jfr-jmx or 9091 -> "Use list (names: ["jfr-jmx"], ports: [9091])
foo,bar or 9999,4444 -> "Use list (names: ["foo", "bar"], ports: [9999, 4444])

I think it would be OK to break this default in 3.0, but not for 2.4.1, so the undefined case really must map to the default singletons when the Cryostat container is deployed - I wouldn't even want to move the assignment of those default values into the Operator or Helm chart logic either.

[tthvo]

Maybe we go back to using 0 for the "no ports" case?

Make sense to me for accommodating the restrictions on integer-only.

I guess I was too concerned about diff between [] and nil when represented on form view. Then, a [-] or [0] to represent empty would be the option.

[andrewazores]

Maybe not what I just said actually... I don't know how the user (via form or yaml view) would indicate to the operator that they want empty lists vs they want default behaviour. Bah.

Let's go with - and 0 as the "empty" signifiers for the 2.4.1 bugfix release.

For 3.0 I would like it so that Cryostat itself does not assume any default values for the names or ports, ie its own defaults are the empty lists, whether the env vars are undefined or blank. That should clean up this confusing UI and remove the need for the -/0 signifiers.

[tthvo]

That sounds good to me! That should also eliminate another tiny problem is that empty input field (i.e. added but not filled) are actually wiped out too after submission. So, the below case would not be really possible via form.

'' -> "Use empty (names: [], ports: [])

[ebaron]

This looks good to me. Does this approach with separate toggles to disable default behaviour seem good to you too @andrewazores?

[andrewazores]

Yup, that makes sense.

[andrewazores]

/build_test

[andrewazores]

https://github.com/cryostatio/cryostat-operator/actions/runs/7876903655

[ebaron]

@Mergifyio backport cryostat3

[mergify]

backport cryostat3

✅ Backports have been created

• #725 feat(discovery): options to configure discovery port names and numbers (backport #715) has been created for branch cryostat3 but encountered conflicts