feat(api): combine Cryostat and ClusterCryostat CRDs #721

ebaron · 2024-02-02T21:01:43Z

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: #583

Description of the change:

Adds a v1beta2 version of the Cryostat CRD. At the moment, this is identical to v1beta1 except for the TargetNamespaces field in the spec and status.
Also temporarily adds a v1beta2 version of the ClusterCryostat CRD. This is identical to v1beta1, except the TargetNamespaces are now inherited from the v1beta2 CryostatSpec/Status. I intend to remove this CRD before release of 3.0, but left it in for now to avoid complicating this PR any further.
Adds a conversion webhook to convert to and from the v1beta1 and v1beta2 CRDs. This is unfortunately a lot of manual copying between structs, but I included some tests that take advantage of our many existing different test CRs. I copied these CRs to a test/conversion.go file where they return a v1beta1 copy of the CR instead.
Adds a defaulting webhook to automatically set the Spec.TargetNamespaces to the CR's namespace, if left unset. This retains the previous behaviour of the v1beta1 CRD.
Adds a validating webhook, which is probably the most interesting part of this PR. When a user creates or updates a Cryostat CR, the webhook performs a SubjectAccessReview on the requesting user against each of the target namespaces specified. The user is required to have create permissions on the Cryostat CRD in those namespaces for the check to pass. This ensures that a user can only create a multi-namespace Cryostat if they would have the permissions to create single namespace Cryostats in each of those namespaces.

Motivation for the change:

Consolidating the CRDs simplifies the experience of using the operator and also simplifies the amount of cases we need to test and support. Using the same CRD, a user will be able to create a single namespace Cryostat, or a multi-namespace Cryostat for any namespaces they're authorized for.

How to manually test:

To test the conversion webhook: create a v1beta1 Cryostat CR. Query it back with kubectl and you should see it's a v1beta2 with the targetNamespaces set to the CR's namespace.
To test the defaulting webhook: create a v1beta2 Cryostat CR with no spec.targetNamespaces. Query it with kubectl, and you should see the targetNamespaces set to the CR's namespace.
To test the validating webhook: create a multi-namespace Cryostat CR with target namespaces that you permission to create Cryostat CRs in. This should succeed. Then try again with a target namespace that you don't have the create Cryostat permission for. This should fail.

ebaron · 2024-02-12T15:29:39Z

This should be ready for review now. Apologies for the large PR, there's a lot of interconnected pieces when bumping the API version.

andrewazores

Just a few thoughts that came up. These are all things to address later.

andrewazores · 2024-02-12T15:21:04Z

api/v1beta1/cryostat_conversion.go

+	dst.ServiceOptions = convertServiceOptionsTo(src.ServiceOptions)
+	dst.NetworkOptions = convertNetworkOptionsTo(src.NetworkOptions)
+	dst.ReportOptions = convertReportOptionsTo(src.ReportOptions)
+	dst.MaxWsConnections = src.MaxWsConnections


I don't think we have an equivalent MaxWsConnections config in 3.0 and I'm not sure if we should bother reimplementing this. I think this is an old holdover from when the UI didn't work too well with concurrent users in the 1.0 days.

andrewazores · 2024-02-12T15:23:01Z

api/v1beta1/cryostat_conversion.go

+	dst.Minimal = src.Minimal
+	dst.EnableCertManager = src.EnableCertManager
+	dst.TrustedCertSecrets = convertCertSecretsTo(src.TrustedCertSecrets)
+	dst.EventTemplates = convertEventTemplatesTo(src.EventTemplates)


3.0 hasn't yet reimplemented custom event templates, but when it does I'm expecting that to be done by using S3 storage rather than direct PVC access. That'll make it more complicated to support the feature from the CR level. Maybe we should drop this like we dropped the FlightRecording CRD?

Would there still be a way to do this declaratively? I haven't really used this feature much, but I could see this being useful for GitOps-managed clusters.

I guess there are a few apparent routes:

the Operator uses an HTTP client and creates the custom event templates on the Cryostat application instances through the API like any other client. This way they still end up in S3 along with any others the user may create, but without relying on the filesystem internal details

when we do reimplement custom event templates in 3.0, we do it by direct filesystem access (PVC) like we had before rather than S3

use S3 as a read-write store in addition to a read-only direct filesystem PVC source where additional templates can be mounted. The PVC-backed storage could also be read-write but then we need to figure out how to make the distinction apparent to users through the UI and give them a choice which one they write into etc. which is probably excessive

I think there are other cases of things that could be useful for things like GitOps clusters, like Automated Rules as a CRD. In 3.0 those are defined in a Postgres table so IMO the most sensible way would be for the Operator to talk to Cryostat via the HTTP API there, too.

That seems reasonable. These sorts of features would likely never be possible for the Helm Chart, but that could be an advantage of using the operator instead.

Then again, we do have other things that seem to make the most sense as files directly injected into the PVC still, like TLS certs...

andrewazores · 2024-02-12T15:23:57Z

api/v1beta1/cryostat_conversion.go

