GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — ADRXXX: Service/Product Discovery (Revision) by sbernauer · Pull Request #442 · stackabletech/documentation

Conversation

@sbernauer

@sbernauer sbernauer commented Aug 24, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Continued on on-site 2023-08-23

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
sbernauer added 6 commits March 30, 2023 14:07
@sbernauer
Initial version
14128de
@sbernauer
Add some options
5a1c80d
@sbernauer
WIP
a4d160c
@sbernauer
WIP
d3b6fdd
@sbernauer
WIP
f2158cf
@sbernauer
WIP
f9dbbfa
@netlify

netlify Bot commented Aug 24, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

✅ 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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sbernauer
sbernauer marked this pull request as draft August 24, 2023 08:51
Techassi added 3 commits August 25, 2023 14:02
@Techassi
Add contributors
806a4a8
@Techassi
Adjust formatting
70b9f12
@Techassi
More formatting
b4bab29
@Techassi Techassi assigned soenkeliebau, Techassi, maltesander and sbernauer Aug 25, 2023
@Techassi
More polish
22737cb
@Techassi Techassi changed the title ADR on Discovery 2.0 ADRXXX: Service/Product Discovery (Revision) Aug 25, 2023
@Techassi
Start adding content for discovery operator
00450c4
@nightkr

nightkr commented Aug 29, 2023

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fhennig

fhennig commented Aug 30, 2023

Copy link
Copy Markdown
Contributor

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nightkr

nightkr commented Aug 30, 2023

Copy link
Copy Markdown
Contributor

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).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fhennig

fhennig commented Aug 30, 2023

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@maltesander

maltesander commented Aug 30, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@soenkeliebau

soenkeliebau commented Sep 8, 2023

Copy link
Copy Markdown
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?

maltesander, fhennig, and Techassi reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@soenkeliebau soenkeliebau

@Techassi Techassi

@maltesander maltesander

@sbernauer sbernauer

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@sbernauer @nightkr @fhennig @maltesander @soenkeliebau @Techassi
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.