Skip to content

Conversation

sbernauer
Copy link
Member

@sbernauer sbernauer commented Aug 22, 2025

Part of stackabletech/issues#642

Note

Load-balancing over the secret-op DaemonSet Pods seems to work fine, but we should also test it on a cloud provider with multiple nodes as well.

This PR marks samAccountName as non-experimental as per #627. It is the first use of the stackable-versioned macro as well as the conversion webhook machinery.

Note

This PR makes changes to things that are templated by operator-templating! We are aware of this and will have a diff until we roll this out to all operators and update operator-templating. secret-operator is our guinea-pig

Copy link
Member

@Techassi Techassi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is mostly done, but I left a few small comments/reminders.

Comment on lines +122 to +123
mkdir -p extra
cargo run --bin stackable-"${OPERATOR_NAME}" -- crd > extra/crds.yaml
Copy link
Member

@Techassi Techassi Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This eventually needs to be upstreamed into operator-templating.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment in the PR description about this

Comment on lines 25 to 33
k8s_yaml(helm(
'deploy/helm/' + operator_name,
name=operator_name,
namespace="stackable-operators",
set=[
'image.repository=' + registry + '/' + operator_name,
'telemetry.consoleLog.level=trace,h2=off',
],
))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also needs to be upstreamed into operator-templating.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment in the PR description about this

@Techassi Techassi marked this pull request as ready for review September 9, 2025 08:41
@Techassi Techassi moved this from Development: In Progress to Development: Waiting for Review in Stackable Engineering Sep 9, 2025
@Techassi Techassi changed the title feat: Introduce CRD versioning feat: Add v1alpha2 for SecretClass, rename experimentalGenerateSamAccountName Sep 9, 2025
@sbernauer
Copy link
Member Author

I can not approve my own PR but LGTM

@sbernauer sbernauer moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering Sep 16, 2025
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@razvan reported

❯ helm upgrade --install --namespace stackable-operators secret-operator deploy/helm/secret-operator/
Error: unable to build kubernetes objects from release manifest: resource mapping not found for name: "tls" namespace: "" from "": no matches for kind "SecretClass" in version "secrets.stackable.tech/v1alpha1"
ensure CRDs are installed first

I guess the easiest solution is to move the SecretClass creation into a Job, WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have an initial crd deployment done by Helm/OLM as before?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would vote "no". There should be no need for it, as the operator immediately deploys it when starting up for the first time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and the connected decision) was discussed amongst @sbernauer, @NickLarsenNZ, and @razvan and we agreed this is the best way forward.

@razvan confirmed in #634 (comment) that the approach works, by manually creating the default custom resource. In the future, this will be done automatically by the operator.

@razvan
Copy link
Member

razvan commented Sep 19, 2025

I'll put this here for lack of a better place now.

I tried installing this op version with OLM on OpenShift.

OLM of course has the same problem as Helm (see comment above). It tries to create a tls secret class but there is no CRD for it.

I manually added the exported CRDs to OLM, but then the operator refuses to start with this error:

secret-operator 2025-09-19T10:47:38.191165Z  INFO reconcile_crds: stackable_webhook::servers::conversion: Reconciling CRDs crds=["secretclasses.secrets.sta
kable.tech", "truststores.secrets.stackable.tech"[]
secret-operator Error: failed to run conversion webhook
secret-operator
secret-operator Caused by:
secret-operator     0: failed to reconcile CRDs
secret-operator     1: failed to update CRD "secretclasses.secrets.stackable.tech"
secret-operator     2: ApiError: Apply failed with 2 conflicts: conflicts with "catalog" using apiextensions.k8s.io/v1:
secret-operator        - .spec.versions
secret-operator        - .spec.conversion.strategy: Conflict (ErrorResponse { status: "Failure", message: "Apply failed with 2 conflicts: conflicts with \"
catalog\" using apiextensions.k8s.io/v1:\n- .spec.versions\n- .spec.conversion.strategy", reason: "Conflict", code: 409 })
secret-operator     3: Apply failed with 2 conflicts: conflicts with "catalog" using apiextensions.k8s.io/v1:
secret-operator        - .spec.versions
secret-operator        - .spec.conversion.strategy: Conflict
stream closed EOF for stackable-operators/secret-operator-daemonset-zvvdt (secret-operator)

No idea if this is expected or a bug.

My 2 cents: I don't know why you decided to take CRD management out of the hands of package managers because I don't remember discussing it and I find no explanation. But I think one implication of this is that we now have to implement Helm ourselves by having to ensure object creation order at least.

@razvan
Copy link
Member

razvan commented Sep 19, 2025

Tests on OKD are 🟢 though no test was added or updated for this PR.

I removed tls from the OLM manifests and installed it manually after the operator installation.

Then I ran the test suite:

❯ ./scripts/run-tests --skip-operator secret --test-suite openshift
...
--- PASS: kuttl (217.55s)
    --- PASS: kuttl/harness (0.00s)
        --- PASS: kuttl/harness/listener_openshift-true (78.90s)
        --- PASS: kuttl/harness/tls_openshift-true_rsa-key-length-2048_custom-secret-names-True (90.95s)
        --- PASS: kuttl/harness/tls_openshift-true_rsa-key-length-3072_custom-secret-names-True (47.89s)
        --- PASS: kuttl/harness/tls_openshift-true_rsa-key-length-3072_custom-secret-names-False (29.27s)
        --- PASS: kuttl/harness/tls-truststore_openshift-true_truststore-target-kind-Secret (11.55s)
        --- PASS: kuttl/harness/kerberos_krb5-1.21.1_openshift-true (80.06s)
        --- PASS: kuttl/harness/tls-truststore_openshift-true_truststore-target-kind-ConfigMap (10.13s)
        --- PASS: kuttl/harness/tls_openshift-true_rsa-key-length-2048_custom-secret-names-False (26.14s)
        --- PASS: kuttl/harness/cert-manager-tls_openshift-true (35.56s)
PASS

This implements are very simple CR applier. This commit is a first step
towards maintaining CRs in our operator. It is not refined and is missing
various features to be usable in the real world.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Development: In Review
Development

Successfully merging this pull request may close these issues.

3 participants