zeroshade commented on code in PR #2546: URL: https://github.com/apache/iceberg-go/pull/2546#discussion_r4543175499
########## internal/remotesigning/config.go: ########## @@ -0,0 +1,120 @@ +// 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 remotesigning shares REST-provided signing settings with FileIOs. +package remotesigning + +import ( + "context" + "errors" + "net/http" + "net/url" +) + +const ( + EndpointProperty = "rest.remote-signing.endpoint" + ConfigProperty = "rest.remote-signing.config" + TokenProperty = "rest.remote-signing.token" +) + +// Config is the REST catalog's remote-signing-config wire format. +type Config struct { + Properties map[string]string `json:"properties,omitempty"` + Headers http.Header `json:"headers,omitempty"` +} + +// Session keeps table defaults outside the properties so refreshed or +// prefix-scoped credentials can still override the signing endpoint. +type Session struct { + Client *http.Client + Endpoint string + CatalogURI string +} + +type sessionKey struct{} + +func WithSession(ctx context.Context, session Session) context.Context { + return context.WithValue(ctx, sessionKey{}, session) +} + +func SessionFromContext(ctx context.Context) Session { + session, _ := ctx.Value(sessionKey{}).(Session) + + return session +} + +// Endpoint resolves modern and legacy explicitly configured signing endpoints. +// The caller supplies the default when it knows the table's catalog path. +func Endpoint(props map[string]string, fallback string) (string, error) { + endpoint := props[EndpointProperty] + if endpoint == "" { + endpoint = props["signer.endpoint"] Review Comment: The [REST spec](https://github.com/apache/iceberg/blob/ab8a86d5e5fd173f682142573de12d3e978fbe7b/open-api/rest-catalog-open-api.yaml#L4309-L4318) says that when `remote-signing-config` is present, `signer.endpoint`/`signer.uri` "are still supported but **DEPRECATED** and SHOULD NOT be used by clients able to consume the remote signing configuration". Here the legacy keys win over the table-scoped default whenever they're set. A server that sends both, to keep older clients working, would make iceberg-go sign against the deprecated endpoint (`<signer.uri>/v1/aws/s3/sign`) instead of the table's `/sign`. Could `Endpoint` skip the `signer.endpoint`/`signer.uri`/`s3.signer.uri` fallback when `ConfigProperty` is set, so that only `rest.remote-signing.endpoint` or the table-scoped default applies in that case? Please also add a test where both are present. -- 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]
