-
Notifications
You must be signed in to change notification settings - Fork 37
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
Added ClusterStaticEntry support #174
Conversation
Signed-off-by: Andrew Harding <[email protected]>
ddfeb88
to
fb9bcf5
Compare
The SPIRE APIs used by the SPIRE Controller Manager are generally stable and | ||
supported since at least SPIRE v1.0. However, the API has gained support for | ||
additional entry fields beyond what was supported in SPIRE v1.0. Notably, these | ||
include both the `jwt_svid_ttl` and the `hint` fields. The ClusterStaticEntry |
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 know there is an issue to add jwt_svid_ttl to regular reconciler, and we may add another one for hint,
are you worried about adding those fields there too?
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.
jwt_svid_ttl makes sense but hint is harder to reason about. I'd probably side with waiting until somebody has a use case for it before adding it to the ClusterSPIFFEID CRD.
@@ -0,0 +1,76 @@ | |||
/* | |||
Copyright 2021 SPIRE Authors. |
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.
since it is a new crd, may we update year to 2023? and what happens with another crds that got updates?
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 we'd only bump the year if there were significant updates. I'll change the year for this CRD.
x509SVIDTTL: | ||
type: string | ||
required: | ||
- dnsNames |
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.
is it intntional to make dns, federatesWith, hint and ttls required?
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.
Good catch. I've updated the json tags and regenerated manifests.
Signed-off-by: Andrew Harding <[email protected]>
00b00f8
to
2a0b383
Compare
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.
we should complete the sample here no? else what would users have to anchor on?
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!!!
Nice! This is definitely needed - what about the |
The -node flag doesn't actually have a first class field on the entry. Instead it sets the parent ID to the SPIFFE ID of the spire server (i.e., |
@azdagron when do you think will we see this in a release? |
This will be released in 0.3.0. There are currently two outstanding issues that have to be fixed before we can release the ClusterStaticEntry CRD. https://github.com/spiffe/spire-controller-manager/milestone/1 I'm personally slammed this week but may have some cycles next week to wrap this up. Unless @MarcosDY has some cycles, it will probably be at least then. |
We should probably also create a migration guide. |
Yeah.... upgrading the chart will likely have some issues with the crds. |
Resolves #149