feat/cert_manager_crds #211

Some refactoring to do, but not too far off! Mostly use the CRDs properly and fix the return types. But also the CertificateScore shoudl rely on the capability, not the other way around.

harmony/src/domain/topology/k8s_anywhere/k8s_anywhere.rs Outdated

						
				@@ -362,0 +432,4 @@

				        cert_name: String,

				        issuer_name: String,

				        config: &CertificateManagementConfig,

				    ) -> Result<PreparationOutcome, PreparationError> {

johnride commented

2026-01-16 22:03:35 +00:00

This should not return PreparationError and PreparationOutcome.

Most of the time it won't be used as part of a preparation phase.

This should not return PreparationError and PreparationOutcome. Most of the time it won't be used as part of a preparation phase.

harmony/src/domain/topology/k8s_anywhere/k8s_anywhere.rs Outdated

						
				@@ -362,0 +440,4 @@

				        )

				        .await?;

				        let cert = CertificateScore {

johnride commented

2026-01-16 22:11:33 +00:00

I would have thought that CertificateScore depends on CertificateManagement capability, not the other way around.

The goal is to allow users to easily create certifiactes by calling CertificateScore on any Topology that implements CertificateManagement.

Or am I missing something?

I would have thought that CertificateScore depends on CertificateManagement capability, not the other way around. The goal is to allow users to easily create certifiactes by calling CertificateScore on any Topology that implements CertificateManagement. Or am I missing something?

wjro commented

2026-01-19 16:54:12 +00:00

I think this is a naming problem which makes the intent unclear.

I have CertificateCreationScore, CertificateIssuerScore, and CertificateManagementScore which depend on CertificateManagement capability, that way we can create certificates, issuers or ensure that CertificateManagement is ready on the Topology that implements the CertificateManagement trait.

The CertificateScore and IssuerScore are specific implentations for K8s that uses kubernetes CRDs which are applied as K8s resources, this is why further down the CertificateScore requires impl of K8sclient.

I modeled this off of postgresql which has a generic score.rs which calls topology.deploy() -> impl PostgreSQL for K8sAnywhereTopology -> K8sPostgreSQLScore.interpret()

I think this is a naming problem which makes the intent unclear. I have CertificateCreationScore, CertificateIssuerScore, and CertificateManagementScore which depend on CertificateManagement capability, that way we can create certificates, issuers or ensure that CertificateManagement is ready on the Topology that implements the CertificateManagement trait. The CertificateScore and IssuerScore are specific implentations for K8s that uses kubernetes CRDs which are applied as K8s resources, this is why further down the CertificateScore requires impl of K8sclient. I modeled this off of postgresql which has a generic score.rs which calls topology.deploy() -> impl PostgreSQL for K8sAnywhereTopology -> K8sPostgreSQLScore.interpret()

johnride marked this conversation as resolved

harmony/src/domain/topology/k8s_anywhere/k8s_anywhere.rs Outdated

						
				@@ -362,0 +460,4 @@

				        config: &CertificateManagementConfig,

				    ) -> Result<String, PreparationError> {

				        let namespace = config.namespace.clone().unwrap();

				        let certificate_gvk = GroupVersionKind {

johnride commented

2026-01-16 22:18:47 +00:00

You should be using the CRD here, not crafting a GVK. See how it's done with NMstate and other CRDs.

johnride marked this conversation as resolved

harmony/src/domain/topology/k8s_anywhere/k8s_anywhere.rs Outdated

						
				@@ -362,0 +485,4 @@

				        trace!("Secret Name {:#?}", secret_name);

				        let secret_name: String = serde_json::from_value(secret_name.clone())

johnride commented

2026-01-16 22:20:49 +00:00

If you were using the CRD properly here you would not have to mess with JSON, you would simply be walking the Rust struct directly.

johnride marked this conversation as resolved

harmony/src/domain/topology/k8s_anywhere/k8s_anywhere.rs Outdated

						
				@@ -381,0 +542,4 @@

				            msg: "namespace is required".to_string(),

				        })?;

				        let gvk = GroupVersionKind {

johnride commented

2026-01-16 22:21:09 +00:00

Same, use the CRD properly

harmony/src/modules/cert_manager/capability.rs Outdated

						
				@@ -0,0 +14,4 @@

				    async fn ensure_certificate_management_ready(

				        &self,

				        config: &CertificateManagementConfig,

				    ) -> Result<PreparationOutcome, PreparationError>;

johnride commented

2026-01-16 22:23:50 +00:00

It should be ensure_ready, no need for the long function name. Let's keep is standard-ish across Harmony. Eventually we will very likely extract the readiness state management in a separate trait.

johnride marked this conversation as resolved

harmony/src/modules/cert_manager/capability.rs Outdated

						
				@@ -0,0 +16,4 @@

				        config: &CertificateManagementConfig,

				    ) -> Result<PreparationOutcome, PreparationError>;

				    async fn create_issuer(

johnride commented

2026-01-16 22:24:50 +00:00

Change the return types. This is something that will require cleanup across harmony eventually. But the semantic is incorrect here. This trait is not scoped in preparation.

johnride marked this conversation as resolved

johnride reviewed 2026-01-16 22:31:41 +00:00

harmony/src/modules/cert_manager/crd/score_certificate.rs Outdated

						
				@@ -0,0 +21,4 @@

				    pub config: CertificateManagementConfig,

				}

				impl<T: Topology + K8sclient> Score<T> for CertificateScore {

johnride commented

2026-01-16 22:31:40 +00:00

We want to be able to create certificates on anything implementing CertificateManagement, not just kubernetes.

This should be

impl<T: Topology + CertificateManagement> Score<T> for CertificateScore {...}

impl<T: Topology + CertificateManagement> Interpret<T> for CertificateInterpret {
  async fn execute(topology: &T, ...) {
     topology.create_certificate(self.score.certificate).await?;
  }
}

We want to be able to create certificates on anything implementing CertificateManagement, not just kubernetes. This should be ```rust impl<T: Topology + CertificateManagement> Score<T> for CertificateScore {...} impl<T: Topology + CertificateManagement> Interpret<T> for CertificateInterpret { async fn execute(topology: &T, ...) { topology.create_certificate(self.score.certificate).await?; } } ```

wjro added 1 commit 2026-01-19 17:48:55 +00:00

fix: modified score names for better clarity

Run Check Script / check (pull_request) Successful in 1m29s

Details

54a53fa982

jvtrudel commented

2026-01-19 18:00:44 +00:00

@wjro Could you describe me how to test it (manually). I'll try to.

wjro commented

2026-01-19 20:43:53 +00:00

@jvtrudel I used the example/cert-manager to install, create an issuer and create a certificate. the trait also has a get_ca_cert function which you can call via topology.get_ca_cert()

johnride added 1 commit 2026-01-20 17:11:53 +00:00

lint: Remove useless variable assignment

Run Check Script / check (pull_request) Successful in 1m27s

Details

c3ac0bafad

johnride requested changes 2026-01-20 17:12:30 +00:00

examples/cert_manager/src/main.rs Outdated

						
				@@ -0,0 +16,4 @@

				        self_signed: true,

				    };

				    let cert_manager = CertificateManagementScore {

johnride commented

2026-01-20 16:39:03 +00:00

Not the place

harmony/src/domain/topology/k8s.rs Outdated

						
				@@ -239,2 +234,3 @@

				        resource.get(name).await

				        // 1. Try namespaced first (if a namespace was provided)

				        if let Some(ns) = namespace {

johnride commented

2026-01-20 16:43:17 +00:00

else -> default namespace

wjro added 1 commit 2026-01-20 18:23:48 +00:00

fix: moved cert management ensure ready to k8sanywhere

Run Check Script / check (pull_request) Failing after 26s

Details

ab33aba776

wjro added 1 commit 2026-01-20 18:43:57 +00:00

fix: mod.rs

Run Check Script / check (pull_request) Successful in 1m21s

Details

1e98100ed4

wjro merged commit 2dc65531c3 into master

2026-01-20 18:46:21 +00:00

wjro deleted branch feat/cert_manager_crds

2026-01-20 18:46:22 +00:00

wjro referenced this issue from a commit

2026-01-20 18:46:22 +00:00

Merge pull request 'feat/cert_manager_crds' (#211) from feat/cert_manager_crds into master

Sign in to join this conversation.

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: NationTech/harmony#211