feat: Add response and log functionalities by HomayoonAlimohammadi · Pull Request #603 · canonical/microcluster

HomayoonAlimohammadi · 2026-01-29T07:00:34Z

Context

While migrating Canonical Kubernetes from Microcluster v2 to v3, I noticed that we're moving away from the lxd/reponse and it's now baked into Microcluster itself. These two functions were used in Canonical Kubernetes but are not available in Microcluster. Also, the logging package is internal to Microcluster which makes testing on the k8s snap side impossible. The new pkg/log is supposed to surface the logging functionality.

Overview

This pull request introduces new functionality for error handling in HTTP responses and adds a new logging utility package. The most important changes are the addition of helper functions for standardized error responses and the creation of a log package to simplify logger usage with context.

Error handling improvements:

Added Unauthorized and ErrorResponse helper functions to response.go for generating 401 and custom error responses, respectively.

Logging utility:

Introduced a new microcluster/types/log package that wraps logger context handling, providing a WithLogger function and exposing the logger context key for easier logger management from external repos.

pkg/log/log.go

Copilot

Pull request overview

This PR adds reusable helpers for HTTP error responses and introduces a public logging utility package so external consumers can integrate with Microcluster’s logging/context mechanism.

Changes:

Added Unauthorized and ErrorResponse helpers in microcluster/rest/response/response.go for standardized 401 and arbitrary-code error responses.
Introduced pkg/log/log.go to expose the logger context key (CtxLogger) and a WithLogger helper that stores a slog.Logger in a context.Context.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
pkg/log/log.go	New public logging helper package that wraps the internal logger context key and provides `WithLogger` for attaching a logger to a context.
microcluster/rest/response/response.go	Extends the HTTP response helpers with 401 and generic error-response constructors and wires in `errors.New` for message-based error responses.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

microcluster/rest/response/response.go

roosterfish · 2026-01-30T13:42:49Z

internal/log/logger.go

 	logger, ok := ctx.Value(CtxLogger).(*slog.Logger)
 	if !ok {
-		return nil, errors.New("Logger does not exist on context")
+		logger := slog.Default()


But this is already caught by the outer microcluster.LoggerFromContext isn't it? If this internal LoggerFromContext returns an error, then the outer one is running slog.New.

I don't understand why we have to replicate this here too?

roosterfish

Thanks for adding the two additional response funcs. However I still don't see the need for the proposed logging changes.

HomayoonAlimohammadi · 2026-02-02T13:29:05Z

Thank you for the review @roosterfish. I've reached out on MM with more context around this change.

roosterfish · 2026-02-02T14:06:56Z

Thank you for the review @roosterfish. I've reached out on MM with more context around this change.

Ty for providing the additional context.
When you test your API handlers as a standalone, the passed context requires the logger to be set (which happens automatically when the handler is fired inside Microcluster).

So as part of our unit tests we also run ctx := context.WithValue(context.TODO(), log.CtxLogger, slog.Default()) to ensure we have a context with the required logger set to e.g. run the database funcs which might need to access the logger.

Deriving another default logger on the internal/log.LoggerFromContext func isn't trivial as we don't know at this point which log handler was passed when starting Microcluster.
If you want to test the API handler external from Microcluster, pass a context with the "context" key populated as shown above.
If we start returning a default logger, we might miss places where we unintentionally pass the wrong context (not the daemon's shutdown ctx), so it takes the default but instead we want the logs to be routed to the actual logger which isn't available as the context is wrong.

HomayoonAlimohammadi · 2026-02-02T15:19:56Z

Thank you for all the information @roosterfish! I've updated the PR with what we've discussed

roosterfish · 2026-02-03T08:32:50Z

microcluster/types/log/log.go


 // CtxLogger is the name of the context value for the central logger.
-const CtxLogger ctxKey = "logger"
+const CtxLogger CtxKey = "logger"


Can we move the CtxLogger itself into microcluster/types/log? Then we can reference it both from the internal/ and public packages.

My proposal was to only move the CtxLogger const, not the entire file. It looks you have now moved everything.

If we just move CtxLogger const, we can still reference it from anywhere inside internal/ but you can also make use of it when you try to derive a new context including a logger.

roosterfish · 2026-02-03T08:39:00Z

microcluster/types/log/log.go

+
+// WithLogger returns a new context with the given logger.
+// If no logger is provided, the default logger is used.
+func WithLogger(ctx context.Context, logger ...*slog.Logger) context.Context {


I don't understand the complexity of this func. Why does it have to be variadic and accept multiple instances of the logger if only ever the first instance is used?

If you wanted to add a helper func that takes care of populating the context value, you can just do something like return context.WithValue(ctx, CtxLogger, slog.Default()).
Please also ensure this helper is then used instead, for example here https://github.com/canonical/microcluster/blob/v3/internal/db/db_test.go#L655 (and the other places).

The reason behind this variadic arg is to enable users to do:

ctx := types.WithLogger(context.Background()) // instead of ctx := types.WithLogger(context.Background(), slog.Default())

to set a default logger to the context.
and they're also able to do the following to set a custom logger:

ctx := types.WithLogger(context.Background(), myCustomLogger)

I've seen this pattern in multiple places and found it interesting. If you don't like it I can change it.

I see thanks.
Let's call it ContextWithLogger to be more descriptive. This would also align with the LoggerFromContext funcs.

roosterfish · 2026-02-03T08:40:50Z

microcluster/types/log.go

+	"context"
+	"log/slog"
+
+	"github.com/canonical/microcluster/v3/internal/log"


From microcluster/types we should not start importing from internal/ to prevent import loops in the future.
Therefore I mentioned to move the CtxLogger into microcluster/types/log directly.

roosterfish · 2026-02-04T08:19:55Z

Can you please rebase, there are now conflicts after merging in other PRs.

… handling Signed-off-by: Homayoon Alimohammadi <homayoon.alimohammadi@canonical.com>

roosterfish · 2026-02-05T08:32:32Z

microcluster/types/log/log.go


 // CtxLogger is the name of the context value for the central logger.
-const CtxLogger ctxKey = "logger"
+const CtxLogger CtxKey = "logger"


My proposal was to only move the CtxLogger const, not the entire file. It looks you have now moved everything.

roosterfish · 2026-02-05T08:33:15Z

microcluster/types/log/log.go


 // CtxLogger is the name of the context value for the central logger.
-const CtxLogger ctxKey = "logger"
+const CtxLogger CtxKey = "logger"


If we just move CtxLogger const, we can still reference it from anywhere inside internal/ but you can also make use of it when you try to derive a new context including a logger.

Signed-off-by: Homayoon Alimohammadi <homayoon.alimohammadi@canonical.com>

HomayoonAlimohammadi · 2026-02-05T09:29:21Z

@roosterfish Thank you for the review. I've updated the PR with your instructions. Please let me know if it needs further polishing.

roosterfish

The current logger changes feel too invasive to me.

In one of the recent comments I mentioned the approach we take for logging.

I understand you want to have an easy way to derive a context with the logger set to mimic what is done internally by Microcluster so you can test e.g. API handlers as standalone (as we do in our unit tests).
The easiest (which already works today) is to derive a context on your own using context.WithValue(context.TODO(), log.CtxLogger, slog.Default()).
But together we identified that log.CtxLogger is in the internal/ package. To overcome this limitation my suggestion was to move this constant into microcluster/types/log instead so you can access it too. It can even be a just a file like log.go (no need for an extra log/ package).

But this should not require this amount of changes. Only the places where we currently use log.CtxLogger need to be updated to point to the micrcluster/types package which would then contain the constant.

roosterfish · 2026-02-05T13:02:36Z

internal/log/logger.go

+)

-// CtxLogger is the name of the context value for the central logger.
-const CtxLogger ctxKey = "logger"


Let's move this const (and type ctxKey) entirely into microcluster/types. There is no need to define another const here once it's moved.

Maybe my comment here was not clear.

roosterfish · 2026-02-05T13:04:49Z

microcluster/types/log/log.go

+
+// WithLogger returns a new context with the given logger.
+// If no logger is provided, the default logger is used.
+func WithLogger(ctx context.Context, logger ...*slog.Logger) context.Context {


I see thanks.
Let's call it ContextWithLogger to be more descriptive. This would also align with the LoggerFromContext funcs.

roosterfish · 2026-02-05T13:05:33Z

microcluster/app.go

 	internalClient "github.com/canonical/microcluster/v3/internal/rest/client"
 	"github.com/canonical/microcluster/v3/internal/sys"
 	"github.com/canonical/microcluster/v3/microcluster/types"
+	mclog "github.com/canonical/microcluster/v3/microcluster/types/log"


We tend to use micro prefix if there is a collision. I would suggest microLog.

roosterfish · 2026-02-05T13:12:48Z

internal/daemon/daemon.go

 // We always expect the logger to be present.
 func (d *Daemon) log() *slog.Logger {
-	return d.shutdownCtx.Value(internalLog.CtxLogger).(*slog.Logger) //nolint:revive
+	l, err := internalLog.LoggerFromContext(d.shutdownCtx)


I am a bit surprised to see those changes.

At this stage there already is a logger set on the d.shutdownCtx. See the comment on the func.

HomayoonAlimohammadi mentioned this pull request Jan 29, 2026

feat: Bump microcluster to v3 canonical/k8sd#9

Open

HomayoonAlimohammadi changed the title ~~feat(response): Add Unauthorized and ErrorResponse functions~~ feat: Add response and log functionalities Jan 29, 2026

roosterfish requested changes Jan 29, 2026

View reviewed changes

pkg/log/log.go Outdated Show resolved Hide resolved

roosterfish requested a review from Copilot January 29, 2026 09:00

Copilot started reviewing on behalf of roosterfish January 29, 2026 09:01 View session

Copilot AI reviewed Jan 29, 2026

View reviewed changes

microcluster/rest/response/response.go Outdated Show resolved Hide resolved

HomayoonAlimohammadi force-pushed the KU-5052/unauth-response branch 2 times, most recently from 7c09f3b to e30187f Compare January 29, 2026 13:36

HomayoonAlimohammadi requested a review from roosterfish January 29, 2026 13:36

HomayoonAlimohammadi force-pushed the KU-5052/unauth-response branch from e30187f to 1af6e75 Compare January 30, 2026 09:49

roosterfish requested changes Jan 30, 2026

View reviewed changes

roosterfish requested changes Feb 2, 2026

View reviewed changes

HomayoonAlimohammadi force-pushed the KU-5052/unauth-response branch from 1af6e75 to d7202ad Compare February 2, 2026 15:19

HomayoonAlimohammadi requested a review from roosterfish February 3, 2026 07:41

roosterfish requested changes Feb 3, 2026

View reviewed changes

HomayoonAlimohammadi force-pushed the KU-5052/unauth-response branch 2 times, most recently from e0ae35a to e49501f Compare February 3, 2026 15:42

HomayoonAlimohammadi requested a review from roosterfish February 3, 2026 15:42

HomayoonAlimohammadi force-pushed the KU-5052/unauth-response branch from c429936 to c250354 Compare February 4, 2026 08:27

feat: Add Unauthorized and ErrorResponse functions for improved error…

75cba29

… handling Signed-off-by: Homayoon Alimohammadi <homayoon.alimohammadi@canonical.com>

HomayoonAlimohammadi force-pushed the KU-5052/unauth-response branch from c250354 to e81370e Compare February 4, 2026 08:27

roosterfish requested changes Feb 5, 2026

View reviewed changes

refactor: Introduce types/log to export internal logging functionality

dde6ba3

Signed-off-by: Homayoon Alimohammadi <homayoon.alimohammadi@canonical.com>

HomayoonAlimohammadi force-pushed the KU-5052/unauth-response branch from e81370e to dde6ba3 Compare February 5, 2026 09:27

HomayoonAlimohammadi requested a review from roosterfish February 5, 2026 12:57

roosterfish requested changes Feb 5, 2026

View reviewed changes

Conversation

HomayoonAlimohammadi commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

roosterfish Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roosterfish left a comment

Choose a reason for hiding this comment

Uh oh!

HomayoonAlimohammadi commented Feb 2, 2026

Uh oh!

roosterfish commented Feb 2, 2026

Uh oh!

HomayoonAlimohammadi commented Feb 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roosterfish commented Feb 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HomayoonAlimohammadi commented Feb 5, 2026

Uh oh!

roosterfish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

HomayoonAlimohammadi commented Jan 29, 2026 •

edited

Loading

roosterfish Jan 30, 2026 •

edited

Loading