Merge pull request juju#18901 from jack-w-shaw/re-enable_set_get_stat…

…us_tests

juju#18901

A number of tests in the apiserver/common package dealing with setting and getting the statuses of entities were being skipped, because they were based on the legacy state factories and thus were failing with the transition to DQLite.

Re-write these tests with mocks, and re-enable them. This included some subtle additions to the production code itself to make it more testable, such as injecting a clock instead of depending on time.Now()

I haven't bothered dealing with ApplicationStatusSetter, because that is being deleted and re-written in a service imminently.

NOTE: I also discovered we were depending on a generated mock file, which did not have a corresponding `go generate` line. This orphan mock was deleted, and tests moved over to the proper generated mock. In the future, more work will be required to move the mocks out of their own dedicated package.

## QA steps

unit tests pass

apiserver/common/applicationsetstatus.go

@@ -0,0 +1,116 @@
+// Copyright 2025 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package common
+
+import (
+	"context"
+	"time"
+
+	"github.com/juju/names/v6"
+
+	apiservererrors "github.com/juju/juju/apiserver/errors"
+	"github.com/juju/juju/core/leadership"
+	"github.com/juju/juju/core/status"
+	"github.com/juju/juju/rpc/params"
+	"github.com/juju/juju/state"
+)
+
+// ApplicationStatusSetter implements a SetApplicationStatus method to be
+// used by facades that can change an application status.
+// This is only slightly less evil than ApplicationStatusGetter. We have
+// StatusSetter already; all this does is set the status for the wrong
+// entity, and render the auth so confused as to be ~worthless.
+type ApplicationStatusSetter struct {
+	leadershipChecker leadership.Checker
+	st                *state.State
+	getCanModify      GetAuthFunc
+}
+
+// NewApplicationStatusSetter returns a ServiceStatusSetter.
+func NewApplicationStatusSetter(st *state.State, getCanModify GetAuthFunc, leadershipChecker leadership.Checker) *ApplicationStatusSetter {
+	return &ApplicationStatusSetter{
+		leadershipChecker: leadershipChecker,
+		st:                st,
+		getCanModify:      getCanModify,
+	}
+}
+
+// SetStatus sets the status on the service given by the unit in args if the unit is the leader.
+func (s *ApplicationStatusSetter) SetStatus(ctx context.Context, args params.SetStatus) (params.ErrorResults, error) {
+	result := params.ErrorResults{
+		Results: make([]params.ErrorResult, len(args.Entities)),
+	}
+	if len(args.Entities) == 0 {
+		return result, nil
+	}
+
+	canModify, err := s.getCanModify()
+	if err != nil {
+		return params.ErrorResults{}, err
+	}
+
+	for i, arg := range args.Entities {
+
+		// TODO(fwereade): the auth is basically nonsense, and basically only
+		// works by coincidence. Read carefully.
+
+		// We "know" that arg.Tag is either the calling unit or its service
+		// (because getCanModify is authUnitOrService, and we'll fail out if
+		// it isn't); and, in practice, it's always going to be the calling
+		// unit (because, /sigh, we don't actually use service tags to refer
+		// to services in this method).
+		tag, err := names.ParseTag(arg.Tag)
+		if err != nil {
+			result.Results[i].Error = apiservererrors.ServerError(err)
+			continue
+		}
+		if !canModify(tag) {
+			result.Results[i].Error = apiservererrors.ServerError(apiservererrors.ErrPerm)
+			continue
+		}
+		unitTag, ok := tag.(names.UnitTag)
+		if !ok {
+			// No matter what the canModify says, if this entity is not
+			// a unit, we say "NO".
+			result.Results[i].Error = apiservererrors.ServerError(apiservererrors.ErrPerm)
+			continue
+		}
+		unitId := unitTag.Id()
+
+		// Now we have the unit, we can get the service that should have been
+		// specified in the first place...
+		serviceId, err := names.UnitApplication(unitId)
+		if err != nil {
+			result.Results[i].Error = apiservererrors.ServerError(err)
+			continue
+		}
+		service, err := s.st.Application(serviceId)
+		if err != nil {
+			result.Results[i].Error = apiservererrors.ServerError(err)
+			continue
+		}
+
+		// ...and set the status, conditional on the unit being (and remaining)
+		// service leader.
+		token := s.leadershipChecker.LeadershipCheck(serviceId, unitId)
+
+		if err := token.Check(); err != nil {
+			result.Results[i].Error = apiservererrors.ServerError(err)
+			continue
+		}
+		// TODO(perrito666) 2016-05-02 lp:1558657
+		now := time.Now()
+		sInfo := status.StatusInfo{
+			Status:  status.Status(arg.Status),
+			Message: arg.Info,
+			Data:    arg.Data,
+			Since:   &now,
+		}
+		if err := service.SetStatus(sInfo); err != nil {
+			result.Results[i].Error = apiservererrors.ServerError(err)
+		}
+
+	}
+	return result, nil
+}

