-
Notifications
You must be signed in to change notification settings - Fork 88
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
Add new VaultAuthGlobal type #735
Add new VaultAuthGlobal type #735
Conversation
74dca96
to
7ecc7c0
Compare
4406bf0
to
071ba35
Compare
303ce2f
to
6eaec64
Compare
Relates to #341 |
6eaec64
to
84003e2
Compare
84003e2
to
86c22bc
Compare
The resource provide a resource holding Vault auth configuration that can be shared across VaultAuth resources. A VaultAuth instance only needs to provide the authentication method and a valid vaultAuthGlobalRef. VSO will automatically merge the VaultAuthGlobal with the referring VaultAuth. This allows for a VaultAuth instance to inherit some global authentication configuration. The VaultAuthGlobal resource can be configured with one or more Vault auth method specific configuration. Given the following VaultAuthGlobal: ``` apiVersion: secrets.hashicorp.com/v1beta1 kind: VaultAuthGlobal metadata: name: default namespace: tenant-ns spec: kubernetes: audiences: - vault namespace: demo-ns mount: demo-auth-mount role: auth-role serviceAccount: default tokenExpirationSeconds: 600 ``` The referring VaultAuth would look like: ``` apiVersion: secrets.hashicorp.com/v1beta1 kind: VaultAuth metadata: name: default namespace: tenant-ns spec: method: kubernetes vaultAuthGlobalRef: default ```
86c22bc
to
b0a73a4
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.
This is looking good to me, my only real concern is that VaultConnectionRef in VaultAuthGlobal doesn't seem to be used.
Valid bool `json:"valid"` | ||
Error string `json:"error"` | ||
Conditions []metav1.Condition `json:"conditions,omitempty"` | ||
SpecHash string `json:"specHash,omitempty"` | ||
} |
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 there a way for users to see what the merged VaultAuth spec is? I wonder if in the future putting the full merged spec in the status would make sense. Would be a a little crowded though.
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.
It was definitely a consideration, since the hash of the spec is rather opaque :) I think it could make things easier to debug. I can spike in another PR to see what we think. Since we are introducing Conditions here, we could always extend it later as well.
Fixes: - vaultConnectionRef not be inherited from the global auth - firstNonZeroLen() always returned last - misc doc updates and improvements - some fixes to the credentials providers
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.
It's looking good for me.
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'm not quite sure I understand the usage of conditions on the VaultAuth status, but this all seems to work for me.
Type: "VaultAuthGlobalRef", | ||
Status: "True", | ||
ObservedGeneration: o.Generation, | ||
LastTransitionTime: metav1.NewTime(nowFunc()), |
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.
This doesn't seem like a transition time, more like an evaluation time?
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.
Yeah, we probably want the transition time to be related to whenever the ref was last updated.
var errs error | ||
|
||
var conditions []metav1.Condition |
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.
Should this be adding onto the existing conditions on the object's status? It looks like this starts over with a single entry every evaluation.
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.
Yeah, that makes sense. I will address that in a follow-on PR.
Status: "True", | ||
ObservedGeneration: o.Generation, | ||
LastTransitionTime: metav1.NewTime(nowFunc()), | ||
Reason: "VaultAuthGlobalRef", |
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 wonder if Type
and Reason
should be different?
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.
Going to address this comment in a follow-on PR
The resource provide a resource holding Vault auth configuration that can be shared across VaultAuth resources. A VaultAuth instance only needs to provide the authentication method and a valid vaultAuthGlobalRef. VSO will automatically merge the VaultAuthGlobal with the referring VaultAuth. This allows for a VaultAuth instance to inherit some global authentication configuration.
The VaultAuthGlobal resource can be configured with one or more Vault auth method specific configuration.
Given the following VaultAuthGlobal:
The referring VaultAuth would look like:
If you wanted to override the
kubernetes.role
the VaultAuth would look like:The referring
VaultAuth
's configuration always overrides itsVaultAuthGlobal
's configuration.