Merge pull request #13135 from wallyworld/cass-image-repo-port

#13135 A small fix for an issue found by QA folks when setting up a custom docker image repo. If the repo URL configured using `caas-image-repo` has a port, the parsing fails. ## QA steps There's a unit test which ensures the expected behaviour. There's no 2.9.8 images but do this ` juju bootstrap microk8s test --config caas-image-repo=docker.io:5000/jujusolutions` and it won't find the image but you can check the correct image URL: ``` microk8s.kubectl -n controller-test get pod/controller-0 -o json | jq .spec.containers[0].image "docker.io:5000/jujusolutions/jujud-operator:2.9.8" ``` Also bootstrap a standard microk8s controller to regression test default behaviour. ## Bug reference https://bugs.launchpad.net/juju/+bug/1934707
juju · Jul 7, 2021 · 185fac6 · 185fac6
2 parents 2b67b0a + fe8bdf7
commit 185fac6
Show file tree

Hide file tree

Showing 13 changed files with 99 additions and 43 deletions.
diff --git a/apiserver/facades/controller/caasapplicationprovisioner/provisioner.go b/apiserver/facades/controller/caasapplicationprovisioner/provisioner.go
@@ -226,7 +226,10 @@ func (a *API) provisioningInfo(appName names.ApplicationTag) (*params.CAASApplic
 			fmt.Sprintf("agent version is missing in model config %q", modelConfig.Name()),
 		)
 	}
-	imagePath := podcfg.GetJujuOCIImagePath(cfg, vers.ToPatch(), version.OfficialBuild)
+	imagePath, err := podcfg.GetJujuOCIImagePath(cfg, vers.ToPatch(), version.OfficialBuild)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
 
 	apiHostPorts, err := a.ctrlSt.APIHostPortsForAgents()
 	if err != nil {

diff --git a/apiserver/facades/controller/caasmodeloperator/operator.go b/apiserver/facades/controller/caasmodeloperator/operator.go
@@ -93,11 +93,15 @@ func (a *API) ModelOperatorProvisioningInfo() (params.ModelOperatorInfo, error)
 		return result, errors.Annotate(err, "getting api addresses")
 	}
 
+	imagePath, err := podcfg.GetJujuOCIImagePath(controllerConf,
+		vers.ToPatch(), version.OfficialBuild)
+	if err != nil {
+		return result, errors.Trace(err)
+	}
 	result = params.ModelOperatorInfo{
 		APIAddresses: apiAddresses.Result,
-		ImagePath: podcfg.GetJujuOCIImagePath(controllerConf,
-			vers.ToPatch(), version.OfficialBuild),
-		Version: vers,
+		ImagePath:    imagePath,
+		Version:      vers,
 	}
 	return result, nil
 }

