feat: graduate Notes API to dedicated notes.miloapis.com group #488
Conversation
- Create new notes.miloapis.com API group with Note (namespaced) and ClusterNote (cluster-scoped) - Implement controllers with PolicyBinding support for both resource types - Add validation and mutation webhooks for Note and ClusterNote - Generate CRDs for notes.miloapis.com/v1alpha1 - Configure ProtectedResources for IAM integration - Add domain note examples demonstrating all use cases - Remove Notes from crm.miloapis.com (graduated to dedicated group) - Clean up CRM API group structure for future CRM-specific resources - Update all kustomizations and configurations This allows Notes to be attached to any resource (including networking.datumapis.com/Domain) while keeping CRM API group available for future CRM-specific resources like Leads, Opportunities, and Accounts. Resolves: #[issue-number]
This comment has been minimized.
This comment has been minimized.
| return ctrl.Result{}, nil | ||
| } | ||
|
|
||
| func (r *ClusterNoteController) ensureCreatorEditorPolicyBinding(ctx context.Context, clusterNote *notesv1alpha1.ClusterNote, creator *iamv1alpha1.User) (bool, string, error) { |
There was a problem hiding this comment.
Both controllers (clusternote and note) share most of the logic. While it’s beneficial to have separate controllers, I would suggest sharing all of the functions that would be common to both.
JoseSzycho
left a comment
JoseSzycho
left a comment
There was a problem hiding this comment.
Overall, great work.
Just left some comments about sharing functionalities and setting up the resources owners.
… collection Controllers: - Extract common PolicyBinding logic to shared.go - Add NoteResource interface implemented by Note and ClusterNote - Share ensureCreatorEditorPolicyBinding() and isPolicyBindingReady() functions - Eliminate code duplication between Note and ClusterNote controllers Webhooks: - Add owner reference to subject resource in Defaulter for automatic garbage collection - Note: sets owner reference if subject is in same namespace - ClusterNote: sets owner reference if subject is cluster-scoped - Non-blocking: webhook succeeds even if owner reference fails to set This ensures notes are automatically deleted when their subject resource is deleted, and reduces code duplication by sharing common controller logic.
Thanks for the feedback @JoseSzycho ! I've implemented both suggestions. Let me know your feedback again please |
|
|
||
| var _ admission.CustomValidator = &ClusterNoteValidator{} | ||
|
|
||
| func (v *ClusterNoteValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { |
There was a problem hiding this comment.
We should add a validation here, if the subjectRef is not cluster scoped, we should raise an exception.
|
|
||
| var _ admission.CustomValidator = &NoteValidator{} | ||
|
|
||
| func (v *NoteValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { |
There was a problem hiding this comment.
We should validate that the subject reference is namespaced, and that the note namespace matches the subject namespace.
| if err := m.setSubjectOwnerReference(ctx, note); err != nil { | ||
| noteLog.Error(err, "Failed to set owner reference to subject", "note", note.Name) | ||
| // Don't fail the webhook if we can't set the owner reference | ||
| // The note will still be created, just without automatic garbage collection | ||
| } |
There was a problem hiding this comment.
We should prevent the note creation if we were not able to setup the owner reference.
The entire work of separating the notes on a namespaced one, and a cluster scoped one, is for being able to setup the notes for garbage collection.
JoseSzycho
left a comment
JoseSzycho
left a comment
There was a problem hiding this comment.
Looks great! I really like the use of gvk, so the notes can be used for any resources.
I’ve left a few minor comments.
What do you think if we rename Note to NamespaceNote? This way, API consumers won’t be confused about when to use each.
Please add some unit tests for the webhooks and end-to-end tests for both note types. You should be able to verify that the garbage collection works and that the creatorRef is correctly set.
This allows Notes to be attached to any resource (including networking.datumapis.com/Domain) while keeping CRM API group available for future CRM-specific resources like Leads, Opportunities, and Accounts.
Needed for datum-cloud/enhancements#501