You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "curtisr7 (via GitHub)" <gi...@apache.org> on 2025/01/22 00:49:41 UTC
[PR] Make rest catalog auth scope configurable [iceberg-go]
curtisr7 opened a new pull request, #265:
URL: https://github.com/apache/iceberg-go/pull/265
As part of debugging the test update I noticed that multiple http clients are created each time a rest client is created
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(catalog): Make rest catalog auth scope configurable [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #265:
URL: https://github.com/apache/iceberg-go/pull/265
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(catalog): Make rest catalog auth scope configurable [iceberg-go]
Posted by "curtisr7 (via GitHub)" <gi...@apache.org>.
curtisr7 commented on code in PR #265:
URL: https://github.com/apache/iceberg-go/pull/265#discussion_r1925812970
##########
catalog/catalog.go:
##########
@@ -144,6 +150,7 @@ type options struct {
sigv4Service string
prefix string
authUri *url.URL
+ scope *string
Review Comment:
> Is there any particular benefit to differentiating that?
Likely not. I've pushed a commit to fix
##########
catalog/rest.go:
##########
@@ -491,11 +485,15 @@ func (r *RestCatalog) fetchAccessToken(cl *http.Client, creds string, opts *opti
clientID, clientSecret = "", clientID
}
+ scope := "catalog"
+ if opts.scope != nil {
+ scope = *opts.scope
+ }
Review Comment:
👍
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(catalog): Make rest catalog auth scope configurable [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #265:
URL: https://github.com/apache/iceberg-go/pull/265#discussion_r1925623222
##########
catalog/rest.go:
##########
@@ -491,11 +485,15 @@ func (r *RestCatalog) fetchAccessToken(cl *http.Client, creds string, opts *opti
clientID, clientSecret = "", clientID
}
+ scope := "catalog"
+ if opts.scope != nil {
+ scope = *opts.scope
+ }
Review Comment:
if it's not a pointer we can just do `if opts.scope != ""` and avoid the heap
##########
catalog/rest.go:
##########
@@ -491,11 +485,15 @@ func (r *RestCatalog) fetchAccessToken(cl *http.Client, creds string, opts *opti
clientID, clientSecret = "", clientID
}
+ scope := "catalog"
+ if opts.scope != nil {
+ scope = *opts.scope
+ }
Review Comment:
if it's not a pointer we can just do `if opts.scope != ""` and avoid the pointer
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(catalog): Make rest catalog auth scope configurable [iceberg-go]
Posted by "curtisr7 (via GitHub)" <gi...@apache.org>.
curtisr7 commented on code in PR #265:
URL: https://github.com/apache/iceberg-go/pull/265#discussion_r1925733042
##########
catalog/catalog.go:
##########
@@ -144,6 +150,7 @@ type options struct {
sigv4Service string
prefix string
authUri *url.URL
+ scope *string
Review Comment:
Yea, I can change it. Intent was to differentiate between empty and not set
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(catalog): Make rest catalog auth scope configurable [iceberg-go]
Posted by "jhump (via GitHub)" <gi...@apache.org>.
jhump commented on PR #265:
URL: https://github.com/apache/iceberg-go/pull/265#issuecomment-2790112264
> As part of debugging the test update I noticed that multiple http clients are created each time a rest client is created
FWIW, I thought this was actually intentional, to support credential vendoring. The first client is used to download properties from the catalog. Those properties are then used to augment the client's config, and the second client is then built using that updated config.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(catalog): Make rest catalog auth scope configurable [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #265:
URL: https://github.com/apache/iceberg-go/pull/265#discussion_r1925619803
##########
catalog/catalog.go:
##########
@@ -144,6 +150,7 @@ type options struct {
sigv4Service string
prefix string
authUri *url.URL
+ scope *string
Review Comment:
could this be a string and not a pointer? just use the zero-value (empty string) to indicate that we should use the default?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(catalog): Make rest catalog auth scope configurable [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #265:
URL: https://github.com/apache/iceberg-go/pull/265#discussion_r1925801040
##########
catalog/catalog.go:
##########
@@ -144,6 +150,7 @@ type options struct {
sigv4Service string
prefix string
authUri *url.URL
+ scope *string
Review Comment:
Is there any particular benefit to differentiating that? I don't think that an empty scope is allowed by OAuth, right?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org