From 0e014e5cd8f92e7c95bb2a9ecf729723bc45c395 Mon Sep 17 00:00:00 2001 From: Harnoor7 Date: Mon, 1 Jun 2026 11:17:36 +0530 Subject: [PATCH] Fix NPE in S3PinotFS credential refresh when initialized with a provided S3Client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `S3PinotFS.init(S3Client, String, PinotConfiguration)` set `_s3Client` but left the `_s3Config` field null — it built an `S3Config` only as a local for the server-side-encryption / multipart / ACL setup. The single-arg `init(PinotConfiguration)` is the only path that assigned `_s3Config`. Any later operation routed through `retryWithS3CredentialRefresh(...)` -> `initOrRefreshS3Client()` reads `_s3Config.getRegion()`, so an instance created with a caller-provided client threw a NullPointerException on the first credential refresh. Assign the already-constructed `S3Config` to the `_s3Config` field (and reuse it for the SSE/multipart/ACL setup). No extra object construction; the field is now populated so a refresh rebuilds the client from the same config instead of NPEing. Adds `S3PinotFSInitTest` which initializes via the three-arg `init` and then calls `initOrRefreshS3Client()` — it reproduces the NPE before this change and passes after. Static credentials keep the rebuild fully offline (no container). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../pinot/plugin/filesystem/S3PinotFS.java | 12 ++-- .../plugin/filesystem/S3PinotFSInitTest.java | 60 +++++++++++++++++++ 2 files changed, 68 insertions(+), 4 deletions(-) create mode 100644 pinot-plugins/pinot-file-system/pinot-s3/src/test/java/org/apache/pinot/plugin/filesystem/S3PinotFSInitTest.java diff --git a/pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java b/pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java index 10b18642f8c7..d4540a1de25c 100644 --- a/pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java +++ b/pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java @@ -316,10 +316,14 @@ public void init(S3Client s3Client) { */ public void init(S3Client s3Client, String serverSideEncryption, PinotConfiguration serverSideEncryptionConfig) { _s3Client = s3Client; - S3Config s3Config = new S3Config(serverSideEncryptionConfig); - setServerSideEncryption(serverSideEncryption, s3Config); - setMultiPartUploadConfigs(s3Config); - setDisableAcl(s3Config); + // Store the config on the _s3Config field rather than a local. A later credential refresh goes + // through initOrRefreshS3Client(), which reads _s3Config.getRegion(); leaving the field null + // caused a NullPointerException on the first refresh for instances initialized with a provided + // client. + _s3Config = new S3Config(serverSideEncryptionConfig); + setServerSideEncryption(serverSideEncryption, _s3Config); + setMultiPartUploadConfigs(_s3Config); + setDisableAcl(_s3Config); } @VisibleForTesting diff --git a/pinot-plugins/pinot-file-system/pinot-s3/src/test/java/org/apache/pinot/plugin/filesystem/S3PinotFSInitTest.java b/pinot-plugins/pinot-file-system/pinot-s3/src/test/java/org/apache/pinot/plugin/filesystem/S3PinotFSInitTest.java new file mode 100644 index 000000000000..65fcf72732f5 --- /dev/null +++ b/pinot-plugins/pinot-file-system/pinot-s3/src/test/java/org/apache/pinot/plugin/filesystem/S3PinotFSInitTest.java @@ -0,0 +1,60 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.plugin.filesystem; + +import org.apache.pinot.spi.env.PinotConfiguration; +import org.testng.annotations.Test; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.s3.S3Client; + + +/** + * Unit tests for {@link S3PinotFS} initialization paths that do not require an S3 mock container. + */ +public class S3PinotFSInitTest { + + /** + * Regression test: {@link S3PinotFS#init(S3Client, String, PinotConfiguration)} must populate the + * {@code _s3Config} field, not just a local. A later credential refresh runs through + * {@link S3PinotFS#initOrRefreshS3Client()}, which reads {@code _s3Config.getRegion()}; leaving + * the field null caused a {@link NullPointerException} on the first refresh for instances + * initialized with a caller-provided client. + */ + @Test + public void testInitWithProvidedClientPopulatesConfigForRefresh() { + S3Client providedClient = S3Client.builder() + .region(Region.US_EAST_1) + .credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("key", "secret"))) + .build(); + + PinotConfiguration config = new PinotConfiguration(); + config.setProperty(S3Config.REGION, "us-east-1"); + config.setProperty(S3Config.ACCESS_KEY, "key"); + config.setProperty(S3Config.SECRET_KEY, "secret"); + + S3PinotFS s3PinotFS = new S3PinotFS(); + s3PinotFS.init(providedClient, null, config); + + // Before the fix this threw NullPointerException because _s3Config was left null. The static + // credentials above keep the rebuild fully offline (no default-provider chain / network). + s3PinotFS.initOrRefreshS3Client(); + } +}