feat(discovery): allow configurations for discovery service #474

tthvo · 2022-10-12T23:10:37Z

This PR includes configurations to discovery service:

The operator now manages a Secret that will provide generated password for CRYOSTAT_JMX_CREDENTIALS_DB_PASSWORD variable.
Built-in Discovery Service can now be disabled via Cryostat CR spec.
Cryostat is deployed with h2:file if PVC is configured/defaulted. Otherwise, h2:mem is used.

I also added missing nil-checks in test resources.

Fixes: #452, #451, #465

internal/test/resources.go

docs/config.md

internal/controllers/common/resource_definitions/resource_definitions.go

internal/controllers/secrets.go

ebaron

Hey @tthvo looks good! Just a few style comments at first glance.

api/v1beta1/cryostat_types.go

docs/config.md

internal/controllers/common/resource_definitions/resource_definitions.go

internal/controllers/secrets.go

tthvo · 2022-10-13T18:49:05Z

With the latest commits, I made the following changes:

The credentials database password must be configured via a spec field to point to an existing secret. The secret must have the key CRYOSTAT_JMX_CREDENTIALS_DB_PASSWORD with password. This secret (or specifically the password) must not be editted.
Operator only accesses the key CRYOSTAT_JMX_CREDENTIALS_DB_PASSWORD in the secret instead of reading every keys.
For test, I add a test secret for all test cases since it is always required for Cryostat to work.

tthvo · 2022-10-13T20:41:53Z

I added new changes from last time:

Database Secret Name spec is now optional. However, it must be specified or not at all for the entire lifetime of the Cryostat CR. If persistent volume is re-regained, it must be in cleaned up. Switching is not allowed to avoid failed access due to password mismatch.
The Operator still manages the changes due to switching (i.e. delete generated secret if spec is set or generate a new one if not) but failure in Cryostat due to this change is not handled.

docs/config.md

andrewazores · 2022-10-13T21:53:09Z

Other than docs typos above the code changes make sense to me. I'll build and test this out shortly.

andrewazores

LGTM, will leave it for ebaron to approve

ebaron

How about this idea to guard against the user breaking their credentials DB by destroying the password?

If the CR doesn't have a defined DatabaseSecretName, create the default secret with a generated password
When creating the deployment, check whether the default secret exists.
- If it does, then the CR initially had no custom secret and the credentials DB would have been created with the generated password.
- If it doesn't, then check if DatabaseSecretName has been set (it should), and use that.
- If for some reason the DatabaseSecretName is not set, then the password has been lost. Likely due to the user deleting the default secret. We could emit an Event here.

ebaron · 2022-10-14T18:18:36Z

How about this idea to guard against the user breaking their credentials DB by destroying the password?

1. If the CR doesn't have a defined `DatabaseSecretName`, create the default secret with a generated password

2. When creating the deployment, check whether the default secret exists.
   
   * If it does, then the CR initially had no custom secret and the credentials DB would have been created with the generated password.
   * If it doesn't, then check if `DatabaseSecretName` has been set (it should), and use that.
   * If for some reason the `DatabaseSecretName` is not set, then the password has been lost. Likely due to the user deleting the default secret. We could emit an Event here.

@tthvo This can be in a follow-up PR I think.

tthvo · 2022-10-14T18:27:42Z

How about this idea to guard against the user breaking their credentials DB by destroying the password?

1. If the CR doesn't have a defined `DatabaseSecretName`, create the default secret with a generated password

2. When creating the deployment, check whether the default secret exists.
   
   * If it does, then the CR initially had no custom secret and the credentials DB would have been created with the generated password.
   * If it doesn't, then check if `DatabaseSecretName` has been set (it should), and use that.
   * If for some reason the `DatabaseSecretName` is not set, then the password has been lost. Likely due to the user deleting the default secret. We could emit an Event here.

Right, thats a good idea. This means we don't delete the default secret if switching from default to provided secret option so that we can check when creating the deployment?

ebaron · 2022-10-14T18:29:50Z

Right, it may be a good idea as a temporary measure to just not delete the generated secret. I don't think that will ever result in a working deployment. At least with it still around, users can revert their changes.

tthvo · 2022-10-14T18:32:15Z

Just a question: What about the case where the user switch from custom secret to default one? The default secret will take place and still make Cryostat fail right?

