-
Notifications
You must be signed in to change notification settings - Fork 314
Support S3 storage that does not have STS #2672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -98,6 +98,12 @@ public URI getInternalEndpointUri() { | |||||
/** Flag indicating whether path-style bucket access should be forced in S3 clients. */ | ||||||
public abstract @Nullable Boolean getPathStyleAccess(); | ||||||
|
||||||
/** | ||||||
* Flag indicating whether STS is available or not. It is modeled in the negative to simplify | ||||||
* support for unset values ({@code null} being interpreted as {@code false}). | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Javadoc is correct, I believe 😅 A null (unset) value would be treated as I believe interpreting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, sorry for the misleading. I was confused by two new methods introduced, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I removed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for doing that, this LGTM. |
||||||
*/ | ||||||
public abstract @Nullable Boolean getStsUnavailable(); | ||||||
|
||||||
/** Endpoint URI for STS API calls */ | ||||||
@Nullable | ||||||
public abstract String getStsEndpoint(); | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be worth expanding this sentence with a bit more of what's in the yaml for easy reference if people are reading the notes without wanting to read the full spec change -- specifically to clarify that it disables credential vending rather than vending some kind of root credentials after skipping STS subscoping.
How does the behavior compare to setting
SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION=true
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION=true
disables all storage config in API responses. Disabling STS only removes credentials.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: message changes: I'm not sure it's worth adding YAML to CHANGELOG... Would it not make it too verbose? In the end CHANGELOG has notes for many releases 🤔
How about I update it with a CLI example when I add CLI support for the new config entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that from an user standpoint, CLI (or
curl
:) ) and updating the configuration list on the documentation would be more than welcome.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will certainly add a docs page about this to "getting started" once the impl. is approved :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dennishuo : are you ok with this path forward? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, to clarify, I didn't mean to add too much, just maybe adding a
; credential vending will then be disabled
in the parentheses so that the change note would sayIn any case, non-blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incidentally,
SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION
really was only intended to pertain to the "credential" aspect, so it's unfortunate that it sort of fell into fully disabling StorageConfig mix-ins even for the non-credential-related config. But we can sort that out some other time.