Skip to content

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

Merged
merged 3 commits into from
Aug 1, 2025

Conversation

javirln
Copy link
Member

@javirln javirln commented Jul 31, 2025

This pull request refines the AuditLogPublisher in nats.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:

  • Introduced 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.
  • Dynamic subject construction in Publish method: Updated the Publish method to dynamically construct specific subject names using the format audit.<target_type>.<action_type>. This allows events to be published to more granular subjects based on their type and action.

@javirln javirln self-assigned this Jul 31, 2025
@javirln javirln requested review from jiparis and migmartri July 31, 2025 11:28
javirln added 2 commits July 31, 2025 16:00
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
Copy link
Member

@jiparis jiparis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! thanks Javi

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is subjectName used yet?

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

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.

@migmartri
Copy link
Member

Is this backwards compatible with existing consumers or do we need to create new ones?

@javirln
Copy link
Member Author

javirln commented Jul 31, 2025

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 audit.> so they are still receiving events!

@migmartri
Copy link
Member

Thanks Javi

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
@javirln javirln merged commit 186984c into chainloop-dev:main Aug 1, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants