More formatting

Author: Techassi

modules/contributor/pages/adr/ADR028-discovery-revision.adoc

@@ -1,4 +1,4 @@
-= ADR028: Discovery revision
+= ADR028: Service Discovery (Revision)
 Sebastian Bernauer <sebastian.bernauer@stackable.tech>
 v0.1, 2023-03-30
 :status: draft
@@ -68,9 +68,15 @@ We have some common use-cases that we need to express via the discovery mechanis
 
 == Considered Options
 
-=== [1] Discovery Object: Use ConfigMap
+=== Discovery Object
 
-Use a ConfigMap:
+This Discovery Object is written by the Product Operator. It describes how the product can be accessed by other
+connecting entities (clients). These clients can be inside the same Kubernetes cluster, in an external Kubernetes
+cluster or deployed using a different approach like running inside a VM or on bare metal hardware. The discovery object
+is used both internally by the SDP and by other actors (for example users). Each Discovery Object is created in the same
+namespace the product runs in. This means cross-namespace discovery needs special attention.
+
+==== Discovery Object: Option 1 - ConfigMap
 
 [source,yaml]
 ----
@@ -94,22 +100,20 @@ spec:
     authenticationSecretClass: client-kerberos
 ----
 
-==== Pros
+===== Pros
 
 * Easier to use for consuming applications outside of Stackable, as they can simply mount the CM or download it to a
   file. On the other hand when we start to put in yaml objects that need to be parsed we would loose the benefit.
 * Single API call to retrieve all running products
 
-==== Cons
+===== Cons
 
 * No complex structure such as enums (we can mitigate this by sticking in custom yaml into the CM). Users don't have any
   form of validation when creating their own discovery CM e.g. pointing to their existing HDFS.
 * Cannot have two products with the same name, as the discovery CM name clashes. One solution could be to prefix the
   product name (e.g. trino-simple), This can impose other problems such as too long CM names.
 
-=== [1] Discovery Object: Use dedicated CRD object for every product
-
-Or use a dedicated HdfsClusterDiscovery crd:
+==== Discovery Object: Option 2 - Dedicated CRD Object for every Product
 
 [source,yaml]
 ----
@@ -133,30 +137,28 @@ spec:
       secretClass: client-tls # Use this SecretClass to obtain a keytab
 ----
 
-==== Pros
+===== Pros
 
 * Validation by using e.g. complex enums
 * Commons structure can be shared between all operators, such as `Listener` endpoints or tls server certificate
   information
 
-==== Cons
+===== Cons
 
-* Operator A needs to compile against operator b to have access to it's discovery struct. An alternative would be to put
+* Operator A needs to compile against operator B to have access to it's discovery struct. An alternative would be to put
   the Discovery CRDs in operator-rs.
 * Operator versioning hell. On the other hand we have the same problem with ConfigMaps, as e.g. a newly introduced key
   is missing because of an older hdfs operator version.
 * Dependant Pods (such as hbase on hdfs) can not simply mount a CM containing the hdfs-site and core-site. Instead the
   hbase-operator needs to read the HdfsClusterDiscovery, copy the hdfs-site and core-site into a CM and than mount that
   into the hbase Pods. This can be solved by the HdfsClusterDiscovery to point to a CM that contains hdfs-site and
-  core-site xmls.
+  core-site XML files.
 * Multiple API calls need to retrieve all running Stackable service (in stackablectl or cockpit). This would be a single
   API call in case of discovery CM or a shared CRD for all product discoveries.
 * Side-Note: `stackablectl stacklet list` should *not* look at discovery objects, as they can come from a user and are
   external systems, where we don't know anything about.
 
-=== [1] Discovery Object: Use dedicated CRD object for every product - in combination with ConfigMap
-
-Or use a dedicated HdfsClusterDiscovery crd:
+==== Discovery Object: Option 3 - Dedicated CRD Object + ConfigMap for every Product
 
 [source,yaml]
 ----
@@ -210,15 +212,13 @@ spec:
 # No CM needed
 ----
 
-==== Pros
+===== Pros
 
 * Fixes mount problem from `Discovery Object: Use dedicated CRD object for every product`
 
-==== Cons
-
-=== [1] Discovery Object: Use dedicated CRD object shared between all products
+===== Cons
 
