From eed0b5cdb77cd10ff7f1fb7a79fc6a41f53d618e Mon Sep 17 00:00:00 2001 From: Nicolas Vinuesa Date: Fri, 29 Nov 2024 17:27:16 +0100 Subject: [PATCH] fix: model machine info checks controller model machines Before this patch, any calls to MachineModelInfo() (located in apiserver/common) would have actually be returning info from the controller model machines. This was clearly reproducible if you had a model with 2 machines (and a no-ha controller), then a call to `juju show-model` would return with an error stating that machine "1" wasn't found. Another way of reproducing was to migrate that model. The fix is to inject the correct MachineService on the modelmanager facade. --- .../client/modelmanager/mocks/service_mock.go | 38 +++++++++++++++++++ .../client/modelmanager/modelinfo_test.go | 20 ++++++++++ .../client/modelmanager/modelmanager.go | 2 +- .../client/modelmanager/modelmanager_test.go | 5 +++ .../facades/client/modelmanager/services.go | 7 ++++ 5 files changed, 71 insertions(+), 1 deletion(-) diff --git a/apiserver/facades/client/modelmanager/mocks/service_mock.go b/apiserver/facades/client/modelmanager/mocks/service_mock.go index 26291ba1ffe..fa5380ab591 100644 --- a/apiserver/facades/client/modelmanager/mocks/service_mock.go +++ b/apiserver/facades/client/modelmanager/mocks/service_mock.go @@ -1464,6 +1464,44 @@ func (c *MockModelDomainServicesConfigCall) DoAndReturn(f func() modelmanager.Mo return c } +// Machine mocks base method. +func (m *MockModelDomainServices) Machine() modelmanager.MachineService { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Machine") + ret0, _ := ret[0].(modelmanager.MachineService) + return ret0 +} + +// Machine indicates an expected call of Machine. +func (mr *MockModelDomainServicesMockRecorder) Machine() *MockModelDomainServicesMachineCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Machine", reflect.TypeOf((*MockModelDomainServices)(nil).Machine)) + return &MockModelDomainServicesMachineCall{Call: call} +} + +// MockModelDomainServicesMachineCall wrap *gomock.Call +type MockModelDomainServicesMachineCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockModelDomainServicesMachineCall) Return(arg0 modelmanager.MachineService) *MockModelDomainServicesMachineCall { + c.Call = c.Call.Return(arg0) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockModelDomainServicesMachineCall) Do(f func() modelmanager.MachineService) *MockModelDomainServicesMachineCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockModelDomainServicesMachineCall) DoAndReturn(f func() modelmanager.MachineService) *MockModelDomainServicesMachineCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + // ModelInfo mocks base method. func (m *MockModelDomainServices) ModelInfo() modelmanager.ModelInfoService { m.ctrl.T.Helper() diff --git a/apiserver/facades/client/modelmanager/modelinfo_test.go b/apiserver/facades/client/modelmanager/modelinfo_test.go index 66bc6af027b..3dc7e37b2d0 100644 --- a/apiserver/facades/client/modelmanager/modelinfo_test.go +++ b/apiserver/facades/client/modelmanager/modelinfo_test.go @@ -210,6 +210,8 @@ func (s *modelInfoSuite) getAPI(c *gc.C) (*modelmanager.ModelManagerAPI, *gomock modelInfoService := mocks.NewMockModelInfoService(ctrl) mockModelDomainServices.EXPECT().ModelInfo().Return(modelInfoService) + mockModelDomainServices.EXPECT().Machine().Return(s.mockMachineService).AnyTimes() + modelAgentService.EXPECT().GetModelTargetAgentVersion(gomock.Any()).Return(jujuversion.Current, nil) modelInfoService.EXPECT().GetModelInfo(gomock.Any()).Return(coremodel.ReadOnlyModel{ AgentVersion: version.MustParse("1.99.9"), @@ -435,6 +437,10 @@ func (s *modelInfoSuite) TestModelInfoWriteAccess(c *gc.C) { api, ctrl := s.getAPIWithUser(c, mary) defer ctrl.Finish() maryName := coreusertesting.GenNewName(c, "mary") + + s.mockMachineService = mocks.NewMockMachineService(ctrl) + s.mockModelDomainServices.EXPECT().Machine().Return(s.mockMachineService) + s.mockAccessService.EXPECT().ReadUserAccessLevelForTarget(gomock.Any(), user.NameFromTag(maryName), permission.ID{ ObjectType: permission.Model, Key: s.st.model.cfg.UUID(), @@ -569,6 +575,7 @@ func (s *modelInfoSuite) TestModelInfoErrorNoAccess(c *gc.C) { func (s *modelInfoSuite) TestRunningMigration(c *gc.C) { api, ctrl := s.getAPI(c) defer ctrl.Finish() + s.mockSecretBackendService.EXPECT().BackendSummaryInfoForModel(gomock.Any(), coremodel.UUID(s.st.model.cfg.UUID())).Return(nil, nil) s.mockModelService.EXPECT().GetModelUsers(gomock.Any(), coremodel.UUID(coretesting.ModelTag.Id())).Return(s.modelUserInfo, nil) start := time.Now().Add(-20 * time.Minute) @@ -625,6 +632,7 @@ func (s *modelInfoSuite) TestFailedMigration(c *gc.C) { func (s *modelInfoSuite) TestNoMigration(c *gc.C) { api, ctrl := s.getAPI(c) defer ctrl.Finish() + s.mockSecretBackendService.EXPECT().BackendSummaryInfoForModel(gomock.Any(), coremodel.UUID(s.st.model.cfg.UUID())).Return(nil, nil) s.mockModelService.EXPECT().GetModelUsers(gomock.Any(), coremodel.UUID(coretesting.ModelTag.Id())).Return(s.modelUserInfo, nil) s.mockMachineService.EXPECT().GetMachineUUID(gomock.Any(), machine.Name("1")).Return("deadbeef1", nil) @@ -734,6 +742,8 @@ func (s *modelInfoSuite) TestDeadModelWithGetModelInfoFailure(c *gc.C) { modelDomainServices.EXPECT().Agent().Return(modelAgentService) modelAgentService.EXPECT().GetModelTargetAgentVersion(gomock.Any()).Return(jujuversion.Current, nil) + modelDomainServices.EXPECT().Machine().Return(s.mockMachineService) + s.assertSuccess(c, api, s.st.model.cfg.UUID(), state.Dead, life.Dead) } @@ -758,6 +768,8 @@ func (s *modelInfoSuite) TestDeadModelWithGetModelTargetAgentVersionFailure(c *g modelDomainServices.EXPECT().Agent().Return(modelAgentService) modelAgentService.EXPECT().GetModelTargetAgentVersion(gomock.Any()).Return(version.Zero, errors.NotFoundf("model agent version")) + modelDomainServices.EXPECT().Machine().Return(s.mockMachineService) + s.assertSuccess(c, api, s.st.model.cfg.UUID(), state.Dead, life.Dead) } @@ -819,6 +831,8 @@ func (s *modelInfoSuite) TestDyingModelWithGetModelInfoFailure(c *gc.C) { modelDomainServices.EXPECT().Agent().Return(modelAgentService) modelAgentService.EXPECT().GetModelTargetAgentVersion(gomock.Any()).Return(jujuversion.Current, nil) + modelDomainServices.EXPECT().Machine().Return(s.mockMachineService) + s.assertSuccess(c, api, s.st.model.cfg.UUID(), state.Dying, life.Dying) } @@ -843,6 +857,8 @@ func (s *modelInfoSuite) TestDyingModelWithGetModelTargetAgentVersionFailure(c * modelDomainServices.EXPECT().Agent().Return(modelAgentService) modelAgentService.EXPECT().GetModelTargetAgentVersion(gomock.Any()).Return(jujuversion.Current, errors.NotFoundf("model agent version")) + modelDomainServices.EXPECT().Machine().Return(s.mockMachineService) + s.assertSuccess(c, api, s.st.model.cfg.UUID(), state.Dying, life.Dying) } @@ -920,6 +936,8 @@ func (s *modelInfoSuite) TestImportingModelWithGetModelInfoFailure(c *gc.C) { modelDomainServices.EXPECT().Agent().Return(modelAgentService) modelAgentService.EXPECT().GetModelTargetAgentVersion(gomock.Any()).Return(jujuversion.Current, nil) + modelDomainServices.EXPECT().Machine().Return(s.mockMachineService) + s.assertSuccess(c, api, s.st.model.cfg.UUID(), state.Alive, life.Alive) } @@ -945,6 +963,8 @@ func (s *modelInfoSuite) TestImportingModelWithGetModelTargetAgentVersionFailure modelDomainServices.EXPECT().Agent().Return(modelAgentService) modelAgentService.EXPECT().GetModelTargetAgentVersion(gomock.Any()).Return(version.Zero, errors.NotFoundf("model agent version")) + modelDomainServices.EXPECT().Machine().Return(s.mockMachineService) + s.assertSuccess(c, api, s.st.model.cfg.UUID(), state.Alive, life.Alive) } diff --git a/apiserver/facades/client/modelmanager/modelmanager.go b/apiserver/facades/client/modelmanager/modelmanager.go index cf4aa832454..cdcee59f755 100644 --- a/apiserver/facades/client/modelmanager/modelmanager.go +++ b/apiserver/facades/client/modelmanager/modelmanager.go @@ -1192,7 +1192,7 @@ func (m *ModelManagerAPI) getModelInfo(ctx context.Context, tag names.ModelTag, } } if canSeeMachinesAndSecrets { - if info.Machines, err = common.ModelMachineInfo(ctx, st, m.machineService); shouldErr(err) { + if info.Machines, err = common.ModelMachineInfo(ctx, st, modelDomainServices.Machine()); shouldErr(err) { return params.ModelInfo{}, err } } diff --git a/apiserver/facades/client/modelmanager/modelmanager_test.go b/apiserver/facades/client/modelmanager/modelmanager_test.go index 7da8b80491b..d8b4f314cac 100644 --- a/apiserver/facades/client/modelmanager/modelmanager_test.go +++ b/apiserver/facades/client/modelmanager/modelmanager_test.go @@ -378,12 +378,15 @@ func (s *modelManagerSuite) expectCreateModelOnModelDB( // Expect calls to get various model services. modelInfoService := mocks.NewMockModelInfoService(ctrl) networkService := mocks.NewMockNetworkService(ctrl) + machineService := mocks.NewMockMachineService(ctrl) + s.modelConfigService = mocks.NewMockModelConfigService(ctrl) modelAgentService := mocks.NewMockModelAgentService(ctrl) modelDomainServices.EXPECT().ModelInfo().Return(modelInfoService).AnyTimes() modelDomainServices.EXPECT().Network().Return(networkService) modelDomainServices.EXPECT().Config().Return(s.modelConfigService).AnyTimes() modelDomainServices.EXPECT().Agent().Return(modelAgentService).AnyTimes() + modelDomainServices.EXPECT().Machine().Return(machineService) // Expect calls to functions of the model services. modelInfoService.EXPECT().CreateModel(gomock.Any(), s.controllerUUID) @@ -1130,11 +1133,13 @@ func (s *modelManagerStateSuite) expectCreateModelStateSuite( modelConfigService := mocks.NewMockModelConfigService(ctrl) modelInfoService := mocks.NewMockModelInfoService(ctrl) networkService := mocks.NewMockNetworkService(ctrl) + machineService := mocks.NewMockMachineService(ctrl) modelDomainServices.EXPECT().Agent().Return(modelAgentService).AnyTimes() modelDomainServices.EXPECT().Config().Return(modelConfigService).AnyTimes() modelDomainServices.EXPECT().ModelInfo().Return(modelInfoService).AnyTimes() modelDomainServices.EXPECT().Network().Return(networkService) + modelDomainServices.EXPECT().Machine().Return(machineService) blockCommandService := mocks.NewMockBlockCommandService(ctrl) modelDomainServices.EXPECT().BlockCommand().Return(blockCommandService).AnyTimes() diff --git a/apiserver/facades/client/modelmanager/services.go b/apiserver/facades/client/modelmanager/services.go index b9f22ae70ee..280ad67026e 100644 --- a/apiserver/facades/client/modelmanager/services.go +++ b/apiserver/facades/client/modelmanager/services.go @@ -51,6 +51,9 @@ type ModelDomainServices interface { // Network returns the space service. Network() NetworkService BlockCommand() BlockCommandService + + // Machine returns the machine service. + Machine() MachineService } // DomainServicesGetter is a factory for creating model services. @@ -320,6 +323,10 @@ func (s domainServices) Network() NetworkService { return s.domainServices.Network() } +func (s domainServices) Machine() MachineService { + return s.domainServices.Machine() +} + func (s domainServices) BlockCommand() BlockCommandService { return s.domainServices.BlockCommand() }