Repository: apache/iceberg-go
Pull request #265 (2025-01-22 00:49:38)

feat(catalog): Make rest catalog auth scope configurable
fixes https://github.com/apache/iceberg-go/issues/263

As part of debugging the test update I noticed that multiple http clients are created each time a rest client is created


Patch:
diff --git a/catalog/catalog.go b/catalog/catalog.go
index c611dd4..bb843b1 100644
--- a/catalog/catalog.go
+++ b/catalog/catalog.go
@@ -128,6 +128,12 @@ func WithPrefix(prefix string) Option[RestCatalog] {
 	}
 }
 
+func WithScope(scope string) Option[RestCatalog] {
+	return func(o *options) {
+		o.scope = scope
+	}
+}
+
 type Option[T GlueCatalog | RestCatalog] func(*options)
 
 type options struct {
@@ -144,6 +150,7 @@ type options struct {
 	sigv4Service      string
 	prefix            string
 	authUri           *url.URL
+	scope             string
 }
 
 type PropertiesUpdateSummary struct {
diff --git a/catalog/registry.go b/catalog/registry.go
index 544d419..e8e6132 100644
--- a/catalog/registry.go
+++ b/catalog/registry.go
@@ -86,7 +86,7 @@ func Unregister(catalogType string) {
 	defaultRegistry.remove(catalogType)
 }
 
-// GetRegsisteredCatalogs returns the list of registered catalog names that can
+// GetRegisteredCatalogs returns the list of registered catalog names that can
 // be looked up via LoadCatalog.
 func GetRegisteredCatalogs() []string {
 	return defaultRegistry.getKeys()
@@ -98,7 +98,7 @@ func GetRegisteredCatalogs() []string {
 // easier catalog loading but also to allow for custom catalog implementations to
 // be registered and loaded external to this module.
 //
-// The name parameter is used to lookup the catalog configuration, if one exists,
+// The name parameter is used to look up the catalog configuration, if one exists,
 // that was loaded from the configuration file, ".iceberg-go.yaml". By default,
 // the config file is loaded from the user's home directory, but the directory can
 // be changed by setting the GOICEBERG_HOME environment variable to the path of the
@@ -110,20 +110,20 @@ func GetRegisteredCatalogs() []string {
 // priority over any loaded config.
 //
 // If there is no "type" in the configuration and no "type" in the passed in properties,
-// then the "uri" property is used to lookup the catalog by checking the scheme. Again,
-// if there is a "uri" key set in the passed in "props" it will take priority over the
+// then the "uri" property is used to look up the catalog by checking the scheme. Again,
+// if there is an "uri" key set in the passed in "props" it will take priority over the
 // configuration.
 //
-// Currently the following catalog types are registered by default:
+// Currently, the following catalog types are registered by default:
 //
 //   - "glue" for AWS Glue Data Catalog, the rest of the URI is ignored, all configuration
 //     should be provided using the properties. "glue.region", "glue.endpoint",
 //     "glue.max-retries", etc. Default AWS credentials are used if found, or can be
 //     overridden by setting "glue.access-key-id", "glue.secret-access-key", and "glue.session-token".
 //
-//   - "rest" for a REST API catalog, if the properties have a "uri" key, then that will be used
+//   - "rest" for a REST API catalog, if the properties have an "uri" key, then that will be used
 //     as the REST endpoint, otherwise the URI is used as the endpoint. The REST catalog also
-//     registers "http" and "https" so that Load with an http/s URI will automatically
+//     registers "http" and "https" so that Load with a http/s URI will automatically
 //     load the REST Catalog.
 func Load(name string, props iceberg.Properties) (Catalog, error) {
 	if name == "" {
diff --git a/catalog/rest.go b/catalog/rest.go
index a38c088..636d04e 100644
--- a/catalog/rest.go
+++ b/catalog/rest.go
@@ -468,16 +468,10 @@ func (r *RestCatalog) init(ops *options, uri string) error {
 	}
 
 	r.baseURI = baseuri.JoinPath("v1")
-	if ops, err = r.fetchConfig(ops); err != nil {
+	if r.cl, ops, err = r.fetchConfig(ops); err != nil {
 		return err
 	}
 
-	cl, err := r.createSession(ops)
-	if err != nil {
-		return err
-	}
-
-	r.cl = cl
 	if ops.prefix != "" {
 		r.baseURI = r.baseURI.JoinPath(ops.prefix)
 	}
@@ -491,11 +485,15 @@ func (r *RestCatalog) fetchAccessToken(cl *http.Client, creds string, opts *opti
 		clientID, clientSecret = "", clientID
 	}
 
+	scope := "catalog"
+	if opts.scope != "" {
+		scope = opts.scope
+	}
 	data := url.Values{
 		"grant_type":    {"client_credentials"},
 		"client_id":     {clientID},
 		"client_secret": {clientSecret},
-		"scope":         {"catalog"},
+		"scope":         {scope},
 	}
 
 	uri := opts.authUri
@@ -575,7 +573,7 @@ func (r *RestCatalog) createSession(opts *options) (*http.Client, error) {
 	return cl, nil
 }
 
-func (r *RestCatalog) fetchConfig(opts *options) (*options, error) {
+func (r *RestCatalog) fetchConfig(opts *options) (*http.Client, *options, error) {
 	params := url.Values{}
 	if opts.warehouseLocation != "" {
 		params.Set(keyWarehouseLocation, opts.warehouseLocation)
@@ -586,12 +584,12 @@ func (r *RestCatalog) fetchConfig(opts *options) (*options, error) {
 
 	sess, err := r.createSession(opts)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	rsp, err := doGet[configResponse](context.Background(), route, []string{}, sess, nil)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	cfg := rsp.Defaults
@@ -608,12 +606,12 @@ func (r *RestCatalog) fetchConfig(opts *options) (*options, error) {
 	if uri, ok := cfg["uri"]; ok {
 		r.baseURI, err = url.Parse(uri)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 		r.baseURI = r.baseURI.JoinPath("v1")
 	}
 
-	return o, nil
+	return sess, o, nil
 }
 
 func (r *RestCatalog) Name() string             { return r.name }


Test patch:
diff --git a/catalog/registry_test.go b/catalog/registry_test.go
index 77347a7..9bace89 100644
--- a/catalog/registry_test.go
+++ b/catalog/registry_test.go
@@ -88,6 +88,67 @@ func TestCatalogRegistry(t *testing.T) {
 	}, catalog.GetRegisteredCatalogs())
 }
 
+func TestRegistryPanic(t *testing.T) {
+	assert.PanicsWithValue(t, "catalog: RegisterCatalog catalog factory is nil", func() { catalog.Register("foobar", nil) })
+}
+
+func TestCatalogWithEmptyName(t *testing.T) {
+	config.EnvConfig.DefaultCatalog = "test-default"
+	config.EnvConfig.Catalogs = map[string]config.CatalogConfig{
+		"test-default": {
+			URI:         "http://localhost:8181/",
+			Credential:  "default-credential",
+			Warehouse:   "/default/warehouse",
+			CatalogType: "mock",
+		},
+	}
+	catalog.Register("mock", catalog.RegistrarFunc(func(name string, props iceberg.Properties) (catalog.Catalog, error) {
+		// Ensure the correct name and properties are passed
+		assert.Equal(t, "test-default", name)
+		assert.Equal(t, "http://localhost:8181/", props.Get("uri", ""))
+		assert.Equal(t, "default-credential", props.Get("credential", ""))
+		assert.Equal(t, "/default/warehouse", props.Get("warehouse", ""))
+		return nil, nil
+	}))
+	c, err := catalog.Load("", nil)
+	assert.Nil(t, c)
+	assert.NoError(t, err)
+	assert.ElementsMatch(t, []string{
+		"rest",
+		"http",
+		"https",
+		"glue",
+		"mock",
+	}, catalog.GetRegisteredCatalogs())
+	catalog.Unregister("mock")
+
+}
+
+func TestCatalogLoadInvalidURI(t *testing.T) {
+	config.EnvConfig.DefaultCatalog = "default"
+	config.EnvConfig.Catalogs = map[string]config.CatalogConfig{
+		"default": {
+			URI:         "http://localhost:8181/",
+			Credential:  "default-credential",
+			Warehouse:   "/default/warehouse",
+			CatalogType: "mock",
+		},
+	}
+
+	catalog.Register("mock", catalog.RegistrarFunc(func(name string, props iceberg.Properties) (catalog.Catalog, error) {
+		return nil, nil
+	}))
+	props := iceberg.Properties{
+		"uri": "://invalid-uri", // This will cause url.Parse to fail
+	}
+	c, err := catalog.Load("mock", props)
+
+	assert.Nil(t, c)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "failed to parse catalog URI")
+	catalog.Unregister("mock")
+}
+
 func TestRegistryFromConfig(t *testing.T) {
 	var params url.Values
 
diff --git a/catalog/rest_test.go b/catalog/rest_test.go
index 479fd70..2d0302c 100644
--- a/catalog/rest_test.go
+++ b/catalog/rest_test.go
@@ -83,6 +83,7 @@ func (r *RestCatalogSuite) TearDownTest() {
 }
 
 func (r *RestCatalogSuite) TestToken200() {
+	const scope = "myscope"
 	r.mux.HandleFunc("/v1/oauth/tokens", func(w http.ResponseWriter, req *http.Request) {
 		r.Equal(http.MethodPost, req.Method)
 
@@ -93,7 +94,7 @@ func (r *RestCatalogSuite) TestToken200() {
 		r.Equal(values.Get("grant_type"), "client_credentials")
 		r.Equal(values.Get("client_id"), "client")
 		r.Equal(values.Get("client_secret"), "secret")
-		r.Equal(values.Get("scope"), "catalog")
+		r.Equal(values.Get("scope"), scope)
 
 		w.WriteHeader(http.StatusOK)
 
@@ -107,7 +108,8 @@ func (r *RestCatalogSuite) TestToken200() {
 
 	cat, err := catalog.NewRestCatalog("rest", r.srv.URL,
 		catalog.WithWarehouseLocation("s3://some-bucket"),
-		catalog.WithCredential(TestCreds))
+		catalog.WithCredential(TestCreds),
+		catalog.WithScope(scope))
 	r.Require().NoError(err)
 
 	r.NotNil(cat)