-
Notifications
You must be signed in to change notification settings - Fork 20
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(crd): add reference to Grafana secret in Cryostat CR #432
Conversation
Wondering if we could get the current |
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
You won't need to worry about this with just the name being stored in the CR now. |
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 @maxcao13, nice work! I just have a few minor suggestions below.
api/v1beta1/cryostat_types.go
Outdated
@@ -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"` |
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.
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
api/v1beta1/cryostat_types.go
Outdated
@@ -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 |
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.
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"
@@ -316,6 +316,12 @@ func (r *CryostatReconciler) Reconcile(ctx context.Context, request ctrl.Request | |||
} | |||
} | |||
|
|||
instance.Status.GrafanaSecret = instance.Name + "-grafana-basic" |
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.
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.
@@ -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) |
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.
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.
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.
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
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.
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:
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 |
|
Right, you'll need to make sure that the bundle points to your custom operator image. Something like this:
|
Fixes #391
Thoughts?
Now, to get the Grafana secrets, users can run
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!