-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 exp
field to JWT if expires_in
is set
#6429
feat: add exp
field to JWT if expires_in
is set
#6429
Conversation
This commit adds the ability to add an `exp` field to the signed JWT. While Terraform has some time-related functions (e.g. `timestamp` and `formatdate`), there does not appear to be a Terraform-native way of generating a _unix_ timestamp for the current time. Without this feature it would be impossible for a user to set `exp` to a meaningful value themselves.
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @c2thorn, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 55 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange|TestAccSqlDatabaseInstance_SqlServerAuditConfig|TestAccComputeInstance_soleTenantNodeAffinities|TestAccComputeInstance_networkPerformanceConfig|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccCloudFunctions2Function_fullUpdate|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
I don't believe the VCR test failures are related to my change, so I've marked this as ready for review. |
Thanks for the contribution @ericnorris! Just stating here that this is on my TODO, just have been running a bit behind on reviews. I'm not as familiar with the resource so I need to gain context first. Thanks for your patience. |
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.
Hi @ericnorris, apologies for the delay. I spoke with my team, and there are concerns about this field playing well with Terraform's plan time validation of datasources. TF assumes that old Datasource values are valid at plan time. This field would add volatility that leave us unsure with the edge cases of.
It would be more comfortable if this was modeled as part of a resource instead, as it has more flexibility. The resource would be extremely similar in structure, but this is fairly common for other resources/datasource pairs.
Hey @c2thorn, no worries. I'm not sure this would make sense as a resource, as it would need to get destroyed and recreated on essentially every run, or else its output would be invalid. That is, if the generated JWT resource had an I'll add that I don't think this would cause a problem with terraform at plan time, and in fact there are some time-based data sources in the provider already, see: magic-modules/mmv1/third_party/terraform/data_sources/data_source_storage_object_signed_url.go Line 116 in ac62831
The data source generates a signed URL that will expire in some amount of time, just like my proposed change to the JWT data source, making it equally as "unstable". |
Thanks for pointing this out, I was not aware of this. That is very similar to this case and is working. I'll move forward with this PR |
dataSourceGoogleServiceAccountJwtNow = func() time.Time { | ||
return staticTime | ||
} | ||
|
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 think testing a scenario where the expires_in
is updated would help me feel like we aren't missing some edge case. Would you mind adding another step in this existing test?
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 65 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccCloudFunctions2Function_fullUpdate|TestAccCGCSnippet_eventarcWorkflowsExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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.
updated test passed. Thank you for your effort and patience here @ericnorris
This commit adds the ability to add an
exp
field to the signed JWT. While Terraform has some time-related functions (e.g.timestamp
andformatdate
), there does not appear to be a Terraform-native way of generating a unix timestamp for the current time. Without this feature it would be impossible for a user to setexp
to a meaningful value themselves.If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)