-Or use a dedicated ClusterDiscovery crd:
+==== Discovery Object: Option 4 - Dedicated CRD Object shared between all Products
 
 [source,yaml]
 ----
@@ -245,13 +245,13 @@ spec:
   # ...
 ----
 
-==== Pros
+===== Pros
 
 * Only one struct in operator-rs => No cross-operator dependencies.
 * Single API call to retrieve all stackable products. Question is if this really helps a lot, as callers probably also
   are interested in the status of the product, which needs further API calls (irrelevant - see Cons).
 
-==== Cons
+===== Cons
 
 * All product discoveries are versioned together. E.g. a new mandatory field for hdfs requires all operators to bump the
   Discovery CRD to `v2`. We hope that this does not happen too often.
@@ -260,25 +260,30 @@ spec:
   systems, where we don't know anything about. So in case we want to introduce a `Stacklet` object listing anyway, so
   the `Pro` regarding the API calls is irrelevant.
 
-=== [1] Discovery Object: Write the discovery to Product CR status
+==== Discovery Object: Option 5 - Write Discovery to Product CR Status
 
 Instead of writing discovery information to dedicated objects - such as CM or custom CR - we "simply" write the
 discovery information to the status of the Cluster CR.
 
-==== Pros
+===== Pros
 
-==== Cons
+* None currently
+
+===== Cons
 
 * It does not enable users to bring their own product and talk to it from Stackable, e.g. a user-provided HDFS.
 * It does not allow things such as a ZNode for Zookeeper as we either use the Zookeeper CR for discovery or we use a
   ZNode but than can't use a Zookeeper CR. Currently we have the freedom of either connection to a Zookeeper root dir or
   a ZNode transparently.
 
-=== [2] TLS: Discovery config contains SecretClass
+'''
 
-The discovery includes the SecretClass used to obtain the ca.crt used to validate the *server* certificate
+=== TLS
+
+==== TLS: Option 1 - Discovery Config contains SecretClass
+
+The discovery includes the SecretClass used to obtain the ca.crt used to validate the *server* certificate.
 
-Trino discovery:
 [source,yaml]
 ----
 apiVersion: trino.stackable.tech/v1alpha1
@@ -305,18 +310,20 @@ backends: # Don't look at the Superset CRD structure, we are only interested in
       discovery: my-trino
 ----
 
-==== Pros
+===== Pros
 
-==== Cons
+* Currently none
 
-=== [2] TLS: Client needs to specify SecretClass
----
+===== Cons
+
+* Currently none
+
+==== TLS: Option 2 - Client specifies SecretClass
 
 The discovery does *not* include the SecretClass used to obtain the *server* certificate. Instead the client must
 specify which SecretClass should be used to verify the *server* certificate. For usability reasons it can be omitted and
 defaults to the SecretClass the client uses for itself.
 
-Trino discovery:
 [source,yaml]
 ----
 apiVersion: trino.stackable.tech/v1alpha1
@@ -343,22 +350,21 @@ backends: # Don't look at the Superset CRD structure, we are only interested in
       tlsSecretClass: my-second-pki
 ----
 
-==== Pros
+===== Pros
 
 * Operator does not need to read/look at the DiscoveryConfig (as we can statically set up the secret-op tls secretClass
   volumes rather than retrieving them from the DiscoveryConfig).
 * Some clients only support a single pki, in that case we could not give the ability to overwrite the secretClass coming
   from the product itself.
 
-==== Cons
+===== Cons
 
 * The client has to know what pki/secretClass the server is using.
 * Superset TrinoConnection could not only say "Connect this Superset and this Trino", but need to say "using this ca.crt
   to validate the Trino server"
 
-=== [2] TLS: Include caCert in Discovery config
+==== TLS: Option 3 - Include caCert in Discovery Config
 
-Trino discovery:
 [source,yaml]
 ----
 metadata:
@@ -376,23 +382,21 @@ endpoint:
         === END CERTIFICATE ===
 ----
 
-==== Pros
+===== Pros
 
 * Easier for external clients to use as they don't need to know the concept of SecretClasses and don't even need to run
   withing k8s.
 * The client has to *not* know what pki/secretClass the server is using.
 
-==== Cons
+===== Cons
 
-* BIG QUESTION: How should the product operator get the ca cert from the SecretClass it uses to get the *server* cert
+* BIG QUESTION: How should the product operator get the CA cert from the SecretClass it uses to get the *server* cert
   from?
 ** The secret-op could e.g. offer an HTTP api to fetch the ca.crt of a given SecretClass or e.g. write the ca.crt into
    the status of a SecretClass
 
 
-=== [2] TLS: Include SecretClass in discovery, user can override it
-
-Trino discovery:
+==== TLS: Option 4 - Include SecretClass in Discovery (User can override it)
 
 [source,yaml]
 ----
@@ -422,17 +426,20 @@ backends: # Don't look at the Superset CRD structure, we are only interested in
       tlsSecretClass: my-second-pki
 ----
 
-==== Pros
+===== Pros
 
 * Compromise with all usability and flexibility
 
-==== Cons
+===== Cons
 
 * Less secure by default
 
-=== [3] Authentication: Add AuthenticationClass to Discovery Config
+'''
+
+=== Authentication
+
+==== Authentication: Option 1 - Add AuthenticationClass to Discovery Config
 
