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.

Sign up for GitHub

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

Jump to bottom

adding support for JWT SVID TTL #189

Merged
merged 5 commits into from
Aug 17, 2023
Merged

adding support for JWT SVID TTL #189

merged 5 commits into from
Aug 17, 2023

Conversation

unnathik
Copy link
Contributor

  • added JWTTTL to ClusterSPIFFEIDSpec
  • added JWTTTL to renderPodEntry func
  • added test to check if entry JWT TTL matches the passed in JWT TTL
  • closes Add support to JWT SVID Ttl #65

@unnathik unnathik marked this pull request as ready for review July 28, 2023 17:43
@unnathik unnathik changed the title adding support to JWT SVID TTL adding support for JWT SVID TTL Jul 28, 2023
MarcosDY
MarcosDY reviewed Aug 1, 2023
View reviewed changes
Copy link
Collaborator

@MarcosDY MarcosDY left a comment

Choose a reason for hiding this comment

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

Looks great! just a minor comment

docs/clusterspiffeid-crd.md Outdated
Comment on lines 24 to 25
| `ttl` | OPTIONAL | Duration value indicating an upper bound on the time-to-live for X509-SVIDs issued to target workload |
| `jwtTtl` | OPTIONAL | Duration value indicating an upper bound on the time-to-live for JWT-SVIDs issued to target workload |
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you solve this indentation issue?

Suggested change
| `ttl` | OPTIONAL | Duration value indicating an upper bound on the time-to-live for X509-SVIDs issued to target workload |
| `jwtTtl` | OPTIONAL | Duration value indicating an upper bound on the time-to-live for JWT-SVIDs issued to target workload |
| `ttl` | OPTIONAL | Duration value indicating an upper bound on the time-to-live for X509-SVIDs issued to target workload |
| `jwtTtl` | OPTIONAL | Duration value indicating an upper bound on the time-to-live for JWT-SVIDs issued to target workload |

@unnathik
indentation fix
7caac51 
Signed-off-by: Unnathi Kumar <[email protected]>
@unnathik unnathik requested a review from MarcosDY August 1, 2023 16:11
MarcosDY
MarcosDY previously approved these changes Aug 2, 2023
View reviewed changes
Copy link
Collaborator

@MarcosDY MarcosDY left a comment

Choose a reason for hiding this comment

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

LGTM!

@unnathik @faisal-memon
Update pkg/spireentry/entries_test.go
fcdcc0c 
Co-authored-by: Faisal Memon <[email protected]>
Signed-off-by: Unnathi Kumar <[email protected]>
@unnathik unnathik requested a review from MarcosDY August 7, 2023 16:06
@MarcosDY MarcosDY merged commit e95aac2 into spiffe:main Aug 17, 2023
6 checks passed
@drewwells
Copy link

Sweet! I didn't fully read how this is integrated. Does this override the ttl flags passed in the config to controller manager?

@MarcosDY MarcosDY added this to the 0.3.0 milestone Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@faisal-memon faisal-memon faisal-memon left review comments

@MarcosDY MarcosDY MarcosDY approved these changes

@azdagron azdagron Awaiting requested review from azdagron azdagron is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.3.0
Development

Successfully merging this pull request may close these issues.

Add support to JWT SVID Ttl
4 participants
@unnathik @drewwells @faisal-memon @MarcosDY