Skip to content

Remove signer override #25791

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

Merged
merged 2 commits into from
May 15, 2025
Merged

Remove signer override #25791

merged 2 commits into from
May 15, 2025

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented May 14, 2025

It's no longer needed since AWS SDK v2 added a LegacyMd5Plugin to ensure backward compatibility with S3-compliant storage.

Should fix #25780

Description

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

## S3 file system (Delta Lake, Iceberg, Hive)
* Improve compatibility with S3-compliant storage systems ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label May 14, 2025
@wendigo wendigo requested review from losipiuk and ebyhr May 14, 2025 11:54
@wendigo wendigo force-pushed the serafin/remove-custom-signer branch from 42ea7c3 to 5a6f4d4 Compare May 14, 2025 12:22
@wendigo
Copy link
Contributor Author

wendigo commented May 14, 2025

Tested manually against Dell ECS

wendigo added 2 commits May 14, 2025 14:28
It's no longer needed since AWS SDK v2 added a LegacyMd5Plugin
to ensure backward compatibility with S3-compliant storage.
This is useful when testing manually against Dell ECS
or other S3 compatible storage solution.
@wendigo wendigo force-pushed the serafin/remove-custom-signer branch from 5a6f4d4 to 816a379 Compare May 14, 2025 12:30
@wendigo
Copy link
Contributor Author

wendigo commented May 14, 2025

@twuebi can you check whether this change fixes your problem?

@twuebi
Copy link

twuebi commented May 15, 2025

@twuebi can you check whether this change fixes your problem?

Hi, thanks for the fix, I'm not sure how to test, I tried building Trino on mac with m3 but it failed on the trino-docs step, is there a docker image available for this PR?

Following my analysis after stepping through the function with a debugger, the fix should work.

@wendigo
Copy link
Contributor Author

wendigo commented May 15, 2025

@twuebi you can build without trino-docs (it should work) by calling ./mvnw with -pl "!:trino-docs"

@twuebi
Copy link

twuebi commented May 15, 2025

Thanks, I managed to build trino now, but cannot build the docker image:

...
  Running scriptlet: filesystem-3.16-5.el9.x86_64                         68/68
rosetta error: Unable to open /proc/self/exe: 2
 warning: %posttrans(filesystem-3.16-5.el9.x86_64) scriptlet failed, signal 5

Error in POSTTRANS scriptlet in rpm package filesystem
  Running scriptlet: libstdc++-11.5.0-5.el9_5.x86_64                      68/68
rosetta error: Unable to open /proc/self/exe: 2
...
Failed:
  ca-certificates-2024.2.69_v8.0.303-91.4.el9_4.noarch

I also haven't gotten it to run on host:

trino-server/target/trino-server-476-SNAPSHOT/bin/launcher run --data-dir /tmp/trino
procname shim does not exist at location .../trino/core/trino-server/target/trino-server-476-SNAPSHOT/bin/darwin-arm64/libprocname.so
Error occurred during initialization of VM
Could not find agent library /usr/lib/trino/bin/libjvmkill.so in absolute path, with error: dlopen(/usr/lib/trino/bin/libjvmkill.so, 0x0001): tried: '/usr/lib/trino/bin/libjvmkill.so' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/usr/lib/trino/bin/libjvmkill.so' (no such file), '/usr/lib/trino/bin/libjvmkill.so' (no such file, not in dyld cache)

I'm quite confident that your fix is working fine, if possible, could you go ahead and merge? I'll happily test once there's an image available. Otherwise, I'd also try suggestions to fix my build / run issues.

@wendigo
Copy link
Contributor Author

wendigo commented May 15, 2025

You can try to build only your architecture: ./core/docker/build.sh -a arm64

@twuebi
Copy link

twuebi commented May 15, 2025

Thanks, that seems to work, I'll get back to you once I finished testing!

@wendigo
Copy link
Contributor Author

wendigo commented May 15, 2025

@twuebi
Copy link

twuebi commented May 15, 2025

I've verified the fix, it works. I ran into some troubles where trino does not seem to send the X-Iceberg-Access-Delegation header. Without changing the default of lakekeeper to vended-credentials, I was hitting missing credentials in this check (io/trino/plugin/iceberg/IcebergMetadata.java:1285), not sure if that was a recent change in trino:

try {
            // S3 Tables internally assigns a unique location for each table
            if (!isS3Tables(location.toString())) {
                TrinoFileSystem fileSystem = fileSystemFactory.create(session.getIdentity(), transaction.table().io().properties());
                if (!replace && fileSystem.listFiles(location).hasNext()) {
                    throw new TrinoException(ICEBERG_FILESYSTEM_ERROR, format("" +
                            "Cannot create a table on a non-empty location: %s, set 'iceberg.unique-table-location=true' in your Iceberg catalog properties " +
                            "to use unique table locations for every table.", location));
                }
            }
            return newWritableTableHandle(tableMetadata.getTable(), transaction.table(), retryMode);
        }
        catch (IOException e) {
            throw new TrinoException(ICEBERG_FILESYSTEM_ERROR, "Failed checking new table's location: " + location, e);
        }

anyways, many thanks for fixing this bug & helping me to build the container.

@wendigo
Copy link
Contributor Author

wendigo commented May 15, 2025

@twuebi this is probably: #25800

@wendigo wendigo merged commit 754a1a8 into master May 15, 2025
159 of 164 checks passed
@wendigo wendigo deleted the serafin/remove-custom-signer branch May 15, 2025 12:08
@wendigo
Copy link
Contributor Author

wendigo commented May 15, 2025

@twuebi the second bug was actually related to iceberg 1.9.0 update (http client changes), credit goes to @chenjian2664 and @anusudarsan for fixing that :)

@github-actions github-actions bot added this to the 476 milestone May 15, 2025
@twuebi
Copy link

twuebi commented May 16, 2025

@twuebi the second bug was actually related to iceberg 1.9.0 update (http client changes), credit goes to @chenjian2664 and @anusudarsan for fixing that :)

Thanks a lot, just tested and it was indeed fixed by #25800!

@wendigo
Copy link
Contributor Author

wendigo commented May 16, 2025

@twuebi our internal testing caught that :)

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

Successfully merging this pull request may close these issues.

Trino forgets vended-credentials in S3FileSystem::deleteObjects
5 participants