-
Notifications
You must be signed in to change notification settings - Fork 38
chore(nats): Send audit logs events to specific subjects #2300
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
Conversation
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
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.
Nice! thanks Javi
app/controlplane/pkg/auditor/nats.go
Outdated
@@ -27,8 +28,12 @@ import ( | |||
) | |||
|
|||
const ( | |||
streamName = "chainloop-audit" | |||
streamName = "chainloop-audit" | |||
// subjectName is the base subject for audit logs for the consumer to subscribe to. |
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 subjectName
used yet?
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 will check if it's being used by another service. I think I saw something that's why I left it but I'll double check it
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 used during stream creation. Not sure if it's a mandatory argument, but we can keep it anyways.
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’ve checked, and we do need the subjectName
here. This function creates or updates a stream when Chainloop starts. If no subject is specified, the default will be the stream name, as explained here. This would cause issues with other services that rely on specific subjects.
if _, err := js.CreateOrUpdateStream(ctx, jetstream.StreamConfig{
Name: "chainloop-audit",
// No Subjects specified
}); err != nil {
return nil, fmt.Errorf("failed to create stream: %w", err)
}
The stream would only listen to messages published to the subject chainloop-audit
(same as the stream name), NOT to all subjects.
Is this backwards compatible with existing consumers or do we need to create new ones? |
We have tested it and all consumers we have are subscribed to |
Thanks Javi |
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
This pull request refines the
AuditLogPublisher
innats.go
to improve the flexibility and granularity of audit log publishing. The changes introduce a new subject naming convention and update the publishing logic to dynamically construct specific subject names based on event types.Enhancements to audit log publishing:
baseSubjectName
constant: Added a new constant,baseSubjectName
, to define the base subject for audit log publishing. This enables a more structured subject naming pattern for publishers.Publish
method: Updated thePublish
method to dynamically construct specific subject names using the formataudit.<target_type>.<action_type>
. This allows events to be published to more granular subjects based on their type and action.