Merge pull request juju#17166 from jack-w-shaw/drop_series_from_legac…

…y_upload_handler juju#17166 This is required since old controllers do not allow charm urls to change during migration, and upload charms with a broken up charm url including a series As such, this handler needs to be able to reason with charm urls with series. This is the only place in Juju this is required, so instead of continuing to support series, string-hack the charm url to include a series for compatibility. This can be dropped when we no longer support migration from juju 3.3 This requires treating charm url as a string, and providing the revision separately in `RepackageAndUploadCharm`. This function is also used in `internal/bootstrap/deployer.go` & `apiserver/objects.go, hence the diffs there This code is already well covered by unit tests. All existing unit tests passing without changes, in my opinion, is sufficient to unit test this change. ## Checklist - [x] Code style: imports ordered, good names, simple structure, etc - [x] Comments saying why design decisions were made - [x] Go unit tests, with comments saying what you're testing - [ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing - [ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages ## QA steps Ensure a local charm is available using: ``` $ juju download ubuntu Fetching charm "ubuntu" revision 24 using "stable" channel and base "amd64/ubuntu/22.04" Install the "ubuntu" charm with: juju deploy ./ubuntu_r24.charm ``` ### Model migrations Repeat the following steps, bootstrapping `lxd-src` from: - This PR - 3.5 - 3.3 (Note, you will need to hack 4.0 to support this. Simply apply https://pastebin.canonical.com/p/QF53nbrFcF/) `lxd-dst` is always bootstraped from this PR ``` $ juju switch lxd-src $ juju add-model m $ juju deploy ubuntu ubu-ch $ juju deploy ./ubuntu_r24.charm ubu-local $ juju status Model Controller Cloud/Region Version SLA Timestamp m lxd-src localhost/localhost 3.5-beta1.1 unsupported 19:04:54+01:00 App Version Status Scale Charm Channel Rev Exposed Message ubu-ch 22.04 active 1 ubuntu latest/stable 24 no ubu-local 22.04 active 1 ubuntu 0 no Unit Workload Agent Machine Public address Ports Message ubu-ch/0* active idle 0 10.219.211.13 ubu-local/0* active idle 1 10.219.211.28 Machine State Address Inst id Base AZ Message 0 started 10.219.211.13 juju-f7f82d-0 [email protected] Running 1 started 10.219.211.28 juju-f7f82d-1 [email protected] Running $ juju migrate m lxd-dst $ juju status ERROR Model "admin/m" has been migrated to controller "lxd-dst". To access it run 'juju switch lxd-dst:admin/m`. $ juju switch lxd-dst:m $ juju status Model Controller Cloud/Region Version Timestamp m lxd-dst localhost/localhost 4.0-beta3.1 19:06:01+01:00 App Version Status Scale Charm Channel Rev Exposed Message ubu-ch 22.04 active 1 ubuntu latest/stable 24 no ubu-local 22.04 active 1 ubuntu 0 no Unit Workload Agent Machine Public address Ports Message ubu-ch/0* active idle 0 10.219.211.147 ubu-local/0* active idle 1 10.219.211.60 Machine State Address Inst id Base AZ Message 0 started 10.219.211.147 juju-80c1e5-0 [email protected] Running 1 started 10.219.211.60 juju-80c1e5-1 [email protected] Running ```
nvinuesa · Apr 9, 2024 · a90f0a5 · a90f0a5
2 parents b37ca55 + 0a0ef42
commit a90f0a5
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 48 deletions.
diff --git a/apiserver/charms.go b/apiserver/charms.go
@@ -9,6 +9,7 @@ import (
 	"context"
 	"crypto/sha256"
 	"encoding/hex"
+	"fmt"
 	"io"
 	"mime"
 	"net/http"
@@ -54,6 +55,10 @@ import (
 // pipeline.
 //
 // As usual big methods lead to untestable code and it causes testing pain.
+//
+// TODO(jack-w-shaw): This handler is only used by juju clients/controllers
+// 3.3 and before for both local charm upload and model migration. When we
+// no longer support model migrations from 3.3 we should drop this handler
 type charmsHTTPHandler struct {
 	postHandler endpointMethodHandlerFunc
 	getHandler  endpointMethodHandlerFunc
@@ -126,7 +131,7 @@ func (h *charmsHandler) ServePost(w http.ResponseWriter, r *http.Request) error
 	if err != nil {
 		return errors.NewBadRequest(err, "")
 	}
-	return errors.Trace(sendStatusAndHeadersAndJSON(w, http.StatusOK, map[string]string{"Juju-Curl": charmURL.String()}, &params.CharmsResponse{CharmURL: charmURL.String()}))
+	return errors.Trace(sendStatusAndHeadersAndJSON(w, http.StatusOK, map[string]string{"Juju-Curl": charmURL}, &params.CharmsResponse{CharmURL: charmURL}))
 }
 
 func (h *charmsHandler) ServeGet(w http.ResponseWriter, r *http.Request) error {
@@ -229,7 +234,7 @@ func (h *charmsHandler) archiveSender(w http.ResponseWriter, r *http.Request, bu
 }
 
 // processPost handles a charm upload POST request after authentication.
-func (h *charmsHandler) processPost(r *http.Request, st *state.State) (*charm.URL, error) {
+func (h *charmsHandler) processPost(r *http.Request, st *state.State) (string, error) {
 	query := r.URL.Query()
 	schema := query.Get("schema")
 	if schema == "" {
@@ -241,39 +246,32 @@ func (h *charmsHandler) processPost(r *http.Request, st *state.State) (*charm.UR
 		// There's currently no other time where it makes sense
 		// to accept repository charms through this endpoint.
 		if isImporting, err := modelIsImporting(st); err != nil {
-			return nil, errors.Trace(err)
+			return "", errors.Trace(err)
 		} else if !isImporting {
-			return nil, errors.New("charms may only be uploaded during model migration import")
-		}
-	}
-
-	series := query.Get("series")
-	if series != "" {
-		if err := charm.ValidateSeries(series); err != nil {
-			return nil, errors.NewBadRequest(err, "")
+			return "", errors.New("charms may only be uploaded during model migration import")
 		}
 	}
 
 	// Attempt to get the object store early, so we're not unnecessarily
 	// creating a parsing/reading if we can't get the object store.
 	objectStore, err := h.objectStoreGetter.GetObjectStore(r.Context(), st.ModelUUID())
 	if err != nil {
-		return nil, errors.Trace(err)
+		return "", errors.Trace(err)
 	}
 
 	charmFileName, err := writeCharmToTempFile(r.Body)
 	if err != nil {
-		return nil, errors.Trace(err)
+		return "", errors.Trace(err)
 	}
 	defer os.Remove(charmFileName)
 
 	err = h.processUploadedArchive(charmFileName)
 	if err != nil {
-		return nil, err
+		return "", err
 	}
 	archive, err := charm.ReadCharmArchive(charmFileName)
 	if err != nil {
-		return nil, errors.BadRequestf("invalid charm archive: %v", err)
+		return "", errors.BadRequestf("invalid charm archive: %v", err)
 	}
 
 	// Use the name from the query string. If we're dealing with an older client
@@ -283,50 +281,74 @@ func (h *charmsHandler) processPost(r *http.Request, st *state.State) (*charm.UR
 		name = archive.Meta().Name
 	}
 	if err := charm.ValidateName(name); err != nil {
-		return nil, errors.NewBadRequest(err, "")
+		return "", errors.NewBadRequest(err, "")
 	}
 
-	// We got it, now let's reserve a charm URL for it in state.
-	curl := &charm.URL{
-		Schema:       schema,
-		Architecture: query.Get("arch"),
-		Name:         name,
-		Revision:     archive.Revision(),
-		Series:       series,
+	var revision int
+	if revisionStr := query.Get("revision"); revisionStr != "" {
+		revision, err = strconv.Atoi(revisionStr)
+		if err != nil {
+			return "", errors.NewBadRequest(errors.NewNotValid(err, "revision"), "")
+		}
+	} else {
+		revision = archive.Revision()
 	}
 
+	// We got it, now let's reserve a charm URL for it in state.
+	curlStr := curlString(schema, query.Get("arch"), name, query.Get("series"), revision)
+
 	switch charm.Schema(schema) {
 	case charm.Local:
-		curl, err = st.PrepareLocalCharmUpload(curl.String())
+		curl, err := st.PrepareLocalCharmUpload(curlStr)
 		if err != nil {
-			return nil, errors.Trace(err)
+			return "", errors.Trace(err)
 		}
+		curlStr = curl.String()
 
 	case charm.CharmHub:
-		// If a revision argument is provided, it takes precedence
-		// over the revision in the charm archive. This is required to
-		// handle the revision differences between unpublished and
-		// published charms in the charm store.
-		revisionStr := query.Get("revision")
-		if revisionStr != "" {
-			curl.Revision, err = strconv.Atoi(revisionStr)
-			if err != nil {
-				return nil, errors.NewBadRequest(errors.NewNotValid(err, "revision"), "")
-			}
-		}
-		if _, err := st.PrepareCharmUpload(curl.String()); err != nil {
-			return nil, errors.Trace(err)
+		if _, err := st.PrepareCharmUpload(curlStr); err != nil {
+			return "", errors.Trace(err)
 		}
 
 	default:
-		return nil, errors.Errorf("unsupported schema %q", schema)
+		return "", errors.Errorf("unsupported schema %q", schema)
 	}
 
-	err = RepackageAndUploadCharm(r.Context(), objectStore, storageStateShim{State: st}, archive, curl)
+	err = RepackageAndUploadCharm(r.Context(), objectStore, storageStateShim{State: st}, archive, curlStr, revision)
 	if err != nil {
-		return nil, errors.Trace(err)
+		return "", errors.Trace(err)
+	}
+	return curlStr, nil
+}
+
+// curlString takes the constituent parts of a charm url and renders the url as a string.
+// This is required since, to support migrations from legacy controllers, we need to support
+// charm urls with series since controllers do not allow migrations to mutate charm urls during
+// migration.
+//
+// This is the only place in Juju 4 where series in a charm url needs to be processed. As such,
+// instead of dragging support for series with us into 4.0, in this one place we string-hack the
+// url
+func curlString(schema, arch, name, series string, revision int) string {
+	if series == "" {
+		curl := &charm.URL{
+			Schema:       schema,
+			Architecture: arch,
+			Name:         name,
+			Revision:     revision,
+		}
+		return curl.String()
+	}
+	var curl string
+	if arch == "" {
+		curl = fmt.Sprintf("%s:%s/%s", schema, series, name)
+	} else {
+		curl = fmt.Sprintf("%s:%s/%s/%s", schema, arch, series, name)
+	}
+	if revision != -1 {
+		curl = fmt.Sprintf("%s-%d", curl, revision)
 	}
-	return curl, nil
+	return curl
 }
 
 // processUploadedArchive opens the given charm archive from path,
@@ -427,7 +449,7 @@ type CharmUploader interface {
 // RepackageAndUploadCharm expands the given charm archive to a
 // temporary directory, repackages it with the given curl's revision,
 // then uploads it to storage, and finally updates the state.
-func RepackageAndUploadCharm(ctx context.Context, objectStore services.Storage, uploader CharmUploader, archive *charm.CharmArchive, curl *charm.URL) error {
+func RepackageAndUploadCharm(ctx context.Context, objectStore services.Storage, uploader CharmUploader, archive *charm.CharmArchive, curl string, charmRevision int) error {
 	// Create a temp dir to contain the extracted charm dir.
 	tempDir, err := os.MkdirTemp("", "charm-download")
 	if err != nil {
@@ -436,8 +458,8 @@ func RepackageAndUploadCharm(ctx context.Context, objectStore services.Storage,
 	defer os.RemoveAll(tempDir)
 	extractPath := filepath.Join(tempDir, "extracted")
 
-	// Expand and repack it with the revision specified by curl.
-	archive.SetRevision(curl.Revision)
+	// Expand and repack it with the specified revision
+	archive.SetRevision(charmRevision)
 	if err := archive.ExpandTo(extractPath); err != nil {
 		return errors.Annotate(err, "cannot extract uploaded charm")
 	}
@@ -478,7 +500,7 @@ func RepackageAndUploadCharm(ctx context.Context, objectStore services.Storage,
 		ObjectStore:  objectStore,
 	})
 
-	return charmStorage.Store(ctx, curl.String(), downloader.DownloadedCharm{
+	return charmStorage.Store(ctx, curl, downloader.DownloadedCharm{
 		Charm:        archive,
 		CharmData:    &repackagedArchive,
 		CharmVersion: version,

diff --git a/apiserver/objects.go b/apiserver/objects.go
@@ -213,7 +213,7 @@ func (h *objectsCharmHTTPHandler) processPut(r *http.Request, st *state.State) (
 		return nil, errors.Errorf("unsupported schema %q", schema)
 	}
 
-	return curl, errors.Trace(RepackageAndUploadCharm(r.Context(), objectStore, storageStateShim{State: st}, archive, curl))
+	return curl, errors.Trace(RepackageAndUploadCharm(r.Context(), objectStore, storageStateShim{State: st}, archive, curl.String(), curl.Revision))
 }
 
 func splitNameAndSHAFromQuery(query url.Values) (string, string, error) {

diff --git a/internal/bootstrap/deployer.go b/internal/bootstrap/deployer.go
@@ -442,7 +442,7 @@ func addLocalControllerCharm(ctx context.Context, objectStore services.Storage,
 
 	// Now we need to repackage it with the reserved URL, upload it to
 	// provider storage and update the state.
-	err = apiserver.RepackageAndUploadCharm(ctx, objectStore, uploader, archive, curl)
+	err = apiserver.RepackageAndUploadCharm(ctx, objectStore, uploader, archive, curl.String(), archive.Revision())
 	if err != nil {
 		return nil, errors.Trace(err)
 	}