diff --git a/apiserver/facades/controller/caasmodeloperator/operator_test.go b/apiserver/facades/controller/caasmodeloperator/operator_test.go
@@ -47,7 +47,8 @@ func (m *ModelOperatorSuite) TestProvisioningInfo(c *gc.C) {
 	controllerConf, err := m.state.ControllerConfig()
 	c.Assert(err, jc.ErrorIsNil)
 
-	imagePath := podcfg.GetJujuOCIImagePath(controllerConf, info.Version, version.OfficialBuild)
+	imagePath, err := podcfg.GetJujuOCIImagePath(controllerConf, info.Version, version.OfficialBuild)
+	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(imagePath, gc.Equals, info.ImagePath)
 
 	model, err := m.state.Model()

diff --git a/apiserver/facades/controller/caasoperatorprovisioner/provisioner.go b/apiserver/facades/controller/caasoperatorprovisioner/provisioner.go
@@ -138,7 +138,10 @@ func (a *API) OperatorProvisioningInfo(args params.Entities) (params.OperatorPro
 		modelConfig,
 	)
 
-	imagePath := podcfg.GetJujuOCIImagePath(cfg, vers.ToPatch(), version.OfficialBuild)
+	imagePath, err := podcfg.GetJujuOCIImagePath(cfg, vers.ToPatch(), version.OfficialBuild)
+	if err != nil {
+		return result, errors.Trace(err)
+	}
 	apiAddresses, err := a.APIAddresses()
 	if err == nil && apiAddresses.Error != nil {
 		err = apiAddresses.Error

diff --git a/apiserver/facades/controller/caasunitprovisioner/provisioner.go b/apiserver/facades/controller/caasunitprovisioner/provisioner.go
@@ -394,7 +394,10 @@ func (f *Facade) provisioningInfo(model Model, tagString string) (*params.Kubern
 			fmt.Sprintf("agent version is missing in model config %q", modelConfig.Name()),
 		)
 	}
-	operatorImagePath := podcfg.GetJujuOCIImagePath(controllerCfg, vers.ToPatch(), version.OfficialBuild)
+	operatorImagePath, err := podcfg.GetJujuOCIImagePath(controllerCfg, vers.ToPatch(), version.OfficialBuild)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
 
 	filesystemParams, err := f.applicationFilesystemParams(app, controllerCfg, modelConfig)
 	if err != nil {

diff --git a/caas/kubernetes/provider/application/application.go b/caas/kubernetes/provider/application/application.go
@@ -539,7 +539,11 @@ func (a *app) upgradeMainResource(applier resources.Applier, ver version.Number)
 			return errors.NotValidf("init container of %q", a.name)
 		}
 		initContainer := initContainers[0]
-		initContainer.Image = podcfg.RebuildOldOperatorImagePath(initContainer.Image, ver)
+		var err error
+		initContainer.Image, err = podcfg.RebuildOldOperatorImagePath(initContainer.Image, ver)
+		if err != nil {
+			return errors.Trace(err)
+		}
 		ss.Spec.Template.Spec.InitContainers = []corev1.Container{initContainer}
 		ss.Spec.Template.SetAnnotations(a.upgradeAnnotations(annotations.New(ss.Spec.Template.GetAnnotations()), ver))
 		ss.SetAnnotations(a.upgradeAnnotations(annotations.New(ss.GetAnnotations()), ver))

diff --git a/caas/kubernetes/provider/bootstrap.go b/caas/kubernetes/provider/bootstrap.go
@@ -1097,7 +1097,7 @@ func (c *controllerStack) buildContainerSpecForController(statefulset *apps.Stat
 	if c.pcfg.Controller.Config.MongoMemoryProfile() == string(mongo.MemoryProfileLow) {
 		wiredTigerCacheSize = mongo.Mongo34LowCacheSize
 	}
-	generateContainerSpecs := func(jujudCmd string) []core.Container {
+	generateContainerSpecs := func(jujudCmd string) ([]core.Container, error) {
 		var containerSpec []core.Container
 		// add container mongoDB.
 		// TODO(bootstrap): refactor mongo package to make it usable for IAAS and CAAS,
@@ -1194,10 +1194,14 @@ func (c *controllerStack) buildContainerSpecForController(statefulset *apps.Stat
 		})
 
 		// add container API server.
+		controllerImage, err := c.pcfg.GetControllerImagePath()
+		if err != nil {
+			return nil, errors.Trace(err)
+		}
 		containerSpec = append(containerSpec, core.Container{
 			Name:            apiServerContainerName,
 			ImagePullPolicy: core.PullIfNotPresent,
-			Image:           c.pcfg.GetControllerImagePath(),
+			Image:           controllerImage,
 			Command: []string{
 				"/bin/sh",
 			},
@@ -1247,7 +1251,7 @@ func (c *controllerStack) buildContainerSpecForController(statefulset *apps.Stat
 			},
 		})
 		c.containerCount = len(containerSpec)
-		return containerSpec
+		return containerSpec, nil
 	}
 
 	loggingOption := "--show-log"
@@ -1285,7 +1289,11 @@ func (c *controllerStack) buildContainerSpecForController(statefulset *apps.Stat
 		c.pcfg.ControllerId,
 		loggingOption,
 	)
-	statefulset.Spec.Template.Spec.Containers = generateContainerSpecs(jujudCmd)
+	containers, err := generateContainerSpecs(jujudCmd)
+	if err != nil {
+		return errors.Trace(err)
+	}
+	statefulset.Spec.Template.Spec.Containers = containers
 	return nil
 }
 

diff --git a/caas/kubernetes/provider/operator_upgrade.go b/caas/kubernetes/provider/operator_upgrade.go
@@ -191,7 +191,10 @@ func operatorUpgrade(
 		return errors.Trace(err)
 	}
 
-	operatorImagePath := podcfg.RebuildOldOperatorImagePath(operator.Config.OperatorImagePath, vers)
+	operatorImagePath, err := podcfg.RebuildOldOperatorImagePath(operator.Config.OperatorImagePath, vers)
+	if err != nil {
+		return errors.Trace(err)
+	}
 	if len(operatorImagePath) == 0 {
 		// This should never happen.
 		return errors.NotValidf("no resource is upgradable for application %q", appName)

diff --git a/caas/kubernetes/provider/upgrade.go b/caas/kubernetes/provider/upgrade.go
@@ -134,8 +134,12 @@ func upgradePodTemplateSpec(
 	}
 
 	if imagePath == "" {
-		imagePath = podcfg.RebuildOldOperatorImagePath(
+		var err error
+		imagePath, err = podcfg.RebuildOldOperatorImagePath(
 			podTemplate.Spec.Containers[jujudContainerIdx].Image, vers)
+		if err != nil {
+			return nil, errors.Trace(err)
+		}
 	}
 
 	upgradedTemp := podTemplate.DeepCopy()

diff --git a/cloudconfig/podcfg/image.go b/cloudconfig/podcfg/image.go
@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/docker/distribution/reference"
 	"github.com/juju/charm/v8"
 	"github.com/juju/errors"
 	"github.com/juju/version/v2"
@@ -15,14 +16,13 @@ import (
 )
 
 const (
-	JujudOCINamespace      = "jujusolutions"
-	JujudOCIName           = "jujud-operator"
-	JujuContainerAgentName = "containeragent"
-	JujudbOCIName          = "juju-db"
+	JujudOCINamespace = "jujusolutions"
+	JujudOCIName      = "jujud-operator"
+	JujudbOCIName     = "juju-db"
 )
 
 // GetControllerImagePath returns oci image path of jujud for a controller.
-func (cfg *ControllerPodConfig) GetControllerImagePath() string {
+func (cfg *ControllerPodConfig) GetControllerImagePath() (string, error) {
 	return GetJujuOCIImagePath(cfg.Controller.Config, cfg.JujuVersion, cfg.OfficialBuild)
 }
 
@@ -42,44 +42,44 @@ func IsJujuOCIImage(imagePath string) bool {
 }
 
 // GetJujuOCIImagePath returns the jujud oci image path.
-func GetJujuOCIImagePath(controllerCfg controller.Config, ver version.Number, build int) string {
+func GetJujuOCIImagePath(controllerCfg controller.Config, ver version.Number, build int) (string, error) {
 	// First check the deprecated "caas-operator-image-path" config.
 	ver.Build = build
-	imagePath := RebuildOldOperatorImagePath(
+	imagePath, err := RebuildOldOperatorImagePath(
 		controllerCfg.CAASOperatorImagePath(), ver,
 	)
-	if imagePath != "" {
-		return imagePath
+	if imagePath != "" || err != nil {
+		return imagePath, err
 	}
 	return imageRepoToPath(controllerCfg.CAASImageRepo(), ver)
 }
 
 // RebuildOldOperatorImagePath returns a updated image path for the specified juju version.
-func RebuildOldOperatorImagePath(imagePath string, ver version.Number) string {
+func RebuildOldOperatorImagePath(imagePath string, ver version.Number) (string, error) {
 	if imagePath == "" {
-		return ""
+		return "", nil
 	}
 	return tagImagePath(imagePath, ver)
 }
 
-func tagImagePath(path string, ver version.Number) string {
-	var verString string
-	splittedPath := strings.Split(path, ":")
-	path = splittedPath[0]
-	if len(splittedPath) > 1 {
-		verString = splittedPath[1]
+func tagImagePath(fullPath string, ver version.Number) (string, error) {
+	ref, err := reference.Parse(fullPath)
+	if err != nil {
+		return "", errors.Trace(err)
 	}
-	if ver != version.Zero {
-		verString = ver.String()
+	imageNamed, ok := ref.(reference.Named)
+	// Safety check only - should never happen.
+	if !ok {
+		return "", errors.Errorf("unexpected docker image path type, got %T, expected reference.Named", ref)
 	}
-	if verString != "" {
-		// tag with version.
-		path += ":" + verString
+	if ver != version.Zero {
+		// ver is always a valid tag.
+		imageNamed, _ = reference.WithTag(imageNamed, ver.String())
 	}
-	return path
+	return imageNamed.String(), nil
 }
 
-func imageRepoToPath(imageRepo string, ver version.Number) string {
+func imageRepoToPath(imageRepo string, ver version.Number) (string, error) {
 	if imageRepo == "" {
 		imageRepo = JujudOCINamespace
 	}

diff --git a/cloudconfig/podcfg/image_test.go b/cloudconfig/podcfg/image_test.go
@@ -25,14 +25,29 @@ func (*imageSuite) TestGetJujuOCIImagePath(c *gc.C) {
 
 	cfg[controller.CAASImageRepo] = "testing-repo"
 	ver := version.MustParse("2.6-beta3")
-	path := podcfg.GetJujuOCIImagePath(cfg, ver, 666)
+	path, err := podcfg.GetJujuOCIImagePath(cfg, ver, 666)
+	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(path, jc.DeepEquals, "testing-repo/jujud-operator:2.6-beta3.666")
 
+	cfg[controller.CAASImageRepo] = "testing-repo:8080"
+	ver = version.MustParse("2.6-beta3")
+	path, err = podcfg.GetJujuOCIImagePath(cfg, ver, 666)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(path, jc.DeepEquals, "testing-repo:8080/jujud-operator:2.6-beta3.666")
+
 	cfg[controller.CAASOperatorImagePath] = "testing-old-repo/jujud-old-operator:1.6"
-	path = podcfg.GetJujuOCIImagePath(cfg, ver, 0)
+	path, err = podcfg.GetJujuOCIImagePath(cfg, ver, 0)
+	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(path, jc.DeepEquals, "testing-old-repo/jujud-old-operator:2.6-beta3")
 }
 
+func (*imageSuite) TestRebuildOldOperatorImagePath(c *gc.C) {
+	ver := version.MustParse("2.6-beta3")
+	path, err := podcfg.RebuildOldOperatorImagePath("jujusolutions/jujud-operator:666", ver)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(path, jc.DeepEquals, "jujusolutions/jujud-operator:2.6-beta3")
+}
+
 func (*imageSuite) TestImageForBase(c *gc.C) {
 	_, err := podcfg.ImageForBase("", charm.Base{})
 	c.Assert(err, gc.ErrorMatches, `empty base name not valid`)

diff --git a/cloudconfig/podcfg/podcfg_test.go b/cloudconfig/podcfg/podcfg_test.go
@@ -66,7 +66,9 @@ func (*podcfgSuite) TestOperatorImagesDefaultRepo(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 	podConfig.JujuVersion = version.MustParse("6.6.6")
 	podConfig.OfficialBuild = 666
-	c.Assert(podConfig.GetControllerImagePath(), gc.Equals, "jujusolutions/jujud-operator:6.6.6.666")
+	path, err := podConfig.GetControllerImagePath()
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(path, gc.Equals, "jujusolutions/jujud-operator:6.6.6.666")
 	c.Assert(podConfig.GetJujuDbOCIImagePath(), gc.Equals, "jujusolutions/juju-db:4.0")
 }
 
@@ -82,7 +84,10 @@ func (*podcfgSuite) TestOperatorImagesCustomRepo(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 	podConfig.JujuVersion = version.MustParse("6.6.6")
 	podConfig.OfficialBuild = 666
-	c.Assert(podConfig.GetControllerImagePath(), gc.Equals, "path/to/my/repo/jujud-operator:6.6.6.666")
+	c.Assert(err, jc.ErrorIsNil)
+	path, err := podConfig.GetControllerImagePath()
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(path, gc.Equals, "path/to/my/repo/jujud-operator:6.6.6.666")
 	c.Assert(podConfig.GetJujuDbOCIImagePath(), gc.Equals, "path/to/my/repo/juju-db:4.0")
 }
 

diff --git a/cmd/juju/commands/upgradecontroller.go b/cmd/juju/commands/upgradecontroller.go
@@ -276,7 +276,10 @@ func (c *baseUpgradeCommand) initCAASVersions(
 	controllerCfg controller.Config, majorVersion int, streamsAgents tools.List,
 ) (tools.Versions, error) {
 	logger.Debugf("searching for agent images with major: %d", majorVersion)
-	imagePath := podcfg.GetJujuOCIImagePath(controllerCfg, version.Zero, 0)
+	imagePath, err := podcfg.GetJujuOCIImagePath(controllerCfg, version.Zero, 0)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
 	availableTags, err := docker.ListOperatorImages(imagePath)
 	if err != nil {
 		return nil, err