feat(crd): add reference to Grafana secret in Cryostat CR #432

maxcao13 · 2022-07-22T23:12:21Z

Fixes #391

Thoughts?

Now, to get the Grafana secrets, users can run

$ kubectl get cryostat -o jsonpath='{$.items[0].status.grafanaSecret.data.GF_SECURITY_ADMIN_USER}' | base64 -d
\\ admin
$ kubectl get cryostat -o jsonpath='{$.items[0].status.grafanaSecret.data.GF_SECURITY_ADMIN_PASSWORD}' | base64 -d
'\\ someRandomGeneratedPassword

I'm not sure the best way to test the password since it's generated randomly so I just tested if it was a string and non-empty. There must be better ways right?
Also, would it be better to also have the secrets be revealed in ASCII form when doing
kubectl get cryostat ?

Note: Thanks to @tthvo a lot for helping me understand a lot of the new concepts and helping me with this PR!

tthvo · 2022-07-25T15:22:44Z

I'm not sure the best way to test the password since it's generated randomly so I just tested if it was a string and non-empty. There must be better ways right?

Wondering if we could get the current grafana-basic secret to check since that is what the status is set to be?

ebaron · 2022-07-25T17:07:30Z

Hi @maxcao13, thanks for working on this! Sorry I wasn't clear in the issue, but I intended to just store the secret name in the Status and not the entire secret. Secrets have some security advantages over other Kubernetes objects [1], so storying the contents of the secret in our custom resource would lose those advantages. Your changes would look somewhat similar though.

[1] https://kubernetes.io/docs/concepts/configuration/secret/#information-security-for-secrets

I'm not sure the best way to test the password since it's generated randomly so I just tested if it was a string and non-empty. There must be better ways right?

You won't need to worry about this with just the name being stored in the CR now.

ebaron

Hey @maxcao13, nice work! I just have a few minor suggestions below.

ebaron · 2022-07-25T21:22:20Z

api/v1beta1/cryostat_types.go

@@ -118,7 +118,7 @@ type CryostatStatus struct {
 	ApplicationURL string `json:"applicationUrl"`
 	// Grafana secret
 	// +operator-sdk:csv:customresourcedefinitions:type=status
-	GrafanaSecret *corev1.Secret `json:"grafanaSecret"`
+	GrafanaSecret string `json:"grafanaSecret"`


Could you add the // +optional marker for this field, and also append omitempty to the JSON tag? Optional tells the Kubernetes API server that this field is not required, such as before the secret is created. omitempty is similar, but functions on the JSON level. If the field is missing, it won't be transmitted in the JSON body.

There's one more marker I'd like to add here. If you look at other fields, you'll see an xDescriptors marker. This controls some generated UI in the OpenShift Console. We can tell the Console that this field refers to the name of a Kubernetes Secret. When it gets rendered in the OpenShift Console, it should create a link to the named Secret that users can click on. The example here conveniently shows how this works for Secrets, but also works for other kinds of Kubernetes objects: https://github.com/openshift/console/blob/master/frontend/packages/operator-lifecycle-manager/src/components/descriptors/reference/reference.md#3-k8sResourcePrefix

ebaron · 2022-07-25T21:24:50Z

api/v1beta1/cryostat_types.go

@@ -116,6 +116,9 @@ type CryostatStatus struct {
 	// Address of the deployed Cryostat web application
 	// +operator-sdk:csv:customresourcedefinitions:type=status,xDescriptors={"urn:alm:descriptor:org.w3:link"}
 	ApplicationURL string `json:"applicationUrl"`
+	// Grafana secret


This comment turns into the field's description, which is shown to the user. We might want to elaborate a bit on what this contains. Maybe something like: "Secret containing the generated Grafana credentials"

ebaron · 2022-07-25T21:28:50Z

internal/controllers/cryostat_controller.go

@@ -316,6 +316,12 @@ func (r *CryostatReconciler) Reconcile(ctx context.Context, request ctrl.Request
 		}
 	}

+	instance.Status.GrafanaSecret = instance.Name + "-grafana-basic"


Perhaps for this, we could use the value of grafanaSecret.Name? It'll be less error-prone if we decide to change the secret's name in the future.

ebaron · 2022-07-25T21:30:51Z

internal/controllers/cryostat_controller.go

@@ -316,6 +316,12 @@ func (r *CryostatReconciler) Reconcile(ctx context.Context, request ctrl.Request
 		}
 	}