-Trino discovery:
 [source,yaml]
 ----
 metadata:
@@ -441,23 +448,23 @@ authentication:
   authenticationClass: my-class
 ----
 
-==== Pros
+===== Pros
+
 * IMPORTANT: This is the only thing the server can know (how he is verifying client identities). He can not recommend an
   SecretClass used to obtain the client credentials. E.g. he uses an LDAP AuthenticationClass, there is no way it can
   now what SecretClass provides credentials accepted by LDAP. (Most cases it will be a user logging into a WebUI and the
   LDAP credentials of the user are not even stored anywhere but just remembered by the user)
 
-==== Cons
+===== Cons
 
 * Operator has to read the AuthenticationClass to determine its type (pw/tls/keytab) and set up the needed volumes and
   commands.
 // * The AuthenticationClass is meant to describe "how should a server verify connecting clients" and re-purpose it to 
 //   mean "how a client should authenticate itself". Image a user creates a Secret `trino-users` with *only* a ca.crt
 //   and a SecretClass `trino-users` on top. The connecting client than has no way of knowing how to get a client cert.
 
-=== [3] Authentication: Add SecretClass to Discovery Config
+==== Authentication: Option 2 - Add SecretClass to Discovery Config
 
-Trino discovery:
 [source,yaml]
 ----
 metadata:
@@ -466,13 +473,17 @@ authentication:
   secretClass: client-tls # Use this SecretClass to obtain your credentials (regardless of type of SecretClass)
 ----
 
-==== Cons
+===== Pros
+
+* Currently none
+
+===== Cons
 
 * Operator has to read the SecretClass to determine its type (pw/tls/keytab) and set up the needed volumes and commands.
 * Image then SecretClass is of type `k8sSearch`. The connection client (e.g. controlled via superset-operator) than has
   no idea if he should expect a tls.crd or a keytab when mounting the SecretClass.
 
-=== [3] Authentication: Add needed details
+==== Authentication: Option 3 - Add needed Details
 
 Trino discovery:
 [source,yaml]
@@ -488,27 +499,33 @@ authentication:
     secretClass: client-kerberos # Use this SecretClass to obtain a keytab
 ----
 
-==== Pros
+===== Pros
 
 * Operator has *not* to read the SecretClass to determine its type (pw/tls/keytab), as the type is already encoded in
   the Discovery config.
 
-==== Cons
+===== Cons
 
-=== [3] Authentication: Don't add information how to authenticate
+* Currently none
+
+==== Authentication: Option 4 - Provide no Authentication Information
 
 Trino discovery does not provide any information on how to authenticate
 
-==== Cons
+===== Pros
+
+* Currently none
+
+===== Cons
 
 * Not viable, as users need to know how to connect, and are not expected to try 50 different auth methods. We need to
   give them a AuthenticationClass, that says them e.g. what LDAP or PKI is used.
 
 == Decision Outcome
 
-[1] Discovery Object: `Discovery Object: Use dedicated CRD object for every product - in combination with ConfigMap`
-[2] Server tls cert: TODO
-[3] Authentication: `Authentication: Add AuthenticationClass to Discovery Config`
+* *Discovery Object:* Option 3
+* *Server TLS:* TBD, but leaning towards Option 2
+* *Authentication:* Option 1
 
 === Appendix A