Skip to content

Conversation

@nastra
Copy link
Contributor

@nastra nastra commented Oct 8, 2024

This depends on the standardized credentials from #10722

@nastra nastra force-pushed the openapi-refresh-vended-credentials branch from f51b060 to e8bad6d Compare October 10, 2024 09:52
type: string
description: Indicates a storage location prefix where the credential is relevant. Clients should choose the most
specific prefix if several credentials of the same type are available.
config:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that there should be a "top level" expiresAt field, so implementations do not have to peal out this common piece of information from the config property bag. If expiresAt is missing, that would mean the credentials do not expire.

Maybe also an optional refreshAt or refreshNotBefore timestamp "top level" field as well, so the server can tell clients when a refresh should happen and not have clients refresh too early (or too late).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
config:
expiresAt:
type: integer
format: int64
description: Timestamp in milliseconds since epoch (1970-01-01-00:00:00Z) when the credentials expire. If credentials do not expire, this field is absent.
refreshNotBefore:
type: integer
format: int64
description: If this field is present, clients must not refresh credentials before this timestamp in milliseconds since epoch (1970-01-01-00:00:00Z). This field must not be present for credentials that do not expire.
config:

Copy link
Contributor Author

@nastra nastra Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could make sense to have a expires-at-ms field here, but I believe it's better if this isn't part of the OpenAPI spec and be rather specific to the underlying storage provider. That being said, I think it should be documented along with other properties related to that storage provider. The underlying refresh handler for the given storage provider can still mandate that the server sends the expiration field for a vended credential

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather specific to the underlying storage provider

The whole effort is about refreshing expiring credentials, so such an attribute is a first-class citizen, not something hidden in a rather generic property bag.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to me to have an "expires at" field of some sort. It might help make the refresh implementation logic more reusable across different providers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we discussed this in yesterday's catalog sync and concluded that we don't want to introduce a separate expires-at-ms field to the spec.

properties:
prefix:
type: string
description: Indicates a storage location prefix where the credential is relevant. Clients should choose the most
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have a definition here how prefixes must be described.
I can think of:

  • schema: (including the colon :) to indicate that the credentials are valid for all FileIOs that use this schema
  • schema://bucket/ (including the trailing slash /) for credentials for a specific bucket
  • schema://bucket/some/path/ (including the trailing slash /) for credentials for a specific path in a specific bucket

I think it's important to define whether a trailing slash / must, should, or must not be included in the prefix to have a common definition across all implementations and behavior on the client side.

Mandating a trailing slash makes paths non-ambiguous (e.g. s3://foo/bar/ vs s3://foo/bar in case there are paths like s3://foo/bar/meep and s3://foo/barbaz/meep). WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be calling out specific things related to storage providers here on how pathing is used. This just makes it more difficult to evolve the OpenAPI spec

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But where else should it be specified/documented?
Javadoc is rather a bad place, because it's Java-specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can talk/think about what the best place would be for documenting this, but I don't think the OpenAPI spec is the right place to handle storage-specific paths

properties:
prefix:
type: string
description: Indicates a storage location prefix where the credential is relevant. Clients should choose the most
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect, it's worth to mention that both the URI's authority and path elements must be URL escaped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is up to the server to provide a valid prefix URL. As I mentioned further above, I don't think mentioning storage-specific paths in the OpenAPI spec is something that we'd want to do

@findepi
Copy link
Member

findepi commented Oct 12, 2024

cc @c-thiel

@nastra nastra force-pushed the openapi-refresh-vended-credentials branch from e8bad6d to 67fb951 Compare October 18, 2024 17:29
@nastra nastra changed the title OpenAPI: Add endpoint for refreshing vended credentials OpenAPI: Add endpoint to retrieve valid credentials for a given table Oct 22, 2024
@nastra nastra merged commit 35a02d0 into apache:main Oct 24, 2024
3 checks passed
@nastra nastra deleted the openapi-refresh-vended-credentials branch October 24, 2024 14:07
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants