From a419ae66de18d952fab5bb22ef64eafb91e6e76b Mon Sep 17 00:00:00 2001 From: Nicolas Vinuesa Date: Thu, 29 Feb 2024 18:11:59 +0100 Subject: [PATCH 1/4] Make sure subnets are updated when a NIC gets an address on manual cloud On manual clouds, when a new link layer device is added, the subnets are processed so that they include the new device's address. This works fine except for the case when a NIC doesn't have an address assigned at the time of provision but gets one later. This patch therefore makes sure that the subnets are updated both when a new NIC is detected and also when a new address for an existing NIC is detected. --- .../networkingcommon/networkconfigapi.go | 16 +++++- .../networkingcommon/networkconfigapi_test.go | 57 +++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/apiserver/common/networkingcommon/networkconfigapi.go b/apiserver/common/networkingcommon/networkconfigapi.go index e30062d08f9..637ba17ef53 100644 --- a/apiserver/common/networkingcommon/networkconfigapi.go +++ b/apiserver/common/networkingcommon/networkconfigapi.go @@ -149,7 +149,7 @@ func (api *NetworkConfigAPI) getMachineForSettingNetworkConfig(machineTag string } m, err := api.getMachine(tag) - if errors.IsNotFound(err) { + if errors.Is(err, errors.NotFound) { return nil, errors.Trace(apiservererrors.ErrPerm) } else if err != nil { return nil, errors.Trace(err) @@ -337,6 +337,8 @@ func (o *updateMachineLinkLayerOp) processExistingDeviceAddress( // processExistingDeviceNewAddresses interrogates the list of incoming // addresses and adds any that were not processed as already existing. +// If there are new address to add then also the subnets are processed +// to make sure they are updated on the state as well. func (o *updateMachineLinkLayerOp) processExistingDeviceNewAddresses( dev LinkLayerDevice, incomingAddrs []state.LinkLayerDeviceAddress, ) ([]txn.Op, error) { @@ -351,6 +353,16 @@ func (o *updateMachineLinkLayerOp) processExistingDeviceNewAddresses( } ops = append(ops, addOps...) + // Since this is a new address, ensure that we have + // discovered all the subnets for the device. + if o.discoverSubnets { + subNetOps, err := o.processSubnets(dev.Name()) + if err != nil { + return nil, errors.Trace(err) + } + ops = append(ops, subNetOps...) + } + o.MarkAddrProcessed(dev.Name(), addr.CIDRAddress) } } @@ -425,7 +437,7 @@ func (o *updateMachineLinkLayerOp) processSubnets(name string) ([]txn.Op, error) for _, cidr := range cidrs { addOps, err := o.st.AddSubnetOps(network.SubnetInfo{CIDR: cidr}) if err != nil { - if errors.IsAlreadyExists(err) { + if errors.Is(err, errors.AlreadyExists) { continue } return nil, errors.Trace(err) diff --git a/apiserver/common/networkingcommon/networkconfigapi_test.go b/apiserver/common/networkingcommon/networkconfigapi_test.go index 5f131e55a03..cb6d0cafec0 100644 --- a/apiserver/common/networkingcommon/networkconfigapi_test.go +++ b/apiserver/common/networkingcommon/networkconfigapi_test.go @@ -436,6 +436,63 @@ func (s *networkConfigSuite) TestUpdateMachineLinkLayerOpNewSubnetsAdded(c *gc.C c.Check(ops, gc.HasLen, 8) } +func (s *networkConfigSuite) TestUpdateMachineLinkLayerAddressOpNewSubnetsAdded(c *gc.C) { + ctrl := s.setupMocks(c) + defer ctrl.Finish() + + // A machine with one link-layer dev. + s.expectMachine() + mExp := s.machine.EXPECT() + + // Device eth0 exists with no addresses and will have one added to it. + ethMAC := "aa:bb:cc:dd:ee:ff" + ethDev := mocks.NewMockLinkLayerDevice(ctrl) + ethExp := ethDev.EXPECT() + ethExp.Name().Return("eth0").MinTimes(1) + ethExp.UpdateOps(state.LinkLayerDeviceArgs{ + Name: "eth0", + Type: "ethernet", + MACAddress: ethMAC, + IsAutoStart: true, + IsUp: true, + }).Return(nil) + ethExp.AddAddressOps(state.LinkLayerDeviceAddress{ + DeviceName: "eth0", + ConfigMethod: "static", + CIDRAddress: "0.10.0.2/24", + GatewayAddress: "0.10.0.1", + IsDefaultGateway: true, + }).Return([]txn.Op{{}}, nil) + + mExp.AllLinkLayerDevices().Return([]networkingcommon.LinkLayerDevice{ethDev}, nil) + mExp.AllDeviceAddresses().Return(nil, nil) + + // Since there is a new address, then we process (and therefore add) + // subnets. + s.state.EXPECT().AddSubnetOps(network.SubnetInfo{CIDR: "0.10.0.0/24"}).Return([]txn.Op{{}}, nil) + + op := s.NewUpdateMachineLinkLayerOp(s.machine, network.InterfaceInfos{ + { + InterfaceName: "eth0", + InterfaceType: "ethernet", + MACAddress: ethMAC, + Addresses: network.ProviderAddresses{ + network.NewMachineAddress("0.10.0.2", network.WithCIDR("0.10.0.0/24")).AsProviderAddress(), + }, + GatewayAddress: network.NewMachineAddress("0.10.0.1").AsProviderAddress(), + IsDefaultGateway: true, + }, + }, true, s.state) + + ops, err := op.Build(0) + c.Assert(err, jc.ErrorIsNil) + + // Expected ops are: + // - One for the new device address. + // - One for the new subnet. + c.Check(ops, gc.HasLen, 2) +} + func (s *networkConfigSuite) TestUpdateMachineLinkLayerOpBridgedDeviceMovesAddress(c *gc.C) { ctrl := s.setupMocks(c) defer ctrl.Finish() From 30cafe28b959fcb3616a79a0fde5129f78104dbe Mon Sep 17 00:00:00 2001 From: Caner Derici Date: Fri, 8 Mar 2024 11:16:12 -0700 Subject: [PATCH 2/4] Substitute model-uuid in placement with actual uuid in deployFromRepository This functionality was somewhat hidden in the old deploy code, so it's missed when we switched to deploying on the server side. --- apiserver/facades/client/application/application.go | 11 +++++++++-- .../facades/client/application/deployrepository.go | 2 +- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/apiserver/facades/client/application/application.go b/apiserver/facades/client/application/application.go index da72ed69c1a..f77844f88c0 100644 --- a/apiserver/facades/client/application/application.go +++ b/apiserver/facades/client/application/application.go @@ -532,7 +532,7 @@ func deployApplication( // This check is done early so that errors deeper in the call-stack do not // leave an application deployment in an unrecoverable error state. - if err := checkMachinePlacement(backend, args.ApplicationName, args.Placement); err != nil { + if err := checkMachinePlacement(backend, model.UUID(), args.ApplicationName, args.Placement); err != nil { return errors.Trace(err) } @@ -783,13 +783,20 @@ type MachinePlacementBackend interface { // If the placement scope is for a machine, ensure that the machine exists. // If the placement is for a machine or a container on an existing machine, // check that the machine is not locked for series upgrade. -func checkMachinePlacement(backend MachinePlacementBackend, app string, placement []*instance.Placement) error { +// If the placement scope is model-uuid, replace it with the actual model uuid. +func checkMachinePlacement(backend MachinePlacementBackend, modelUUID string, app string, placement []*instance.Placement) error { errTemplate := "cannot deploy %q to machine %s" for _, p := range placement { if p == nil { continue } + // Substitute the placeholder with the actual model uuid. + if p.Scope == "model-uuid" { + p.Scope = modelUUID + continue + } + dir := p.Directive toProvisionedMachine := p.Scope == instance.MachineScope diff --git a/apiserver/facades/client/application/deployrepository.go b/apiserver/facades/client/application/deployrepository.go index 32590d5fe51..b494f7dc556 100644 --- a/apiserver/facades/client/application/deployrepository.go +++ b/apiserver/facades/client/application/deployrepository.go @@ -343,7 +343,7 @@ type deployFromRepositoryValidator struct { func (v *deployFromRepositoryValidator) validate(arg params.DeployFromRepositoryArg) (deployTemplate, []error) { errs := make([]error, 0) - if err := checkMachinePlacement(v.state, arg.ApplicationName, arg.Placement); err != nil { + if err := checkMachinePlacement(v.state, v.model.UUID(), arg.ApplicationName, arg.Placement); err != nil { errs = append(errs, err) } From 0ef57ea68b8b50923309dd7f1da5038d48433d1d Mon Sep 17 00:00:00 2001 From: Caner Derici Date: Fri, 8 Mar 2024 12:50:26 -0700 Subject: [PATCH 3/4] Add unit test for model uuid placement substitution --- .../application/application_unit_test.go | 50 +++++++++++++++++++ .../application/deployrepository_test.go | 5 ++ 2 files changed, 55 insertions(+) diff --git a/apiserver/facades/client/application/application_unit_test.go b/apiserver/facades/client/application/application_unit_test.go index 6c331322e8d..d0ab876183b 100644 --- a/apiserver/facades/client/application/application_unit_test.go +++ b/apiserver/facades/client/application/application_unit_test.go @@ -1365,6 +1365,7 @@ func (s *ApplicationSuite) TestDeployAttachStorage(c *gc.C) { ch := s.expectDefaultCharm(ctrl) s.backend.EXPECT().Charm(gomock.Any()).Return(ch, nil).Times(3) + s.model.EXPECT().UUID().Return("").AnyTimes() args := params.ApplicationsDeploy{ Applications: []params.ApplicationDeploy{{ @@ -1401,6 +1402,7 @@ func (s *ApplicationSuite) TestDeployCharmOrigin(c *gc.C) { ch := s.expectDefaultCharm(ctrl) s.backend.EXPECT().Charm(gomock.Any()).Return(ch, nil).Times(2) + s.model.EXPECT().UUID().Return("").AnyTimes() track := "latest" args := params.ApplicationsDeploy{ @@ -1475,6 +1477,7 @@ func (s *ApplicationSuite) TestApplicationDeployWithStorage(c *gc.C) { }}, nil, &charm.Config{}) curl := "ch:utopic/storage-block-10" s.backend.EXPECT().Charm(curl).Return(ch, nil) + s.model.EXPECT().UUID().Return("") storageConstraints := map[string]storage.Constraints{ "data": { @@ -1510,6 +1513,7 @@ func (s *ApplicationSuite) TestApplicationDeployDefaultFilesystemStorage(c *gc.C }}, nil, &charm.Config{}) curl := "ch:utopic/storage-filesystem-1" s.backend.EXPECT().Charm(curl).Return(ch, nil) + s.model.EXPECT().UUID().Return("") args := params.ApplicationDeploy{ ApplicationName: "my-app", @@ -1538,6 +1542,7 @@ func (s *ApplicationSuite) TestApplicationDeployPlacement(c *gc.C) { machine.EXPECT().IsLockedForSeriesUpgrade().Return(false, nil) machine.EXPECT().IsParentLockedForSeriesUpgrade().Return(false, nil) s.backend.EXPECT().Machine("valid").Return(machine, nil) + s.model.EXPECT().UUID().Return("") placement := []*instance.Placement{ {Scope: "deadbeef-0bad-400d-8000-4b1d0d06f00d", Directive: "valid"}, @@ -1559,6 +1564,37 @@ func (s *ApplicationSuite) TestApplicationDeployPlacement(c *gc.C) { c.Assert(s.deployParams["my-app"].Placement, gc.DeepEquals, placement) } +func (s *ApplicationSuite) TestApplicationDeployPlacementModelUUIDSubstitute(c *gc.C) { + ctrl := s.setup(c) + defer ctrl.Finish() + + ch := s.expectDefaultCharm(ctrl) + curl := "ch:precise/dummy-42" + s.backend.EXPECT().Charm(curl).Return(ch, nil) + s.model.EXPECT().UUID().Return("deadbeef-0bad-400d-8000-4b1d0d06f00d") + + placement := []*instance.Placement{ + {Scope: "model-uuid", Directive: "0"}, + } + args := params.ApplicationDeploy{ + ApplicationName: "my-app", + CharmURL: curl, + CharmOrigin: createCharmOriginFromURL(curl), + NumUnits: 1, + Placement: placement, + } + results, err := s.api.Deploy(params.ApplicationsDeploy{ + Applications: []params.ApplicationDeploy{args}}, + ) + c.Assert(err, jc.ErrorIsNil) + c.Assert(results.Results, gc.HasLen, 1) + c.Assert(results.Results[0].Error, gc.IsNil) + + c.Assert(s.deployParams["my-app"].Placement, gc.DeepEquals, []*instance.Placement{ + {Scope: "deadbeef-0bad-400d-8000-4b1d0d06f00d", Directive: "0"}, + }) +} + func validCharmOriginForTest() *params.CharmOrigin { return ¶ms.CharmOrigin{Source: "charm-hub"} } @@ -1574,6 +1610,7 @@ func (s *ApplicationSuite) TestApplicationDeployWithPlacementLockedError(c *gc.C containerMachine.EXPECT().IsLockedForSeriesUpgrade().Return(false, nil).MinTimes(1) containerMachine.EXPECT().IsParentLockedForSeriesUpgrade().Return(true, nil).MinTimes(1) s.backend.EXPECT().Machine("0/lxd/0").Return(containerMachine, nil).MinTimes(1) + s.model.EXPECT().UUID().Return("").AnyTimes() curl := "ch:precise/dummy-42" args := []params.ApplicationDeploy{{ @@ -1660,6 +1697,7 @@ func (s *ApplicationSuite) TestApplicationDeploymentTrust(c *gc.C) { ch := s.expectDefaultCharm(ctrl) curl := "ch:precise/dummy-42" s.backend.EXPECT().Charm(curl).Return(ch, nil).MinTimes(1) + s.model.EXPECT().UUID().Return("") withTrust := map[string]string{"trust": "true"} args := params.ApplicationDeploy{ @@ -1688,6 +1726,7 @@ func (s *ApplicationSuite) TestClientApplicationsDeployWithBindings(c *gc.C) { ch := s.expectDefaultCharm(ctrl) curl := "ch:focal/riak-42" s.backend.EXPECT().Charm(curl).Return(ch, nil).MinTimes(1) + s.model.EXPECT().UUID().Return("").AnyTimes() args := []params.ApplicationDeploy{{ ApplicationName: "old", @@ -1754,6 +1793,7 @@ func (s *ApplicationSuite) TestDeployMinDeploymentVersionTooHigh(c *gc.C) { k8sconstants.StorageProviderType, map[string]interface{}{"foo": "bar"}), ) + s.model.EXPECT().UUID().Return("") args := params.ApplicationsDeploy{ Applications: []params.ApplicationDeploy{{ @@ -1792,6 +1832,7 @@ func (s *ApplicationSuite) TestDeployCAASModel(c *gc.C) { }, ) s.backend.EXPECT().Charm(gomock.Any()).Return(ch, nil).Times(4) + s.model.EXPECT().UUID().Return("").AnyTimes() s.expectDefaultK8sModelConfig() args := params.ApplicationsDeploy{ @@ -1846,6 +1887,7 @@ func (s *ApplicationSuite) TestDeployCAASInvalidServiceType(c *gc.C) { ch := s.expectDefaultCharm(ctrl) s.backend.EXPECT().Charm(gomock.Any()).Return(ch, nil) + s.model.EXPECT().UUID().Return("") curl := "local:foo-0" args := params.ApplicationsDeploy{ @@ -1872,6 +1914,7 @@ func (s *ApplicationSuite) TestDeployCAASBlockStorageRejected(c *gc.C) { Storage: map[string]charm.Storage{"block": {Name: "block", Type: charm.StorageBlock}}, }, &charm.Manifest{}, &charm.Config{}) s.backend.EXPECT().Charm(gomock.Any()).Return(ch, nil) + s.model.EXPECT().UUID().Return("") args := params.ApplicationsDeploy{ Applications: []params.ApplicationDeploy{{ @@ -1905,6 +1948,7 @@ func (s *ApplicationSuite) TestDeployCAASModelNoOperatorStorage(c *gc.C) { "workload-storage": "k8s-storage", }) s.model.EXPECT().ModelConfig().Return(config.New(config.UseDefaults, attrs)).MinTimes(1) + s.model.EXPECT().UUID().Return("") args := params.ApplicationsDeploy{ Applications: []params.ApplicationDeploy{{ @@ -1944,6 +1988,7 @@ func (s *ApplicationSuite) TestDeployCAASModelCharmNeedsNoOperatorStorage(c *gc. "workload-storage": "k8s-storage", }) s.model.EXPECT().ModelConfig().Return(config.New(config.UseDefaults, attrs)).MinTimes(1) + s.model.EXPECT().UUID().Return("") args := params.ApplicationsDeploy{ Applications: []params.ApplicationDeploy{{ @@ -1971,6 +2016,7 @@ func (s *ApplicationSuite) TestDeployCAASModelSidecarCharmNeedsNoOperatorStorage "workload-storage": "k8s-storage", }) s.model.EXPECT().ModelConfig().Return(config.New(config.UseDefaults, attrs)).MinTimes(1) + s.model.EXPECT().UUID().Return("").AnyTimes() args := params.ApplicationsDeploy{ Applications: []params.ApplicationDeploy{{ @@ -1997,6 +2043,7 @@ func (s *ApplicationSuite) TestDeployCAASModelDefaultOperatorStorageClass(c *gc. s.caasBroker.EXPECT().ValidateStorageClass(gomock.Any()).Return(nil) s.storagePoolManager.EXPECT().Get("k8s-operator-storage").Return(nil, errors.NotFoundf("pool")) s.registry.EXPECT().StorageProvider(storage.ProviderType("k8s-operator-storage")).Return(nil, errors.NotFoundf(`provider type "k8s-operator-storage"`)) + s.model.EXPECT().UUID().Return("") args := params.ApplicationsDeploy{ Applications: []params.ApplicationDeploy{{ @@ -2025,6 +2072,7 @@ func (s *ApplicationSuite) TestDeployCAASModelWrongOperatorStorageType(c *gc.C) provider.RootfsProviderType, map[string]interface{}{"foo": "bar"}), ) + s.model.EXPECT().UUID().Return("") args := params.ApplicationsDeploy{ Applications: []params.ApplicationDeploy{{ @@ -2055,6 +2103,7 @@ func (s *ApplicationSuite) TestDeployCAASModelInvalidStorage(c *gc.C) { map[string]interface{}{"foo": "bar"}), ) s.caasBroker.EXPECT().ValidateStorageClass(gomock.Any()).Return(errors.NotFoundf("storage class")) + s.model.EXPECT().UUID().Return("") args := params.ApplicationsDeploy{ Applications: []params.ApplicationDeploy{{ @@ -2092,6 +2141,7 @@ func (s *ApplicationSuite) TestDeployCAASModelDefaultStorageClass(c *gc.C) { map[string]interface{}{"foo": "bar"}), ) s.caasBroker.EXPECT().ValidateStorageClass(gomock.Any()).Return(nil) + s.model.EXPECT().UUID().Return("") args := params.ApplicationsDeploy{ Applications: []params.ApplicationDeploy{{ diff --git a/apiserver/facades/client/application/deployrepository_test.go b/apiserver/facades/client/application/deployrepository_test.go index 434d08f9053..128a4e7155b 100644 --- a/apiserver/facades/client/application/deployrepository_test.go +++ b/apiserver/facades/client/application/deployrepository_test.go @@ -59,6 +59,7 @@ func (s *validatorSuite) TestValidateSuccess(c *gc.C) { resolvedData := getResolvedData(resultURL, resolvedOrigin) s.repo.EXPECT().ResolveForDeploy(charmID).Return(resolvedData, nil) s.repo.EXPECT().ResolveResources(nil, corecharm.CharmID{URL: resultURL, Origin: resolvedOrigin}).Return(nil, nil) + s.model.EXPECT().UUID().Return("") // getCharm s.state.EXPECT().ModelConstraints().Return(constraints.Value{Arch: strptr("arm64")}, nil) @@ -112,6 +113,7 @@ func (s *validatorSuite) testValidateIAASAttachStorage(c *gc.C, argStorage []str resolvedData := getResolvedData(resultURL, resolvedOrigin) s.repo.EXPECT().ResolveForDeploy(charmID).Return(resolvedData, nil) s.repo.EXPECT().ResolveResources(nil, corecharm.CharmID{URL: resultURL, Origin: resolvedOrigin}).Return(nil, nil) + s.model.EXPECT().UUID().Return("") // getCharm s.state.EXPECT().ModelConstraints().Return(constraints.Value{Arch: strptr("arm64")}, nil) s.state.EXPECT().Charm(gomock.Any()).Return(nil, errors.NotFoundf("charm")) @@ -160,6 +162,7 @@ func (s *validatorSuite) TestValidatePlacementSuccess(c *gc.C) { resolvedData := getResolvedData(resultURL, resolvedOrigin) s.repo.EXPECT().ResolveForDeploy(charmID).Return(resolvedData, nil) s.repo.EXPECT().ResolveResources(nil, corecharm.CharmID{URL: resultURL, Origin: resolvedOrigin}).Return(nil, nil) + s.model.EXPECT().UUID().Return("") // Placement s.state.EXPECT().Machine("0").Return(s.machine, nil).Times(2) @@ -213,6 +216,7 @@ func (s *validatorSuite) TestValidateEndpointBindingSuccess(c *gc.C) { resolvedData := getResolvedData(resultURL, resolvedOrigin) s.repo.EXPECT().ResolveForDeploy(charmID).Return(resolvedData, nil) s.repo.EXPECT().ResolveResources(nil, corecharm.CharmID{URL: resultURL, Origin: resolvedOrigin}).Return(nil, nil) + s.model.EXPECT().UUID().Return("") // state bindings endpointMap := map[string]string{"to": "from"} @@ -948,6 +952,7 @@ func (s *validatorSuite) TestCaasDeployFromRepositoryValidator(c *gc.C) { s.state.EXPECT().ModelConstraints().Return(constraints.Value{ Arch: strptr("arm64"), }, nil) + s.model.EXPECT().UUID().Return("") arg := params.DeployFromRepositoryArg{ CharmName: "testcharm", From fb3c674e930258801ecb6d9e0901d0eab89f987d Mon Sep 17 00:00:00 2001 From: Jack Shaw Date: Wed, 28 Feb 2024 11:57:53 +0000 Subject: [PATCH 4/4] Fix --to placement directive This resolves https://bugs.launchpad.net/juju/+bug/2054375 The charmhub api does not expect a risk to be present in OS base channels. However, under some circumstances (importantly, using the --to placement directive), a risk is included. Filter these out Add an integration test to cover this case Also, as a flyby, remove the ChannelTrack function from core charm. This function is confusing and encourages misuse. It was only used twice, on both occasions to filter the risk out of an OS base. However, the function lived in the charm package, and it's name/description imply it's to be used for charm channels. A few options were considered for what to do instead. In the end, I made the decision to just remove it, and in the places it was used to parse a base channel and just read the Track. This turns out to be exactly what a 'BaseChannelTrack' function would do, and not actually add any complexity to the code. In my opinion, a function/method to do this for us is not a helpful abstraction, since dropping it actually increases readability. Confusion around OS channels and charm channels is avoided because call a concrete base.ParseChannel function, and deal with the concrete base.Channel struct. This would be hidden if we use a 'BaseChannelTrack' function. This isn't perfect, however. There is still a bit of overlap between Juju-base-channel and charmhub-base-channel, which aren't quite always the same. We would now need to support channels with track "all" for instance. This was the case beforehand charm channels, so this is an improvement. --- .../client/machinemanager/upgradebase.go | 4 +-- charmhub/refresh.go | 20 ++++++++++- charmhub/refresh_test.go | 33 +++++++++++++++++++ core/base/channel_test.go | 3 ++ core/charm/origin.go | 14 -------- core/charm/origin_test.go | 32 ------------------ core/charm/repository/charmhub.go | 5 +-- core/charm/repository/charmhub_test.go | 5 +-- tests/suites/deploy/deploy_charms.sh | 16 +++++++++ 9 files changed, 79 insertions(+), 53 deletions(-) diff --git a/apiserver/facades/client/machinemanager/upgradebase.go b/apiserver/facades/client/machinemanager/upgradebase.go index f34b8ed7a49..b672ab56b58 100644 --- a/apiserver/facades/client/machinemanager/upgradebase.go +++ b/apiserver/facades/client/machinemanager/upgradebase.go @@ -524,11 +524,11 @@ func (s charmhubSeriesValidator) ValidateApplications(applications []Application for _, resp := range refreshResp { var found bool for _, base := range resp.Entity.Bases { - track, err := corecharm.ChannelTrack(base.Channel) + channel, err := corebase.ParseChannel(base.Channel) if err != nil { return errors.Trace(err) } - if channelToValidate == track || force { + if channelToValidate == channel.Track || force { found = true break } diff --git a/charmhub/refresh.go b/charmhub/refresh.go index 56211b09363..178419b5941 100644 --- a/charmhub/refresh.go +++ b/charmhub/refresh.go @@ -429,7 +429,10 @@ func constructRefreshBase(base RefreshBase) (transport.Base, error) { if err == nil { channel = potential } else { - channel = base.Channel + channel, err = sanitiseChannel(base.Channel) + if err != nil { + return transport.Base{}, logAndReturnError(errors.Trace(err)) + } } } @@ -440,6 +443,21 @@ func constructRefreshBase(base RefreshBase) (transport.Base, error) { }, nil } +// sanitiseChannel returns a channel, sanitised for charmhub +// +// Sometimes channels we receive include a risk, which charmhub +// cannot understand. So ensure any risk is dropped. +func sanitiseChannel(channel string) (string, error) { + if channel == "" { + return channel, nil + } + ch, err := corebase.ParseChannel(channel) + if err != nil { + return "", errors.Trace(err) + } + return ch.Track, nil +} + // validateBase ensures that we do not pass "all" as part of base. // This function is to help find programming related failures. func validateBase(rp RefreshBase) error { diff --git a/charmhub/refresh_test.go b/charmhub/refresh_test.go index ed122a66bf7..e1ee2becab2 100644 --- a/charmhub/refresh_test.go +++ b/charmhub/refresh_test.go @@ -466,6 +466,39 @@ func (s *RefreshConfigSuite) TestRefreshOneBuild(c *gc.C) { }) } +func (s *RefreshConfigSuite) TestRefreshOneWithBaseChannelRiskBuild(c *gc.C) { + id := "foo" + config, err := RefreshOne("instance-key", id, 1, "latest/stable", RefreshBase{ + Name: "ubuntu", + Channel: "20.04/stable", + Architecture: arch.DefaultArchitecture, + }) + c.Assert(err, jc.ErrorIsNil) + + req, err := config.Build() + c.Assert(err, jc.ErrorIsNil) + c.Assert(req, gc.DeepEquals, transport.RefreshRequest{ + Context: []transport.RefreshRequestContext{{ + InstanceKey: "instance-key", + ID: "foo", + Revision: 1, + Base: transport.Base{ + Name: "ubuntu", + Channel: "20.04", + Architecture: arch.DefaultArchitecture, + }, + TrackingChannel: "latest/stable", + }}, + Actions: []transport.RefreshRequestAction{{ + Action: "refresh", + InstanceKey: "instance-key", + ID: &id, + }}, + Fields: expRefreshFields, + }) + +} + func (s *RefreshConfigSuite) TestRefreshOneBuildInstanceKeyCompatibility(c *gc.C) { id := "foo" config, err := RefreshOne("", id, 1, "latest/stable", RefreshBase{ diff --git a/core/base/channel_test.go b/core/base/channel_test.go index 6ce7f0b9e27..ac991f9d738 100644 --- a/core/base/channel_test.go +++ b/core/base/channel_test.go @@ -22,6 +22,9 @@ func (s *ChannelSuite) TestParse(c *gc.C) { ch, err = ParseChannel("22.04/edge") c.Assert(err, jc.ErrorIsNil) c.Assert(ch, jc.DeepEquals, Channel{Track: "22.04", Risk: "edge"}) + ch, err = ParseChannel("all") + c.Assert(err, jc.ErrorIsNil) + c.Assert(ch, jc.DeepEquals, Channel{Track: "all"}) } func (s *ChannelSuite) TestParseError(c *gc.C) { diff --git a/core/charm/origin.go b/core/charm/origin.go index 813cbf6ae9f..ea159571f38 100644 --- a/core/charm/origin.go +++ b/core/charm/origin.go @@ -192,17 +192,3 @@ func (p Platform) String() string { return path } - -func ChannelTrack(channel string) (string, error) { - // Base channel can be found as either just the version `20.04` (focal) - // or as `20.04/latest` (focal latest). We should future proof ourself - // for now and just drop the risk on the floor. - ch, err := charm.ParseChannel(channel) - if err != nil { - return "", errors.Trace(err) - } - if ch.Track == "" { - return "", errors.NotValidf("channel track") - } - return ch.Track, nil -} diff --git a/core/charm/origin_test.go b/core/charm/origin_test.go index 73395c39c3e..85ca55ecb10 100644 --- a/core/charm/origin_test.go +++ b/core/charm/origin_test.go @@ -139,35 +139,3 @@ func (s platformSuite) TestString(c *gc.C) { c.Assert(platform.String(), gc.DeepEquals, test.Expected) } } - -type channelTrackSuite struct { - testing.IsolationSuite -} - -var _ = gc.Suite(&channelTrackSuite{}) - -func (*channelTrackSuite) TestChannelTrack(c *gc.C) { - tests := []struct { - channel string - result string - }{{ - channel: "20.10", - result: "20.10", - }, { - channel: "focal", - result: "focal", - }, { - channel: "20.10/stable", - result: "20.10", - }, { - channel: "focal/stable", - result: "focal", - }} - - for i, test := range tests { - c.Logf("test %d - %s", i, test.channel) - got, err := charm.ChannelTrack(test.channel) - c.Assert(err, jc.ErrorIsNil) - c.Assert(got, gc.Equals, test.result) - } -} diff --git a/core/charm/repository/charmhub.go b/core/charm/repository/charmhub.go index 784738263bc..0e161a81027 100644 --- a/core/charm/repository/charmhub.go +++ b/core/charm/repository/charmhub.go @@ -1006,12 +1006,13 @@ func (c *CharmHubRepository) composeSuggestions(releases []transport.Release, or base corebase.Base err error ) - track, err := corecharm.ChannelTrack(release.Base.Channel) + + channel, err := corebase.ParseChannel(release.Base.Channel) if err != nil { c.logger.Errorf("invalid base channel %v: %s", release.Base.Channel, err) continue } - if track == "all" || release.Base.Name == "all" { + if channel.Track == "all" || release.Base.Name == "all" { base, err = corebase.ParseBase(origin.Platform.OS, origin.Platform.Channel) } else { base, err = corebase.ParseBase(release.Base.Name, release.Base.Channel) diff --git a/core/charm/repository/charmhub_test.go b/core/charm/repository/charmhub_test.go index fb5fe6bf7c7..dd055c5a12e 100644 --- a/core/charm/repository/charmhub_test.go +++ b/core/charm/repository/charmhub_test.go @@ -1005,7 +1005,8 @@ func (refreshConfigSuite) TestRefreshByChannel(c *gc.C) { func (refreshConfigSuite) TestRefreshByChannelVersion(c *gc.C) { name := "wordpress" - platform := corecharm.MustParsePlatform("amd64/ubuntu/20.10/latest") + // 'mistakenly' include a risk in the platform + platform := corecharm.MustParsePlatform("amd64/ubuntu/20.10/stable") channel := corecharm.MustParseChannel("latest/stable").Normalize() origin := corecharm.Origin{ Platform: platform, @@ -1028,7 +1029,7 @@ func (refreshConfigSuite) TestRefreshByChannelVersion(c *gc.C) { Channel: &ch, Base: &transport.Base{ Name: "ubuntu", - Channel: "20.10/latest", + Channel: "20.10", Architecture: "amd64", }, }}, diff --git a/tests/suites/deploy/deploy_charms.sh b/tests/suites/deploy/deploy_charms.sh index 49ae47f47c7..65d16d37ed0 100644 --- a/tests/suites/deploy/deploy_charms.sh +++ b/tests/suites/deploy/deploy_charms.sh @@ -14,6 +14,21 @@ run_deploy_charm() { destroy_model "test-deploy-charm" } +run_deploy_charm_placement_directive() { + echo + + file="${TEST_DIR}/test-deploy-charm-placement-directive.log" + + ensure "test-deploy-charm-placement-directive" "${file}" + + juju add-machine --base ubuntu@20.04 + juju deploy jameinel-ubuntu-lite --to 0 + wait_for "ubuntu-lite" "$(idle_condition "ubuntu-lite")" + + destroy_model "test-deploy-charm-placement-directive" + +} + run_deploy_charm_unsupported_series() { # Test trying to deploy a charmhub charm to an operating system # never supported in the specified channel. It should fail. @@ -318,6 +333,7 @@ test_deploy_charms() { cd .. || exit run "run_deploy_charm" + run "run_deploy_charm_placement_directive" run "run_deploy_specific_series" run "run_resolve_charm" run "run_deploy_charm_unsupported_series"