rambleraptor commented on code in PR #16:
URL: https://github.com/apache/iceberg-terraform/pull/16#discussion_r2880876071
##########
internal/provider/provider.go:
##########
@@ -41,20 +43,24 @@ func New() func() provider.Provider {
// icebergProvider is the provider implementation.
type icebergProvider struct {
- catalogURI string
- catalogType string
- token string
- warehouse string
- headers map[string]string
+ catalogURI string
+ catalogType string
+ token string
+ warehouse string
+ headers map[string]string
+ polarisManagementURI string
+ polarisCatalogName string
}
// icebergProviderModel maps provider schema data to a Go type.
type icebergProviderModel struct {
- CatalogURI types.String `tfsdk:"catalog_uri"`
- Type types.String `tfsdk:"type"`
- Token types.String `tfsdk:"token"`
- Warehouse types.String `tfsdk:"warehouse"`
- Headers types.Map `tfsdk:"headers"`
+ CatalogURI types.String `tfsdk:"catalog_uri"`
+ Type types.String `tfsdk:"type"`
+ Token types.String `tfsdk:"token"`
+ Warehouse types.String `tfsdk:"warehouse"`
+ Headers types.Map `tfsdk:"headers"`
+ PolarisManagementURI types.String `tfsdk:"polaris_management_uri"`
Review Comment:
We should have a separate struct for these polaris fields.
Then, catalog_uri should support a 'polaris' type. It'll send 'rest' to
iceberg-go, but you need to set 'polaris' to allow those polaris fields.
##########
internal/provider/resource_polaris_principal_test.go:
##########
@@ -0,0 +1,190 @@
+// 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 provider
+
+import (
+ "encoding/json"
+ "fmt"
+ "net/http"
+ "net/http/httptest"
+ "os"
+ "strings"
+ "testing"
+
+ "github.com/hashicorp/terraform-plugin-testing/helper/resource"
+)
+
+func testAccPolarisProviderConfig(catalogURI, managementURI string) string {
+ return testAccPolarisProviderConfigWithToken(catalogURI, managementURI,
"")
+}
+
+func testAccPolarisProviderConfigWithToken(catalogURI, managementURI, token
string) string {
+ tokenAttr := ""
+ if token != "" {
+ tokenAttr = fmt.Sprintf("\n token = %q", token)
+ }
+ return fmt.Sprintf(`
+provider "iceberg" {
+ catalog_uri = "%s"
+ polaris_management_uri = "%s"%s
+}
+`, catalogURI, managementURI, tokenAttr)
+}
+
+func newPolarisPrincipalTestServer(t *testing.T) *httptest.Server {
+ t.Helper()
+
+ principals := make(map[string]polarisPrincipal)
+
+ mux := http.NewServeMux()
+
+ mux.HandleFunc("/api/management/v1/principals", func(w
http.ResponseWriter, r *http.Request) {
+ if r.Method != http.MethodPost {
+ http.Error(w, "method not allowed",
http.StatusMethodNotAllowed)
+ return
+ }
+
+ var req polarisCreatePrincipalRequest
+ if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+ http.Error(w, err.Error(), http.StatusBadRequest)
+ return
+ }
+
+ name := req.Principal.Name
+ if name == "" {
+ http.Error(w, "missing name", http.StatusBadRequest)
+ return
+ }
+
+ p := polarisPrincipal{
+ Name: name,
+ Properties: req.Principal.Properties,
+ EntityVersion: 1,
+ }
+ principals[name] = p
+
+ resp := polarisPrincipalWithCredentials{
+ Principal: p,
+ }
+ resp.Credentials.ClientID = "id-" + name
+ resp.Credentials.ClientSecret = "secret-" + name
+
+ w.Header().Set("Content-Type", "application/json")
+ w.WriteHeader(http.StatusCreated)
+ _ = json.NewEncoder(w).Encode(resp)
+ })
+
+ mux.HandleFunc("/api/management/v1/principals/", func(w
http.ResponseWriter, r *http.Request) {
+ name := strings.TrimPrefix(r.URL.Path,
"/api/management/v1/principals/")
+ if name == "" {
+ http.NotFound(w, r)
+ return
+ }
+
+ switch r.Method {
+ case http.MethodGet:
+ p, ok := principals[name]
+ if !ok {
+ http.NotFound(w, r)
+ return
+ }
+ w.Header().Set("Content-Type", "application/json")
+ _ = json.NewEncoder(w).Encode(p)
+ case http.MethodDelete:
+ if _, ok := principals[name]; !ok {
+ http.NotFound(w, r)
+ return
+ }
+ delete(principals, name)
+ w.WriteHeader(http.StatusNoContent)
+ default:
+ http.Error(w, "method not allowed",
http.StatusMethodNotAllowed)
+ }
+ })
+
+ return httptest.NewServer(mux)
+}
+
+func TestAccPolarisPrincipal(t *testing.T) {
+ t.Parallel()
+
+ srv := newPolarisPrincipalTestServer(t)
Review Comment:
I don't personally see the need for a mocked unit test since we have the
integration tests in Docker. I'll defer to @zeroshade though.
##########
internal/provider/provider.go:
##########
@@ -142,6 +156,23 @@ func (p *icebergProvider) Configure(ctx context.Context,
req provider.ConfigureR
p.headers = headers
}
+ if !data.PolarisCatalogName.IsNull() &&
!data.PolarisCatalogName.IsUnknown() {
+ p.polarisCatalogName = data.PolarisCatalogName.ValueString()
+ }
+
+ if !data.PolarisManagementURI.IsNull() &&
!data.PolarisManagementURI.IsUnknown() {
+ p.polarisManagementURI =
strings.TrimRight(data.PolarisManagementURI.ValueString(), "/")
+ } else {
+ if p.catalogURI != "" {
+ if u, err := url.Parse(p.catalogURI); err == nil {
+ u.Path = "/api/management/v1"
+ u.RawQuery = ""
Review Comment:
Why do we need to set these other two parameters? What if we override them?
##########
internal/provider/resource_polaris_principal.go:
##########
@@ -0,0 +1,353 @@
+// 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 provider
+
+import (
+ "context"
+ "errors"
+
+ "github.com/hashicorp/terraform-plugin-framework/diag"
+ "github.com/hashicorp/terraform-plugin-framework/path"
+ "github.com/hashicorp/terraform-plugin-framework/resource"
+ "github.com/hashicorp/terraform-plugin-framework/resource/schema"
+
"github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault"
+
"github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
+
"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
+ "github.com/hashicorp/terraform-plugin-framework/types"
+ "github.com/hashicorp/terraform-plugin-log/tflog"
+)
+
+var (
+ _ resource.Resource = &polarisPrincipalResource{}
+ _ resource.ResourceWithImportState = &polarisPrincipalResource{}
+)
+
+func NewPolarisPrincipalResource() resource.Resource {
+ return &polarisPrincipalResource{}
+}
+
+type polarisPrincipalResource struct {
+ provider *icebergProvider
+ client *polarisClient
+}
+
+type polarisPrincipalResourceModel struct {
+ ID types.String `tfsdk:"id"`
+ Name types.String `tfsdk:"name"`
+ Properties types.Map `tfsdk:"properties"`
+ CredentialRotationRequired types.Bool
`tfsdk:"credential_rotation_required"`
+ ClientID types.String `tfsdk:"client_id"`
+ ClientSecret types.String `tfsdk:"client_secret"`
+ EntityVersion types.Int64 `tfsdk:"entity_version"`
+}
+
+func (r *polarisPrincipalResource) Metadata(_ context.Context, req
resource.MetadataRequest, resp *resource.MetadataResponse) {
+ resp.TypeName = req.ProviderTypeName + "_polaris_principal"
+}
+
+func (r *polarisPrincipalResource) Schema(_ context.Context, _
resource.SchemaRequest, resp *resource.SchemaResponse) {
+ resp.Schema = schema.Schema{
+ Description: "A resource for managing Polaris principals and
their client credentials.",
+ Attributes: map[string]schema.Attribute{
+ "id": schema.StringAttribute{
+ Computed: true,
+ PlanModifiers: []planmodifier.String{
+ stringplanmodifier.UseStateForUnknown(),
+ },
+ },
+ "name": schema.StringAttribute{
+ Description: "The name of the Polaris
principal.",
+ Required: true,
+ PlanModifiers: []planmodifier.String{
+ stringplanmodifier.RequiresReplace(),
+ },
+ },
+ "properties": schema.MapAttribute{
+ Description: "Arbitrary metadata properties for
the principal.",
+ Optional: true,
+ ElementType: types.StringType,
+ },
+ "credential_rotation_required": schema.BoolAttribute{
+ Description: "If true, the initial credentials
can only be used to call rotateCredentials.",
+ Optional: true,
+ Computed: true,
+ Default: booldefault.StaticBool(false),
+ },
+ "client_id": schema.StringAttribute{
+ Description: "The client ID associated with
this principal.",
+ Computed: true,
+ Sensitive: true,
+ },
+ "client_secret": schema.StringAttribute{
+ Description: "The client secret associated with
this principal.",
+ Computed: true,
+ Sensitive: true,
+ },
+ "entity_version": schema.Int64Attribute{
+ Description: "The entity version used for
optimistic concurrency control when updating the principal.",
Review Comment:
Is this something that we can let terraform manage? Every time this is
updated, we don't want users to have to find the old entity version + change it.
##########
Makefile:
##########
@@ -22,8 +22,9 @@ test-integration-setup: ## Start Docker services for
integration tests
docker compose -f dev/docker-compose.yml rm -f
docker compose -f dev/docker-compose.yml up -d --wait
-test-integration-exec: ## Run integration tests
- TF_ACC=1 ICEBERG_CATALOG_URI=http://localhost:8181 go test ./... -v
+test-integration-exec: ## Run integration tests (Iceberg REST at 8181, Polaris
at 8191)
+ POLARIS_TOKEN=$$(curl -sf
http://localhost:8191/api/catalog/v1/oauth/tokens --user root:s3cr3t -d
grant_type=client_credentials -d 'scope=PRINCIPAL_ROLE:ALL' | python3 -c
"import sys,json; d=json.load(sys.stdin); print(d.get('access_token',''))") && \
Review Comment:
I'd prefer if we had a `dev/provision_polaris.py` script that could hand
over the access token. I don't love having source code live in Makefiles.
--
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]
