-
Notifications
You must be signed in to change notification settings - Fork 273
feat: Add grafana_cloud_access_policy_rotating_token resource
#2390
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
feat: Add grafana_cloud_access_policy_rotating_token resource
#2390
Conversation
|
In order to lower resource usage and have a faster runtime, PRs will not run Cloud tests automatically. |
3073f18 to
4f7fc33
Compare
|
Looks like #2394 will fix the acceptance tests |
…otate-after-new-resource
…otate-after-new-resource
dmihai
left a comment
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.
LGTM, I just have some minor comments. I leave it up to you if you want to address them. Thank you!
internal/resources/cloud/resource_cloud_access_policy_rotating_token.go
Outdated
Show resolved
Hide resolved
internal/resources/cloud/resource_cloud_access_policy_rotating_token.go
Outdated
Show resolved
Hide resolved
internal/resources/cloud/resource_cloud_access_policy_rotating_token.go
Outdated
Show resolved
Hide resolved
internal/resources/cloud/resource_cloud_access_policy_rotating_token.go
Outdated
Show resolved
Hide resolved
internal/resources/cloud/resource_cloud_access_policy_rotating_token.go
Outdated
Show resolved
Hide resolved
FYI @volcanonoodle discussed offline and decided to experiment with pursuing this. We agreed it makes for a cleaner UX. He's going to work on that over the next couple of days. |
internal/resources/cloud/resource_cloud_access_policy_rotating_token.go
Outdated
Show resolved
Hide resolved
internal/resources/cloud/resource_cloud_access_policy_rotating_token.go
Outdated
Show resolved
Hide resolved
Duologic
left a comment
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.
General approve on the idea, didn't go in depth, folks on the team working on this are probably better suited.
Rydez
left a comment
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.
Spent some time looking through everything yesterday and, from my end, I think this looks good now that #2405 is merged in. Nice Work!
|
I'm planning on merging this and creating a minor release of the Terraform provider on Monday |
Related to #1705
Design decisions
Separate resource
Originally we planned on updating the
grafana_cloud_access_policy_tokenresource with a couple of new fields to specify the rotation parameters and a couple of new fields to specify thecomputed_nameandcomputed_expires_at, but since it created a bit of a split behaviour on the resource depending on whether rotation was enabled or not, we ended up going with a separate resource for rotating tokens.We have extracted as much logic into common functions that can be shared by both the existing token resource and the new rotating token resource, such as most of each of the CRUD operations and the common schema fields between the 2 resources.
Computed
nameSince Grafana Cloud tokens have the restriction of having unique names within an org, and we need to have both the old and new tokens co-exist when a rotation happens, we need to ensure that the each token's name is unique. We accomplish that by adding strings to each token's name that are unique for each rotation, like
rotate_afterandpost_rotation_lifetime. By doing this, we also get to use the name as the "cloud" storage for those fields, since otherwise these would only live in Terraform because the API does not store them and we'd not be able to set them when importing a rotating token into Terraform, for example.Computed
expires_atfromrotate_afterandpost_rotation_lifetimefieldsFor rotating tokens,
expires_atis not user-defined. It's computed from adding therotate_aftertimestamp and thepost_rotation_lifetimeduration. If a user wants their token to expire in 3 months and they want to rotate it at some point in the last month of that period, they should setrotate_after = <timestamp of 2 months in the future>andpost_rotation_lifetime = 30d.Ideas considered that were not implemented but are not completely discarded either
-v1suffix to the rotating tokens'name. This way if we need to evolve the resource, we have an easy way to distinguish between existing rotating tokens from the current provider version and rotating tokens from a newer version of the provider which might use a different pattern to compute thenamefield.rotate_afteran RFC3339 string, as opposed to a unix timestamp, which was the original idea. The former seems better for humans but we need to add diff-suppression logic to take timezone differences into account, since 2 RFC3339 strings can represent the same time in different timezones but swapping between them would trigger a resource renewal by default in Terraform. However, TF hasstatic_timeandrotation_timeresources that allow turning RFC3339 strings into unix timestamps, so it might not be a big deal to leave it as a unix timestamp.expires_atuser-defined, as the hard expiration time, and just add arotate_beforefield that would specify how early the token can be rotated before it expires. So essentially, any TF plan that happened afterexpires_at - rotate_beforewould trigger a rotation, while the token would continue to live until itsexpires_attime. It would replace therotate_afterandpost_lifetime_rotationfields. It makes it easier to reason about the date at which the token will expire, but it might make it more difficult to think about the rotation timings.Additionally it fixes a problem with Cloud Access Policy token tests using LBAC with scopes that aren't compatible with it in 7363cbc