wgtmac commented on code in PR #893:
URL: https://github.com/apache/iceberg-cpp/pull/893#discussion_r3854594895
##########
src/iceberg/util/location_util.cc:
##########
@@ -19,14 +19,45 @@
#include "iceberg/util/location_util.h"
+#include <algorithm>
+#include <cctype>
+
namespace iceberg {
+namespace {
+
+/// Whether `candidate` is a syntactically valid URI scheme (RFC 3986 section
+/// 3.1): a letter followed by letters, digits, `+`, `-` or `.`.
+bool IsValidScheme(std::string_view candidate) {
+ if (candidate.empty() || !std::isalpha(static_cast<unsigned
char>(candidate.front()))) {
+ return false;
+ }
+ return std::ranges::all_of(candidate, [](char c) {
+ const auto uc = static_cast<unsigned char>(c);
+ return std::isalnum(uc) || c == '+' || c == '-' || c == '.';
+ });
+}
+
+} // namespace
+
std::string_view LocationUtil::ParseScheme(std::string_view location) {
const auto colon = location.find(':');
if (colon == std::string_view::npos || colon == 0) {
return {};
}
- return location.substr(0, colon);
+ const auto candidate = location.substr(0, colon);
+ // Cannot be a scheme -> a path whose first segment has a colon, such as the
+ // extended-length Windows form `\\?\C:\...`.
+ if (!IsValidScheme(candidate)) {
+ return {};
+ }
+#ifdef _WIN32
+ // A single letter before the colon is a drive, not a scheme.
+ if (candidate.size() == 1) {
Review Comment:
Java does not special-case Windows here: `ResolvingFileIO.scheme()` returns
the text before the first colon, and `LocationUtil.hasScheme()` only applies
RFC 3986 character checks. Please remove the `_WIN32` single-letter drive
handling and keep `ParseScheme` aligned with Java.
##########
src/iceberg/arrow/s3/arrow_s3_file_io.cc:
##########
@@ -177,10 +177,17 @@ Result<std::shared_ptr<::arrow::fs::FileSystem>>
BuildArrowS3FileSystem(
return std::shared_ptr<::arrow::fs::FileSystem>(std::move(fs));
}
+// Rewrites a foreign alias to `s3://` so locations and credential prefixes
+// compare equal. Derived from kS3Schemes: an alias missing here would not
+// fail, it would silently stop matching its credential.
std::string CanonicalizeS3Scheme(std::string_view location) {
- for (std::string_view scheme : {"s3a://", "s3n://"}) {
- if (location.starts_with(scheme)) {
- return std::string("s3://").append(location.substr(scheme.size()));
+ for (std::string_view scheme : kS3Schemes) {
+ if (scheme == S3Properties::kS3Schema) {
+ continue;
+ }
+ if (location.starts_with(scheme) &&
Review Comment:
`FileIORegistry::Resolve` lowercases the URI scheme, so `OSS://...` reaches
this S3 FileIO, but this comparison only recognizes lowercase aliases. The
credential prefix then misses and `FileIOForPath` silently falls back to
default credentials. Please normalize the scheme before matching.
##########
src/iceberg/test/arrow_s3_file_io_test.cc:
##########
@@ -191,8 +191,8 @@ TEST_F(ArrowS3FileIOTest, SkipsNonS3CredentialPrefix) {
// credential that is silently skipped leaves S3 access on the default
// credentials, which only surfaces much later as an auth error.
TEST_F(ArrowS3FileIOTest, AppliesEveryS3CompatibleCredentialPrefix) {
- for (std::string_view prefix :
- {"s3", "s3://bucket/table", "s3a://bucket/table",
"s3n://bucket/table"}) {
+ for (std::string_view prefix : {"s3", "s3://bucket/table",
"s3a://bucket/table",
+ "s3n://bucket/table", "oss://bucket/table"})
{
Review Comment:
This only checks that `SetStorageCredentials` emits no warning; it does not
verify that `FileIOForPath` selects the credentialed delegate. Please cover
`oss` in the real round-trip matrix, or rename this test to reflect prefix
acceptance rather than application.
##########
mkdocs/docs/file-io.md:
##########
@@ -56,6 +56,48 @@ For a REST catalog, set `io-impl` to the registry name. If
it is omitted, the
REST catalog uses `ResolvingFileIO` and selects a registered implementation for
each file location's scheme.
+## Configure S3
+
+| Key | Example | Description |
+|---|---|---|
+| `s3.access-key-id` | `admin` | Static access key ID; must be set together
with the secret key |
+| `s3.secret-access-key` | `password` | Static secret access key |
+| `s3.session-token` | `AQoDYXdzEJr...` | Session token, for temporary
credentials. Ignored unless both static keys are set |
+| `client.region` | `us-east-1` | Region to sign requests for |
+| `s3.endpoint` | `https://127.0.0.1:9000` | Endpoint to use instead of the
AWS one |
+| `s3.path-style-access` | `true` | Address buckets as a path
(`endpoint/bucket`) instead of a virtual host (`bucket.endpoint`). Only takes
effect together with `s3.endpoint` |
+| `s3.ssl.enabled` | `true` | Scheme to use for the endpoint, overriding the
one it carries |
Review Comment:
These three keys (`s3.ssl.enabled`, `s3.connect-timeout-ms`, and
`s3.socket-timeout-ms`) predate this PR, but they are not Java Iceberg or
REST-spec properties. Since this PR exposes them as public S3 configuration,
please distinguish them from the standard keys.
##########
src/iceberg/util/location_util.cc:
##########
@@ -19,14 +19,45 @@
#include "iceberg/util/location_util.h"
+#include <algorithm>
+#include <cctype>
+
namespace iceberg {
+namespace {
+
+/// Whether `candidate` is a syntactically valid URI scheme (RFC 3986 section
+/// 3.1): a letter followed by letters, digits, `+`, `-` or `.`.
+bool IsValidScheme(std::string_view candidate) {
+ if (candidate.empty() || !std::isalpha(static_cast<unsigned
char>(candidate.front()))) {
Review Comment:
`std::isalpha` and `std::isalnum` are locale-sensitive, but RFC 3986 schemes
are ASCII-only, as in Java's `LocationUtil`. With a non-C locale, a path such
as `é:file` can be misclassified as a scheme; please use explicit ASCII checks.
##########
src/iceberg/test/rest_arrow_file_io_test.cc:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.
+ */
+
+/// \file
+/// \brief Covers REST -> ResolvingFileIO -> registry -> Arrow FileIO against
the
+/// real registered implementations, which mock delegates cannot exercise.
+
+#include <algorithm>
+#include <cstdlib>
+#include <memory>
+#include <optional>
+#include <string>
+#include <string_view>
+#include <tuple>
+#include <unordered_map>
+#include <utility>
+#include <vector>
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include "iceberg/arrow/arrow_io_util.h"
+#include "iceberg/arrow/arrow_register.h"
+#include "iceberg/catalog/rest/rest_file_io.h"
+#include "iceberg/logging/logger.h"
+#include "iceberg/storage_credential.h"
+#include "iceberg/test/logging_test_helpers.h"
+#include "iceberg/test/matchers.h"
+#include "iceberg/test/temp_file_test_base.h"
+
+namespace iceberg::rest {
+
+namespace {
+
+class RestArrowFileIOTest : public TempFileTestBase {
+ protected:
+ static void SetUpTestSuite() { iceberg::arrow::RegisterAll(); }
+ static void TearDownTestSuite() { std::ignore =
iceberg::arrow::FinalizeS3(); }
+};
+
+TEST_F(RestArrowFileIOTest, ReadsBackWhatItWroteThroughRealLocalFileIO) {
+ auto io = MakeTableFileIO({{"warehouse", "logical_warehouse_name"}},
+ /*table_config=*/{}, /*storage_credentials=*/{});
+ ASSERT_THAT(io, IsOk());
+
+ const auto path = CreateNewTempFilePathWithSuffix(".txt");
+ constexpr std::string_view kContent = "resolved through the real local
FileIO";
+
+ ASSERT_THAT(io.value()->WriteFile(path, kContent), IsOk());
+ EXPECT_THAT(io.value()->ReadFile(path, std::nullopt),
+ HasValue(::testing::Eq(std::string(kContent))));
+ EXPECT_THAT(io.value()->DeleteFile(path), IsOk());
+}
+
+#if ICEBERG_S3_ENABLED
+
+bool HasWarning(const CapturingLogger& logger) {
+ const auto records = logger.records();
+ return std::ranges::any_of(
+ records, [](const LogMessage& record) { return record.level ==
LogLevel::kWarn; });
+}
+
+std::optional<std::string> GetEnvIfSet(const char* key) {
+ const char* value = std::getenv(key);
+ if (value == nullptr || std::string_view(value).empty()) {
+ return std::nullopt;
+ }
+ return std::string(value);
+}
+
+/// Addresses the store an S3 test reaches as `s3://` the way a catalog vending
+/// `oss://` locations would.
+/// Temporarily removes AWS credential variables, so nothing in the process
+/// environment can stand in for the vended credential under test.
+class ScopedScrubbedAwsCredentialEnv {
+ public:
+ ScopedScrubbedAwsCredentialEnv() {
+ for (const char* name : kNames) {
+ const char* value = std::getenv(name);
+ saved_.emplace_back(
+ name, value != nullptr ? std::optional<std::string>(value) :
std::nullopt);
+ Unset(name);
+ }
+ }
+
+ ~ScopedScrubbedAwsCredentialEnv() {
+ for (const auto& [name, value] : saved_) {
+ if (value.has_value()) {
+ Set(name.c_str(), value->c_str());
+ } else {
+ Unset(name.c_str());
+ }
+ }
+ }
+
+ private:
+ static constexpr const char* kNames[] = {"AWS_ACCESS_KEY_ID",
"AWS_SECRET_ACCESS_KEY",
+ "AWS_SESSION_TOKEN"};
+
+ static void Set(const char* name, const char* value) {
+# ifdef _WIN32
+ _putenv_s(name, value);
+# else
+ ::setenv(name, value, /*overwrite=*/1);
+# endif
+ }
+
+ static void Unset(const char* name) {
+# ifdef _WIN32
+ _putenv_s(name, "");
+# else
+ ::unsetenv(name);
+# endif
+ }
+
+ std::vector<std::pair<std::string, std::optional<std::string>>> saved_;
+};
+
+std::string AsOssUri(std::string_view uri) {
+ const auto pos = uri.find("://");
+ const auto authority = pos == std::string_view::npos ? uri : uri.substr(pos
+ 3);
+ return std::string("oss://").append(authority);
+}
+
+// Resolution, credential matching and real I/O for `oss://`. The credential
+// env vars are scrubbed, so only the vended `s3`-scoped credential matching
+// the canonicalized location can authenticate.
+TEST_F(RestArrowFileIOTest, ReadsBackWhatItWroteThroughAnOssLocation) {
+ const auto base_uri = GetEnvIfSet("ICEBERG_TEST_S3_URI");
+ if (!base_uri.has_value()) {
+ GTEST_SKIP() << "Set ICEBERG_TEST_S3_URI to enable the oss:// round trip";
+ }
+
+ const auto access_key = GetEnvIfSet("AWS_ACCESS_KEY_ID");
+ const auto secret_key = GetEnvIfSet("AWS_SECRET_ACCESS_KEY");
+ ASSERT_TRUE(access_key.has_value() && secret_key.has_value())
+ << "Set AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY alongside "
+ "ICEBERG_TEST_S3_URI";
+ std::unordered_map<std::string, std::string> credential_config = {
+ {"s3.access-key-id", *access_key}, {"s3.secret-access-key",
*secret_key}};
+ if (const auto session_token = GetEnvIfSet("AWS_SESSION_TOKEN")) {
+ credential_config["s3.session-token"] = *session_token;
+ }
+
+ ScopedScrubbedAwsCredentialEnv scrubbed;
+
+ // Scoped to `s3`, while the data it grants access to is addressed as
`oss://`.
+ auto io = MakeTableFileIO({{"warehouse", "logical_warehouse_name"}},
+ /*table_config=*/{},
+ {{.prefix = "s3", .config =
std::move(credential_config)}});
+ ASSERT_THAT(io, IsOk());
+
+ const auto object_uri = AsOssUri(*base_uri) +
"/iceberg_oss_scheme_round_trip.txt";
+ constexpr std::string_view kContent = "resolved and written through an
oss:// location";
+
+ ASSERT_THAT(io.value()->WriteFile(object_uri, kContent), IsOk());
+ EXPECT_THAT(io.value()->ReadFile(object_uri, std::nullopt),
+ HasValue(::testing::Eq(std::string(kContent))));
+ EXPECT_THAT(io.value()->DeleteFile(object_uri), IsOk());
+}
+
+TEST_F(RestArrowFileIOTest, AppliesOssCredentialThroughRealArrowS3FileIO) {
+ auto logger = std::make_shared<CapturingLogger>();
+ ScopedDefaultLogger scoped(logger);
+
+ auto io =
+ MakeTableFileIO({{"warehouse", "logical_warehouse_name"}},
/*table_config=*/{},
+ {{.prefix = "oss://bucket/table", .config = {{"k",
"v"}}}});
+ ASSERT_THAT(io, IsOk());
+
+ // Opening only builds the delegate, so just the pre-network failure modes
+ // are asserted: kNotSupported for a routing break, the warning for a drop.
+ auto input =
io.value()->NewInputFile("oss://bucket/table/data/file.parquet");
+ EXPECT_THAT(input, ::testing::Not(IsError(ErrorKind::kNotSupported)));
Review Comment:
`NewInputFile` only resolves the URI and constructs a handle; it does not
use credentials or hit the network. With `{"k", "v"}`, this only checks that
the prefix was not skipped. Please replace it with a real round-trip using an
`oss://` credential prefix and scrubbed default credentials, or remove it as
redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
