Skip to content
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

Support max session duration for roles #18

Merged
merged 3 commits into from
Oct 4, 2021

Conversation

patrobinson
Copy link
Contributor

Context

Max session duration allows you to allow users to assign a role for longer than the default 1h

Changes

This PR adds support by adding an additional field to IAMY roles and the necessary plumbing to make that work

Discussion

I feel a little uncomfortable with how marshalRoleAsync works, by updating a pointer to a struct "owned" by another go routine. Go race condition detector doesn't detect anything, but it feels like a foot gun for later development.

$ go test ./... -race
ok  	github.com/envato/iamy	0.051s
ok  	github.com/envato/iamy/iamy	0.072s

Patrick Robinson added 2 commits September 3, 2021 10:22
Without this change everyone would get spurious updates to their roles
Adding "MaxSessionDuration: 3600" to every role with no value

This change is difficult to test because there's no
existing test suite for pulls
@andrewjhumphrey
Copy link
Contributor

Discussion

I feel a little uncomfortable with how marshalRoleAsync works, by updating a pointer to a struct "owned" by another go routine. Go race condition detector doesn't detect anything, but it feels like a foot gun for later development.

I think we are all okay as the consumer of the struct (the diff generator) is gated by the WaitGroup around the fetching isn't it?

Copy link
Contributor

@andrewjhumphrey andrewjhumphrey left a comment

Choose a reason for hiding this comment

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

Nice addition!
Happy enough with the shape of this, I added a couple of single comments but nothing to stop merging.

@andrewjhumphrey
Copy link
Contributor

Going to roll this into the next release, so merging.

@andrewjhumphrey andrewjhumphrey merged commit a755bfc into main Oct 4, 2021
@andrewjhumphrey andrewjhumphrey deleted the support-max-session-duration branch October 4, 2021 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants