[sbernauer]

Part of #849

Needs stackabletech/operator-rs#1293

Description

This PR allows users to connect Trino to a Iceberg REST catalog via the CRD.
As it's a breaking change (moving to a complex enum), we introduce v1alpha2 for this.

CRD change

See the extra/crds.yaml change. It's a bit hard to spot the diff because of the addition of v1alpha2.

Usage examples

Up to date examples are in the PR, but some examples:

- connector:
    iceberg:
      catalog:
        hiveMetastore:
          configMap: simple-hive
- connector:
    iceberg:
      catalog:
        rest:
          hostname: my.rest.com
- connector:
    iceberg:
      catalog:
        rest:
          hostname: my.secure.rest
          tls:
            verification:
              server:
                caCert:
                  secretClass: tls
          authentication:
            oAuth2:
              server:
                hostname: keycloak.default.svc.cluster.local
                port: 8443
                path: /realms/test/protocol/openid-connect/token
                tls:
                  verification:
                    server:
                      caCert:
                        secretClass: tls
              clientCredentials:
                secretName: my-keycloak-credentials
                scopes:
                  - lakehouse
                  - read
              # OR
              token:
                secretName: my-keycloak-token
- connector:
    iceberg:
      catalog:
        userProvided: {}

Considered alternatives

Take URI as URI

- connector:
    iceberg:
      catalog:
        rest:
          uri: https://my.secure.rest
          authentication:
            oAuth2:
              serverUri: https://keycloak.default.svc.cluster.local:8443/realms/test/protocol/openid-connect/token
              tokenSecretName: my-keycloak-token

Pros:

• Not so lengthy
• Less complex

Cons:

• Missing consistency
• No way to specify TLS verification

Definition of Done Checklist

• Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
• Please make sure all these things are done and tick the boxes

Author

• Changes are OpenShift compatible
• CRD changes approved
• CRD documentation for all fields, following the style guide.
• Helm chart can be installed and deployed operator works
• Integration tests passed (for non trivial changes)
• Changes need to be "offline" compatible
• Links to generated (nightly) docs added
• Release note snippet added

Reviewer

• Code contains useful comments
• Code contains useful logging statements
• (Integration-)Test cases added
• Documentation added or updated. Follows the style guide.
• Changelog updated
• Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

• Feature Tracker has been updated
• Proper release label has been added
• Links to generated (nightly) docs added
• Release note snippet added
• Add type/deprecation label & add to the deprecation schedule
• Add type/experimental label & add to the experimental features tracker

Release notes

• The Iceberg connector can now connect to an Apache Iceberg REST catalog via the new spec.connector.iceberg.catalog.rest field of the TrinoCatalog.
  Trino can optionally authenticate against the REST catalog using OAuth2, with either client credentials or a static bearer token read from a Secret.
  Connections use TLS with webPki verification by default, and custom CA certificates can be provided via a SecretClass.
  Previously, a REST catalog could only be configured using configOverrides.

  For this, TrinoCatalog version v1alpha2 was added and is now the stored version.
  It replaces the optional spec.connector.iceberg.metastore field with the required spec.connector.iceberg.catalog field, which is one of rest, hiveMetastore or userProvided.
  TrinoCatalogs can still be applied and retrieved in v1alpha1, and conversion between both versions is handled automatically by default.
  A v1alpha1 metastore becomes catalog.hiveMetastore, and a missing metastore becomes catalog.userProvided, so TrinoCatalogs that configure a REST catalog via configOverrides keep working unchanged.
  A rest catalog can't be represented in v1alpha1, so use v1alpha2 for all TrinoCatalogs that use one.

  See the Iceberg connector documentation for details.
  Implemented in trino-operator#950.

[lfrancke]

• credential.credentialSecretName doesn't read well. Can we move this up one level next to serverUri?
  clientCredentialsSecret & tokenSecret. I'm not entirely sure if that works with our enum stuff that said as I just reviewed the OpenLineageConnection I believe we do it exactly the same there so I hope it does work.
  Renaming without name because that's what we use elsewhere in the CRD and others (we also use with name though....)

[sbernauer]

All good points, thanks for your feedback!

1. Re security vs authentication

I think authentication is better, however this maps to the Trino property iceberg.rest-catalog.security (docs: The type of security to use (default: NONE). Possible values are NONE, SIGV4, GOOGLE or OAUTH2. OAUTH2 requires either a token or a credential.).
However, in such discussions in the past I was on the team of "We are a platform, we aim for consistency between products over product experts that know all Trino config properties already". Thus you convinced me, I changed it to security in b3e3e54.

2. Re credential.credentialSecretName

I was considering using an untagged enum like

security:
  oAuth2:
    serverUri: https://keycloak.default.svc.cluster.local:8443/realms/test/protocol/openid-connect/token
    credentialSecretName: my-keycloak-credentials
    # or
    tokenSecretName: my-keycloak-token

This has one level of indentation less, but allows users to configure both (credentials and token) and our operator defines what takes precedence (I'd say token?) and silently ignores the other. That's the tradeoff. I don't have a strong opinion, but I feel like in the past we tended towards less untagged enums and more "impossible to express invalid config". But as mentioned no strong opinion, would be interested in other opinions.

3. Re secret vs secretName

I personally prefer secret over secretName, however in past decisions (e.g. of mine) this was changed to include "Name". Something to do with Kubernetes conventions, and we did that for the last fields I can remember (e.g. https://github.com/stackabletech/decisions/issues/90 or generic database connection).

[razvan]

Did you consider adding a new catalog type icebergREST (naming ...) instead if mutating the existing one?

No breaking changes, no conversion problems no new CRD version needed.

[sbernauer]

I have do admit I never though of such a change.
But to be honest it feels like a hack to me.
What catalog is used (REST, glue, dynamodb, hive, jdbc or nessie) is a detail of the iceberg connector, but all of them are the same iceberg connector.
If we follow this approach we would also need a icebergUserProvidedCatalog catalog.

[sbernauer]

@lfrancke @razvan are you ok with the explanations and thus the current design?

[sbernauer]

CRD decision accepted

[sbernauer]

CRD decision update:
@Techassi had a valid point and I split the server URI to <host, port, path and tls> struct as we normally do in our platform

[sbernauer]

The new CRD change (with host, port, path and tls split out) was approved in todays daily