apiserver/common/applicationsetstatus_test.go

@@ -0,0 +1,180 @@
+// Copyright 2025 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package common_test
+
+import (
+	"context"
+
+	"github.com/juju/errors"
+	"github.com/juju/names/v6"
+	jc "github.com/juju/testing/checkers"
+	gc "gopkg.in/check.v1"
+
+	"github.com/juju/juju/apiserver/common"
+	"github.com/juju/juju/core/leadership"
+	"github.com/juju/juju/core/status"
+	"github.com/juju/juju/internal/testing/factory"
+	"github.com/juju/juju/rpc/params"
+	"github.com/juju/juju/state/testing"
+)
+
+type serviceStatusSetterSuite struct {
+	testing.StateSuite
+	leadershipChecker *fakeLeadershipChecker
+	badTag            names.Tag
+	setter            *common.ApplicationStatusSetter
+}
+
+var _ = gc.Suite(&serviceStatusSetterSuite{})
+
+func (s *serviceStatusSetterSuite) SetUpTest(c *gc.C) {
+	c.Skip("skipping factory based tests. TODO: Re-write without factories")
+	s.badTag = nil
+	s.leadershipChecker = &fakeLeadershipChecker{isLeader: true}
+}
+
+func (s *serviceStatusSetterSuite) TestUnauthorized(c *gc.C) {
+	// Machines are unauthorized since they are not units
+	tag := names.NewUnitTag("foo/0")
+	s.badTag = tag
+	result, err := s.setter.SetStatus(context.Background(), params.SetStatus{Entities: []params.EntityStatusArgs{{
+		Tag:    tag.String(),
+		Status: status.Active.String(),
+	}}})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(result.Results, gc.HasLen, 1)
+	c.Assert(result.Results[0].Error, jc.Satisfies, params.IsCodeUnauthorized)
+}
+
+func (s *serviceStatusSetterSuite) TestNotATag(c *gc.C) {
+	result, err := s.setter.SetStatus(context.Background(), params.SetStatus{Entities: []params.EntityStatusArgs{{
+		Tag:    "not a tag",
+		Status: status.Active.String(),
+	}}})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(result.Results, gc.HasLen, 1)
+	c.Assert(result.Results[0].Error, gc.ErrorMatches, `"not a tag" is not a valid tag`)
+}
+
+func (s *serviceStatusSetterSuite) TestNotFound(c *gc.C) {
+	result, err := s.setter.SetStatus(context.Background(), params.SetStatus{Entities: []params.EntityStatusArgs{{
+		Tag:    names.NewUnitTag("foo/0").String(),
+		Status: status.Active.String(),
+	}}})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(result.Results, gc.HasLen, 1)
+	c.Assert(result.Results[0].Error, jc.Satisfies, params.IsCodeNotFound)
+}
+
+func (s *serviceStatusSetterSuite) TestSetMachineStatus(c *gc.C) {
+	machine := s.Factory.MakeMachine(c, nil)
+	result, err := s.setter.SetStatus(context.Background(), params.SetStatus{Entities: []params.EntityStatusArgs{{
+		Tag:    machine.Tag().String(),
+		Status: status.Active.String(),
+	}}})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(result.Results, gc.HasLen, 1)
+	// Can't call set service status on a machine.
+	c.Assert(result.Results[0].Error, jc.Satisfies, params.IsCodeUnauthorized)
+}
+
+func (s *serviceStatusSetterSuite) TestSetServiceStatus(c *gc.C) {
+	// TODO: the correct way to fix this is to have the authorizer on the
+	// simple status setter to check to see if the unit (authTag) is a leader
+	// and able to set the service status. However, that is for another day.
+	service := s.Factory.MakeApplication(c, &factory.ApplicationParams{Status: &status.StatusInfo{
+		Status: status.Maintenance,
+	}})
+	result, err := s.setter.SetStatus(context.Background(), params.SetStatus{Entities: []params.EntityStatusArgs{{
+		Tag:    service.Tag().String(),
+		Status: status.Active.String(),
+	}}})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(result.Results, gc.HasLen, 1)
+	// Can't call set service status on a service. Weird I know, but the only
+	// way is to go through the unit leader.
+	c.Assert(result.Results[0].Error, jc.Satisfies, params.IsCodeUnauthorized)
+}
+
+func (s *serviceStatusSetterSuite) TestSetUnitStatusNotLeader(c *gc.C) {
+	// If the unit isn't the leader, it can't set it.
+	s.leadershipChecker.isLeader = false
+	unit := s.Factory.MakeUnit(c, &factory.UnitParams{Status: &status.StatusInfo{
+		Status: status.Maintenance,
+	}})
+	result, err := s.setter.SetStatus(context.Background(), params.SetStatus{Entities: []params.EntityStatusArgs{{
+		Tag:    unit.Tag().String(),
+		Status: status.Active.String(),
+	}}})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(result.Results, gc.HasLen, 1)
+	status := result.Results[0]
+	c.Assert(status.Error, gc.ErrorMatches, "not leader")
+}
+
+func (s *serviceStatusSetterSuite) TestSetUnitStatusIsLeader(c *gc.C) {
+	service := s.Factory.MakeApplication(c, &factory.ApplicationParams{Status: &status.StatusInfo{
+		Status: status.Maintenance,
+	}})
+	unit := s.Factory.MakeUnit(c, &factory.UnitParams{
+		Application: service,
+		Status: &status.StatusInfo{
+			Status: status.Maintenance,
+		}})
+	// No need to claim leadership - the checker passed in in setup
+	// always returns true.
+	result, err := s.setter.SetStatus(context.Background(), params.SetStatus{Entities: []params.EntityStatusArgs{{
+		Tag:    unit.Tag().String(),
+		Status: status.Active.String(),
+	}}})
+
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(result.Results, gc.HasLen, 1)
+	c.Assert(result.Results[0].Error, gc.IsNil)
+
+	err = service.Refresh()
+	c.Assert(err, jc.ErrorIsNil)
+	unitStatus, err := service.Status()
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(unitStatus.Status, gc.Equals, status.Active)
+}
+
+func (s *serviceStatusSetterSuite) TestBulk(c *gc.C) {
+	s.badTag = names.NewMachineTag("42")
+	machine := s.Factory.MakeMachine(c, nil)
+	result, err := s.setter.SetStatus(context.Background(), params.SetStatus{Entities: []params.EntityStatusArgs{{
+		Tag:    s.badTag.String(),
+		Status: status.Active.String(),
+	}, {
+		Tag:    machine.Tag().String(),
+		Status: status.Active.String(),
+	}, {
+		Tag:    "bad-tag",
+		Status: status.Active.String(),
+	}}})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(result.Results, gc.HasLen, 3)
+	c.Assert(result.Results[0].Error, jc.Satisfies, params.IsCodeUnauthorized)
+	c.Assert(result.Results[1].Error, jc.Satisfies, params.IsCodeUnauthorized)
+	c.Assert(result.Results[2].Error, gc.ErrorMatches, `"bad-tag" is not a valid tag`)
+}
+
+type fakeLeadershipChecker struct {
+	isLeader bool
+}
+
+type token struct {
+	isLeader bool
+}
+
+func (t *token) Check() error {
+	if !t.isLeader {
+		return errors.New("not leader")
+	}
+	return nil
+}
+
+func (f *fakeLeadershipChecker) LeadershipCheck(applicationName, unitName string) leadership.Token {
+	return &token{isLeader: f.isLeader}
+}

apiserver/common/ensuredead_test.go

@@ -13,6 +13,7 @@ import (
 	gc "gopkg.in/check.v1"
 
 	"github.com/juju/juju/apiserver/common"
+	"github.com/juju/juju/apiserver/common/mocks"
 	apiservertesting "github.com/juju/juju/apiserver/testing"
 	"github.com/juju/juju/rpc/params"
 	"github.com/juju/juju/state"
@@ -59,7 +60,7 @@ func (*deadEnsurerSuite) TestEnsureDead(c *gc.C) {
 
 	ctrl := gomock.NewController(c)
 	defer ctrl.Finish()
-	d := common.NewDeadEnsurer(st, getCanModify, NewMockEnsureDeadMachineService(ctrl))
+	d := common.NewDeadEnsurer(st, getCanModify, mocks.NewMockEnsureDeadMachineService(ctrl))
 	entities := params.Entities{[]params.Entity{
 		{"unit-x-0"}, {"unit-x-1"}, {"unit-x-2"}, {"unit-x-3"}, {"unit-x-4"}, {"unit-x-5"},
 	}}
@@ -83,7 +84,7 @@ func (*deadEnsurerSuite) TestEnsureDeadError(c *gc.C) {
 	}
 	ctrl := gomock.NewController(c)
 	defer ctrl.Finish()
-	d := common.NewDeadEnsurer(&fakeState{}, getCanModify, NewMockEnsureDeadMachineService(ctrl))
+	d := common.NewDeadEnsurer(&fakeState{}, getCanModify, mocks.NewMockEnsureDeadMachineService(ctrl))
 	_, err := d.EnsureDead(context.Background(), params.Entities{[]params.Entity{{"x0"}}})
 	c.Assert(err, gc.ErrorMatches, "pow")
 }
@@ -94,7 +95,7 @@ func (*deadEnsurerSuite) TestEnsureDeadNoArgsNoError(c *gc.C) {
 	}
 	ctrl := gomock.NewController(c)
 	defer ctrl.Finish()
-	d := common.NewDeadEnsurer(&fakeState{}, getCanModify, NewMockEnsureDeadMachineService(ctrl))
+	d := common.NewDeadEnsurer(&fakeState{}, getCanModify, mocks.NewMockEnsureDeadMachineService(ctrl))
 	result, err := d.EnsureDead(context.Background(), params.Entities{})
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(result.Results, gc.HasLen, 0)