tthvo · 2022-10-14T18:41:34Z

I opened the issue for the follow up at #475 :D

ebaron · 2022-10-14T18:48:13Z

Just a question: What about the case where the user switch from custom secret to default one? The default secret will take place and still make Cryostat fail right?

True, that would be harder to detect. How about this idea instead? We add a Status.DatabaseSecret to the CRD. We write to this field once, which will be the canonical database secret for the lifetime of that Cryostat. We could do this by comparing against the existing value of the field and only setting it when nil. Then the secret referenced here would be what the Deployment uses.

tthvo · 2022-10-14T18:54:23Z

Right that would be a better safe guarding against it. Would you prefer in a separate PR later or in this PR too?

Also, if Cryostat is re-created and somehow persistent volume is regained (not clean up), then switching could cause failure since the status is now initiated differently.

ebaron · 2022-10-14T19:04:12Z

Right that would be a better safe guarding against it. Would you prefer in a separate PR later or in this PR too?

Separate PR is fine. That would actually be the fix for #475.

tthvo · 2022-10-14T19:09:47Z

@ebaron When I run go mod tidy, the module require github.com/google/go-cmp v0.5.6 seems to changed to be a direct required instead of indirect. Is this change desired?

diff --git a/go.mod b/go.mod
index 80dfb7f..1695415 100644
--- a/go.mod
+++ b/go.mod
@@ -15,6 +15,8 @@ require (
        sigs.k8s.io/controller-runtime v0.12.1
 )
        sigs.k8s.io/controller-runtime v0.12.1
 )
 
+require github.com/google/go-cmp v0.5.6
+
 require (
        cloud.google.com/go v0.90.0 // indirect
        github.com/Azure/go-autorest v14.2.0+incompatible // indirect
@@ -41,7 +43,6 @@ require (
        github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
        github.com/golang/protobuf v1.5.2 // indirect
        github.com/google/gnostic v0.5.7-v3refs // indirect
-       github.com/google/go-cmp v0.5.6 // indirect
        github.com/google/gofuzz v1.2.0 // indirect

ebaron · 2022-10-14T19:14:41Z

@ebaron When I run go mod tidy, the module require github.com/google/go-cmp v0.5.6 seems to changed to be a direct required instead of indirect. Is this change desired?

diff --git a/go.mod b/go.mod
index 80dfb7f..1695415 100644
--- a/go.mod
+++ b/go.mod
@@ -15,6 +15,8 @@ require (
        sigs.k8s.io/controller-runtime v0.12.1
 )
        sigs.k8s.io/controller-runtime v0.12.1
 )
 
+require github.com/google/go-cmp v0.5.6
+
 require (
        cloud.google.com/go v0.90.0 // indirect
        github.com/Azure/go-autorest v14.2.0+incompatible // indirect
@@ -41,7 +43,6 @@ require (
        github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
        github.com/golang/protobuf v1.5.2 // indirect
        github.com/google/gnostic v0.5.7-v3refs // indirect
-       github.com/google/go-cmp v0.5.6 // indirect
        github.com/google/gofuzz v1.2.0 // indirect

Oh, that must have been caused by #433 where I used cmp to compare selectors. This change is wanted, but I would add it into the existing require ( ... ) block.

ebaron

Everything seems to work as expected. Nice work @tthvo!

Thuan Vo added 3 commits October 12, 2022 17:49

feat(discovery): allow configuring discovery mechanism

1f798a2

feat(secrets): add secret for JMX credential database

2f2d38b

test(discovery): update tests

7abf8d2

tthvo added the feat New feature or request label Oct 12, 2022

tthvo commented Oct 12, 2022

View reviewed changes

internal/test/resources.go Show resolved Hide resolved

tthvo requested review from ebaron and andrewazores October 12, 2022 23:17

docs(discovery): update config docs

b17bbfb

This was linked to issues Oct 13, 2022

Cryostat should be deployed in h2:file mode #451

Closed

Generate credentials DB password #465

Closed

andrewazores reviewed Oct 13, 2022

View reviewed changes

docs/config.md Outdated Show resolved Hide resolved

andrewazores reviewed Oct 13, 2022

View reviewed changes

internal/controllers/common/resource_definitions/resource_definitions.go Outdated Show resolved Hide resolved

andrewazores reviewed Oct 13, 2022

View reviewed changes

internal/controllers/secrets.go Show resolved Hide resolved

Thuan Vo added 2 commits October 13, 2022 11:59

docs(discovery): add doc links

be639a6

fix(discovery): fix env var definitions

3204f76

ebaron reviewed Oct 13, 2022

View reviewed changes

Thuan Vo added 6 commits October 13, 2022 13:19

fix(apis): update spec comments

a12d2cd

docs(discovery): update docs

b4481fd

chore(discovery): clean up func calls

7f3c408

feat(discovery): set JDBC user/password env vars

01a8e69

feat(discovery): password for credentials database is configurable

b2f0d2a

fix(secrets): only access needed entry in secret

e574f8e

feat(discovery): database secret is optional

5d9b8a6

docs(discovery): update docs

30a9b9b

andrewazores mentioned this pull request Oct 13, 2022

fix(discovery): leave Custom Targets enabled when builtins are disabled cryostatio/cryostat-legacy#1114

Merged

fix(resources): update default database credentials

f2f0a5d

andrewazores reviewed Oct 13, 2022

View reviewed changes

docs/config.md Outdated Show resolved Hide resolved

andrewazores reviewed Oct 13, 2022

View reviewed changes

docs/config.md Outdated Show resolved Hide resolved

docs(discovery): fix typos

4a7e46d

andrewazores reviewed Oct 13, 2022

View reviewed changes

Merge branch 'main' into discovery-config

4d207f1

ebaron reviewed Oct 14, 2022

View reviewed changes

fix(discovery): do not delete default database secret

66b6347

tthvo mentioned this pull request Oct 14, 2022

[Bug] Credentials DB password should not update #475

Closed

tests(secrets): fix failed tests

8e0d98c

chore(discovery): use boolean literals

a13a3b1

build(deps): go mod tidy

a04af68

ebaron approved these changes Oct 14, 2022

View reviewed changes

ebaron merged commit 57457ba into cryostatio:main Oct 14, 2022

tthvo deleted the discovery-config branch October 14, 2022 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(discovery): allow configurations for discovery service #474

feat(discovery): allow configurations for discovery service #474

tthvo commented Oct 12, 2022 •

edited

Loading

ebaron left a comment

tthvo commented Oct 13, 2022

tthvo commented Oct 13, 2022 •

edited

Loading

andrewazores commented Oct 13, 2022

andrewazores left a comment

ebaron left a comment

ebaron commented Oct 14, 2022

tthvo commented Oct 14, 2022 •

edited

Loading

ebaron commented Oct 14, 2022

tthvo commented Oct 14, 2022

tthvo commented Oct 14, 2022

ebaron commented Oct 14, 2022 •

edited

Loading

tthvo commented Oct 14, 2022 •

edited

Loading

ebaron commented Oct 14, 2022

tthvo commented Oct 14, 2022

ebaron commented Oct 14, 2022

ebaron left a comment

feat(discovery): allow configurations for discovery service #474

feat(discovery): allow configurations for discovery service #474

Conversation

tthvo commented Oct 12, 2022 • edited Loading

ebaron left a comment

Choose a reason for hiding this comment

tthvo commented Oct 13, 2022

tthvo commented Oct 13, 2022 • edited Loading

andrewazores commented Oct 13, 2022

andrewazores left a comment

Choose a reason for hiding this comment

ebaron left a comment

Choose a reason for hiding this comment

ebaron commented Oct 14, 2022

tthvo commented Oct 14, 2022 • edited Loading

ebaron commented Oct 14, 2022

tthvo commented Oct 14, 2022

tthvo commented Oct 14, 2022

ebaron commented Oct 14, 2022 • edited Loading

tthvo commented Oct 14, 2022 • edited Loading

ebaron commented Oct 14, 2022

tthvo commented Oct 14, 2022

ebaron commented Oct 14, 2022

ebaron left a comment

Choose a reason for hiding this comment

tthvo commented Oct 12, 2022 •

edited

Loading

tthvo commented Oct 13, 2022 •

edited

Loading

tthvo commented Oct 14, 2022 •

edited

Loading

ebaron commented Oct 14, 2022 •

edited

Loading

tthvo commented Oct 14, 2022 •

edited

Loading