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

feat: Add support for Trino 470 #705

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6202d19
test(integration): Add Trino 469, update latest version
NickLarsenNZ Feb 5, 2025
f4ca614
docs: Update trino version to 469
NickLarsenNZ Feb 5, 2025
d3de6b5
feat: Add support for Trino 469
NickLarsenNZ Feb 5, 2025
bc1b03b
test(unit): Update trino version used in tests
NickLarsenNZ Feb 5, 2025
94f3159
chore: regenerate Cargo.nix
NickLarsenNZ Feb 5, 2025
9a695e3
chore: Update changelog
NickLarsenNZ Feb 5, 2025
83ceb12
wip: native s3 initial cut
NickLarsenNZ Feb 6, 2025
72a5573
wip: more versiony stuff
NickLarsenNZ Feb 6, 2025
30240b4
wip: parse Trino version as u16 version from String
NickLarsenNZ Feb 6, 2025
5b6b45a
wip: remove comment and restore newline
NickLarsenNZ Feb 6, 2025
46b26f1
feat: Replace 469 with 470
NickLarsenNZ Feb 10, 2025
6983f3f
test: Trino now requires TLS for S3
NickLarsenNZ Feb 10, 2025
dbf9409
chore: Search and replace Trino productVersions
NickLarsenNZ Feb 12, 2025
d74ba65
chore: Rename trino_version to product_version for consistency
NickLarsenNZ Feb 12, 2025
2640426
refactor: Change product_version in the jvm mod to u16 for consistency
NickLarsenNZ Feb 12, 2025
661fead
chore: Improve note about native s3 support
NickLarsenNZ Feb 12, 2025
10c0bd8
chore: Update changelog
NickLarsenNZ Feb 12, 2025
0953c0a
chore: Update changelog
NickLarsenNZ Feb 12, 2025
4753151
chore: Add warning about upcoming requirement for TLS requirement for…
NickLarsenNZ Feb 12, 2025
f868c45
refactor: Use ensure! instead of if not
NickLarsenNZ Feb 12, 2025
81fe1be
wip!: Hard-code s3.region to us-east-1 until is can be configured by …
NickLarsenNZ Feb 12, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,21 @@ All notable changes to this project will be documented in this file.
- Run a `containerdebug` process in the background of each Trino container to collect debugging information ([#687]).
- Support configuring JVM arguments ([#677]).
- Aggregate emitted Kubernetes events on the CustomResources ([#677]).
- Support for Trino 469 ([#705]).

## Changed

- Increased the default temporary secret lifetime for coordinators from 1 day to 15 days.
This is because Trino currently does not offer a HA setup for them, a restart kills all running queries ([#694]).
- Default to OCI for image metadata and product image selection ([#695]).
- Set `fs.hadoop.enabled=true` catalog config property to enable Legacy S3 support ([#705]).
sbernauer marked this conversation as resolved.
Show resolved Hide resolved

[#676]: https://github.com/stackabletech/trino-operator/pull/676
[#677]: https://github.com/stackabletech/trino-operator/pull/677
[#687]: https://github.com/stackabletech/trino-operator/pull/687
[#694]: https://github.com/stackabletech/trino-operator/pull/694
[#695]: https://github.com/stackabletech/trino-operator/pull/695
[#705]: https://github.com/stackabletech/trino-operator/pull/705

## [24.11.1] - 2025-01-10

Expand Down
8 changes: 4 additions & 4 deletions Cargo.nix

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ metadata:
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector:
matchLabels:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ metadata:
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector:
matchLabels:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ metadata:
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector:
matchLabels:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ metadata:
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
tls:
internalSecretClass: trino-internal-tls # <1>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ metadata:
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
tls:
serverSecretClass: trino-tls # <1>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ metadata:
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
tls:
serverSecretClass: trino-tls # <1>
Expand Down
2 changes: 1 addition & 1 deletion docs/modules/trino/pages/usage-guide/catalogs/index.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ metadata:
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector:
matchLabels:
Expand Down
1 change: 1 addition & 0 deletions docs/modules/trino/partials/supported-versions.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@
// This is a separate file, since it is used by both the direct Trino documentation, and the overarching
// Stackable Platform documentation.

- 469
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- 469
- 470

- 455
- 451 (LTS)
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ metadata:
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
authentication:
- authenticationClass: simple-trino-users
Expand Down
2 changes: 1 addition & 1 deletion examples/simple-trino-cluster-hive-ha-s3.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ metadata:
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector:
matchLabels:
Expand Down
2 changes: 1 addition & 1 deletion examples/simple-trino-cluster-resource-limits.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ metadata:
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector: {}
coordinators:
Expand Down
2 changes: 1 addition & 1 deletion examples/simple-trino-cluster-s3.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ metadata:
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector:
matchLabels:
Expand Down
2 changes: 1 addition & 1 deletion examples/simple-trino-cluster.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ metadata:
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector:
matchLabels:
Expand Down
2 changes: 1 addition & 1 deletion examples/simple-trino-oauth2.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ metadata:
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
authentication:
- authenticationClass: simple-trino-oidc
Expand Down
4 changes: 2 additions & 2 deletions rust/crd/src/affinity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ mod tests {
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector:
matchLabels:
Expand Down Expand Up @@ -199,7 +199,7 @@ mod tests {
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector:
matchLabels:
Expand Down
20 changes: 10 additions & 10 deletions rust/crd/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -897,7 +897,7 @@ mod tests {
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
NickLarsenNZ marked this conversation as resolved.
Show resolved Hide resolved
clusterConfig:
catalogLabelSelector: {}
"#;
Expand All @@ -912,7 +912,7 @@ mod tests {
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector: {}
tls:
Expand All @@ -929,7 +929,7 @@ mod tests {
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector: {}
tls:
Expand All @@ -947,7 +947,7 @@ mod tests {
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector: {}
tls:
Expand All @@ -967,7 +967,7 @@ mod tests {
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
NickLarsenNZ marked this conversation as resolved.
Show resolved Hide resolved
clusterConfig:
catalogLabelSelector: {}
"#;
Expand All @@ -982,7 +982,7 @@ mod tests {
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector: {}
tls:
Expand All @@ -999,7 +999,7 @@ mod tests {
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector: {}
tls:
Expand All @@ -1020,7 +1020,7 @@ mod tests {
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector: {}
"#;
Expand All @@ -1040,7 +1040,7 @@ mod tests {
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector: {}
workers:
Expand All @@ -1066,7 +1066,7 @@ mod tests {
name: simple-trino
spec:
image:
productVersion: "455"
productVersion: "469"
clusterConfig:
catalogLabelSelector: {}
workers:
Expand Down
1 change: 1 addition & 0 deletions rust/operator-binary/src/catalog/black_hole.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ impl ToCatalogConfig for BlackHoleConnector {
catalog_name: &str,
_catalog_namespace: Option<String>,
_client: &Client,
_trino_version: u16,
) -> Result<CatalogConfig, FromTrinoCatalogError> {
// No additional properties needed
Ok(CatalogConfig::new(catalog_name.to_string(), CONNECTOR_NAME))
Expand Down
51 changes: 41 additions & 10 deletions rust/operator-binary/src/catalog/commons.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use super::{
from_trino_catalog_error::{
ConfigureS3Snafu, FailedToGetDiscoveryConfigMapDataKeySnafu,
FailedToGetDiscoveryConfigMapDataSnafu, FailedToGetDiscoveryConfigMapSnafu,
ObjectHasNoNamespaceSnafu, S3TlsNoVerificationNotSupportedSnafu,
ObjectHasNoNamespaceSnafu, S3TlsNoVerificationNotSupportedSnafu, S3TlsRequiredSnafu,
},
ExtendCatalogConfig, FromTrinoCatalogError,
};
Expand All @@ -33,6 +33,7 @@ impl ExtendCatalogConfig for MetastoreConnection {
catalog_name: &str,
catalog_namespace: Option<String>,
client: &Client,
_trino_version: u16,
) -> Result<(), FromTrinoCatalogError> {
let hive_cm: ConfigMap = client
.get(
Expand Down Expand Up @@ -79,6 +80,7 @@ impl ExtendCatalogConfig for S3ConnectionInlineOrReference {
catalog_name: &str,
catalog_namespace: Option<String>,
client: &Client,
trino_version: u16,
) -> Result<(), FromTrinoCatalogError> {
let s3 = self
.clone()
Expand All @@ -91,22 +93,50 @@ impl ExtendCatalogConfig for S3ConnectionInlineOrReference {
.await
.context(ConfigureS3Snafu)?;

catalog_config.add_property("hive.s3.endpoint", s3.endpoint().context(ConfigureS3Snafu)?);
catalog_config.add_property(
"hive.s3.path-style-access",
(s3.access_style == S3AccessStyle::Path).to_string(),
);

let (volumes, mounts) = s3.volumes_and_mounts().context(ConfigureS3Snafu)?;
catalog_config.volumes.extend(volumes);
catalog_config.volume_mounts.extend(mounts);

if trino_version >= 469 {
// Trino 469 deprecates the Legacy S3 (via the Hadoop FS interface) implementation.
// Trino 470 completely removes Legacy S3 support.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it's a bit tricky.
Can you please link to trinodb/trino#24878 in the comments?
According to it, it was deprecated in 470 and will be removed in the future.
No idea what happened before 470 that required this change ^^

Copy link
Member Author

@NickLarsenNZ NickLarsenNZ Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While they only advertise the deprecation in 470, it was functionally deprecated in 469, hence this work to get around it.

I will link that issue you mentioned.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we're jumping straight to 470, would you like me to remove the comment about 469?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that makes sense, thanks!
But I let you decide

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I think we should still make the code valid even if we aren't shipping 469. We discovered the change necessary since then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See improved comment in: 661fead

catalog_config.add_property("fs.native-s3.enabled", "true");
}

let (endpoint_prop, path_style_prop) = match trino_version {
..=468 => ("hive.s3.endpoint", "hive.s3.path-style-access"),
469.. => ("s3.endpoint", "s3.path-style-access"),
};
catalog_config.add_property(endpoint_prop, s3.endpoint().context(ConfigureS3Snafu)?);
catalog_config.add_property(
path_style_prop,
(s3.access_style == S3AccessStyle::Path).to_string(),
);

if let Some((access_key, secret_key)) = s3.credentials_mount_paths() {
catalog_config.add_env_property_from_file("hive.s3.aws-access-key", access_key);
catalog_config.add_env_property_from_file("hive.s3.aws-secret-key", secret_key);
let (access_key_prop, secret_key_prop) = match trino_version {
..=468 => ("hive.s3.aws-access-key", "hive.s3.aws-secret-key"),
469.. => ("s3.aws-access-key", "s3.aws-secret-key"),
};
catalog_config.add_env_property_from_file(access_key_prop, access_key);
catalog_config.add_env_property_from_file(secret_key_prop, secret_key);
}

catalog_config.add_property("hive.s3.ssl.enabled", s3.tls.uses_tls().to_string());
match trino_version {
// Older trino versions allowed TLS to be optional
..=468 => {
// TODO (@NickLarsenNZ): Should we warn that TLS verification is required in future versions of Trino?
sbernauer marked this conversation as resolved.
Show resolved Hide resolved
catalog_config.add_property("hive.s3.ssl.enabled", s3.tls.uses_tls().to_string());
}
// TLS is required when using native S3 implementation.
// https://trino.io/docs/469/object-storage/legacy-s3.html#migration-to-s3-file-system
469.. => {
if !s3.tls.uses_tls() {
return S3TlsRequiredSnafu.fail();
}
sbernauer marked this conversation as resolved.
Show resolved Hide resolved
}
};

if let Some(tls) = s3.tls.tls.as_ref() {
match &tls.verification {
TlsVerification::None {} => return S3TlsNoVerificationNotSupportedSnafu.fail(),
Expand Down Expand Up @@ -141,6 +171,7 @@ impl ExtendCatalogConfig for HdfsConnection {
catalog_name: &str,
_catalog_namespace: Option<String>,
_client: &Client,
_trino_version: u16,
) -> Result<(), FromTrinoCatalogError> {
let hdfs_site_dir = format!("{CONFIG_DIR_NAME}/catalog/{catalog_name}/hdfs-config");
catalog_config.add_property(
Expand Down
Loading