+	instance.Status.GrafanaSecret = instance.Name + "-grafana-basic"
+	err = r.Client.Status().Update(context.Background(), instance)


This should use ctx instead of context.Background(). We used context.Background() a lot before the Reconcile method provided a context to use, but I've been trying to replace this whenever making other changes to the affected code.

ebaron

The code changes look good, but I'm having trouble pushing to Quay.io at the moment so I can't test it. Hopefully I'll have better luck tomorrow

ebaron

Hi Max, got Quay working. Your PR works as expected. Thanks for taking this on!

If you're curious, this is how the Grafana Secret looks in the OpenShift Console:

The Cryostat CR page has a clickable link to the Grafana Secret.
Clicking on the Secret takes you to the Secret in the console, and you can scroll down to reveal the Grafana credentials.

maxcao13 · 2022-07-27T19:07:19Z

Thanks for helping Elliott!

For future reference, how are you able to install my build onto the OperatorHub as an "Installed Operator"? I have been testing using crc and have only been able to make deploy so I can only see the operator as part of the Deployments and Pods.

andrewazores · 2022-07-27T19:09:00Z

For future reference, how are you able to install my build onto the OperatorHub as an "Installed Operator"? I have been testing using crc and have only been able to make deploy so I can only see the operator as part of the Deployments and Pods.

make bundle, make bundle-build, and make deploy_bundle, essentially.

ebaron · 2022-07-27T19:38:41Z

Right, you'll need to make sure that the bundle points to your custom operator image. Something like this:

export OPERATOR_IMG=quay.io/me/cryostat-operator:latest
export BUNDLE_IMG=quay.io/me/cryostat-operator-bundle:latest
make oci-build
podman push $OPERATOR_IMG
make bundle 
make bundle-build
podman push $BUNDLE_IMG
make deploy_bundle

maxcao13 added 2 commits July 22, 2022 19:01

commit for now

5cc0f26

add updated yaml

027f90a

maxcao13 added the feat New feature or request label Jul 22, 2022

maxcao13 requested a review from ebaron July 22, 2022 23:12

maxcao13 added 2 commits July 25, 2022 14:45

fixed to only include grafana secret name

c265612

fixed image_namespace thing

462cc4e

ebaron reviewed Jul 25, 2022

View reviewed changes

maxcao13 added 2 commits July 25, 2022 21:07

added xdescriptor for secret and fixed other things

dadaaeb

fixed wrong image_namespace

d4002b3

ebaron reviewed Jul 26, 2022

View reviewed changes

ebaron approved these changes Jul 27, 2022

View reviewed changes

ebaron merged commit 4e6f0c7 into cryostatio:main Jul 27, 2022

maxcao13 deleted the grafana-secret branch July 27, 2022 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(crd): add reference to Grafana secret in Cryostat CR #432

feat(crd): add reference to Grafana secret in Cryostat CR #432

maxcao13 commented Jul 22, 2022

tthvo commented Jul 25, 2022

ebaron commented Jul 25, 2022

ebaron left a comment

ebaron Jul 25, 2022

ebaron Jul 25, 2022

ebaron Jul 25, 2022

ebaron Jul 25, 2022

ebaron left a comment

ebaron left a comment

maxcao13 commented Jul 27, 2022

andrewazores commented Jul 27, 2022 •

edited

Loading

ebaron commented Jul 27, 2022

feat(crd): add reference to Grafana secret in Cryostat CR #432

feat(crd): add reference to Grafana secret in Cryostat CR #432

Conversation

maxcao13 commented Jul 22, 2022

tthvo commented Jul 25, 2022

ebaron commented Jul 25, 2022

ebaron left a comment

Choose a reason for hiding this comment

ebaron Jul 25, 2022

Choose a reason for hiding this comment

ebaron Jul 25, 2022

Choose a reason for hiding this comment

ebaron Jul 25, 2022

Choose a reason for hiding this comment

ebaron Jul 25, 2022

Choose a reason for hiding this comment

ebaron left a comment

Choose a reason for hiding this comment

ebaron left a comment

Choose a reason for hiding this comment

maxcao13 commented Jul 27, 2022

andrewazores commented Jul 27, 2022 • edited Loading

ebaron commented Jul 27, 2022

andrewazores commented Jul 27, 2022 •

edited

Loading