-
Notifications
You must be signed in to change notification settings - Fork 1
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
Invitation API and Storage #98
Conversation
43eab87
to
aeb8c40
Compare
aeb8c40
to
a043535
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.
LGTM, just a few nitpicks and don't forget RBAC I think the apiserver doesn't yet have access to secrets.
|
||
require.NoError(t, c.Get(ctx, types.NamespacedName{Name: subject.Name}, &subject)) | ||
assert.NotEmpty(t, subject.Status.Token) | ||
assert.WithinDuration(t, time.Now().Add(tokenValidFor), subject.Status.ValidUntil.Time, time.Second) |
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.
Ntipick: This is technically a race condition. I really hope that this test will complete in a second, but technically it'd be better if we faked time.Now(). However that's a bit of a PITA, so feel free to ignore this 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
} | ||
|
||
// TargetRef is a reference to a target resource | ||
type TargetRef struct { |
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.
There's probably a good reason why we define TargetRef
ourselves, but I'm wondering if we could just use the standard ObjectReference
type instead.
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 the reason was looking into the RoleBinding
subjects instead of other references. Those don't have the right documentation when generated.
Using ObjectReference
seems like a good idea but will introduce more validation overhead since there are more fields to respect and we'll most likely never use any of them.
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 fine with having our own ref type especially since that will give us flexibility if we figure out that we need something that we can't get from ObjectReference
. Additionally, there seems to be no clear consensus even in the core K8s types on whether you're supposed to use LocalObjectReference
/ObjectReference
vs your own ref types
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 lack a lot of the context on this change so I can't say much about the overall approach. I didn't find any obvious issues in the code though, fwiw
i did as always 😅 |
Summary
Adds an
Invitation
object and stores it in a genericsecretstorage
.The status handling of the resource is modeled after the official interfaces, but has prefixed methods to not trigger a storage registration bug.
Adds a reconciler for the newly created
Invitation
object that adds a token to the invitation.apiserver/secretstorage/storage.go
is the most important to review.Checklist
Still way to big. Sorry 🤷♀️
bug
,enhancement
,documentation
,change
,breaking
,dependency
as they show up in the changelog.