laskoviymishka commented on code in PR #2190: URL: https://github.com/apache/iceberg-go/pull/2190#discussion_r4286585760
########## CONTRIBUTING.md: ########## @@ -64,3 +64,42 @@ Integration tests require Docker and are gated behind a build tag: docker compose -f internal/recipe/docker-compose.yml up -d rest object-store create-bucket --wait go test -tags integration ./... ``` + +#### Polaris Conformance (optional) + +The catalog conformance suite can also run against [Apache Polaris](https://polaris.apache.org/), a second REST catalog implementation. `TestPolarisCatalogConformance` is skipped unless `POLARIS_URI` is set. To run it against a local Polaris server with file storage (requires `jq`): + +```bash +docker run -d --name polaris -p 8191:8181 -p 8192:8182 \ + -e POLARIS_BOOTSTRAP_CREDENTIALS='POLARIS,root,s3cr3t' \ + -e 'polaris.features."ALLOW_INSECURE_STORAGE_TYPES"=true' \ + -e 'polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"=["FILE"]' \ + -e 'polaris.readiness.ignore-severe-issues=true' \ Review Comment: This flag silences a readiness check people might otherwise want, so it's worth one clause on why it's here. I assume Polaris flags insecure FILE storage as a severe issue at startup. ########## CONTRIBUTING.md: ########## @@ -64,3 +64,42 @@ Integration tests require Docker and are gated behind a build tag: docker compose -f internal/recipe/docker-compose.yml up -d rest object-store create-bucket --wait go test -tags integration ./... ``` + +#### Polaris Conformance (optional) + +The catalog conformance suite can also run against [Apache Polaris](https://polaris.apache.org/), a second REST catalog implementation. `TestPolarisCatalogConformance` is skipped unless `POLARIS_URI` is set. To run it against a local Polaris server with file storage (requires `jq`): + +```bash +docker run -d --name polaris -p 8191:8181 -p 8192:8182 \ + -e POLARIS_BOOTSTRAP_CREDENTIALS='POLARIS,root,s3cr3t' \ + -e 'polaris.features."ALLOW_INSECURE_STORAGE_TYPES"=true' \ + -e 'polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"=["FILE"]' \ + -e 'polaris.readiness.ignore-severe-issues=true' \ + apache/polaris:1.8.0 +until curl -sf localhost:8192/q/health/ready > /dev/null; do sleep 1; done Review Comment: If the container crash-loops (bad feature flag, port clash, pull failure), this spins forever with no output. I'd bound it, plus a line pointing at `docker logs polaris` if it never comes up: ```suggestion for _ in $(seq 60); do curl -sf localhost:8192/q/health/ready > /dev/null && break; sleep 1; done ``` ########## catalog/rest/polaris_conformance_test.go: ########## @@ -0,0 +1,83 @@ +// 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. + +//go:build integration + +package rest_test + +import ( + "context" + "os" + "testing" + + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/catalog" + "github.com/apache/iceberg-go/catalog/catalogtest" + "github.com/stretchr/testify/require" +) + +// Environment variables that point the conformance suite at an Apache Polaris +// server, a second REST implementation alongside the Java REST fixture. +// POLARIS_URI must include Polaris's catalog prefix, for example +// http://localhost:8191/api/catalog; the OAuth token endpoint is derived from it. +const ( + polarisURIEnv = "POLARIS_URI" + polarisCredentialEnv = "POLARIS_CREDENTIAL" + polarisWarehouseEnv = "POLARIS_WAREHOUSE" +) + +// polarisScope activates all of the principal's roles. Polaris rejects the +// client's default "catalog" scope. +const polarisScope = "PRINCIPAL_ROLE:ALL" + +// TestPolarisCatalogConformance runs the catalog conformance suite against an +// Apache Polaris server. It is opt-in: the test is skipped unless POLARIS_URI +// is set. See CONTRIBUTING.md for how to start a local Polaris server. +func TestPolarisCatalogConformance(t *testing.T) { + uri := os.Getenv(polarisURIEnv) + if uri == "" { + t.Skipf("%s is not set; skipping Polaris conformance tests", polarisURIEnv) + } + + credential := os.Getenv(polarisCredentialEnv) + require.NotEmpty(t, credential, "%s must be set to a client_id:client_secret pair", polarisCredentialEnv) Review Comment: The message promises a `client_id:client_secret` pair, but a bare secret passes `NotEmpty`. The client then quietly takes its secret-only path with an empty client_id, and Polaris rejects it with an auth error that doesn't point back here. `Contains` covers the empty case too: ```suggestion require.Contains(t, credential, ":", "%s must be set to a client_id:client_secret pair", polarisCredentialEnv) ``` ########## CONTRIBUTING.md: ########## @@ -64,3 +64,42 @@ Integration tests require Docker and are gated behind a build tag: docker compose -f internal/recipe/docker-compose.yml up -d rest object-store create-bucket --wait go test -tags integration ./... ``` + +#### Polaris Conformance (optional) + +The catalog conformance suite can also run against [Apache Polaris](https://polaris.apache.org/), a second REST catalog implementation. `TestPolarisCatalogConformance` is skipped unless `POLARIS_URI` is set. To run it against a local Polaris server with file storage (requires `jq`): + +```bash +docker run -d --name polaris -p 8191:8181 -p 8192:8182 \ + -e POLARIS_BOOTSTRAP_CREDENTIALS='POLARIS,root,s3cr3t' \ + -e 'polaris.features."ALLOW_INSECURE_STORAGE_TYPES"=true' \ + -e 'polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"=["FILE"]' \ + -e 'polaris.readiness.ignore-severe-issues=true' \ + apache/polaris:1.8.0 +until curl -sf localhost:8192/q/health/ready > /dev/null; do sleep 1; done + +TOKEN=$(curl -sf -X POST localhost:8191/api/catalog/v1/oauth/tokens \ + -d grant_type=client_credentials -d client_id=root -d client_secret=s3cr3t \ + -d scope=PRINCIPAL_ROLE:ALL | jq -r .access_token) + +# Create a catalog backed by local files. +curl -sf -X POST localhost:8191/api/management/v1/catalogs \ + -H "Authorization: Bearer $TOKEN" -H 'Content-Type: application/json' \ + -d '{"catalog":{"name":"conformance","type":"INTERNAL", + "properties":{"default-base-location":"file:///tmp/polaris/conformance"}, + "storageConfigInfo":{"storageType":"FILE","allowedLocations":["file:///tmp/polaris/conformance"]}}}' + +# The REST client requests vended credentials (X-Iceberg-Access-Delegation) by +# default, which Polaris authorizes with TABLE_WRITE_DATA; CATALOG_MANAGE_CONTENT +# includes it. +curl -sf -X PUT localhost:8191/api/management/v1/catalogs/conformance/catalog-roles/catalog_admin/grants \ + -H "Authorization: Bearer $TOKEN" -H 'Content-Type: application/json' \ + -d '{"grant":{"type":"catalog","privilege":"CATALOG_MANAGE_CONTENT"}}' + +POLARIS_URI=http://localhost:8191/api/catalog \ +POLARIS_CREDENTIAL=root:s3cr3t \ +POLARIS_WAREHOUSE=conformance \ + go test -tags integration -run TestPolarisCatalogConformance ./catalog/rest +``` + +The setup steps only need to run once per server. Each test run uses fresh, uniquely named namespaces and drops them afterward, so the `go test` step can be rerun against the same Polaris catalog. These credentials are for local testing only; remove the server with `docker rm -f polaris`. Review Comment: Rerunning the block against an existing server fails: `--name polaris` is taken, and the catalog POST returns 409, which with `-sf` shows up only as exit code 22. I'd add a sentence saying that on a rerun you skip straight to `go test`, or start fresh with `docker rm -f polaris`. ########## CONTRIBUTING.md: ########## @@ -64,3 +64,42 @@ Integration tests require Docker and are gated behind a build tag: docker compose -f internal/recipe/docker-compose.yml up -d rest object-store create-bucket --wait go test -tags integration ./... ``` + +#### Polaris Conformance (optional) + +The catalog conformance suite can also run against [Apache Polaris](https://polaris.apache.org/), a second REST catalog implementation. `TestPolarisCatalogConformance` is skipped unless `POLARIS_URI` is set. To run it against a local Polaris server with file storage (requires `jq`): + +```bash +docker run -d --name polaris -p 8191:8181 -p 8192:8182 \ + -e POLARIS_BOOTSTRAP_CREDENTIALS='POLARIS,root,s3cr3t' \ + -e 'polaris.features."ALLOW_INSECURE_STORAGE_TYPES"=true' \ + -e 'polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"=["FILE"]' \ + -e 'polaris.readiness.ignore-severe-issues=true' \ + apache/polaris:1.8.0 +until curl -sf localhost:8192/q/health/ready > /dev/null; do sleep 1; done + +TOKEN=$(curl -sf -X POST localhost:8191/api/catalog/v1/oauth/tokens \ + -d grant_type=client_credentials -d client_id=root -d client_secret=s3cr3t \ + -d scope=PRINCIPAL_ROLE:ALL | jq -r .access_token) Review Comment: The pipeline's status is `jq`'s, so a failed token request leaves `TOKEN` empty (or the literal `null`). The next two calls then 401, and `-sf` hides the body, so there's no clue why. `jq -e` fails fast in both cases: ```suggestion -d scope=PRINCIPAL_ROLE:ALL | jq -er .access_token) ``` ########## CONTRIBUTING.md: ########## @@ -64,3 +64,42 @@ Integration tests require Docker and are gated behind a build tag: docker compose -f internal/recipe/docker-compose.yml up -d rest object-store create-bucket --wait go test -tags integration ./... ``` + +#### Polaris Conformance (optional) Review Comment: This section ends up larger than the whole Development Setup above it, for a flow most contributors won't run, and a shell block in a doc is the setup most likely to rot unnoticed. I'd move the bootstrap into a script (`dev/` already has `check-api-compat.sh`, or next to the compose file in `internal/recipe/` so CI can reuse it if Polaris lands there) and keep this to a couple of lines: run the script, then the `go test` invocation with the three env vars. ########## catalog/rest/polaris_conformance_test.go: ########## @@ -0,0 +1,83 @@ +// 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. + +//go:build integration + +package rest_test + +import ( + "context" + "os" + "testing" + + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/catalog" + "github.com/apache/iceberg-go/catalog/catalogtest" + "github.com/stretchr/testify/require" +) + +// Environment variables that point the conformance suite at an Apache Polaris +// server, a second REST implementation alongside the Java REST fixture. +// POLARIS_URI must include Polaris's catalog prefix, for example +// http://localhost:8191/api/catalog; the OAuth token endpoint is derived from it. +const ( + polarisURIEnv = "POLARIS_URI" + polarisCredentialEnv = "POLARIS_CREDENTIAL" + polarisWarehouseEnv = "POLARIS_WAREHOUSE" +) + +// polarisScope activates all of the principal's roles. Polaris rejects the +// client's default "catalog" scope. +const polarisScope = "PRINCIPAL_ROLE:ALL" + +// TestPolarisCatalogConformance runs the catalog conformance suite against an +// Apache Polaris server. It is opt-in: the test is skipped unless POLARIS_URI +// is set. See CONTRIBUTING.md for how to start a local Polaris server. +func TestPolarisCatalogConformance(t *testing.T) { + uri := os.Getenv(polarisURIEnv) + if uri == "" { + t.Skipf("%s is not set; skipping Polaris conformance tests", polarisURIEnv) + } + + credential := os.Getenv(polarisCredentialEnv) + require.NotEmpty(t, credential, "%s must be set to a client_id:client_secret pair", polarisCredentialEnv) + + warehouse := os.Getenv(polarisWarehouseEnv) + require.NotEmpty(t, warehouse, "%s must be set to the name of a Polaris catalog", polarisWarehouseEnv) + + catalogtest.RunCatalogTests(t, catalogtest.Config{ + SupportsNamespaceProperties: true, + SupportsRenameTable: true, Review Comment: small thing on what this run proves: `testRenameTableToSelf` passes on either success or `ErrTableAlreadyExists`, so a green run here doesn't tell us which branch Polaris took. The client side of the mapping is already pinned by `TestRenameTable409` in `rest_test.go`, so what Polaris adds is a real server actually sending `AlreadyExistsException` for self-rename. If we want that to stay meaningful, I'd log the branch taken (or let the config say which outcome a backend is expected to produce) so a Polaris behavior change shows up instead of quietly passing. -- 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]
