make AWS_ROLE_SESSION_NAME optional #265
Conversation
|
bors r+ |
|
Build failed: |
|
bors r+ |
|
Build failed: |
|
Bump. |
|
Is the build failure on julia 1.3 a blocker? Seems to have to do with auth expiration times not changing as expected, I'm not understanding what that has to do with this PR or with the julia version. |
|
The module is already |
AWS docs indicate that it should be optional. If unspecified, a default is set and warning logged.
Co-authored-by: Curtis Vogt <curtis.vogt@gmail.com>
Co-authored-by: Curtis Vogt <curtis.vogt@gmail.com>
|
bors try |
tryBuild failed: |
|
After running into this issue myself I realized that the warning would get old very quickly. In order to improve the situation I added the role name into the default session name. Using this and automatically restricting the role session name to 64 characters (the session name limit) I think the default is definitely good enough that we don't need to promote having users specify their own role names. |
|
bors try |
|
bors try |
|
I've validated this behaviour works correctly in a live AWS session |
|
@mattBrzezinski do you want to give this another look over? |
Yeah I can take a look at this tonight / tomorrow morning and merge it in afterwards! |
| _role_session_name( | ||
| "AWS.jl-role-", | ||
| basename(role_arn), | ||
| "-" * Dates.format(@mock(now(UTC)), dateformat"yyyymmdd\THHMMSS\Z"), |
There was a problem hiding this comment.
Dates.format(@mock(now(UTC)), dateformat"yyyymmdd\THHMMSS\Z")
Maybe we pull this out as an internal function, a similar piece of code already exists in test/runtests.jl
There was a problem hiding this comment.
I'll look into that. Do you happen to know why that test function uses lowercase?
There was a problem hiding this comment.
Not sure this is ported over from AWSCore.jl, the only difference here is that the t becomes lowercase. I assume this is to increase readability. Removing the lowercase call wouldn't affect anything in terms of running on the AWS account, feel free to remove it.
julia> using Dates
julia> Dates.format(now(Dates.UTC), dateformat"yyyymmdd\THHMMSSsss\Z")
"20210331T145218874Z"
julia> lowercase(Dates.format(now(Dates.UTC), dateformat"yyyymmdd\THHMMSSsss\Z"))
"20210331t145225679z"There was a problem hiding this comment.
I'm not sure it's worth it to refactor this into a separate function. The _now_formatted function includes milliseconds (which we could use here) and there are several different DateFormats used throughout the repo already:
src/AWS.jl:224: query["Expires"] = Dates.format(time + Dates.Minute(2), dateformat"yyyy-mm-dd\THH:MM:SS\Z")
src/AWS.jl:246: date = Dates.format(time, dateformat"yyyymmdd")
src/AWS.jl:247: datetime = Dates.format(time, dateformat"yyyymmdd\THHMMSS\Z")
src/AWSCredentials.jl:482: "-" * Dates.format(@mock(now(UTC)), dateformat"yyyymmdd\THHMMSS\Z"),
test/AWS.jl:49: date = Dates.format(time, dateformat"yyyymmdd")
test/AWS.jl:88: expected_x_amz_date = Dates.format(time, dateformat"yyyymmdd\THHMMSS\Z")
test/runtests.jl:33: return lowercase(Dates.format(now(Dates.UTC), dateformat"yyyymmdd\THHMMSSsss\Z"))
As some of these formats are required to be specific to follow spec and others are just there to make identifiers unique I think there isn't a point in extracting this date formatting code. I could see some further refactoring of the _role_session_name to automatically add a timestamp suffix and possibly use it with other AWS services that support names
|
bors r+ |
AWS docs indicate that it should be optional.
If unspecified, a default is set and warning logged.