-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(discovery): allow configurations for discovery service #474
Conversation
internal/controllers/common/resource_definitions/resource_definitions.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @tthvo looks good! Just a few style comments at first glance.
internal/controllers/common/resource_definitions/resource_definitions.go
Outdated
Show resolved
Hide resolved
internal/controllers/common/resource_definitions/resource_definitions.go
Outdated
Show resolved
Hide resolved
With the latest commits, I made the following changes:
|
I added new changes from last time:
|
Other than docs typos above the code changes make sense to me. I'll build and test this out shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, will leave it for ebaron to approve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@tthvo This can be in a follow-up PR I think. |
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? |
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. |
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? |
I opened the issue for the follow up at #475 :D |
True, that would be harder to detect. How about this idea instead? We add a |
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. |
Separate PR is fine. That would actually be the fix for #475. |
@ebaron When I run 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything seems to work as expected. Nice work @tthvo!
This PR includes configurations to discovery service:
CRYOSTAT_JMX_CREDENTIALS_DB_PASSWORD
variable.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