Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADRXXX: Service/Product Discovery (Revision) #442

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Conversation

sbernauer
Copy link
Member

@sbernauer sbernauer commented Aug 24, 2023

Continued on on-site 2023-08-23

@netlify
Copy link

netlify bot commented Aug 24, 2023

Deploy Preview for stackable-docs ready!

Name Link
🔨 Latest commit 00450c4
🔍 Latest deploy log https://app.netlify.com/sites/stackable-docs/deploys/64e8c21470b407000860a661
😎 Deploy Preview https://deploy-preview-442--stackable-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sbernauer sbernauer marked this pull request as draft August 24, 2023 08:51
@Techassi Techassi changed the title ADR on Discovery 2.0 ADRXXX: Service/Product Discovery (Revision) Aug 25, 2023
@nightkr
Copy link
Member

nightkr commented Aug 29, 2023

I would disagree with the entire premise of this.

  • Discovery option 3: who would still be using the HdfsClusterDiscovery? They'd still be subject to the same versioning nightmare of 2.
  • Discovery options 2/3: moving more types into op-rs adds a lot of overhead when changing them (especially since this still needs to be coordinated with the owning operators).
  • TLS/authnclass: Having to read this from the discovery objects adds a lot of complexity and overhead to the consuming operators. Especially when the expected use case is that the whole cluster exists in the same TLS PKI/Kerberos realm. Overriding the secretclasses should be the exceptional case.
  • TLS/authn: Negotiating the precise authentication method is largely up to the product in question. I'd expect it to be more along the lines of the client specifying "when using this method, get the credential this way". Knowing that passwords are validated against LDAP is useless for the client, they just want a valid username/password.
  • TLS/authn 2: There is no way to exhaustively know the type of credential that a SecretClass provisions.

@fhennig
Copy link
Contributor

fhennig commented Aug 30, 2023

@nightkr What do you mean with "I would disagree with the entire premise of this." What's the premise?

I also read it now, and wanted to share some thoughts too. I think the ease of use of the ConfigMap is actually really nice. I think most discovery configmaps contain connect strings or files, and those can then either be mounted as an env var or as files, so it's very easy to use.

The document mentions that cross-namespace is an issue with ConfigMaps, and also name clashes. If these are problems, I'd like to see these aspects mentioned as decision drivers in the earlier section of the ADR. I personally do not think that these two things are very important.

@nightkr
Copy link
Member

nightkr commented Aug 30, 2023

What's the premise?

The primary driver for this (motivations 2/3) seem to be authentication. I disagree that discovery is the right place to tackle either.

The document mentions that cross-namespace is an issue with ConfigMaps, and also name clashes. If these are problems, I'd like to see these aspects mentioned as decision drivers in the earlier section of the ADR. I personally do not think that these two things are very important.

Agreed that both of these are effectively treated as decision drivers looking at the rest of the document. I'd also disagree with the document about discovery being the right place to tackle either of those either.

Cross-namespace use: Kubernetes namespaces are effectively a tenancy border, we should be very careful blurring that line. To me, cross-namespace use should be something you request explicitly, and which is ideally isolated from other "tenants" (ZookeeperZnode ticks both of these boxes, but isolation is easier done for a storage system like ZK or HDFS than something like Trino).

@fhennig
Copy link
Contributor

fhennig commented Aug 30, 2023

Hmm, can you elaborate why the discovery mechanism shouldn't also contain information on the authentication mechanism? It makes sense to me, it's information that a client would need when connecting to the service.

@maltesander
Copy link
Member

Cross-namespace use: Kubernetes namespaces are effectively a tenancy border, we should be very careful blurring that line. To me, cross-namespace use should be something you request explicitly, and which is ideally isolated from other "tenants" (ZookeeperZnode ticks both of these boxes, but isolation is easier done for a storage system like ZK or HDFS than something like Trino).

The discovery-operator proposal is partly based on the ZNode logic. E.g. for deploying / mounting configmaps across namespaces.

@soenkeliebau
Copy link
Member

It sounds to me like this is worth an online debate?
Would everybody be happy for me to organize and prepare a meeting to discuss?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants