Implement tls support for the app #6007

carlesarnal · 2025-02-27T11:00:12Z

No description provided.

Frawless · 2025-02-27T15:11:56Z

operator/controller/src/main/java/io/apicurio/registry/operator/feat/TLS.java

+        if (truststore.isValid() && truststorePassword.isValid()) {
+            // ===== Truststore
+            truststore.applySecretVolume(deployment, containerName);
+            addEnvVar(env, QUARKUS_TLS_TRUST_STORE_P12_PATH, truststore.getSecretVolumeKeyPath());
+            truststorePassword.applySecretEnvVar(env, QUARKUS_TLS_TRUST_STORE_P12_PASSWORD);
+        }


what if I will use just PEM cert as truststore? Like

kind: ApicurioRegistry3 apiVersion: registry.apicur.io/v1 metadata: name: schema-registry spec: app: storage: type: "kafkasql" kafkasql: bootstrapServers: anubis-kafka-bootstrap.strimzi-kafka.svc:9093 tls: truststoreSecretRef: name: anubis-cluster-ca-cert key: ca.crt

wouldn't it cause failure? I would more expect that keystore password will be required

At the operator level we're supporting P12 (since it's usually a better practice than just specifying cert by cert). You will still be able to use PEM files using Quarkus own env vars.

I see, thanks.

Frawless · 2025-02-27T15:12:20Z

operator/controller/src/main/java/io/apicurio/registry/operator/feat/TLS.java

+import java.util.Map;
+import java.util.Optional;
+
+import static io.apicurio.registry.operator.EnvironmentVariables.*;


Just a nit, but * is not much good practice form my pov (different projects, different standards) :)

jsenko

Looks good, but have some questions. Some of the comments are optional/can be done later.

jsenko · 2025-03-05T09:47:03Z

operator/controller/src/main/java/io/apicurio/registry/operator/feat/TLS.java

+                .map(ApicurioRegistry3Spec::getApp)
+                .map(AppSpec::getTls)
+                .map(TLSSpec::getInsecureRequests)
+                .orElse("enabled"));


Can we use boolean for insecureRequests?

No, the possible values are enabled, disabled and redirect, so I don't think boolean makes sense here.

jsenko · 2025-03-05T09:52:03Z

operator/controller/src/main/java/io/apicurio/registry/operator/resource/ResourceFactory.java

+                .map(ApicurioRegistry3Spec::getApp)
+                .map(AppSpec::getTls);
+
+        if (tlsSpec.isPresent()) {


Assuming TLS is enabled only by the presence of the tls object in the CR seems like it could cause user errors. I suggest we also check that the truststore/keystore is valid via io.apicurio.registry.operator.utils.SecretKeyRefTool#isValid before we assume user wants to enable TLS. Could be done by a static method in io.apicurio.registry.operator.feat.TLS.

True, but, if it's misconfigured, the application won't even start, which, if you tried to configure TLS, might be even better than a "healthy" application and then having to look at the logs, wdyt?

jsenko · 2025-03-05T09:53:33Z

operator/controller/src/main/java/io/apicurio/registry/operator/resource/ResourceFactory.java

@@ -56,6 +57,19 @@ public Deployment getDefaultAppDeployment(ApicurioRegistry3 primary) {
                        .map(AppSpec::getReplicas).orElse(DEFAULT_REPLICAS),
                ofNullable(primary.getSpec()).map(ApicurioRegistry3Spec::getApp)
                        .map(AppSpec::getPodTemplateSpec).orElse(null)); // TODO:
+
+        var readinessProbe = new ProbeBuilder().withHttpGet(new HTTPGetActionBuilder().withPath("/health/ready").withPort(new IntOrString(8080)).withScheme("HTTP").build()).build();
+        var livenessProbe = new ProbeBuilder().withHttpGet(new HTTPGetActionBuilder().withPath("/health/live").withPort(new IntOrString(8080)).withScheme("HTTP").build()).build();


Can you merge the non-tls probes above into io.apicurio.registry.operator.Constants as well? Some of the values in that class are unused and will need to be removed, but now it's a bit confusing.

jsenko · 2025-03-05T09:55:46Z

operator/controller/src/main/java/io/apicurio/registry/operator/EnvironmentVariables.java

+    public static final String QUARKUS_TLS_KEY_STORE_P12_PASSWORD = "QUARKUS_TLS_KEY_STORE_P12_PASSWORD";
+    public static final String QUARKUS_TLS_TRUST_STORE_P12_PATH = "QUARKUS_TLS_TRUST_STORE_P12_PATH";
+    public static final String QUARKUS_TLS_TRUST_STORE_P12_PASSWORD = "QUARKUS_TLS_TRUST_STORE_P12_PASSWORD";
+    public static final String QUARKUS_OIDC_TLS_TLS_CONFIGURATION_NAME = "QUARKUS_OIDC_TLS_TLS_CONFIGURATION_NAME";


Unused in this PR

jsenko · 2025-03-05T09:59:56Z

operator/controller/src/main/java/io/apicurio/registry/operator/feat/TLS.java

+
+        var keystore = new SecretKeyRefTool(getTlsSpec(primary)
+                .map(TLSSpec::getKeystoreSecretRef)
+                .orElse(null), "user.p12");


Maybe a constructor that takes Optional would be useful here. We've got this pattern elsewhere, e.g. in io.apicurio.registry.operator.feat.KafkaSqlTLS. Optional suggestion, could be done in a separate PR.

I don't have a strong opinion, I was just following the pattern from KafkaSqlTLS. I'll let you decide if you want to do it in a separate PR.

jsenko · 2025-03-05T10:18:44Z

operator/controller/src/main/java/io/apicurio/registry/operator/resource/ResourceFactory.java

@@ -64,11 +78,12 @@ public Deployment getDefaultAppDeployment(ApicurioRegistry3 primary) {
                REGISTRY_APP_CONTAINER_NAME,
                Configuration.getAppImage(),
                List.of(new ContainerPortBuilder().withName("http").withProtocol("TCP").withContainerPort(8080).build()),


Do we set the 8443 port in the container? The probes will succeed even if the connection can't be actually established. Do we make an actual connection in the tests?

This is a complete miss on my part, we must add the container to the port. No real connection is made in the tests, but that's expected, we only check for the needed resources to be created and for the deployment to become ready.

jsenko · 2025-03-05T10:20:59Z

operator/controller/src/test/java/io/apicurio/registry/operator/it/TlsITTest.java

+import static org.awaitility.Awaitility.await;
+
+@QuarkusTest
+public class TlsITTest extends ITBase {


We should ping the REST API in the tests, port-forwarding should be enough since we do not support yet create HTTPS Ingresses.

If https is not properly configured at the application level, the pod won't even start, so I think checking whether the deployment is ready or not should be enough.

jsenko · 2025-03-05T10:21:42Z

operator/controller/src/test/java/io/apicurio/registry/operator/it/TlsITTest.java

+        await().ignoreExceptions().until(() -> {
+            assertThat(client.network().v1().networkPolicies().inNamespace(namespace)
+                    .withName("simple-app-networkpolicy").get().getSpec().getIngress()
+                    .get(0).getPorts().get(0).getPort().getIntVal()).isEqualTo(8443);


Also check for other ports.

jsenko · 2025-03-05T10:27:32Z

operator/controller/src/test/java/io/apicurio/registry/operator/it/TlsITTest.java

+            return true;
+        });
+
+        // Ingresses


Since we do not create https Ingresses automatically, this check is probably unnecessary?

It is, yes.

jsenko · 2025-03-05T10:30:55Z

...troller/src/test/resources/k8s/examples/tls/insecure-traffic-with_tls.apicurioregistry3.yaml

+      host: simple-ui.apps.cluster.example
+    env:
+      - name: REGISTRY_API_URL
+        value: https://simple-app.apps.cluster.example/apis/registry/v3


I think we should add a note to this example that the feature is intended for TLS inside of the cluster for now, and they would have to create their own https reencrypt Ingress.

apicurio-bot bot added the triage/needs-triage label Feb 27, 2025

Frawless reviewed Feb 27, 2025

View reviewed changes

carlesarnal force-pushed the tls-support branch from 50efb10 to b027922 Compare March 3, 2025 08:59

carlesarnal assigned jsenko Mar 3, 2025

Implement tls support for the app

2f29977

carlesarnal force-pushed the tls-support branch 2 times, most recently from 6e93999 to 7fadf42 Compare March 4, 2025 12:27

carlesarnal requested a review from jsenko March 4, 2025 12:30

Fix auth tests

0cfa62a

carlesarnal force-pushed the tls-support branch from 7fadf42 to 0cfa62a Compare March 4, 2025 14:06

jsenko requested changes Mar 5, 2025

View reviewed changes

jsenko removed the triage/needs-triage label Mar 5, 2025

Address pr feedback

00157d1

carlesarnal force-pushed the tls-support branch from 26383ba to 00157d1 Compare March 6, 2025 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement tls support for the app #6007

Implement tls support for the app #6007

carlesarnal commented Feb 27, 2025

Frawless Feb 27, 2025

carlesarnal Feb 27, 2025

Frawless Feb 27, 2025

Frawless Feb 27, 2025

jsenko left a comment

jsenko Mar 5, 2025

carlesarnal Mar 6, 2025

jsenko Mar 5, 2025

carlesarnal Mar 6, 2025

jsenko Mar 5, 2025

jsenko Mar 5, 2025

jsenko Mar 5, 2025

carlesarnal Mar 6, 2025

jsenko Mar 5, 2025

carlesarnal Mar 6, 2025

jsenko Mar 5, 2025

carlesarnal Mar 6, 2025

jsenko Mar 5, 2025

jsenko Mar 5, 2025

carlesarnal Mar 6, 2025

jsenko Mar 5, 2025

Implement tls support for the app #6007

Are you sure you want to change the base?

Implement tls support for the app #6007

Conversation

carlesarnal commented Feb 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment