Skip to content

Refactor manager.go for improved safety and maintainability #1082

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Refactor manager.go for improved safety and maintainability #1082

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
890ec94
Refactor manager.go for improved safety and maintainability
Muaz-Ibn-Kamal Sep 16, 2025
0158059
Merge branch 'main' into patch-1
Muaz-Ibn-Kamal Sep 18, 2025
35661ed
Fix error message to include ID in broker lookup
Muaz-Ibn-Kamal Sep 18, 2025
fe7ca67
Fix typo
Muaz-Ibn-Kamal Sep 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 21 additions & 20 deletions internal/brokers/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ func NewManager(ctx context.Context, brokersConfPath string, configuredBrokers [
}()

// Connect to the system bus
// Don't call dbus.SystemBus which caches globally system dbus (issues in tests)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't remove this comment

Copy link
Author

Choose a reason for hiding this comment

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

okey thank You

bus, err := dbus.ConnectSystemBus()
if err != nil {
return m, err
Expand Down Expand Up @@ -119,6 +118,7 @@ func NewManager(ctx context.Context, brokersConfPath string, configuredBrokers [

// AvailableBrokers returns currently loaded and available brokers in preference order.
func (m *Manager) AvailableBrokers() (r []*Broker) {
r = make([]*Broker, 0, len(m.brokersOrder))
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is fine, I see that it can avoid unnecessary allocations when appending the brokers. However, that's not explained in the commit message. Please create a separate commit with this change and explain in the commit message why it is useful.

Copy link
Author

Choose a reason for hiding this comment

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

yes

Copy link
Author

Choose a reason for hiding this comment

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

Optimize AvailableBrokers to preallocate slice capacity

Preallocate the slice in AvailableBrokers with len(m.brokersOrder) capacity:

r = make([]*Broker, 0, len(m.brokersOrder))

This avoids repeated slice reallocations when appending brokers,
improves efficiency, and reduces memory churn, especially when
many brokers are loaded.

Copy link
Collaborator

Choose a reason for hiding this comment

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

True, we don't have that many though... But well still fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Optimize AvailableBrokers to preallocate slice capacity

Preallocate the slice in AvailableBrokers with len(m.brokersOrder) capacity:

r = make([]*Broker, 0, len(m.brokersOrder))

This avoids repeated slice reallocations when appending brokers,
improves efficiency, and reduces memory churn, especially when
many brokers are loaded.

This would be a good commit message. Again, please create a separate commit with this change.

for _, id := range m.brokersOrder {
r = append(r, m.brokers[id])
}
Expand All @@ -145,7 +145,7 @@ func (m *Manager) SetDefaultBrokerForUser(brokerID, username string) error {
func (m *Manager) BrokerForUser(username string) (broker *Broker) {
m.usersToBrokerMu.RLock()
defer m.usersToBrokerMu.RUnlock()
return m.usersToBroker[username]
return m.usersToBroker[strings.ToLower(username)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit undecided if we want this change or not. We do use lowercase usernames in authd, so this username must indeed be lowercase. However, we have so many layers of functions which take a username as an argument that it's a bit inefficient to convert it lowercase in each function.

Here we have these layers:

sequenceDiagram
    box PAM
        participant A as authd-pam
    end
    box authd
        participant B as pam.Service
        participant C as brokers.Manager
    end

    Note over A,B: Communication via gRPC

    A->>B: GetPreviousBroker
    B->>C: BrokerForUser
    C-->>B: Return broker
    B-->>A: Response
Loading

If the brokers.Manager doesn't find the broker, the users.Manager is called, which results in even more layers:

sequenceDiagram
    box PAM
        participant A as authd-pam
    end
    box authd
        participant B as pam.Service
        participant C as users.Manager
        participant D as db.Manager
    end

    Note over A,B: Communication via gRPC

    A->>B: GetPreviousBroker
    B->>C: BrokerForUser
    C->>D: UserByName
    D-->>C: UserRow.BrokerID
    C-->>B: Return BrokerID
    B-->>A: Response
Loading

We currently call strings.ToLower in all of these layers (or at least in some functions of each layer).

Looking at it like this, it's my impression that it would make most sense to just convert the usernames to lowercase once in the gRPC methods, and in the other layers (brokers.Manager, users.Manager, db.Manager) assume that the usernames are already in lowercase.

Note that the same applies to all other methods of pam.Service which takes a username as an argument, and also to the methods of user.Service.

What do you think, @3v1n0?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I was noticing the change too and I was wondering whether is needed... Ideally we should do it in any public APIs though at this point...

}

// BrokerFromSessionID returns broker currently in use for a given transaction sessionID.
Expand All @@ -160,49 +160,49 @@ func (m *Manager) BrokerFromSessionID(id string) (broker *Broker, err error) {

broker, exists := m.transactionsToBroker[id]
if !exists {
return nil, fmt.Errorf("no broker found for session %q", id)
return nil, fmt.Errorf("no broker found for session ID %q", id)
Copy link
Contributor

Choose a reason for hiding this comment

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

We use "session %q" all over the place in error and debug messages, so for consistency we should not change it here.

}

return broker, nil
}

// NewSession create a new session for the broker and store the sesssionID on the manager.
func (m *Manager) NewSession(brokerID, username, lang, mode string) (sessionID string, encryptionKey string, err error) {
// NewSession create a new session for the broker and store the sessionID on the manager.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create a separate commit with this change. The commit message can simply be "Fix typo".

Copy link
Contributor

Choose a reason for hiding this comment

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

I see you pushed a commit with the commit message "Fix typo". However, it doesn't contain this change but adds a comment //Fix typo.

// Fix typo
func (m *Manager) NewSession(ctx context.Context, brokerID, username, lang, mode string) (sessionID string, encryptionKey string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not add unused context parameters to functions. The log functions don't actually use their context arguments (for now), so that's not a good reason to add a context parameter here.

broker, err := m.brokerFromID(brokerID)
if err != nil {
return "", "", fmt.Errorf("invalid broker: %v", err)
}

sessionID, encryptionKey, err = broker.newSession(context.Background(), username, lang, mode)
sessionID, encryptionKey, err = broker.newSession(ctx, username, lang, mode)
if err != nil {
return "", "", err
}

m.transactionsToBrokerMu.Lock()
defer m.transactionsToBrokerMu.Unlock()
log.Debugf(context.Background(), "%s: New %s session for %q",
sessionID, mode, username)
log.Debugf(ctx, "%s: New %s session for %q", sessionID, mode, username)
m.transactionsToBroker[sessionID] = broker
return sessionID, encryptionKey, nil
}

// EndSession signals the end of the session to the broker associated with the sessionID and then removes the
// session -> broker mapping.
func (m *Manager) EndSession(sessionID string) error {
b, err := m.BrokerFromSessionID(sessionID)
if err != nil {
return err
func (m *Manager) EndSession(ctx context.Context, sessionID string) error {
m.transactionsToBrokerMu.Lock()
broker, exists := m.transactionsToBroker[sessionID]
if !exists {
m.transactionsToBrokerMu.Unlock()
return fmt.Errorf("no broker found for session ID %q", sessionID)
}
Comment on lines 189 to 197
Copy link
Contributor

Choose a reason for hiding this comment

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

why should we change the order of deleting session mapping and calling endSession? if we want to change this (which I'm not convinced of), we should also update the comment.

delete(m.transactionsToBroker, sessionID)
m.transactionsToBrokerMu.Unlock()

if err = b.endSession(context.Background(), sessionID); err != nil {
if err := broker.endSession(ctx, sessionID); err != nil {
return err
}

m.transactionsToBrokerMu.Lock()
log.Debugf(context.Background(), "%s: End session %q",
sessionID, m.transactionsToBroker[sessionID].Name)
delete(m.transactionsToBroker, sessionID)
m.transactionsToBrokerMu.Unlock()
log.Debugf(ctx, "%s: Ended session", sessionID)
return nil
}

Expand All @@ -216,8 +216,9 @@ func (m *Manager) BrokerExists(brokerID string) bool {
func (m *Manager) brokerFromID(id string) (broker *Broker, err error) {
broker, exists := m.brokers[id]
if !exists {
return nil, fmt.Errorf("no broker found matching %q", id)
}
// Fix error message to include ID in broker lookup
return nil, fmt.Errorf("no broker found matching ID %q", id)

}
return broker, nil
}
Loading