From 7040dfab185593f627dcbb3bd7f1b1ac2bc9bfaf Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Fri, 16 Feb 2024 10:50:08 +0000 Subject: [PATCH 1/5] Refactor socket listener out of control socket. The control socket can be seperated into the code to listen to the control socket, and the code to handle the requests recived on it (i.e. exposing controller metrics). The socket listener can be used elsewhere so it is moved to the internal package. --- .../agent/machine/manifolds.go | 11 +- cmd/jujud/agent/machine/manifolds.go | 2 +- internal/socketlistener/package_test.go | 14 + internal/socketlistener/socketlistener.go | 116 +++++ .../socketlistener/socketlistener_test.go | 157 ++++++ internal/worker/controlsocket/handlers.go | 243 ---------- .../worker/controlsocket/handlers_test.go | 446 ------------------ internal/worker/controlsocket/manifold.go | 31 +- internal/worker/controlsocket/worker.go | 301 ++++++++++-- internal/worker/controlsocket/worker_test.go | 440 +++++++++++++++-- 10 files changed, 985 insertions(+), 776 deletions(-) create mode 100644 internal/socketlistener/package_test.go create mode 100644 internal/socketlistener/socketlistener.go create mode 100644 internal/socketlistener/socketlistener_test.go delete mode 100644 internal/worker/controlsocket/handlers.go delete mode 100644 internal/worker/controlsocket/handlers_test.go diff --git a/cmd/jujud-controller/agent/machine/manifolds.go b/cmd/jujud-controller/agent/machine/manifolds.go index c0165fe1aa3..162e1962236 100644 --- a/cmd/jujud-controller/agent/machine/manifolds.go +++ b/cmd/jujud-controller/agent/machine/manifolds.go @@ -1,4 +1,4 @@ -// Copyright 2015 Canonical Ltd. +// Copyright 2024 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. package machine @@ -820,10 +820,11 @@ func commonManifolds(config ManifoldsConfig) dependency.Manifolds { // The controlsocket worker runs on the controller machine. controlSocketName: ifController(controlsocket.Manifold(controlsocket.ManifoldConfig{ - StateName: stateName, - Logger: loggo.GetLogger("juju.worker.controlsocket"), - NewWorker: controlsocket.NewWorker, - SocketName: paths.ControlSocket(paths.OSUnixLike), + StateName: stateName, + Logger: loggo.GetLogger("juju.worker.controlsocket"), + NewWorker: controlsocket.NewWorker, + NewSocketListener: controlsocket.NewSocketListener, + SocketName: paths.ControlSocket(paths.OSUnixLike), })), objectStoreName: ifController(objectstore.Manifold(objectstore.ManifoldConfig{ diff --git a/cmd/jujud/agent/machine/manifolds.go b/cmd/jujud/agent/machine/manifolds.go index b0c5f25c464..8b443d1319d 100644 --- a/cmd/jujud/agent/machine/manifolds.go +++ b/cmd/jujud/agent/machine/manifolds.go @@ -1,4 +1,4 @@ -// Copyright 2015 Canonical Ltd. +// Copyright 2024 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. package machine diff --git a/internal/socketlistener/package_test.go b/internal/socketlistener/package_test.go new file mode 100644 index 00000000000..cf07f24e464 --- /dev/null +++ b/internal/socketlistener/package_test.go @@ -0,0 +1,14 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package socketlistener_test + +import ( + "testing" + + gc "gopkg.in/check.v1" +) + +func Test(t *testing.T) { + gc.TestingT(t) +} diff --git a/internal/socketlistener/socketlistener.go b/internal/socketlistener/socketlistener.go new file mode 100644 index 00000000000..aa191e72e6c --- /dev/null +++ b/internal/socketlistener/socketlistener.go @@ -0,0 +1,116 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package socketlistener + +import ( + "context" + "net" + "net/http" + "time" + + "github.com/gorilla/mux" + "github.com/juju/errors" + "gopkg.in/tomb.v2" + + "github.com/juju/juju/juju/sockets" +) + +// Logger represents the methods used by the worker to log information. +type Logger interface { + Warningf(string, ...any) + Debugf(string, ...any) +} + +// Config represents configuration for the socketlistener worker. +type Config struct { + Logger Logger + SocketName string + RegisterHandlers func(router *mux.Router) + ShutdownTimeout time.Duration +} + +// Validate returns an error if config cannot drive the SocketListener. +func (config Config) Validate() error { + if config.Logger == nil { + return errors.NotValidf("nil Logger") + } + if config.SocketName == "" { + return errors.NotValidf("empty SocketName") + } + if config.RegisterHandlers == nil { + return errors.NotValidf("nil RegisterHandlers func") + } + if config.ShutdownTimeout == 0 { + return errors.NotValidf("zero value for ShutdownTimeout") + } + return nil +} + +// SocketListener is a socketlistener worker. +type SocketListener struct { + config Config + tomb tomb.Tomb + listener net.Listener + shutdownTimeout time.Duration +} + +// NewSocketListener returns a socketlistener with the given config. +func NewSocketListener(config Config) (*SocketListener, error) { + if err := config.Validate(); err != nil { + return nil, errors.Trace(err) + } + + l, err := sockets.Listen(sockets.Socket{ + Address: config.SocketName, + Network: "unix", + }) + if err != nil { + return nil, errors.Annotate(err, "unable to listen on unix socket") + } + config.Logger.Debugf("socketlistener listening on socket %q", config.SocketName) + + sl := &SocketListener{ + config: config, + listener: l, + } + sl.tomb.Go(sl.run) + return sl, nil +} + +func (sl *SocketListener) Kill() { + sl.tomb.Kill(nil) +} + +func (sl *SocketListener) Wait() error { + return sl.tomb.Wait() +} + +// run listens on the control socket and handles requests. +func (sl *SocketListener) run() error { + router := mux.NewRouter() + sl.config.RegisterHandlers(router) + + srv := http.Server{Handler: router} + defer func() { + err := srv.Close() + if err != nil { + sl.config.Logger.Warningf("error closing HTTP server: %v", err) + } + }() + + sl.tomb.Go(func() error { + // Wait for the tomb to start dying and then shut the server down. + <-sl.tomb.Dying() + ctx, cancel := context.WithTimeout(context.Background(), sl.shutdownTimeout) + defer cancel() + return srv.Shutdown(ctx) + }) + + sl.config.Logger.Debugf("socketlistener now serving on socket %q", sl.config.SocketName) + defer sl.config.Logger.Debugf("socketlistener finished serving on socket %q", sl.config.SocketName) + if err := srv.Serve(sl.listener); !errors.Is(err, http.ErrServerClosed) { + return err + } + return nil +} diff --git a/internal/socketlistener/socketlistener_test.go b/internal/socketlistener/socketlistener_test.go new file mode 100644 index 00000000000..d116f114cbf --- /dev/null +++ b/internal/socketlistener/socketlistener_test.go @@ -0,0 +1,157 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package socketlistener + +import ( + "context" + "fmt" + "io/fs" + "net" + "net/http" + "os" + "path" + "time" + + "github.com/gorilla/mux" + jc "github.com/juju/testing/checkers" + "github.com/juju/worker/v4/workertest" + gc "gopkg.in/check.v1" + + coretesting "github.com/juju/juju/core/testing" +) + +type socketListenerSuite struct { + logger *fakeLogger +} + +var _ = gc.Suite(&socketListenerSuite{}) + +func (s *socketListenerSuite) SetUpTest(c *gc.C) { + s.logger = &fakeLogger{} +} + +func handleTestEndpoint1(resp http.ResponseWriter, req *http.Request) { + resp.WriteHeader(http.StatusOK) +} + +func registerTestHandlers(r *mux.Router) { + r.HandleFunc("/test-endpoint", handleTestEndpoint1). + Methods(http.MethodGet) +} + +func (s *socketListenerSuite) TestStartStopWorker(c *gc.C) { + tmpDir := c.MkDir() + socket := path.Join(tmpDir, "test.socket") + + sl, err := NewSocketListener(Config{ + Logger: s.logger, + SocketName: socket, + RegisterHandlers: registerTestHandlers, + }) + c.Assert(err, jc.ErrorIsNil) + + // Check socket is created with correct permissions. + fi, err := os.Stat(socket) + c.Assert(err, jc.ErrorIsNil) + c.Assert(fi.Mode(), gc.Equals, fs.ModeSocket|0700) + + // Check server is up. + cl := client(socket) + resp, err := cl.Get("http://localhost:8080/foo") + c.Assert(err, jc.ErrorIsNil) + c.Assert(resp.StatusCode, gc.Equals, http.StatusNotFound) + + // Check server is serving. + cl = client(socket) + resp, err = cl.Get("http://localhost:8080/test-endpoint") + c.Assert(err, jc.ErrorIsNil) + c.Assert(resp.StatusCode, gc.Equals, http.StatusOK) + + sl.Kill() + err = sl.Wait() + c.Assert(err, jc.ErrorIsNil) + + // Check server has stopped. + resp, err = cl.Get("http://localhost:8080/foo") + c.Assert(err, gc.ErrorMatches, ".*connection refused") + + // No warnings/errors should have been logged. + for _, entry := range s.logger.entries { + if entry.level == "ERROR" || entry.level == "WARNING" { + c.Errorf("%s: %s", entry.level, entry.msg) + } + } +} + +func (s *socketListenerSuite) TestEnsureShutdown(c *gc.C) { + tmpDir := c.MkDir() + socket := path.Join(tmpDir, "test.socket") + + start := make(chan struct{}) + sl, err := NewSocketListener(Config{ + Logger: s.logger, + SocketName: socket, + RegisterHandlers: func(r *mux.Router) { + r.HandleFunc("/test-endpoint", func(resp http.ResponseWriter, req *http.Request) { + close(start) + time.Sleep(time.Second) + resp.WriteHeader(http.StatusOK) + }).Methods(http.MethodGet) + }, + }) + c.Assert(err, jc.ErrorIsNil) + defer workertest.DirtyKill(c, sl) + done := make(chan struct{}) + go func() { + // Check server is up. + cl := client(socket) + _, err := cl.Get("http://localhost:8080/test-endpoint") + c.Assert(err, jc.ErrorIsNil) + }() + + go func() { + select { + case <-start: + case <-time.After(coretesting.ShortWait): + c.Errorf("took too long to start") + } + workertest.CleanKill(c, sl) + close(done) + }() + select { + case <-done: + case <-time.After(coretesting.LongWait): + c.Errorf("took too long to finish") + } +} + +// Return an *http.Client with custom transport that allows it to connect to +// the given Unix socket. +func client(socketPath string) *http.Client { + return &http.Client{ + Transport: &http.Transport{ + DialContext: func(_ context.Context, _, _ string) (conn net.Conn, err error) { + return net.Dial("unix", socketPath) + }, + }, + } +} + +type fakeLogger struct { + entries []logEntry +} + +type logEntry struct{ level, msg string } + +func (f *fakeLogger) write(level string, format string, args ...any) { + f.entries = append(f.entries, logEntry{level, fmt.Sprintf(format, args...)}) +} + +func (f *fakeLogger) Warningf(format string, args ...any) { + f.write("WARNING", format, args...) +} + +func (f *fakeLogger) Debugf(format string, args ...any) { + f.write("DEBUG", format, args...) +} diff --git a/internal/worker/controlsocket/handlers.go b/internal/worker/controlsocket/handlers.go deleted file mode 100644 index d1d6e1c1d6b..00000000000 --- a/internal/worker/controlsocket/handlers.go +++ /dev/null @@ -1,243 +0,0 @@ -// Copyright 2023 Canonical Ltd. -// Licensed under the AGPLv3, see LICENCE file for details. - -package controlsocket - -import ( - "encoding/json" - "fmt" - "io" - "net/http" - "strings" - - "github.com/gorilla/mux" - "github.com/juju/errors" - "github.com/juju/names/v5" - - "github.com/juju/juju/core/permission" - "github.com/juju/juju/environs/bootstrap" - "github.com/juju/juju/state" - stateerrors "github.com/juju/juju/state/errors" -) - -const ( - // jujuMetricsUserPrefix defines the "namespace" in which this worker is - // allowed to create/remove users. - jujuMetricsUserPrefix = "juju-metrics-" - - // userCreator is the listed "creator" of metrics users in state. - // This user CANNOT be a local user (it must have a domain), otherwise the - // model addUser code will complain about the user not existing. - userCreator = "controller@juju" -) - -func (w *Worker) registerHandlers(r *mux.Router) { - r.HandleFunc("/metrics-users", w.handleAddMetricsUser). - Methods(http.MethodPost) - r.HandleFunc("/metrics-users/{username}", w.handleRemoveMetricsUser). - Methods(http.MethodDelete) -} - -type addMetricsUserBody struct { - Username string `json:"username"` - Password string `json:"password"` -} - -func (w *Worker) handleAddMetricsUser(resp http.ResponseWriter, req *http.Request) { - var parsedBody addMetricsUserBody - defer req.Body.Close() - err := json.NewDecoder(req.Body).Decode(&parsedBody) - if errors.Is(err, io.EOF) { - w.writeResponse(resp, http.StatusBadRequest, errorf("missing request body")) - return - } else if err != nil { - w.writeResponse(resp, http.StatusBadRequest, errorf("request body is not valid JSON: %v", err)) - return - } - - code, err := w.addMetricsUser(parsedBody.Username, parsedBody.Password) - if err != nil { - w.writeResponse(resp, code, errorf("%v", err)) - return - } - - w.writeResponse(resp, code, infof("created user %q", parsedBody.Username)) -} - -func (w *Worker) addMetricsUser(username, password string) (int, error) { - err := validateMetricsUsername(username) - if err != nil { - return http.StatusBadRequest, err - } - - if password == "" { - return http.StatusBadRequest, errors.NotValidf("empty password") - } - - user, err := w.config.State.AddUser(username, username, password, userCreator) - cleanup := true - // Error handling here is a bit subtle. - switch { - case errors.Is(err, errors.AlreadyExists): - // Retrieve existing user - user, err = w.config.State.User(names.NewUserTag(username)) - if err != nil { - return http.StatusInternalServerError, - fmt.Errorf("retrieving existing user %q: %v", username, err) - } - - // We want this operation to be idempotent, but at the same time, this - // worker shouldn't mess with users that have not been created by it. - // So ensure the user is identical to what we would have created, and - // otherwise error. - if user.CreatedBy() != userCreator { - return http.StatusConflict, errors.AlreadyExistsf("user %q (created by %q)", user.Name(), user.CreatedBy()) - } - if !user.PasswordValid(password) { - return http.StatusConflict, errors.AlreadyExistsf("user %q", user.Name()) - } - - case err == nil: - // At this point, the operation is in a partially completed state - we've - // added the user, but haven't granted them the correct model permissions. - // If there is an error granting permissions, we should attempt to "rollback" - // and remove the user again. - defer func() { - if cleanup == false { - // Operation successful - nothing to clean up - return - } - - err := w.config.State.RemoveUser(user.UserTag()) - if err != nil { - // Best we can do here is log an error. - w.config.Logger.Warningf("add metrics user failed, but could not clean up user %q: %v", - username, err) - } - }() - - default: - return http.StatusInternalServerError, errors.Annotatef(err, "failed to create user %q: %v", username, err) - } - - // Give the new user permission to access the metrics endpoint. - var model model - model, err = w.config.State.Model() - if err != nil { - return http.StatusInternalServerError, errors.Annotatef(err, "retrieving current model: %v", err) - } - - _, err = model.AddUser(state.UserAccessSpec{ - User: user.UserTag(), - CreatedBy: names.NewUserTag(userCreator), - Access: permission.ReadAccess, - }) - if err != nil && !errors.Is(err, errors.AlreadyExists) { - return http.StatusInternalServerError, errors.Annotatef(err, "adding user %q to model %q: %v", username, bootstrap.ControllerModelName, err) - } - - cleanup = false - return http.StatusOK, nil -} - -func (w *Worker) handleRemoveMetricsUser(resp http.ResponseWriter, req *http.Request) { - username := mux.Vars(req)["username"] - code, err := w.removeMetricsUser(username) - if err != nil { - w.writeResponse(resp, code, errorf("%v", err)) - return - } - - w.writeResponse(resp, code, infof("deleted user %q", username)) -} - -func (w *Worker) removeMetricsUser(username string) (int, error) { - err := validateMetricsUsername(username) - if err != nil { - return http.StatusBadRequest, err - } - - userTag := names.NewUserTag(username) - // We shouldn't mess with users that weren't created by us. - user, err := w.config.State.User(userTag) - if errors.Is(err, errors.NotFound) || errors.Is(err, errors.UserNotFound) || stateerrors.IsDeletedUserError(err) { - // succeed as no-op - return http.StatusOK, nil - } else if err != nil { - return http.StatusInternalServerError, err - } - if user.CreatedBy() != userCreator { - return http.StatusForbidden, errors.Forbiddenf("cannot remove user %q created by %q", user.Name(), user.CreatedBy()) - } - - err = w.config.State.RemoveUser(userTag) - // Any "not found" errors should have been caught above, so fail here. - if err != nil { - return http.StatusInternalServerError, err - } - - return http.StatusOK, nil -} - -func validateMetricsUsername(username string) error { - if username == "" { - return errors.BadRequestf("missing username") - } - - if !names.IsValidUserName(username) { - return errors.NotValidf("username %q", username) - } - - if !strings.HasPrefix(username, jujuMetricsUserPrefix) { - return errors.BadRequestf("metrics username %q should have prefix %q", username, jujuMetricsUserPrefix) - } - - return nil -} - -func (w *Worker) writeResponse(resp http.ResponseWriter, statusCode int, body any) { - w.config.Logger.Debugf("operation finished with HTTP status %v", statusCode) - resp.Header().Set("Content-Type", "application/json") - - message, err := json.Marshal(body) - if err != nil { - w.config.Logger.Errorf("error marshalling response body to JSON: %v", err) - w.config.Logger.Errorf("response body was %#v", body) - - // Mark this as an "internal server error" - statusCode = http.StatusInternalServerError - // Just write an empty response - message = []byte("{}") - } - - resp.WriteHeader(statusCode) - w.config.Logger.Tracef("returning response %q", message) - _, err = resp.Write(message) - if err != nil { - w.config.Logger.Warningf("error writing HTTP response: %v", err) - } -} - -// infof returns an informational response body that can be marshalled into -// JSON (in the case of a successful operation). It has the form -// -// {"message": } -func infof(format string, args ...any) any { - return struct { - Message string `json:"message"` - }{ - Message: fmt.Sprintf(format, args...), - } -} - -// errorf returns an error response body that can be marshalled into JSON (in -// the case of a failed operation). It has the form -// -// {"error": } -func errorf(format string, args ...any) any { - return struct { - Error string `json:"error"` - }{ - Error: fmt.Sprintf(format, args...), - } -} diff --git a/internal/worker/controlsocket/handlers_test.go b/internal/worker/controlsocket/handlers_test.go deleted file mode 100644 index 6bef360f410..00000000000 --- a/internal/worker/controlsocket/handlers_test.go +++ /dev/null @@ -1,446 +0,0 @@ -// Copyright 2023 Canonical Ltd. -// Licensed under the AGPLv3, see LICENCE file for details. - -package controlsocket - -import ( - "encoding/json" - "fmt" - "io" - "net" - "net/http" - "path" - "strings" - - "github.com/juju/errors" - "github.com/juju/loggo/v2" - "github.com/juju/names/v5" - jc "github.com/juju/testing/checkers" - gc "gopkg.in/check.v1" - - "github.com/juju/juju/core/permission" - "github.com/juju/juju/state" - stateerrors "github.com/juju/juju/state/errors" -) - -type handlerSuite struct { - state *fakeState - logger Logger -} - -var _ = gc.Suite(&handlerSuite{}) - -type handlerTest struct { - // Request - method string - endpoint string - body string - // Response - statusCode int - response string // response body - ignoreBody bool // if true, test will not read the request body -} - -func (s *handlerSuite) SetUpTest(c *gc.C) { - s.state = &fakeState{} - s.logger = loggo.GetLogger(c.TestName()) -} - -func (s *handlerSuite) runHandlerTest(c *gc.C, test handlerTest) { - tmpDir := c.MkDir() - socket := path.Join(tmpDir, "test.socket") - - _, err := NewWorker(Config{ - State: s.state, - Logger: s.logger, - SocketName: socket, - }) - c.Assert(err, jc.ErrorIsNil) - - serverURL := "http://localhost:8080" - req, err := http.NewRequest( - test.method, - serverURL+test.endpoint, - strings.NewReader(test.body), - ) - c.Assert(err, jc.ErrorIsNil) - - resp, err := client(socket).Do(req) - c.Assert(err, jc.ErrorIsNil) - c.Assert(resp.StatusCode, gc.Equals, test.statusCode) - - if test.ignoreBody { - return - } - data, err := io.ReadAll(resp.Body) - c.Assert(err, jc.ErrorIsNil) - err = resp.Body.Close() - c.Assert(err, jc.ErrorIsNil) - - // Response should be valid JSON - c.Check(resp.Header.Get("Content-Type"), gc.Equals, "application/json") - err = json.Unmarshal(data, &struct{}{}) - c.Assert(err, jc.ErrorIsNil) - if test.response != "" { - c.Check(string(data), gc.Matches, test.response) - } -} - -func (s *handlerSuite) assertState(c *gc.C, users []fakeUser) { - c.Assert(len(s.state.users), gc.Equals, len(users)) - - for _, expected := range users { - actual, ok := s.state.users[expected.name] - c.Assert(ok, gc.Equals, true) - c.Check(actual.creator, gc.Equals, expected.creator) - c.Check(actual.password, gc.Equals, expected.password) - } -} - -func (s *handlerSuite) TestMetricsUsersAddInvalidMethod(c *gc.C) { - s.runHandlerTest(c, handlerTest{ - method: http.MethodGet, - endpoint: "/metrics-users", - statusCode: http.StatusMethodNotAllowed, - ignoreBody: true, - }) -} - -func (s *handlerSuite) TestMetricsUsersAddMissingBody(c *gc.C) { - s.runHandlerTest(c, handlerTest{ - method: http.MethodPost, - endpoint: "/metrics-users", - statusCode: http.StatusBadRequest, - response: ".*missing request body.*", - }) -} - -func (s *handlerSuite) TestMetricsUsersAddInvalidBody(c *gc.C) { - s.runHandlerTest(c, handlerTest{ - method: http.MethodPost, - endpoint: "/metrics-users", - body: "username foo, password bar", - statusCode: http.StatusBadRequest, - response: ".*request body is not valid JSON.*", - }) -} - -func (s *handlerSuite) TestMetricsUsersAddMissingUsername(c *gc.C) { - s.runHandlerTest(c, handlerTest{ - method: http.MethodPost, - endpoint: "/metrics-users", - body: `{"password":"bar"}`, - statusCode: http.StatusBadRequest, - response: ".*missing username.*", - }) -} - -func (s *handlerSuite) TestMetricsUsersAddMissingPassword(c *gc.C) { - s.runHandlerTest(c, handlerTest{ - method: http.MethodPost, - endpoint: "/metrics-users", - body: `{"username":"juju-metrics-r0"}`, - statusCode: http.StatusBadRequest, - response: ".*empty password.*", - }) -} - -func (s *handlerSuite) TestMetricsUsersAddUsernameMissingPrefix(c *gc.C) { - s.runHandlerTest(c, handlerTest{ - method: http.MethodPost, - endpoint: "/metrics-users", - body: `{"username":"foo","password":"bar"}`, - statusCode: http.StatusBadRequest, - response: `.*username .* should have prefix \\\"juju-metrics-\\\".*`, - }) -} - -func (s *handlerSuite) TestMetricsUsersAddSuccess(c *gc.C) { - s.state = newFakeState(nil) - s.runHandlerTest(c, handlerTest{ - method: http.MethodPost, - endpoint: "/metrics-users", - body: `{"username":"juju-metrics-r0","password":"bar"}`, - statusCode: http.StatusOK, - response: `.*created user \\\"juju-metrics-r0\\\".*`, - }) - s.assertState(c, []fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: "controller@juju"}, - }) -} - -func (s *handlerSuite) TestMetricsUsersAddAlreadyExists(c *gc.C) { - s.state = newFakeState([]fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: "not-you"}, - }) - s.runHandlerTest(c, handlerTest{ - method: http.MethodPost, - endpoint: "/metrics-users", - body: `{"username":"juju-metrics-r0","password":"bar"}`, - statusCode: http.StatusConflict, - response: ".*user .* already exists.*", - }) - // Nothing should have changed. - s.assertState(c, []fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: "not-you"}, - }) -} - -func (s *handlerSuite) TestMetricsUsersAddDifferentPassword(c *gc.C) { - s.state = newFakeState([]fakeUser{ - {name: "juju-metrics-r0", password: "foo", creator: userCreator}, - }) - s.runHandlerTest(c, handlerTest{ - method: http.MethodPost, - endpoint: "/metrics-users", - body: `{"username":"juju-metrics-r0","password":"bar"}`, - statusCode: http.StatusConflict, - response: `.*user \\\"juju-metrics-r0\\\" already exists.*`, - }) - // Nothing should have changed. - s.assertState(c, []fakeUser{ - {name: "juju-metrics-r0", password: "foo", creator: userCreator}, - }) -} - -func (s *handlerSuite) TestMetricsUsersAddAddErr(c *gc.C) { - s.state = newFakeState(nil) - s.state.addErr = fmt.Errorf("spanner in the works") - - s.runHandlerTest(c, handlerTest{ - method: http.MethodPost, - endpoint: "/metrics-users", - body: `{"username":"juju-metrics-r0","password":"bar"}`, - statusCode: http.StatusInternalServerError, - response: ".*spanner in the works.*", - }) - // Nothing should have changed. - s.assertState(c, nil) -} - -func (s *handlerSuite) TestMetricsUsersAddIdempotent(c *gc.C) { - s.state = newFakeState([]fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: userCreator}, - }) - s.runHandlerTest(c, handlerTest{ - method: http.MethodPost, - endpoint: "/metrics-users", - body: `{"username":"juju-metrics-r0","password":"bar"}`, - statusCode: http.StatusOK, // succeed as a no-op - response: `.*created user \\\"juju-metrics-r0\\\".*`, - }) - // Nothing should have changed. - s.assertState(c, []fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: userCreator}, - }) -} - -func (s *handlerSuite) TestMetricsUsersAddFailed(c *gc.C) { - s.state = newFakeState(nil) - s.state.model.err = fmt.Errorf("spanner in the works") - - s.runHandlerTest(c, handlerTest{ - method: http.MethodPost, - endpoint: "/metrics-users", - body: `{"username":"juju-metrics-r0","password":"bar"}`, - statusCode: http.StatusInternalServerError, - response: ".*spanner in the works.*", - }) - s.assertState(c, nil) -} - -func (s *handlerSuite) TestMetricsUsersRemoveInvalidMethod(c *gc.C) { - s.runHandlerTest(c, handlerTest{ - method: http.MethodGet, - endpoint: "/metrics-users/foo", - statusCode: http.StatusMethodNotAllowed, - ignoreBody: true, - }) -} - -func (s *handlerSuite) TestMetricsUsersRemoveUsernameMissingPrefix(c *gc.C) { - s.runHandlerTest(c, handlerTest{ - method: http.MethodDelete, - endpoint: "/metrics-users/foo", - statusCode: http.StatusBadRequest, - response: `.*username .* should have prefix \\\"juju-metrics-\\\".*`, - }) -} - -func (s *handlerSuite) TestMetricsUsersRemoveSuccess(c *gc.C) { - s.state = newFakeState([]fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: "controller@juju"}, - }) - s.runHandlerTest(c, handlerTest{ - method: http.MethodDelete, - endpoint: "/metrics-users/juju-metrics-r0", - statusCode: http.StatusOK, - response: `.*deleted user \\\"juju-metrics-r0\\\".*`, - }) - s.assertState(c, nil) -} - -func (s *handlerSuite) TestMetricsUsersRemoveForbidden(c *gc.C) { - s.state = newFakeState([]fakeUser{ - {name: "juju-metrics-r0", password: "foo", creator: "not-you"}, - }) - s.runHandlerTest(c, handlerTest{ - method: http.MethodDelete, - endpoint: "/metrics-users/juju-metrics-r0", - statusCode: http.StatusForbidden, - response: `.*cannot remove user \\\"juju-metrics-r0\\\" created by \\\"not-you\\\".*`, - }) - // Nothing should have changed. - s.assertState(c, []fakeUser{ - {name: "juju-metrics-r0", password: "foo", creator: "not-you"}, - }) -} - -func (s *handlerSuite) TestMetricsUsersRemoveNotFound(c *gc.C) { - s.state = newFakeState(nil) - s.runHandlerTest(c, handlerTest{ - method: http.MethodDelete, - endpoint: "/metrics-users/juju-metrics-r0", - statusCode: http.StatusOK, // succeed as a no-op - response: `.*deleted user \\\"juju-metrics-r0\\\".*`, - }) - s.assertState(c, nil) -} - -func (s *handlerSuite) TestMetricsUsersRemoveIdempotent(c *gc.C) { - s.state = newFakeState(nil) - s.state.userErr = stateerrors.NewDeletedUserError("juju-metrics-r0") - - s.runHandlerTest(c, handlerTest{ - method: http.MethodDelete, - endpoint: "/metrics-users/juju-metrics-r0", - statusCode: http.StatusOK, // succeed as a no-op - response: `.*deleted user \\\"juju-metrics-r0\\\".*`, - }) - // Nothing should have changed. - s.assertState(c, nil) -} - -func (s *handlerSuite) TestMetricsUsersRemoveFailed(c *gc.C) { - s.state = newFakeState([]fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: userCreator}, - }) - s.state.removeErr = fmt.Errorf("spanner in the works") - - s.runHandlerTest(c, handlerTest{ - method: http.MethodDelete, - endpoint: "/metrics-users/juju-metrics-r0", - body: `{"username":"juju-metrics-r0","password":"bar"}`, - statusCode: http.StatusInternalServerError, - response: ".*spanner in the works.*", - }) - // Nothing should have changed. - s.assertState(c, []fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: userCreator}, - }) -} - -type fakeState struct { - users map[string]fakeUser - model *fakeModel - - userErr, addErr, removeErr error -} - -func newFakeState(users []fakeUser) *fakeState { - s := &fakeState{ - users: make(map[string]fakeUser, len(users)), - } - for _, user := range users { - s.users[user.name] = user - } - s.model = &fakeModel{nil} - return s -} - -func (s *fakeState) User(tag names.UserTag) (user, error) { - if s.userErr != nil { - return nil, s.userErr - } - - username := tag.Name() - u, ok := s.users[username] - if !ok { - return nil, errors.UserNotFoundf("user %q", username) - } - return u, nil -} - -func (s *fakeState) AddUser(name, displayName, password, creator string) (user, error) { - if s.addErr != nil { - return nil, s.addErr - } - - if _, ok := s.users[name]; ok { - // The real state code doesn't return the user if it already exists, it - // returns a typed nil value. - return (*fakeUser)(nil), errors.AlreadyExistsf("user %q", name) - } - - u := fakeUser{name, displayName, password, creator} - s.users[name] = u - return u, nil -} - -func (s *fakeState) RemoveUser(tag names.UserTag) error { - if s.removeErr != nil { - return s.removeErr - } - - username := tag.Name() - if _, ok := s.users[username]; !ok { - return errors.UserNotFoundf("user %q", username) - } - - delete(s.users, username) - return nil -} - -func (s *fakeState) Model() (model, error) { - return s.model, nil -} - -type fakeUser struct { - name, displayName, password, creator string -} - -func (u fakeUser) Name() string { - return u.name -} - -func (u fakeUser) CreatedBy() string { - return u.creator -} - -func (u fakeUser) UserTag() names.UserTag { - return names.NewUserTag(u.name) -} - -func (u fakeUser) PasswordValid(s string) bool { - return s == u.password -} - -type fakeModel struct { - err error -} - -func (m *fakeModel) AddUser(_ state.UserAccessSpec) (permission.UserAccess, error) { - return permission.UserAccess{}, m.err -} - -// Return an *http.Client with custom transport that allows it to connect to -// the given Unix socket. -func client(socketPath string) *http.Client { - return &http.Client{ - Transport: &http.Transport{ - Dial: func(_, _ string) (conn net.Conn, err error) { - return net.Dial("unix", socketPath) - }, - }, - } -} diff --git a/internal/worker/controlsocket/manifold.go b/internal/worker/controlsocket/manifold.go index 120c40fb302..362b82384c2 100644 --- a/internal/worker/controlsocket/manifold.go +++ b/internal/worker/controlsocket/manifold.go @@ -1,4 +1,4 @@ -// Copyright 2023 Canonical Ltd. +// Copyright 2024 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. package controlsocket @@ -10,17 +10,28 @@ import ( "github.com/juju/worker/v4" "github.com/juju/worker/v4/dependency" + "github.com/juju/juju/internal/socketlistener" "github.com/juju/juju/internal/worker/common" workerstate "github.com/juju/juju/internal/worker/state" "github.com/juju/juju/state" ) +// SocketListener describes a worker that listens on a unix socket. +type SocketListener interface { + worker.Worker +} + +func NewSocketListener(config socketlistener.Config) (SocketListener, error) { + return socketlistener.NewSocketListener(config) +} + // ManifoldConfig describes the dependencies required by the controlsocket worker. type ManifoldConfig struct { - StateName string - Logger Logger - NewWorker func(Config) (worker.Worker, error) - SocketName string + StateName string + Logger Logger + NewWorker func(Config) (worker.Worker, error) + NewSocketListener func(socketlistener.Config) (SocketListener, error) + SocketName string } // Manifold returns a Manifold that encapsulates the controlsocket worker. @@ -44,6 +55,9 @@ func (cfg ManifoldConfig) Validate() error { if cfg.NewWorker == nil { return errors.NotValidf("nil NewWorker func") } + if cfg.NewSocketListener == nil { + return errors.NotValidf("nil NewSocketListener func") + } if cfg.SocketName == "" { return errors.NotValidf("empty SocketName") } @@ -75,9 +89,10 @@ func (cfg ManifoldConfig) start(ctx context.Context, getter dependency.Getter) ( var w worker.Worker w, err = cfg.NewWorker(Config{ - State: stateShim{st}, - Logger: cfg.Logger, - SocketName: cfg.SocketName, + State: stateShim{st}, + Logger: cfg.Logger, + SocketName: cfg.SocketName, + NewSocketListener: cfg.NewSocketListener, }) if err != nil { return nil, errors.Trace(err) diff --git a/internal/worker/controlsocket/worker.go b/internal/worker/controlsocket/worker.go index 7af539d4171..e680b77d837 100644 --- a/internal/worker/controlsocket/worker.go +++ b/internal/worker/controlsocket/worker.go @@ -1,4 +1,4 @@ -// Copyright 2023 Canonical Ltd. +// Copyright 2024 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. //go:build linux @@ -6,24 +6,36 @@ package controlsocket import ( - "context" - "net" + "encoding/json" + "fmt" + "io" "net/http" + "strings" "time" "github.com/gorilla/mux" "github.com/juju/errors" + "github.com/juju/names/v5" "github.com/juju/worker/v4" - "gopkg.in/tomb.v2" + "github.com/juju/worker/v4/catacomb" - "github.com/juju/juju/juju/sockets" + "github.com/juju/juju/core/permission" + "github.com/juju/juju/environs/bootstrap" + "github.com/juju/juju/internal/socketlistener" + "github.com/juju/juju/state" + stateerrors "github.com/juju/juju/state/errors" ) -// logger is here to stop the desire of creating a package level logger. -// Don't do this, instead use the one passed as manifold config. -type logger any +const ( + // jujuMetricsUserPrefix defines the "namespace" in which this worker is + // allowed to create/remove users. + jujuMetricsUserPrefix = "juju-metrics-" -var _ logger = struct{}{} + // userCreator is the listed "creator" of metrics users in state. + // This user CANNOT be a local user (it must have a domain), otherwise the + // model addUser code will complain about the user not existing. + userCreator = "controller@juju" +) // Logger represents the methods used by the worker to log information. type Logger interface { @@ -36,9 +48,10 @@ type Logger interface { // Config represents configuration for the controlsocket worker. type Config struct { - State State - Logger Logger - SocketName string + State State + Logger Logger + SocketName string + NewSocketListener func(socketlistener.Config) (SocketListener, error) } // Validate returns an error if config cannot drive the Worker. @@ -52,14 +65,16 @@ func (config Config) Validate() error { if config.SocketName == "" { return errors.NotValidf("empty SocketName") } + if config.NewSocketListener == nil { + return errors.NotValidf("nil NewSocketListener func") + } return nil } // Worker is a controlsocket worker. type Worker struct { config Config - tomb tomb.Tomb - listener net.Listener + catacomb catacomb.Catacomb } // NewWorker returns a controlsocket worker with the given config. @@ -68,58 +83,252 @@ func NewWorker(config Config) (worker.Worker, error) { return nil, errors.Trace(err) } - l, err := sockets.Listen(sockets.Socket{ - Address: config.SocketName, - Network: "unix", + w := &Worker{ + config: config, + } + sl, err := config.NewSocketListener(socketlistener.Config{ + Logger: config.Logger, + SocketName: config.SocketName, + RegisterHandlers: w.registerHandlers, + ShutdownTimeout: 500 * time.Millisecond, }) if err != nil { - return nil, errors.Annotate(err, "unable to listen on unix socket") + return nil, errors.Annotate(err, "control socket listener:") } - config.Logger.Debugf("controlsocket worker listening on socket %q", config.SocketName) - w := &Worker{ - config: config, - listener: l, + err = catacomb.Invoke(catacomb.Plan{ + Site: &w.catacomb, + Work: w.run, + Init: []worker.Worker{sl}, + }) + if err != nil { + return nil, errors.Trace(err) } - w.tomb.Go(w.run) return w, nil } func (w *Worker) Kill() { - w.tomb.Kill(nil) + w.catacomb.Kill(nil) } func (w *Worker) Wait() error { - return w.tomb.Wait() + return w.catacomb.Wait() } -// run listens on the control socket and handles requests. func (w *Worker) run() error { - router := mux.NewRouter() - w.registerHandlers(router) + select { + case <-w.catacomb.Dying(): + return w.catacomb.ErrDying() + } +} + +func (w *Worker) registerHandlers(r *mux.Router) { + r.HandleFunc("/metrics-users", w.handleAddMetricsUser). + Methods(http.MethodPost) + r.HandleFunc("/metrics-users/{username}", w.handleRemoveMetricsUser). + Methods(http.MethodDelete) +} + +type addMetricsUserBody struct { + Username string `json:"username"` + Password string `json:"password"` +} + +func (w *Worker) handleAddMetricsUser(resp http.ResponseWriter, req *http.Request) { + var parsedBody addMetricsUserBody + defer req.Body.Close() + err := json.NewDecoder(req.Body).Decode(&parsedBody) + if errors.Is(err, io.EOF) { + w.writeResponse(resp, http.StatusBadRequest, errorf("missing request body")) + return + } else if err != nil { + w.writeResponse(resp, http.StatusBadRequest, errorf("request body is not valid JSON: %v", err)) + return + } + + code, err := w.addMetricsUser(parsedBody.Username, parsedBody.Password) + if err != nil { + w.writeResponse(resp, code, errorf("%v", err)) + return + } - srv := http.Server{Handler: router} - defer func() { - err := srv.Close() + w.writeResponse(resp, code, infof("created user %q", parsedBody.Username)) +} + +func (w *Worker) addMetricsUser(username, password string) (int, error) { + err := validateMetricsUsername(username) + if err != nil { + return http.StatusBadRequest, err + } + + if password == "" { + return http.StatusBadRequest, errors.NotValidf("empty password") + } + + user, err := w.config.State.AddUser(username, username, password, userCreator) + cleanup := true + // Error handling here is a bit subtle. + switch { + case errors.Is(err, errors.AlreadyExists): + // Retrieve existing user + user, err = w.config.State.User(names.NewUserTag(username)) if err != nil { - w.config.Logger.Warningf("error closing HTTP server: %v", err) + return http.StatusInternalServerError, + fmt.Errorf("retrieving existing user %q: %v", username, err) } - }() - - go func() { - // Wait for the tomb to start dying and then shut the server down. - <-w.tomb.Dying() - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - if err := srv.Shutdown(ctx); err != nil { - w.config.Logger.Warningf("error shutting down HTTP server: %v", err) + + // We want this operation to be idempotent, but at the same time, this + // worker shouldn't mess with users that have not been created by it. + // So ensure the user is identical to what we would have created, and + // otherwise error. + if user.CreatedBy() != userCreator { + return http.StatusConflict, errors.AlreadyExistsf("user %q (created by %q)", user.Name(), user.CreatedBy()) } - }() + if !user.PasswordValid(password) { + return http.StatusConflict, errors.AlreadyExistsf("user %q", user.Name()) + } + + case err == nil: + // At this point, the operation is in a partially completed state - we've + // added the user, but haven't granted them the correct model permissions. + // If there is an error granting permissions, we should attempt to "rollback" + // and remove the user again. + defer func() { + if cleanup == false { + // Operation successful - nothing to clean up + return + } + + err := w.config.State.RemoveUser(user.UserTag()) + if err != nil { + // Best we can do here is log an error. + w.config.Logger.Warningf("add metrics user failed, but could not clean up user %q: %v", + username, err) + } + }() + + default: + return http.StatusInternalServerError, errors.Annotatef(err, "failed to create user %q: %v", username, err) + } + + // Give the new user permission to access the metrics endpoint. + var model model + model, err = w.config.State.Model() + if err != nil { + return http.StatusInternalServerError, errors.Annotatef(err, "retrieving current model: %v", err) + } + + _, err = model.AddUser(state.UserAccessSpec{ + User: user.UserTag(), + CreatedBy: names.NewUserTag(userCreator), + Access: permission.ReadAccess, + }) + if err != nil && !errors.Is(err, errors.AlreadyExists) { + return http.StatusInternalServerError, errors.Annotatef(err, "adding user %q to model %q: %v", username, bootstrap.ControllerModelName, err) + } - w.config.Logger.Debugf("controlsocket worker now serving") - defer w.config.Logger.Debugf("controlsocket worker serving finished") - if err := srv.Serve(w.listener); err != http.ErrServerClosed { - return err + cleanup = false + return http.StatusOK, nil +} + +func (w *Worker) handleRemoveMetricsUser(resp http.ResponseWriter, req *http.Request) { + username := mux.Vars(req)["username"] + code, err := w.removeMetricsUser(username) + if err != nil { + w.writeResponse(resp, code, errorf("%v", err)) + return + } + + w.writeResponse(resp, code, infof("deleted user %q", username)) +} + +func (w *Worker) removeMetricsUser(username string) (int, error) { + err := validateMetricsUsername(username) + if err != nil { + return http.StatusBadRequest, err + } + + userTag := names.NewUserTag(username) + // We shouldn't mess with users that weren't created by us. + user, err := w.config.State.User(userTag) + if errors.Is(err, errors.NotFound) || errors.Is(err, errors.UserNotFound) || stateerrors.IsDeletedUserError(err) { + // succeed as no-op + return http.StatusOK, nil + } else if err != nil { + return http.StatusInternalServerError, err + } + if user.CreatedBy() != userCreator { + return http.StatusForbidden, errors.Forbiddenf("cannot remove user %q created by %q", user.Name(), user.CreatedBy()) + } + + err = w.config.State.RemoveUser(userTag) + // Any "not found" errors should have been caught above, so fail here. + if err != nil { + return http.StatusInternalServerError, err + } + + return http.StatusOK, nil +} + +func validateMetricsUsername(username string) error { + if username == "" { + return errors.BadRequestf("missing username") + } + + if !names.IsValidUserName(username) { + return errors.NotValidf("username %q", username) + } + + if !strings.HasPrefix(username, jujuMetricsUserPrefix) { + return errors.BadRequestf("metrics username %q should have prefix %q", username, jujuMetricsUserPrefix) } + return nil } + +func (w *Worker) writeResponse(resp http.ResponseWriter, statusCode int, body any) { + w.config.Logger.Debugf("operation finished with HTTP status %v", statusCode) + resp.Header().Set("Content-Type", "application/json") + + message, err := json.Marshal(body) + if err != nil { + w.config.Logger.Errorf("error marshalling response body to JSON: %v", err) + w.config.Logger.Errorf("response body was %#v", body) + + // Mark this as an "internal server error" + statusCode = http.StatusInternalServerError + // Just write an empty response + message = []byte("{}") + } + + resp.WriteHeader(statusCode) + w.config.Logger.Tracef("returning response %q", message) + _, err = resp.Write(message) + if err != nil { + w.config.Logger.Warningf("error writing HTTP response: %v", err) + } +} + +// infof returns an informational response body that can be marshalled into +// JSON (in the case of a successful operation). It has the form +// +// {"message": } +func infof(format string, args ...any) any { + return struct { + Message string `json:"message"` + }{ + Message: fmt.Sprintf(format, args...), + } +} + +// errorf returns an error response body that can be marshalled into JSON (in +// the case of a failed operation). It has the form +// +// {"error": } +func errorf(format string, args ...any) any { + return struct { + Error string `json:"error"` + }{ + Error: fmt.Sprintf(format, args...), + } +} diff --git a/internal/worker/controlsocket/worker_test.go b/internal/worker/controlsocket/worker_test.go index 947b3aa9920..d7b781726c0 100644 --- a/internal/worker/controlsocket/worker_test.go +++ b/internal/worker/controlsocket/worker_test.go @@ -1,64 +1,450 @@ -// Copyright 2023 Canonical Ltd. +// Copyright 2024 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. package controlsocket import ( + "context" + "encoding/json" "fmt" - "io/fs" + "io" + "net" "net/http" - "os" "path" + "strings" + + "github.com/juju/errors" + "github.com/juju/loggo/v2" + "github.com/juju/names/v5" + + "github.com/juju/juju/core/permission" + "github.com/juju/juju/state" + stateerrors "github.com/juju/juju/state/errors" jc "github.com/juju/testing/checkers" gc "gopkg.in/check.v1" ) type workerSuite struct { - logger *fakeLogger + state *fakeState + logger Logger } var _ = gc.Suite(&workerSuite{}) +type handlerTest struct { + // Request + method string + endpoint string + body string + // Response + statusCode int + response string // response body + ignoreBody bool // if true, test will not read the request body +} + func (s *workerSuite) SetUpTest(c *gc.C) { - s.logger = &fakeLogger{} + s.state = &fakeState{} + s.logger = loggo.GetLogger(c.TestName()) } -func (s *workerSuite) TestStartStopWorker(c *gc.C) { +func (s *workerSuite) runHandlerTest(c *gc.C, test handlerTest) { tmpDir := c.MkDir() socket := path.Join(tmpDir, "test.socket") - worker, err := NewWorker(Config{ - State: &fakeState{}, - Logger: s.logger, - SocketName: socket, + _, err := NewWorker(Config{ + State: s.state, + Logger: s.logger, + SocketName: socket, + NewSocketListener: NewSocketListener, }) c.Assert(err, jc.ErrorIsNil) - // Check socket is created with correct permissions - fi, err := os.Stat(socket) + serverURL := "http://localhost:8080" + req, err := http.NewRequest( + test.method, + serverURL+test.endpoint, + strings.NewReader(test.body), + ) c.Assert(err, jc.ErrorIsNil) - c.Assert(fi.Mode(), gc.Equals, fs.ModeSocket|0700) - // Check server is up - cl := client(socket) - resp, err := cl.Get("http://a/foo") + resp, err := client(socket).Do(req) + c.Assert(err, jc.ErrorIsNil) + c.Assert(resp.StatusCode, gc.Equals, test.statusCode) + + if test.ignoreBody { + return + } + data, err := io.ReadAll(resp.Body) + c.Assert(err, jc.ErrorIsNil) + err = resp.Body.Close() c.Assert(err, jc.ErrorIsNil) - c.Assert(resp.StatusCode, gc.Equals, http.StatusNotFound) - worker.Kill() - err = worker.Wait() + // Response should be valid JSON + c.Check(resp.Header.Get("Content-Type"), gc.Equals, "application/json") + err = json.Unmarshal(data, &struct{}{}) c.Assert(err, jc.ErrorIsNil) + if test.response != "" { + c.Check(string(data), gc.Matches, test.response) + } +} + +func (s *workerSuite) assertState(c *gc.C, users []fakeUser) { + c.Assert(len(s.state.users), gc.Equals, len(users)) + + for _, expected := range users { + actual, ok := s.state.users[expected.name] + c.Assert(ok, gc.Equals, true) + c.Check(actual.creator, gc.Equals, expected.creator) + c.Check(actual.password, gc.Equals, expected.password) + } +} + +func (s *workerSuite) TestMetricsUsersAddInvalidMethod(c *gc.C) { + s.runHandlerTest(c, handlerTest{ + method: http.MethodGet, + endpoint: "/metrics-users", + statusCode: http.StatusMethodNotAllowed, + ignoreBody: true, + }) +} + +func (s *workerSuite) TestMetricsUsersAddMissingBody(c *gc.C) { + s.runHandlerTest(c, handlerTest{ + method: http.MethodPost, + endpoint: "/metrics-users", + statusCode: http.StatusBadRequest, + response: ".*missing request body.*", + }) +} + +func (s *workerSuite) TestMetricsUsersAddInvalidBody(c *gc.C) { + s.runHandlerTest(c, handlerTest{ + method: http.MethodPost, + endpoint: "/metrics-users", + body: "username foo, password bar", + statusCode: http.StatusBadRequest, + response: ".*request body is not valid JSON.*", + }) +} + +func (s *workerSuite) TestMetricsUsersAddMissingUsername(c *gc.C) { + s.runHandlerTest(c, handlerTest{ + method: http.MethodPost, + endpoint: "/metrics-users", + body: `{"password":"bar"}`, + statusCode: http.StatusBadRequest, + response: ".*missing username.*", + }) +} + +func (s *workerSuite) TestMetricsUsersAddMissingPassword(c *gc.C) { + s.runHandlerTest(c, handlerTest{ + method: http.MethodPost, + endpoint: "/metrics-users", + body: `{"username":"juju-metrics-r0"}`, + statusCode: http.StatusBadRequest, + response: ".*empty password.*", + }) +} + +func (s *workerSuite) TestMetricsUsersAddUsernameMissingPrefix(c *gc.C) { + s.runHandlerTest(c, handlerTest{ + method: http.MethodPost, + endpoint: "/metrics-users", + body: `{"username":"foo","password":"bar"}`, + statusCode: http.StatusBadRequest, + response: `.*username .* should have prefix \\\"juju-metrics-\\\".*`, + }) +} + +func (s *workerSuite) TestMetricsUsersAddSuccess(c *gc.C) { + s.state = newFakeState(nil) + s.runHandlerTest(c, handlerTest{ + method: http.MethodPost, + endpoint: "/metrics-users", + body: `{"username":"juju-metrics-r0","password":"bar"}`, + statusCode: http.StatusOK, + response: `.*created user \\\"juju-metrics-r0\\\".*`, + }) + s.assertState(c, []fakeUser{ + {name: "juju-metrics-r0", password: "bar", creator: "controller@juju"}, + }) +} + +func (s *workerSuite) TestMetricsUsersAddAlreadyExists(c *gc.C) { + s.state = newFakeState([]fakeUser{ + {name: "juju-metrics-r0", password: "bar", creator: "not-you"}, + }) + s.runHandlerTest(c, handlerTest{ + method: http.MethodPost, + endpoint: "/metrics-users", + body: `{"username":"juju-metrics-r0","password":"bar"}`, + statusCode: http.StatusConflict, + response: ".*user .* already exists.*", + }) + // Nothing should have changed. + s.assertState(c, []fakeUser{ + {name: "juju-metrics-r0", password: "bar", creator: "not-you"}, + }) +} + +func (s *workerSuite) TestMetricsUsersAddDifferentPassword(c *gc.C) { + s.state = newFakeState([]fakeUser{ + {name: "juju-metrics-r0", password: "foo", creator: userCreator}, + }) + s.runHandlerTest(c, handlerTest{ + method: http.MethodPost, + endpoint: "/metrics-users", + body: `{"username":"juju-metrics-r0","password":"bar"}`, + statusCode: http.StatusConflict, + response: `.*user \\\"juju-metrics-r0\\\" already exists.*`, + }) + // Nothing should have changed. + s.assertState(c, []fakeUser{ + {name: "juju-metrics-r0", password: "foo", creator: userCreator}, + }) +} + +func (s *workerSuite) TestMetricsUsersAddAddErr(c *gc.C) { + s.state = newFakeState(nil) + s.state.addErr = fmt.Errorf("spanner in the works") + + s.runHandlerTest(c, handlerTest{ + method: http.MethodPost, + endpoint: "/metrics-users", + body: `{"username":"juju-metrics-r0","password":"bar"}`, + statusCode: http.StatusInternalServerError, + response: ".*spanner in the works.*", + }) + // Nothing should have changed. + s.assertState(c, nil) +} + +func (s *workerSuite) TestMetricsUsersAddIdempotent(c *gc.C) { + s.state = newFakeState([]fakeUser{ + {name: "juju-metrics-r0", password: "bar", creator: userCreator}, + }) + s.runHandlerTest(c, handlerTest{ + method: http.MethodPost, + endpoint: "/metrics-users", + body: `{"username":"juju-metrics-r0","password":"bar"}`, + statusCode: http.StatusOK, // succeed as a no-op + response: `.*created user \\\"juju-metrics-r0\\\".*`, + }) + // Nothing should have changed. + s.assertState(c, []fakeUser{ + {name: "juju-metrics-r0", password: "bar", creator: userCreator}, + }) +} + +func (s *workerSuite) TestMetricsUsersAddFailed(c *gc.C) { + s.state = newFakeState(nil) + s.state.model.err = fmt.Errorf("spanner in the works") + + s.runHandlerTest(c, handlerTest{ + method: http.MethodPost, + endpoint: "/metrics-users", + body: `{"username":"juju-metrics-r0","password":"bar"}`, + statusCode: http.StatusInternalServerError, + response: ".*spanner in the works.*", + }) + s.assertState(c, nil) +} + +func (s *workerSuite) TestMetricsUsersRemoveInvalidMethod(c *gc.C) { + s.runHandlerTest(c, handlerTest{ + method: http.MethodGet, + endpoint: "/metrics-users/foo", + statusCode: http.StatusMethodNotAllowed, + ignoreBody: true, + }) +} + +func (s *workerSuite) TestMetricsUsersRemoveUsernameMissingPrefix(c *gc.C) { + s.runHandlerTest(c, handlerTest{ + method: http.MethodDelete, + endpoint: "/metrics-users/foo", + statusCode: http.StatusBadRequest, + response: `.*username .* should have prefix \\\"juju-metrics-\\\".*`, + }) +} + +func (s *workerSuite) TestMetricsUsersRemoveSuccess(c *gc.C) { + s.state = newFakeState([]fakeUser{ + {name: "juju-metrics-r0", password: "bar", creator: "controller@juju"}, + }) + s.runHandlerTest(c, handlerTest{ + method: http.MethodDelete, + endpoint: "/metrics-users/juju-metrics-r0", + statusCode: http.StatusOK, + response: `.*deleted user \\\"juju-metrics-r0\\\".*`, + }) + s.assertState(c, nil) +} + +func (s *workerSuite) TestMetricsUsersRemoveForbidden(c *gc.C) { + s.state = newFakeState([]fakeUser{ + {name: "juju-metrics-r0", password: "foo", creator: "not-you"}, + }) + s.runHandlerTest(c, handlerTest{ + method: http.MethodDelete, + endpoint: "/metrics-users/juju-metrics-r0", + statusCode: http.StatusForbidden, + response: `.*cannot remove user \\\"juju-metrics-r0\\\" created by \\\"not-you\\\".*`, + }) + // Nothing should have changed. + s.assertState(c, []fakeUser{ + {name: "juju-metrics-r0", password: "foo", creator: "not-you"}, + }) +} + +func (s *workerSuite) TestMetricsUsersRemoveNotFound(c *gc.C) { + s.state = newFakeState(nil) + s.runHandlerTest(c, handlerTest{ + method: http.MethodDelete, + endpoint: "/metrics-users/juju-metrics-r0", + statusCode: http.StatusOK, // succeed as a no-op + response: `.*deleted user \\\"juju-metrics-r0\\\".*`, + }) + s.assertState(c, nil) +} + +func (s *workerSuite) TestMetricsUsersRemoveIdempotent(c *gc.C) { + s.state = newFakeState(nil) + s.state.userErr = stateerrors.NewDeletedUserError("juju-metrics-r0") + + s.runHandlerTest(c, handlerTest{ + method: http.MethodDelete, + endpoint: "/metrics-users/juju-metrics-r0", + statusCode: http.StatusOK, // succeed as a no-op + response: `.*deleted user \\\"juju-metrics-r0\\\".*`, + }) + // Nothing should have changed. + s.assertState(c, nil) +} + +func (s *workerSuite) TestMetricsUsersRemoveFailed(c *gc.C) { + s.state = newFakeState([]fakeUser{ + {name: "juju-metrics-r0", password: "bar", creator: userCreator}, + }) + s.state.removeErr = fmt.Errorf("spanner in the works") + + s.runHandlerTest(c, handlerTest{ + method: http.MethodDelete, + endpoint: "/metrics-users/juju-metrics-r0", + body: `{"username":"juju-metrics-r0","password":"bar"}`, + statusCode: http.StatusInternalServerError, + response: ".*spanner in the works.*", + }) + // Nothing should have changed. + s.assertState(c, []fakeUser{ + {name: "juju-metrics-r0", password: "bar", creator: userCreator}, + }) +} - // Check server has stopped - resp, err = cl.Get("http://a/foo") - c.Assert(err, gc.ErrorMatches, ".*connection refused") +type fakeState struct { + users map[string]fakeUser + model *fakeModel + + userErr, addErr, removeErr error +} + +func newFakeState(users []fakeUser) *fakeState { + s := &fakeState{ + users: make(map[string]fakeUser, len(users)), + } + for _, user := range users { + s.users[user.name] = user + } + s.model = &fakeModel{nil} + return s +} + +func (s *fakeState) User(tag names.UserTag) (user, error) { + if s.userErr != nil { + return nil, s.userErr + } + + username := tag.Name() + u, ok := s.users[username] + if !ok { + return nil, errors.UserNotFoundf("user %q", username) + } + return u, nil +} + +func (s *fakeState) AddUser(name, displayName, password, creator string) (user, error) { + if s.addErr != nil { + return nil, s.addErr + } + + if _, ok := s.users[name]; ok { + // The real state code doesn't return the user if it already exists, it + // returns a typed nil value. + return (*fakeUser)(nil), errors.AlreadyExistsf("user %q", name) + } + + u := fakeUser{name, displayName, password, creator} + s.users[name] = u + return u, nil +} + +func (s *fakeState) RemoveUser(tag names.UserTag) error { + if s.removeErr != nil { + return s.removeErr + } + + username := tag.Name() + if _, ok := s.users[username]; !ok { + return errors.UserNotFoundf("user %q", username) + } + + delete(s.users, username) + return nil +} + +func (s *fakeState) Model() (model, error) { + return s.model, nil +} + +type fakeUser struct { + name, displayName, password, creator string +} + +func (u fakeUser) Name() string { + return u.name +} + +func (u fakeUser) CreatedBy() string { + return u.creator +} + +func (u fakeUser) UserTag() names.UserTag { + return names.NewUserTag(u.name) +} + +func (u fakeUser) PasswordValid(s string) bool { + return s == u.password +} + +type fakeModel struct { + err error +} + +func (m *fakeModel) AddUser(_ state.UserAccessSpec) (permission.UserAccess, error) { + return permission.UserAccess{}, m.err +} - // No warnings/errors should have been logged - for _, entry := range s.logger.entries { - if entry.level == "ERROR" || entry.level == "WARNING" { - c.Errorf("%s: %s", entry.level, entry.msg) - } +// Return an *http.Client with custom transport that allows it to connect to +// the given Unix socket. +func client(socketPath string) *http.Client { + return &http.Client{ + Transport: &http.Transport{ + DialContext: func(_ context.Context, _, _ string) (conn net.Conn, err error) { + return net.Dial("unix", socketPath) + }, + }, } } From 72289c14cf62a9c8b24c5582e3ded5d3ce4b333b Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Wed, 21 Feb 2024 11:57:35 +0000 Subject: [PATCH 2/5] Use socket instead of SIGHUP to trigger controller config reload. The controllerconfigagent used to listen for a SIGHUP. If it recived one it would reload all the workers watching the controller config. This does not work on kuberenetes since there may be multiple controllers in diferent enviroments that the SIGHUP does not reach. The solution to this problem is to add a new socket and change the controllerconfigagent to listen out on that socket for a reload request. When it recives the reload request it can take the same action as before. --- .../agent/machine/manifolds.go | 6 +- core/paths/paths.go | 7 + internal/socketlistener/socketlistener.go | 17 +- .../socketlistener/socketlistener_test.go | 14 +- .../worker/controlleragentconfig/manifold.go | 44 +++-- .../controlleragentconfig/manifold_test.go | 16 +- .../worker/controlleragentconfig/worker.go | 89 +++++++--- .../controlleragentconfig/worker_test.go | 161 +++++++++++------- internal/worker/controlsocket/worker.go | 8 +- internal/worker/controlsocket/worker_test.go | 35 +--- 10 files changed, 249 insertions(+), 148 deletions(-) diff --git a/cmd/jujud-controller/agent/machine/manifolds.go b/cmd/jujud-controller/agent/machine/manifolds.go index 162e1962236..eb3563050a0 100644 --- a/cmd/jujud-controller/agent/machine/manifolds.go +++ b/cmd/jujud-controller/agent/machine/manifolds.go @@ -360,8 +360,10 @@ func commonManifolds(config ManifoldsConfig) dependency.Manifolds { // Controller agent config manifold watches the controller // agent config and bounces if it changes. controllerAgentConfigName: ifController(controlleragentconfig.Manifold(controlleragentconfig.ManifoldConfig{ - Clock: config.Clock, - Logger: loggo.GetLogger("juju.worker.controlleragentconfig"), + Clock: config.Clock, + Logger: loggo.GetLogger("juju.worker.controlleragentconfig"), + NewSocketListener: controlleragentconfig.NewSocketListener, + SocketName: paths.ConfigChangeSocket(paths.OSUnixLike), })), // The stateconfigwatcher manifold watches the machine agent's diff --git a/core/paths/paths.go b/core/paths/paths.go index dc25e17ba00..e5d906b3c1f 100644 --- a/core/paths/paths.go +++ b/core/paths/paths.go @@ -37,6 +37,7 @@ const ( curtinInstallConfig transientDataDir controlSocket + configChangeSocket ) const ( @@ -68,6 +69,7 @@ var nixVals = map[osVarType]string{ cloudInitCfgDir: "/etc/cloud/cloud.cfg.d", curtinInstallConfig: "/root/curtin-install-cfg.yaml", controlSocket: "/var/lib/juju/control.socket", + configChangeSocket: "/var/lib/juju/configchange.socket", } var winVals = map[osVarType]string{ @@ -203,3 +205,8 @@ func CloudInitCfgDir(os OS) string { func ControlSocket(os OS) string { return osVal(os, controlSocket) } + +// ConfigChangeSocket returns the absolute path to the Juju config change socket. +func ConfigChangeSocket(os OS) string { + return osVal(os, configChangeSocket) +} diff --git a/internal/socketlistener/socketlistener.go b/internal/socketlistener/socketlistener.go index aa191e72e6c..34ee3d5216a 100644 --- a/internal/socketlistener/socketlistener.go +++ b/internal/socketlistener/socketlistener.go @@ -1,6 +1,10 @@ // Copyright 2024 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. +// Package socketlistener provides a worker that will listen on a specified unix +// socket identified by a file descriptor. Handlers are provided to the worker +// that specify endpoints and define the action to be taken when they are +// reached. package socketlistener import ( @@ -24,10 +28,15 @@ type Logger interface { // Config represents configuration for the socketlistener worker. type Config struct { - Logger Logger - SocketName string + Logger Logger + // SocketName is the socket file descriptor. + SocketName string + // RegisterHandlers should register handlers on the router with + // router.HandlerFunc or similar. RegisterHandlers func(router *mux.Router) - ShutdownTimeout time.Duration + // ShutdownTimeout is how long the socketlistener has to gracefully shutdown + // when Kill is called on the worker. + ShutdownTimeout time.Duration } // Validate returns an error if config cannot drive the SocketListener. @@ -78,10 +87,12 @@ func NewSocketListener(config Config) (*SocketListener, error) { return sl, nil } +// Kill is part of the Worker.worker interface. func (sl *SocketListener) Kill() { sl.tomb.Kill(nil) } +// Wait is part of the Worker.worker interface. func (sl *SocketListener) Wait() error { return sl.tomb.Wait() } diff --git a/internal/socketlistener/socketlistener_test.go b/internal/socketlistener/socketlistener_test.go index d116f114cbf..56b3ba0c3db 100644 --- a/internal/socketlistener/socketlistener_test.go +++ b/internal/socketlistener/socketlistener_test.go @@ -48,6 +48,7 @@ func (s *socketListenerSuite) TestStartStopWorker(c *gc.C) { Logger: s.logger, SocketName: socket, RegisterHandlers: registerTestHandlers, + ShutdownTimeout: coretesting.LongWait, }) c.Assert(err, jc.ErrorIsNil) @@ -84,6 +85,8 @@ func (s *socketListenerSuite) TestStartStopWorker(c *gc.C) { } } +// TestEnsureShutdown checks that a slow handler does not return an error if the +// socket listener is shutdown as it handles. func (s *socketListenerSuite) TestEnsureShutdown(c *gc.C) { tmpDir := c.MkDir() socket := path.Join(tmpDir, "test.socket") @@ -93,24 +96,28 @@ func (s *socketListenerSuite) TestEnsureShutdown(c *gc.C) { Logger: s.logger, SocketName: socket, RegisterHandlers: func(r *mux.Router) { - r.HandleFunc("/test-endpoint", func(resp http.ResponseWriter, req *http.Request) { + r.HandleFunc("/slow-handler", func(resp http.ResponseWriter, req *http.Request) { + // Signal that the handler has started. close(start) time.Sleep(time.Second) resp.WriteHeader(http.StatusOK) }).Methods(http.MethodGet) }, + ShutdownTimeout: coretesting.LongWait, }) c.Assert(err, jc.ErrorIsNil) defer workertest.DirtyKill(c, sl) done := make(chan struct{}) go func() { - // Check server is up. + // Send request to slow handler and ensure it does not return error, + // even though server is shut down as soon as it starts. cl := client(socket) - _, err := cl.Get("http://localhost:8080/test-endpoint") + _, err := cl.Get("http://localhost:8080/slow-handler") c.Assert(err, jc.ErrorIsNil) }() go func() { + // Kill socket listener once handler has started. select { case <-start: case <-time.After(coretesting.ShortWait): @@ -119,6 +126,7 @@ func (s *socketListenerSuite) TestEnsureShutdown(c *gc.C) { workertest.CleanKill(c, sl) close(done) }() + // Wait for server to cleanly shutdown select { case <-done: case <-time.After(coretesting.LongWait): diff --git a/internal/worker/controlleragentconfig/manifold.go b/internal/worker/controlleragentconfig/manifold.go index bef721f7ab8..8bd6c30cc46 100644 --- a/internal/worker/controlleragentconfig/manifold.go +++ b/internal/worker/controlleragentconfig/manifold.go @@ -1,23 +1,33 @@ -// Copyright 2023 Canonical Ltd. +// Copyright 2024 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. package controlleragentconfig import ( "context" - "os" - "os/signal" - "syscall" "github.com/juju/clock" "github.com/juju/errors" "github.com/juju/worker/v4" "github.com/juju/worker/v4/dependency" + + "github.com/juju/juju/internal/socketlistener" ) +// SocketListener describes a worker that listens on a unix socket. +type SocketListener interface { + worker.Worker +} + +// NewSocketListener returns a new socket listener with the desired config. +func NewSocketListener(config socketlistener.Config) (SocketListener, error) { + return socketlistener.NewSocketListener(config) +} + // Logger represents the logging methods called. type Logger interface { Errorf(message string, args ...any) + Warningf(message string, args ...any) Infof(message string, args ...any) Debugf(message string, args ...any) } @@ -27,6 +37,10 @@ type Logger interface { type ManifoldConfig struct { Logger Logger Clock clock.Clock + // SocketName is the socket file descriptor. + SocketName string + // NewSocketListener is the function that creates a new socket listener. + NewSocketListener func(socketlistener.Config) (SocketListener, error) } // Validate validates the manifold configuration. @@ -37,6 +51,12 @@ func (cfg ManifoldConfig) Validate() error { if cfg.Clock == nil { return errors.NotValidf("nil Clock") } + if cfg.SocketName == "" { + return errors.NotValidf("empty SocketName") + } + if cfg.NewSocketListener == nil { + return errors.NotValidf("nil NewSocketListener func") + } return nil } @@ -50,9 +70,10 @@ func Manifold(config ManifoldConfig) dependency.Manifold { } w, err := NewWorker(WorkerConfig{ - Logger: config.Logger, - Notify: Notify, - Clock: config.Clock, + Logger: config.Logger, + Clock: config.Clock, + NewSocketListener: config.NewSocketListener, + SocketName: config.SocketName, }) if err != nil { return nil, errors.Trace(err) @@ -80,12 +101,3 @@ func configOutput(in worker.Worker, out any) error { } return nil } - -// Notify sets up the signal handler for the worker. -func Notify(ctx context.Context, ch chan os.Signal) { - if ctx.Err() != nil { - return - } - - signal.Notify(ch, syscall.SIGHUP) -} diff --git a/internal/worker/controlleragentconfig/manifold_test.go b/internal/worker/controlleragentconfig/manifold_test.go index 881c1f935b2..9fc2722387a 100644 --- a/internal/worker/controlleragentconfig/manifold_test.go +++ b/internal/worker/controlleragentconfig/manifold_test.go @@ -1,4 +1,4 @@ -// Copyright 2023 Canonical Ltd. +// Copyright 2024 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. package controlleragentconfig @@ -34,12 +34,22 @@ func (s *manifoldSuite) TestValidateConfig(c *gc.C) { cfg = s.getConfig() cfg.Clock = nil c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) + + cfg = s.getConfig() + cfg.NewSocketListener = nil + c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) + + cfg = s.getConfig() + cfg.SocketName = "" + c.Check(cfg.Validate(), jc.ErrorIs, errors.NotValid) } func (s *manifoldSuite) getConfig() ManifoldConfig { return ManifoldConfig{ - Logger: s.logger, - Clock: clock.WallClock, + Logger: s.logger, + Clock: clock.WallClock, + NewSocketListener: NewSocketListener, + SocketName: "test.socket", } } diff --git a/internal/worker/controlleragentconfig/worker.go b/internal/worker/controlleragentconfig/worker.go index bf5bdb54ee0..a901bacd201 100644 --- a/internal/worker/controlleragentconfig/worker.go +++ b/internal/worker/controlleragentconfig/worker.go @@ -1,19 +1,25 @@ -// Copyright 2023 Canonical Ltd. +// Copyright 2024 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. +// Package controlleragentconfig provides a worker that listens on the "/reload" +// endpoint of the config change socket and restarts any workers that have +// requested to watch the config. package controlleragentconfig import ( - "context" "fmt" - "os" + "net/http" "sync/atomic" + "time" + "github.com/gorilla/mux" "github.com/juju/clock" "github.com/juju/errors" "github.com/juju/worker/v4" "github.com/juju/worker/v4/catacomb" "gopkg.in/tomb.v2" + + "github.com/juju/juju/internal/socketlistener" ) const ( @@ -26,8 +32,12 @@ const ( // agent controller config worker. type WorkerConfig struct { Logger Logger - Notify func(context.Context, chan os.Signal) - Clock clock.Clock + // Clock is needed for worker.NewRunner. + Clock clock.Clock + // SocketName is the socket file descriptor. + SocketName string + // NewSocketListener is the function that creates a new socket listener. + NewSocketListener func(socketlistener.Config) (SocketListener, error) } // Validate ensures that the config values are valid. @@ -35,12 +45,15 @@ func (c *WorkerConfig) Validate() error { if c.Logger == nil { return errors.NotValidf("nil Logger") } - if c.Notify == nil { - return errors.NotValidf("nil Notify") - } if c.Clock == nil { return errors.NotValidf("nil Clock") } + if c.SocketName == "" { + return errors.NotValidf("empty SocketName") + } + if c.NewSocketListener == nil { + return errors.NotValidf("nil NewSocketListener func") + } return nil } @@ -61,14 +74,15 @@ type ConfigWatcher interface { } type configWorker struct { - internalStates chan string - cfg WorkerConfig - catacomb catacomb.Catacomb - runner *worker.Runner - unique int64 + internalStates chan string + catacomb catacomb.Catacomb + runner *worker.Runner + cfg WorkerConfig + reloadRequested chan struct{} + unique int64 } -// NewWorker creates a new tracer worker. +// NewWorker creates a new config worker. func NewWorker(cfg WorkerConfig) (*configWorker, error) { return newWorker(cfg, nil) } @@ -80,8 +94,9 @@ func newWorker(cfg WorkerConfig, internalStates chan string) (*configWorker, err } w := &configWorker{ - internalStates: internalStates, - cfg: cfg, + internalStates: internalStates, + cfg: cfg, + reloadRequested: make(chan struct{}), runner: worker.NewRunner(worker.RunnerParams{ Clock: cfg.Clock, IsFatal: func(err error) bool { @@ -94,11 +109,22 @@ func newWorker(cfg WorkerConfig, internalStates chan string) (*configWorker, err }), } + sl, err := cfg.NewSocketListener(socketlistener.Config{ + Logger: cfg.Logger, + SocketName: cfg.SocketName, + RegisterHandlers: w.registerHandlers, + ShutdownTimeout: 500 * time.Millisecond, + }) + if err != nil { + return nil, errors.Annotate(err, "controller agent config reload socket listener setup:") + } + if err := catacomb.Invoke(catacomb.Plan{ Site: &w.catacomb, Work: w.loop, Init: []worker.Worker{ w.runner, + sl, }, }); err != nil { return nil, errors.Trace(err) @@ -107,6 +133,21 @@ func newWorker(cfg WorkerConfig, internalStates chan string) (*configWorker, err return w, nil } +func (w *configWorker) registerHandlers(r *mux.Router) { + r.HandleFunc("/reload", w.reloadHandler). + Methods(http.MethodGet) +} + +// reloadHandler sends a signal to the configWorker when a config reload is +// requested. +func (w *configWorker) reloadHandler(resp http.ResponseWriter, req *http.Request) { + select { + case <-w.catacomb.Dying(): + case <-req.Context().Done(): + case w.reloadRequested <- struct{}{}: + } +} + // Kill is part of the worker.Worker interface. func (w *configWorker) Kill() { w.catacomb.Kill(nil) @@ -135,11 +176,9 @@ func (w *configWorker) Watcher() (ConfigWatcher, error) { return watcher.(ConfigWatcher), nil } +// loop listens for a reload request picked up by the socket listener and +// restarts all subscribed workers watching the config. func (w *configWorker) loop() error { - // We must use a buffered channel or risk missing the signal - // if we're not ready to receive when the signal is sent. - ch := make(chan os.Signal, 1) - w.cfg.Notify(w.catacomb.Context(context.Background()), ch) // Report the initial started state. w.reportInternalState(stateStarted) @@ -148,15 +187,21 @@ func (w *configWorker) loop() error { select { case <-w.catacomb.Dying(): return w.catacomb.Err() - case <-ch: + case <-w.reloadRequested: w.reportInternalState(stateReload) - w.cfg.Logger.Infof("SIGHUP received, reloading config") + // Empty channel of extra reload requests. + for len(w.reloadRequested) > 0 { + <-w.reloadRequested + } + + w.cfg.Logger.Infof("reload config request received, reloading config") for _, name := range w.runner.WorkerNames() { runnerWorker, err := w.runner.Worker(name, w.catacomb.Dying()) if err != nil { if errors.Is(err, errors.NotFound) { + w.cfg.Logger.Debugf("worker %q not found, skipping", name) continue } diff --git a/internal/worker/controlleragentconfig/worker_test.go b/internal/worker/controlleragentconfig/worker_test.go index de7f100a877..00633664ca0 100644 --- a/internal/worker/controlleragentconfig/worker_test.go +++ b/internal/worker/controlleragentconfig/worker_test.go @@ -1,23 +1,24 @@ -// Copyright 2023 Canonical Ltd. +// Copyright 2024 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. package controlleragentconfig import ( "context" - "os" + "net" + "net/http" + "path" "sync" "sync/atomic" "syscall" "time" "github.com/juju/clock" + coretesting "github.com/juju/testing" jc "github.com/juju/testing/checkers" "github.com/juju/worker/v4/workertest" "go.uber.org/mock/gomock" gc "gopkg.in/check.v1" - - "github.com/juju/juju/testing" ) type workerSuite struct { @@ -37,40 +38,53 @@ func (s *workerSuite) TestStartup(c *gc.C) { workertest.CleanKill(c, w) } -func (s *workerSuite) TestSighup(c *gc.C) { +func (s *workerSuite) TestReloadRequest(c *gc.C) { defer s.setupMocks(c).Finish() - w, notify, states := s.newWorker(c) + w, socket, states := s.newWorker(c) defer workertest.DirtyKill(c, w) s.ensureStartup(c, states) - s.sendSignal(c, notify) + s.requestReload(c, socket) s.ensureReload(c, states) workertest.CleanKill(c, w) } -func (s *workerSuite) TestSighupMultipleTimes(c *gc.C) { +func (s *workerSuite) TestIncorrectEndpoint(c *gc.C) { defer s.setupMocks(c).Finish() - w, notify, states := s.newWorker(c) + w, socket, states := s.newWorker(c) + defer workertest.DirtyKill(c, w) + + s.ensureStartup(c, states) + + s.ensureEndpointNotFound(c, socket, "/relod") + + workertest.CleanKill(c, w) +} + +func (s *workerSuite) TestReloadRequestMultipleTimes(c *gc.C) { + defer s.setupMocks(c).Finish() + + w, socket, states := s.newWorker(c) defer workertest.DirtyKill(c, w) s.ensureStartup(c, states) for i := 0; i < 10; i++ { - s.sendSignal(c, notify) + s.requestReload(c, socket) s.ensureReload(c, states) } workertest.CleanKill(c, w) } -func (s *workerSuite) TestSighupAfterDeath(c *gc.C) { +func (s *workerSuite) TestReloadRequestAfterDeath(c *gc.C) { defer s.setupMocks(c).Finish() - w, notify, states := s.newWorker(c) + w, socket, states := s.newWorker(c) defer workertest.DirtyKill(c, w) s.ensureStartup(c, states) @@ -78,12 +92,12 @@ func (s *workerSuite) TestSighupAfterDeath(c *gc.C) { workertest.CleanKill(c, w) // We should not receive a reload signal after the worker has died. - s.sendSignal(c, notify) + s.ensureReloadRequestRefused(c, socket) select { case state := <-states: c.Fatalf("should not have received state %q", state) - case <-time.After(testing.ShortWait * 10): + case <-time.After(coretesting.ShortWait * 10): } } @@ -103,14 +117,14 @@ func (s *workerSuite) TestWatchWithNoChange(c *gc.C) { select { case <-changes: c.Fatal("should not have received a change") - case <-time.After(testing.ShortWait * 10): + case <-time.After(coretesting.ShortWait * 10): } } func (s *workerSuite) TestWatchWithSubscribe(c *gc.C) { defer s.setupMocks(c).Finish() - w, notify, states := s.newWorker(c) + w, socket, states := s.newWorker(c) defer workertest.DirtyKill(c, w) s.ensureStartup(c, states) @@ -119,7 +133,7 @@ func (s *workerSuite) TestWatchWithSubscribe(c *gc.C) { c.Assert(err, jc.ErrorIsNil) defer watcher.Unsubscribe() - s.sendSignal(c, notify) + s.requestReload(c, socket) s.ensureReload(c, states) changes := watcher.Changes() @@ -128,7 +142,7 @@ func (s *workerSuite) TestWatchWithSubscribe(c *gc.C) { select { case <-changes: count++ - case <-time.After(testing.ShortWait): + case <-time.After(coretesting.ShortWait): c.Fatal("should have received a change") } @@ -137,7 +151,7 @@ func (s *workerSuite) TestWatchWithSubscribe(c *gc.C) { select { case <-watcher.Done(): c.Fatalf("should not have received a done signal") - case <-time.After(testing.ShortWait): + case <-time.After(coretesting.ShortWait): } workertest.CleanKill(c, w) @@ -148,7 +162,7 @@ func (s *workerSuite) TestWatchWithSubscribe(c *gc.C) { func (s *workerSuite) TestWatchAfterUnsubscribe(c *gc.C) { defer s.setupMocks(c).Finish() - w, notify, states := s.newWorker(c) + w, socket, states := s.newWorker(c) defer workertest.DirtyKill(c, w) s.ensureStartup(c, states) @@ -157,7 +171,7 @@ func (s *workerSuite) TestWatchAfterUnsubscribe(c *gc.C) { c.Assert(err, jc.ErrorIsNil) defer watcher.Unsubscribe() - s.sendSignal(c, notify) + s.requestReload(c, socket) s.ensureReload(c, states) watcher.Unsubscribe() @@ -168,7 +182,7 @@ func (s *workerSuite) TestWatchAfterUnsubscribe(c *gc.C) { select { case _, ok := <-changes: c.Assert(ok, jc.IsFalse) - case <-time.After(testing.ShortWait * 10): + case <-time.After(coretesting.ShortWait * 10): } ensureDone(c, watcher) @@ -193,7 +207,7 @@ func (s *workerSuite) TestWatchWithKilledWorker(c *gc.C) { select { case _, ok := <-changes: c.Assert(ok, jc.IsFalse) - case <-time.After(testing.ShortWait * 10): + case <-time.After(coretesting.ShortWait * 10): } ensureDone(c, watcher) @@ -202,7 +216,7 @@ func (s *workerSuite) TestWatchWithKilledWorker(c *gc.C) { func (s *workerSuite) TestWatchMultiple(c *gc.C) { defer s.setupMocks(c).Finish() - w, notify, states := s.newWorker(c) + w, socket, states := s.newWorker(c) defer workertest.CleanKill(c, w) s.ensureStartup(c, states) @@ -215,7 +229,7 @@ func (s *workerSuite) TestWatchMultiple(c *gc.C) { watchers[i] = watcher } - s.sendSignal(c, notify) + s.requestReload(c, socket) s.ensureReload(c, states) var wg sync.WaitGroup @@ -231,7 +245,7 @@ func (s *workerSuite) TestWatchMultiple(c *gc.C) { case _, ok := <-changes: atomic.AddInt64(&count, 1) c.Assert(ok, jc.IsTrue) - case <-time.After(testing.ShortWait * 10): + case <-time.After(coretesting.ShortWait * 10): c.Fatal("should have received a change") } }(watchers[i]) @@ -245,7 +259,7 @@ func (s *workerSuite) TestWatchMultiple(c *gc.C) { select { case <-done: - case <-time.After(testing.LongWait): + case <-time.After(coretesting.LongWait): c.Fatal("timed out waiting for changes to finish") } @@ -255,7 +269,7 @@ func (s *workerSuite) TestWatchMultiple(c *gc.C) { func (s *workerSuite) TestWatchMultipleWithUnsubscribe(c *gc.C) { defer s.setupMocks(c).Finish() - w, notify, states := s.newWorker(c) + w, socket, states := s.newWorker(c) defer workertest.CleanKill(c, w) s.ensureStartup(c, states) @@ -267,7 +281,7 @@ func (s *workerSuite) TestWatchMultipleWithUnsubscribe(c *gc.C) { watchers[i] = watcher } - s.sendSignal(c, notify) + s.requestReload(c, socket) s.ensureReload(c, states) var wg sync.WaitGroup @@ -293,7 +307,7 @@ func (s *workerSuite) TestWatchMultipleWithUnsubscribe(c *gc.C) { case _, ok := <-changes: atomic.AddInt64(&count, 1) c.Assert(ok, jc.IsTrue) - case <-time.After(testing.ShortWait * 10): + case <-time.After(coretesting.ShortWait * 10): c.Fatal("should have received a change") } }(i, watchers[i]) @@ -307,7 +321,7 @@ func (s *workerSuite) TestWatchMultipleWithUnsubscribe(c *gc.C) { select { case <-done: - case <-time.After(testing.LongWait): + case <-time.After(coretesting.LongWait): c.Fatal("timed out waiting for changes to finish") } @@ -319,41 +333,29 @@ func (s *workerSuite) setupMocks(c *gc.C) *gomock.Controller { return ctrl } -func (s *workerSuite) newWorker(c *gc.C) (*configWorker, chan struct{}, chan string) { +func (s *workerSuite) newWorker(c *gc.C) (*configWorker, string, chan string) { // Buffer the channel, so we don't drop signals if we're not ready. states := make(chan string, 10) - // Buffer the channel, so we don't miss signals if we're not ready. - notify := make(chan struct{}, 1) + + // Create a unix socket for testing. + tmpDir := c.MkDir() + socket := path.Join(tmpDir, "test.socket") + w, err := newWorker(WorkerConfig{ - Logger: s.logger, - Clock: clock.WallClock, - Notify: func(ctx context.Context, ch chan os.Signal) { - go func() { - for { - select { - case <-notify: - select { - case ch <- syscall.SIGHUP: - case <-ctx.Done(): - return - } - - case <-ctx.Done(): - return - } - } - }() - }, + Logger: s.logger, + Clock: clock.WallClock, + SocketName: socket, + NewSocketListener: NewSocketListener, }, states) c.Assert(err, jc.ErrorIsNil) - return w, notify, states + return w, socket, states } func (s *workerSuite) ensureStartup(c *gc.C, states chan string) { select { case state := <-states: c.Assert(state, gc.Equals, stateStarted) - case <-time.After(testing.ShortWait * 10): + case <-time.After(coretesting.ShortWait * 10): c.Fatalf("timed out waiting for startup") } } @@ -362,23 +364,56 @@ func (s *workerSuite) ensureReload(c *gc.C, states chan string) { select { case state := <-states: c.Assert(state, gc.Equals, stateReload) - case <-time.After(testing.ShortWait * 100): + case <-time.After(coretesting.ShortWait * 100): c.Fatalf("timed out waiting for reload") } } -func (s *workerSuite) sendSignal(c *gc.C, notify chan struct{}) { - select { - case notify <- struct{}{}: - case <-time.After(testing.ShortWait * 10): - c.Fatalf("timed out sending signal") - } +func (s *workerSuite) requestReload(c *gc.C, socket string) { + resp, err := newRequest(c, socket, "/reload") + c.Assert(err, jc.ErrorIsNil) + c.Assert(resp.StatusCode, gc.Equals, http.StatusOK) +} + +func (s *workerSuite) ensureReloadRequestRefused(c *gc.C, socket string) { + _, err := newRequest(c, socket, "/reload") + c.Assert(err, jc.ErrorIs, syscall.ECONNREFUSED) +} + +func (s *workerSuite) ensureEndpointNotFound(c *gc.C, socket, method string) { + resp, err := newRequest(c, socket, method) + c.Assert(err, jc.ErrorIsNil) + c.Assert(resp.StatusCode, gc.Equals, http.StatusNotFound) } func ensureDone(c *gc.C, watcher ConfigWatcher) { select { case <-watcher.Done(): - case <-time.After(testing.ShortWait): + case <-time.After(coretesting.ShortWait): c.Fatal("should have received a done signal") } } + +func newRequest(c *gc.C, socket, method string) (*http.Response, error) { + serverURL := "http://localhost:8080" + method + req, err := http.NewRequest( + http.MethodGet, + serverURL, + nil, + ) + c.Assert(err, jc.ErrorIsNil) + + return client(socket).Do(req) +} + +// Return an *http.Client with custom transport that allows it to connect to +// the given Unix socket. +func client(socketPath string) *http.Client { + return &http.Client{ + Transport: &http.Transport{ + DialContext: func(_ context.Context, _, _ string) (conn net.Conn, err error) { + return net.Dial("unix", socketPath) + }, + }, + } +} diff --git a/internal/worker/controlsocket/worker.go b/internal/worker/controlsocket/worker.go index e680b77d837..a7c08458144 100644 --- a/internal/worker/controlsocket/worker.go +++ b/internal/worker/controlsocket/worker.go @@ -48,9 +48,11 @@ type Logger interface { // Config represents configuration for the controlsocket worker. type Config struct { - State State - Logger Logger - SocketName string + State State + Logger Logger + // SocketName is the socket file descriptor. + SocketName string + // NewSocketListener is the function that creates a new socket listener. NewSocketListener func(socketlistener.Config) (SocketListener, error) } diff --git a/internal/worker/controlsocket/worker_test.go b/internal/worker/controlsocket/worker_test.go index d7b781726c0..a9a698013d0 100644 --- a/internal/worker/controlsocket/worker_test.go +++ b/internal/worker/controlsocket/worker_test.go @@ -16,13 +16,12 @@ import ( "github.com/juju/errors" "github.com/juju/loggo/v2" "github.com/juju/names/v5" + jc "github.com/juju/testing/checkers" + gc "gopkg.in/check.v1" "github.com/juju/juju/core/permission" "github.com/juju/juju/state" stateerrors "github.com/juju/juju/state/errors" - - jc "github.com/juju/testing/checkers" - gc "gopkg.in/check.v1" ) type workerSuite struct { @@ -447,33 +446,3 @@ func client(socketPath string) *http.Client { }, } } - -type fakeLogger struct { - entries []logEntry -} - -type logEntry struct{ level, msg string } - -func (f *fakeLogger) write(level string, format string, args ...any) { - f.entries = append(f.entries, logEntry{level, fmt.Sprintf(format, args...)}) -} - -func (f *fakeLogger) Errorf(format string, args ...any) { - f.write("ERROR", format, args...) -} - -func (f *fakeLogger) Warningf(format string, args ...any) { - f.write("WARNING", format, args...) -} - -func (f *fakeLogger) Infof(format string, args ...any) { - f.write("INFO", format, args...) -} - -func (f *fakeLogger) Debugf(format string, args ...any) { - f.write("DEBUG", format, args...) -} - -func (f *fakeLogger) Tracef(format string, args ...any) { - f.write("TRACE", format, args...) -} From 84300565f5ae3b36ccfff132105112705de36c0a Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Fri, 23 Feb 2024 10:58:49 +0000 Subject: [PATCH 3/5] SQUASH Add status headers to responses --- internal/worker/controlleragentconfig/worker.go | 3 +++ internal/worker/controlleragentconfig/worker_test.go | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/worker/controlleragentconfig/worker.go b/internal/worker/controlleragentconfig/worker.go index a901bacd201..03d8f4f5c00 100644 --- a/internal/worker/controlleragentconfig/worker.go +++ b/internal/worker/controlleragentconfig/worker.go @@ -143,8 +143,11 @@ func (w *configWorker) registerHandlers(r *mux.Router) { func (w *configWorker) reloadHandler(resp http.ResponseWriter, req *http.Request) { select { case <-w.catacomb.Dying(): + resp.WriteHeader(http.StatusInternalServerError) case <-req.Context().Done(): + resp.WriteHeader(http.StatusInternalServerError) case w.reloadRequested <- struct{}{}: + resp.WriteHeader(http.StatusNoContent) } } diff --git a/internal/worker/controlleragentconfig/worker_test.go b/internal/worker/controlleragentconfig/worker_test.go index 00633664ca0..c71a56ebb98 100644 --- a/internal/worker/controlleragentconfig/worker_test.go +++ b/internal/worker/controlleragentconfig/worker_test.go @@ -372,7 +372,7 @@ func (s *workerSuite) ensureReload(c *gc.C, states chan string) { func (s *workerSuite) requestReload(c *gc.C, socket string) { resp, err := newRequest(c, socket, "/reload") c.Assert(err, jc.ErrorIsNil) - c.Assert(resp.StatusCode, gc.Equals, http.StatusOK) + c.Assert(resp.StatusCode, gc.Equals, http.StatusNoContent) } func (s *workerSuite) ensureReloadRequestRefused(c *gc.C, socket string) { From 138f3b76b28547528494a3a62bf9e1c230c039d4 Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Fri, 23 Feb 2024 10:59:15 +0000 Subject: [PATCH 4/5] SQUASH Stop ignoring extra reload requests --- internal/worker/controlleragentconfig/worker.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/internal/worker/controlleragentconfig/worker.go b/internal/worker/controlleragentconfig/worker.go index 03d8f4f5c00..c10afff2120 100644 --- a/internal/worker/controlleragentconfig/worker.go +++ b/internal/worker/controlleragentconfig/worker.go @@ -193,11 +193,6 @@ func (w *configWorker) loop() error { case <-w.reloadRequested: w.reportInternalState(stateReload) - // Empty channel of extra reload requests. - for len(w.reloadRequested) > 0 { - <-w.reloadRequested - } - w.cfg.Logger.Infof("reload config request received, reloading config") for _, name := range w.runner.WorkerNames() { From a64887e005d4e6debdcab45b8d589d462c9b8335 Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Thu, 29 Feb 2024 08:04:10 +0000 Subject: [PATCH 5/5] SQUASH Switch to POST and address other review issues --- cmd/jujud-controller/agent/machine/manifolds.go | 2 +- cmd/jujud/agent/machine/manifolds.go | 2 +- internal/socketlistener/socketlistener_test.go | 7 ++++--- internal/worker/controlleragentconfig/manifold.go | 2 +- internal/worker/controlleragentconfig/manifold_test.go | 2 +- internal/worker/controlleragentconfig/worker.go | 4 ++-- internal/worker/controlleragentconfig/worker_test.go | 4 ++-- internal/worker/controlsocket/manifold.go | 2 +- internal/worker/controlsocket/worker.go | 4 ++-- internal/worker/controlsocket/worker_test.go | 2 +- 10 files changed, 16 insertions(+), 15 deletions(-) diff --git a/cmd/jujud-controller/agent/machine/manifolds.go b/cmd/jujud-controller/agent/machine/manifolds.go index eb3563050a0..efe1a923492 100644 --- a/cmd/jujud-controller/agent/machine/manifolds.go +++ b/cmd/jujud-controller/agent/machine/manifolds.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical Ltd. +// Copyright 2015 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. package machine diff --git a/cmd/jujud/agent/machine/manifolds.go b/cmd/jujud/agent/machine/manifolds.go index 8b443d1319d..b0c5f25c464 100644 --- a/cmd/jujud/agent/machine/manifolds.go +++ b/cmd/jujud/agent/machine/manifolds.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical Ltd. +// Copyright 2015 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. package machine diff --git a/internal/socketlistener/socketlistener_test.go b/internal/socketlistener/socketlistener_test.go index 56b3ba0c3db..ea418e9983b 100644 --- a/internal/socketlistener/socketlistener_test.go +++ b/internal/socketlistener/socketlistener_test.go @@ -1,7 +1,7 @@ // Copyright 2024 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. -package socketlistener +package socketlistener_test import ( "context" @@ -19,6 +19,7 @@ import ( gc "gopkg.in/check.v1" coretesting "github.com/juju/juju/core/testing" + "github.com/juju/juju/internal/socketlistener" ) type socketListenerSuite struct { @@ -44,7 +45,7 @@ func (s *socketListenerSuite) TestStartStopWorker(c *gc.C) { tmpDir := c.MkDir() socket := path.Join(tmpDir, "test.socket") - sl, err := NewSocketListener(Config{ + sl, err := socketlistener.NewSocketListener(socketlistener.Config{ Logger: s.logger, SocketName: socket, RegisterHandlers: registerTestHandlers, @@ -92,7 +93,7 @@ func (s *socketListenerSuite) TestEnsureShutdown(c *gc.C) { socket := path.Join(tmpDir, "test.socket") start := make(chan struct{}) - sl, err := NewSocketListener(Config{ + sl, err := socketlistener.NewSocketListener(socketlistener.Config{ Logger: s.logger, SocketName: socket, RegisterHandlers: func(r *mux.Router) { diff --git a/internal/worker/controlleragentconfig/manifold.go b/internal/worker/controlleragentconfig/manifold.go index 8bd6c30cc46..077d16b7936 100644 --- a/internal/worker/controlleragentconfig/manifold.go +++ b/internal/worker/controlleragentconfig/manifold.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical Ltd. +// Copyright 2023 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. package controlleragentconfig diff --git a/internal/worker/controlleragentconfig/manifold_test.go b/internal/worker/controlleragentconfig/manifold_test.go index 9fc2722387a..5b2f023af17 100644 --- a/internal/worker/controlleragentconfig/manifold_test.go +++ b/internal/worker/controlleragentconfig/manifold_test.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical Ltd. +// Copyright 2023 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. package controlleragentconfig diff --git a/internal/worker/controlleragentconfig/worker.go b/internal/worker/controlleragentconfig/worker.go index c10afff2120..fbf3618770c 100644 --- a/internal/worker/controlleragentconfig/worker.go +++ b/internal/worker/controlleragentconfig/worker.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical Ltd. +// Copyright 2023 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. // Package controlleragentconfig provides a worker that listens on the "/reload" @@ -135,7 +135,7 @@ func newWorker(cfg WorkerConfig, internalStates chan string) (*configWorker, err func (w *configWorker) registerHandlers(r *mux.Router) { r.HandleFunc("/reload", w.reloadHandler). - Methods(http.MethodGet) + Methods(http.MethodPost) } // reloadHandler sends a signal to the configWorker when a config reload is diff --git a/internal/worker/controlleragentconfig/worker_test.go b/internal/worker/controlleragentconfig/worker_test.go index c71a56ebb98..2dceeda9e6a 100644 --- a/internal/worker/controlleragentconfig/worker_test.go +++ b/internal/worker/controlleragentconfig/worker_test.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical Ltd. +// Copyright 2023 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. package controlleragentconfig @@ -397,7 +397,7 @@ func ensureDone(c *gc.C, watcher ConfigWatcher) { func newRequest(c *gc.C, socket, method string) (*http.Response, error) { serverURL := "http://localhost:8080" + method req, err := http.NewRequest( - http.MethodGet, + http.MethodPost, serverURL, nil, ) diff --git a/internal/worker/controlsocket/manifold.go b/internal/worker/controlsocket/manifold.go index 362b82384c2..15831cc7d99 100644 --- a/internal/worker/controlsocket/manifold.go +++ b/internal/worker/controlsocket/manifold.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical Ltd. +// Copyright 2023 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. package controlsocket diff --git a/internal/worker/controlsocket/worker.go b/internal/worker/controlsocket/worker.go index a7c08458144..f4a97bde4fa 100644 --- a/internal/worker/controlsocket/worker.go +++ b/internal/worker/controlsocket/worker.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical Ltd. +// Copyright 2023 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. //go:build linux @@ -210,7 +210,7 @@ func (w *Worker) addMetricsUser(username, password string) (int, error) { }() default: - return http.StatusInternalServerError, errors.Annotatef(err, "failed to create user %q: %v", username, err) + return http.StatusInternalServerError, errors.Annotatef(err, "creating user %q: %v", username, err) } // Give the new user permission to access the metrics endpoint. diff --git a/internal/worker/controlsocket/worker_test.go b/internal/worker/controlsocket/worker_test.go index a9a698013d0..52fec59c09a 100644 --- a/internal/worker/controlsocket/worker_test.go +++ b/internal/worker/controlsocket/worker_test.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical Ltd. +// Copyright 2023 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. package controlsocket