[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

[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]

It tried it with two variants and the API server rejects it

I wasn't aware of that, TIL! I also tested it and it's the perfect solution, so I went with that in 1b9f0a8

[lfrancke]

I learned it from Razvan on the OpenLineage thing :)

[lfrancke]

I have not looked at the code but CRD wise I'm fine now.

[sbernauer]

Perfect, thanks for the feedback! I'm also happy :)
Please vote on this comment

[sbernauer]

note to myself: Check if we want to use host, port and tls field similar to https://docs.stackable.tech/home/stable/concepts/authentication/#OIDC

[sbernauer]

CRD decision accepted