+}
+
+func convertSpecFrom(src *operatorv1beta2.CryostatSpec, dst *CryostatSpec) {
+	dst.Minimal = src.Minimal


I'm in favour of removing the minimal config option: cryostatio/cryostat-helm#122

ebaron · 2024-02-12T15:30:28Z

/build_test

ebaron · 2024-02-12T17:32:33Z

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

andrewazores · 2024-02-12T19:48:54Z

So the equivalent to the old ClusterCryostat InstallNamespace is now just that the Operator will deploy Cryostat into the Namespace in which the Cryostat CR is created, right?

When I try to create a new Cryostat CR through the OpenShift UI:

The TargetNamespaces list is just a text list, no dropdown selector of namespaces to choose from. Is that expected now?

ebaron · 2024-02-12T22:10:58Z

So the equivalent to the old ClusterCryostat InstallNamespace is now just that the Operator will deploy Cryostat into the Namespace in which the Cryostat CR is created, right?

That's right.

When I try to create a new Cryostat CR through the OpenShift UI:

The TargetNamespaces list is just a text list, no dropdown selector of namespaces to choose from. Is that expected now?

Thanks for noticing this. We had a Kustomize patch for the ClusterCryostat CRD to add the descriptors. It didn't seem to be possible to define the x-descriptor using markers for the array elements, as opposed to the array itself.

I updated the patch and also the initialization resource (the UI prompt to create a Cryostat) from v1beta1 to v1beta2.

Signed-off-by: Elliott Baron <ebaron@redhat.com>

* feat(api): combine Cryostat and ClusterCryostat CRDs * Make webhook internal, add tests * Add v1beta2 apiVersion with conversion webhook * Add conversion webhook tests * Run multi-namespace tests for Cryostat CRD * Formatting, generate bundle * Enable conversion webhooks * add conversion webhook to bundle, fixes * Remove log message * Update spec descriptors patch and initialization resource --------- Signed-off-by: Elliott Baron <ebaron@redhat.com>

ebaron added the feat New feature or request label Feb 2, 2024

mergify bot added the safe-to-test label Feb 2, 2024

ebaron added 9 commits February 12, 2024 10:08

feat(api): combine Cryostat and ClusterCryostat CRDs

a6692f5

Make webhook internal, add tests

2b6fcc3

Add v1beta2 apiVersion with conversion webhook

697b06b

Add conversion webhook tests

60eb991

Run multi-namespace tests for Cryostat CRD

0029db0

Formatting, generate bundle

7634030

Enable conversion webhooks

6c1ba9b

add conversion webhook to bundle, fixes

c84ba07

Remove log message

abe56e6

ebaron force-pushed the webhook branch from 5b4ef20 to abe56e6 Compare February 12, 2024 15:08

ebaron marked this pull request as ready for review February 12, 2024 15:08

ebaron requested review from a team February 12, 2024 15:08

andrewazores reviewed Feb 12, 2024

View reviewed changes

Update spec descriptors patch and initialization resource

fe5988e

andrewazores approved these changes Feb 13, 2024

View reviewed changes

Merge branch 'cryostat3' into webhook

e493030

Signed-off-by: Elliott Baron <ebaron@redhat.com>

ebaron merged commit 30ad2ba into cryostatio:cryostat3 Feb 13, 2024
5 checks passed

This was referenced Feb 13, 2024

[Story] Remove Minimal mode #728

Closed

[Story] Remove MaxWsConnections #729

Closed

andrewazores mentioned this pull request Feb 14, 2024

feat(discovery): discovery port name/number options for v1betav2 #733

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): combine Cryostat and ClusterCryostat CRDs #721

feat(api): combine Cryostat and ClusterCryostat CRDs #721

ebaron commented Feb 2, 2024 •

edited

Loading

ebaron commented Feb 12, 2024

andrewazores left a comment

andrewazores Feb 12, 2024

andrewazores Feb 12, 2024

ebaron Feb 12, 2024

andrewazores Feb 12, 2024 •

edited

Loading

andrewazores Feb 12, 2024

ebaron Feb 12, 2024

andrewazores Feb 12, 2024

andrewazores Feb 12, 2024

ebaron commented Feb 12, 2024

ebaron commented Feb 12, 2024

andrewazores commented Feb 12, 2024

ebaron commented Feb 12, 2024

feat(api): combine Cryostat and ClusterCryostat CRDs #721

feat(api): combine Cryostat and ClusterCryostat CRDs #721

Conversation

ebaron commented Feb 2, 2024 • edited Loading

Welcome to Cryostat! 👋

Before contributing, make sure you have:

Description of the change:

Motivation for the change:

How to manually test:

ebaron commented Feb 12, 2024

andrewazores left a comment

Choose a reason for hiding this comment

andrewazores Feb 12, 2024

Choose a reason for hiding this comment

andrewazores Feb 12, 2024

Choose a reason for hiding this comment

ebaron Feb 12, 2024

Choose a reason for hiding this comment

andrewazores Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

andrewazores Feb 12, 2024

Choose a reason for hiding this comment

ebaron Feb 12, 2024

Choose a reason for hiding this comment

andrewazores Feb 12, 2024

Choose a reason for hiding this comment

andrewazores Feb 12, 2024

Choose a reason for hiding this comment

ebaron commented Feb 12, 2024

ebaron commented Feb 12, 2024

andrewazores commented Feb 12, 2024

ebaron commented Feb 12, 2024

ebaron commented Feb 2, 2024 •

edited

Loading

andrewazores Feb 12, 2024 •

edited

Loading