Skip to content
This repository was archived by the owner on Jun 11, 2025. It is now read-only.

Refactor/nats subject change#356

Merged
nxtcoder17 merged 9 commits into
release-v1.0.7from
refactor/nats-subject-change
Jul 26, 2024
Merged

Refactor/nats subject change#356
nxtcoder17 merged 9 commits into
release-v1.0.7from
refactor/nats-subject-change

Conversation

@nxtcoder17
Copy link
Copy Markdown
Member

@nxtcoder17 nxtcoder17 commented Jul 26, 2024

This PR introduces enhancements and fixes focusing on security, infrastructure improvements, and logging enhancements across various modules.

Security Updates

Infrastructure and Logging Improvements

  • NATS subject changes and logging improvements:
    • Refactored NATS subject to resolve storage and performance issues.
    • Enhanced logging to provide better insight into NATS client operations and interactions.
  • Implemented workqueue stream subject in the message office and improved overall logging by removing duplicate dispatch logic.

Tenant-Agent Enhancements

  • Fixed inconsistent reconnect behavior in tenant-agent:
    • Cleaned up unnecessary code.
    • Improved reconnection handling and logging, now using log/slog.

General Improvements

  • Excluded /_healthy from Fiber logs and added an option to set SlogLogger as a global slog logger.
  • Updated the gRPC client to handle auto-reconnects more effectively.

Summary by Sourcery

This pull request introduces several enhancements and fixes focusing on security, infrastructure improvements, and logging enhancements across various modules. Key changes include addressing security vulnerabilities, refactoring NATS subject handling, improving logging, and updating the gRPC client for better auto-reconnect handling.

  • New Features:
    • Introduced a new function UUID in pkg/functions/main.go to generate UUID strings of specified sizes.
    • Added support for setting a global slog logger in various modules, including apps/infra, apps/message-office, and apps/console.
  • Bug Fixes:
    • Fixed several security vulnerabilities reported by Dependabot, including session middleware token injection, basic auth credentials leakage in go-retryablehttp, and denial of service vulnerability in gqlparser.
  • Enhancements:
    • Refactored NATS subject handling to resolve storage and performance issues.
    • Enhanced logging across various modules to provide better insights into operations and interactions.
    • Improved reconnection handling and logging in the tenant-agent, now using log/slog.
    • Excluded /_healthy from Fiber logs and added an option to set SlogLogger as a global slog logger.
    • Updated the gRPC client to handle auto-reconnects more effectively.
    • Replaced the logging.Logger with slog.Logger in multiple modules for improved logging capabilities.

- improves reconnection triggering and handling
- improves logging, introduces `log/slog`
- cleans up lot of junk code
- improves logging with `log/slog`
…es logging

- cleans up duplicated dispatch logic
- adds option to set SlogLogger as a global slog logger
- grpc client auto-reconnect updates
@nxtcoder17 nxtcoder17 requested a review from karthik1729 as a code owner July 26, 2024 06:11
@nxtcoder17 nxtcoder17 self-assigned this Jul 26, 2024
@sourcery-ai
Copy link
Copy Markdown

sourcery-ai Bot commented Jul 26, 2024

Reviewer's Guide by Sourcery

This pull request refactors the NATS subject handling, improves logging, and addresses several security vulnerabilities. The changes include updates to the NATS subject structure, enhanced logging using log/slog, and fixes for security issues reported by Dependabot. Additionally, the tenant-agent's reconnection behavior has been improved, and the gRPC client now handles auto-reconnects more effectively.


Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

Hey @nxtcoder17 - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider adding integration tests to verify the new NATS subject naming convention works correctly across all affected components.
  • The switch to log/slog is a good improvement. Ensure all team members are familiar with the new logging patterns to maintain consistency in future development.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.


// ReceiveConsoleResourceUpdate implements messages.MessageDispatchServiceServer.
func (g *grpcServer) ReceiveConsoleResourceUpdate(ctx context.Context, msg *messages.ResourceUpdate) (*messages.Empty, error) {
func (g *grpcServer) ReceiveConsoleResourceUpdate(ctx context.Context, msg *messages.ResourceUpdate) (_ *messages.Empty, err error) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion: Consider wrapping errors for better context

The error handling has changed to return errors directly instead of wrapping them. While this simplifies the code, it might lead to loss of context. Consider using errors.Wrap or a similar method to preserve the error context, especially for non-trivial errors.

Suggested change
func (g *grpcServer) ReceiveConsoleResourceUpdate(ctx context.Context, msg *messages.ResourceUpdate) (_ *messages.Empty, err error) {
import "github.com/pkg/errors"
func (g *grpcServer) ReceiveConsoleResourceUpdate(ctx context.Context, msg *messages.ResourceUpdate) (_ *messages.Empty, err error) {
accountName, clusterName, err := g.validateAndDecodeFromGrpcContext(ctx, g.ev.TokenHashingSecret)
if err != nil {
return nil, errors.Wrap(err, "failed to validate and decode from gRPC context")
}

@nxtcoder17 nxtcoder17 force-pushed the refactor/nats-subject-change branch 2 times, most recently from 76098c0 to 9ae659f Compare July 26, 2024 06:25
- [Session Middleware Token Injection Vulnerability](https://github.com/kloudlite/api/security/dependabot/31)
- [go-retryablehttp can leak basic auth credentials to log files](https://github.com/kloudlite/api/security/dependabot/32)
- [gqlparser denial of service vulnerability via the parserDirectives function](https://github.com/kloudlite/api/security/dependabot/33)
@nxtcoder17 nxtcoder17 force-pushed the refactor/nats-subject-change branch from 9ae659f to 2d0684f Compare July 26, 2024 06:39
@nxtcoder17 nxtcoder17 merged commit a8d3efc into release-v1.0.7 Jul 26, 2024
@nxtcoder17 nxtcoder17 deleted the refactor/nats-subject-change branch July 26, 2024 06:43
abdheshnayak pushed a commit that referenced this pull request Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant