From 54bc7a30c11cd08db25a6f83a912b7cd447adc01 Mon Sep 17 00:00:00 2001 From: Daniel Shubin Date: Wed, 10 Apr 2024 00:30:52 +0000 Subject: [PATCH] [BACKPORT 2024.1][PLAT-13324] We should not have multiple versions with matching release tags Summary: Original commit: e016db2c9f0a2468a2729aaa78e929862314ec1e / D33932 Making a release version unique except when a tag is present. To support this for internal machines - especially portals, we also have to be able to generate a tag from the file name. This is done by using any characters that are in place of "-b123" in the file name. Add a constraint unique(version, release_tag) on the release table to support this. But, in PG14 we cannot make null 'unique' (its in pg15). Instead, the Release model will encode a null tag to a constant to store in the db, and decode it back to null for any clients. Fixed some bugs with the migration process, especially around checking for helm charts. Test Plan: 1. do not allow multiple releases of the same version without release tags 2. Migration of older "matched" versions works correctly (older = not parsing version_metadata.json) 3. Migration of newer "matched" versions works correctly (parses version_metadata.json) 4. Tag generation works correctly 5. Migration from ReleaseMetadata works correctly 6. null encoding for release tags works correctly Reviewers: muthu, #yba-api-review, anijhawan Reviewed By: muthu, #yba-api-review, anijhawan Subscribers: yugaware, sanketh Tags: #jenkins-ready Differential Revision: https://phorge.dev.yugabyte.com/D33963 --- .../yw/common/ExtraMigrationManager.java | 4 +- .../yugabyte/yw/common/ReleaseManager.java | 25 ++++--- .../com/yugabyte/yw/common/ReleasesUtils.java | 27 +++++-- .../yw/controllers/ReleaseController.java | 5 +- .../yw/controllers/ReleasesController.java | 1 + .../java/com/yugabyte/yw/models/Release.java | 70 +++++++++++++++++-- .../common/R__Release_Metadata_Migration.java | 7 +- .../V341__Release_Version_Tag_Unique.sql | 6 ++ .../com/yugabyte/yw/models/ReleasesTest.java | 17 +++++ 9 files changed, 140 insertions(+), 22 deletions(-) create mode 100644 managed/src/main/resources/db/migration/default_/postgres/V341__Release_Version_Tag_Unique.sql diff --git a/managed/src/main/java/com/yugabyte/yw/common/ExtraMigrationManager.java b/managed/src/main/java/com/yugabyte/yw/common/ExtraMigrationManager.java index 677f567af75c..110a9cde47b4 100644 --- a/managed/src/main/java/com/yugabyte/yw/common/ExtraMigrationManager.java +++ b/managed/src/main/java/com/yugabyte/yw/common/ExtraMigrationManager.java @@ -93,7 +93,9 @@ public void R__Release_Metadata_Migration() { private void createArtifacts(ReleaseMetadata metadata, Release release) { // create helm artifact if necessary - if (metadata.chartPath != null && release.getKubernetesArtifact() == null) { + if (metadata.chartPath != null + && !metadata.chartPath.isEmpty() + && release.getKubernetesArtifact() == null) { ReleaseLocalFile rlf = ReleaseLocalFile.create(metadata.chartPath); release.addArtifact( ReleaseArtifact.create( diff --git a/managed/src/main/java/com/yugabyte/yw/common/ReleaseManager.java b/managed/src/main/java/com/yugabyte/yw/common/ReleaseManager.java index 9c46b106194c..3609681462f5 100644 --- a/managed/src/main/java/com/yugabyte/yw/common/ReleaseManager.java +++ b/managed/src/main/java/com/yugabyte/yw/common/ReleaseManager.java @@ -643,17 +643,21 @@ public synchronized void importLocalReleases() { .forEach( p -> { ReleasesUtils.ExtractedMetadata metadata = null; - releasesUtils.metadataFromPath(p); try { + log.debug("checking local file {}", p.toString()); metadata = releasesUtils.metadataFromPath(p); + String rawVersion = metadata.version.split("-")[0]; if (metadata.platform == ReleaseArtifact.Platform.KUBERNETES) { - localReleaseNameValidation(metadata.version, null, p.toString()); + localReleaseNameValidation(rawVersion, null, p.toString()); } else { - localReleaseNameValidation(metadata.version, p.toString(), null); + localReleaseNameValidation(rawVersion, p.toString(), null); } } catch (RuntimeException e) { log.error( - "local release " + p.getFileName().toString() + " failed validation"); + "local release " + + p.getFileName().toString() + + " failed validation: " + + e.getLocalizedMessage()); // continue forEach return; } @@ -664,13 +668,15 @@ public synchronized void importLocalReleases() { metadata.platform, metadata.architecture, rlf.getFileUUID()); - Release release = Release.getByVersion(metadata.version); + Release release = Release.getByVersion(metadata.version, metadata.releaseTag); if (release == null) { - release = Release.create(metadata.version, metadata.release_type); + release = + Release.create( + metadata.version, metadata.release_type, metadata.releaseTag); } release.addArtifact(artifact); }); - } catch (IOException e) { + } catch (Exception e) { log.error("failed to read local releases", e); } @@ -1152,7 +1158,10 @@ public Map getAllReleaseContainersByVersion() { return Release.getAll().stream() .collect( Collectors.toMap( - release -> release.getVersion(), + release -> + release.getReleaseTag() == null + ? release.getVersion() + : String.format("%s-%s", release.getVersion(), release.getReleaseTag()), release -> releaseContainerFactory.newReleaseContainer(release))); } else { return getReleaseMetadata().entrySet().stream() diff --git a/managed/src/main/java/com/yugabyte/yw/common/ReleasesUtils.java b/managed/src/main/java/com/yugabyte/yw/common/ReleasesUtils.java index d953c9408f84..bb1eeab14005 100644 --- a/managed/src/main/java/com/yugabyte/yw/common/ReleasesUtils.java +++ b/managed/src/main/java/com/yugabyte/yw/common/ReleasesUtils.java @@ -10,6 +10,7 @@ import com.typesafe.config.Config; import com.yugabyte.yw.cloud.PublicCloudConstants.Architecture; import com.yugabyte.yw.common.ReleaseManager.ReleaseMetadata; +import com.yugabyte.yw.common.utils.FileUtils; import com.yugabyte.yw.models.Release; import com.yugabyte.yw.models.ReleaseArtifact; import com.yugabyte.yw.models.ReleaseLocalFile; @@ -52,10 +53,14 @@ public class ReleasesUtils { public final String YB_PACKAGE_REGEX = "yugabyte-(?:ee-)?(.*)-(alma|centos|linux|el8|darwin)(.*).tar.gz"; // We can see helm packages as either yugabyte-.tgz or yugabyte-version-helm.tgz - public final String YB_HELM_PACKAGE_REGEX = "yugabyte-(?:ee-)?(.*?)(?:-helm)?.(?:tar.gz|tgz)"; + public final String YB_HELM_PACKAGE_REGEX = + "yugabyte-(?:ee-)?(?:(?:(.*?)(?:-helm))|(\\d+\\.\\d+\\.\\d+)).(?:tar.gz|tgz)"; // Match release form 2.16.1.2 and return 2.16 or 2024.1.0.0 and return 2024 public final String YB_VERSION_TYPE_REGEX = "(2\\.\\d+|\\d\\d\\d\\d)"; + public final String YB_TAG_REGEX = + "yugabyte-(?:ee-)?(?:.*)-(?!b\\d+)(.*)-(?:alma|centos|linux|el8|darwin)(?:.*).tar.gz"; + // The first version where YBDB must include a metadata.json file public final String YBDB_METADATA_REQUIRED_VERSION = "2024.1.0.0"; @@ -79,6 +84,7 @@ public MetadataParseException(String errMsg) { public static class ExtractedMetadata { public String version; + public String releaseTag; public Release.YbType yb_type; public String sha256; public ReleaseArtifact.Platform platform; @@ -105,6 +111,7 @@ public ExtractedMetadata metadataFromPath(Path releaseFilePath) { versionMetadataFromInputStream( new BufferedInputStream(new FileInputStream(releaseFilePath.toFile()))); em.sha256 = sha256; + em.releaseTag = tagFromName(releaseFilePath.toString()); return em; } catch (MetadataParseException e) { // Fallback to file name validation @@ -128,6 +135,7 @@ public ExtractedMetadata versionMetadataFromURL(URL url) { // Fallback to file name validation log.warn("falling back to file name metadata parsing for url " + url.toString(), e); ExtractedMetadata em = metadataFromName(url.getFile()); + em.releaseTag = tagFromName(url.toString()); return em; } catch (IOException e) { log.error("failed to open url " + url.toString()); @@ -144,7 +152,7 @@ private ExtractedMetadata versionMetadataFromInputStream(InputStream inputStream TarArchiveEntry entry; while ((entry = tarInput.getNextEntry()) != null) { if (entry.getName().endsWith("version_metadata.json")) { - log.debug("found version_metadata.json"); + log.trace("found version_metadata.json"); // We can reasonably assume that the version metadata json is small enough to read in // oneshot byte[] fileContent = new byte[(int) entry.getSize()]; @@ -217,7 +225,7 @@ private ExtractedMetadata versionMetadataFromInputStream(InputStream inputStream return metadata; } } // end of while loop - log.error("No verison_metadata found in given input stream"); + log.error("No version_metadata found in given input stream"); throw new MetadataParseException("no version_metadata found"); } catch (java.io.IOException e) { log.error("failed reading the local file", e); @@ -249,7 +257,7 @@ public void cleanupUntracked() { UUID dirUUID = UUID.fromString(uploadedDir.getName()); if (ReleaseLocalFile.get(dirUUID) == null) { log.debug("deleting untracked local file " + dirUUID); - if (!uploadedDir.delete()) { + if (!FileUtils.deleteDirectory(uploadedDir)) { log.error("failed to delete " + uploadedDir.getAbsolutePath()); } continue; @@ -318,7 +326,7 @@ private ExtractedMetadata metadataFromHelmChart(InputStream helmStream) { TarArchiveEntry entry; while ((entry = tarInput.getNextEntry()) != null) { if (entry.getName().endsWith("Chart.yaml") || entry.getName().endsWith("Chart.yml")) { - log.debug("Found Chart.yml"); + log.trace("Found Chart.yml"); // We can reasonably assume that the version metadata json is small enough to read in // oneshot byte[] fileContent = new byte[(int) entry.getSize()]; @@ -439,4 +447,13 @@ private String ybaCurrentVersion() { } return (String) versionCfg.get("version"); } + + private String tagFromName(String name) { + Pattern tagPattern = Pattern.compile(YB_TAG_REGEX); + Matcher tagMatch = tagPattern.matcher(name); + if (tagMatch.find()) { + return tagMatch.group(1); + } + return null; + } } diff --git a/managed/src/main/java/com/yugabyte/yw/controllers/ReleaseController.java b/managed/src/main/java/com/yugabyte/yw/controllers/ReleaseController.java index 11475070e258..6e824796fdd6 100644 --- a/managed/src/main/java/com/yugabyte/yw/controllers/ReleaseController.java +++ b/managed/src/main/java/com/yugabyte/yw/controllers/ReleaseController.java @@ -233,7 +233,10 @@ public Result list(UUID customerUUID, Boolean includeMetadata, @Nullable String }) .collect( Collectors.toMap( - entry -> entry.getVersion(), + entry -> + entry.getReleaseTag() == null + ? entry.getVersion() + : String.format("%s-%s", entry.getVersion(), entry.getReleaseTag()), entry -> releasesUtils.releaseToReleaseMetadata(entry))); } else { Map releases = releaseManager.getReleaseMetadata(); diff --git a/managed/src/main/java/com/yugabyte/yw/controllers/ReleasesController.java b/managed/src/main/java/com/yugabyte/yw/controllers/ReleasesController.java index c294c106db9f..6aab1e0cb5ba 100644 --- a/managed/src/main/java/com/yugabyte/yw/controllers/ReleasesController.java +++ b/managed/src/main/java/com/yugabyte/yw/controllers/ReleasesController.java @@ -75,6 +75,7 @@ public Result create(UUID customerUUID, Http.Request request) { Customer.getOrBadRequest(customerUUID); CreateRelease reqRelease = formFactory.getFormDataOrBadRequest(request.body().asJson(), CreateRelease.class); + // Validate the version/tag combo doesn't exist if (reqRelease.release_uuid == null) { log.trace("generating random release UUID as one was not provided"); reqRelease.release_uuid = UUID.randomUUID(); diff --git a/managed/src/main/java/com/yugabyte/yw/models/Release.java b/managed/src/main/java/com/yugabyte/yw/models/Release.java index 1bfcf3b570f7..e44844094b57 100644 --- a/managed/src/main/java/com/yugabyte/yw/models/Release.java +++ b/managed/src/main/java/com/yugabyte/yw/models/Release.java @@ -2,6 +2,7 @@ import static play.mvc.Http.Status.BAD_REQUEST; +import autovalue.shaded.com.google.common.annotations.VisibleForTesting; import com.yugabyte.yw.cloud.PublicCloudConstants.Architecture; import com.yugabyte.yw.common.PlatformServiceException; import com.yugabyte.yw.controllers.apiModels.CreateRelease; @@ -30,6 +31,9 @@ @Setter @Slf4j public class Release extends Model { + // on pg14, we can't make a null value unique, so instead use this hard-coded constant + public static final String NULL_CONSTANT = "NULL-VALUE-DO-NOT-USE-AS-INPUT"; + @Id private UUID releaseUUID; @Column(nullable = false) @@ -70,10 +74,25 @@ public static Release create(String version, String releaseType) { return create(UUID.randomUUID(), version, releaseType); } + public static Release create(String version, String releaseType, String releaseTag) { + return create(UUID.randomUUID(), version, releaseType, releaseTag); + } + public static Release create(UUID releaseUUID, String version, String releaseType) { + return create(releaseUUID, version, releaseType, null); + } + + public static Release create( + UUID releaseUUID, String version, String releaseType, String releaseTag) { + if (Release.getByVersion(version, releaseTag) != null) { + throw new PlatformServiceException( + BAD_REQUEST, + String.format("release version %s with tag %s already exists", version, releaseTag)); + } Release release = new Release(); release.releaseUUID = releaseUUID; release.version = version; + release.releaseTag = encodeReleaseTag(releaseTag); release.releaseType = releaseType; release.yb_type = YbType.YBDB; release.state = ReleaseState.INCOMPLETE; @@ -89,6 +108,18 @@ public static Release createFromRequest(CreateRelease reqRelease) { } else { release.releaseUUID = UUID.randomUUID(); } + // Validate the release doesn't already exist - either by uuid or verison/tag combo + if (Release.get(release.releaseUUID) != null) { + throw new PlatformServiceException( + BAD_REQUEST, "release with uuid " + release.releaseUUID + " already exists"); + } + if (Release.getByVersion(reqRelease.version, reqRelease.release_tag) != null) { + throw new PlatformServiceException( + BAD_REQUEST, + String.format( + "release version %s with tag %s already exists", + reqRelease.version, reqRelease.release_tag)); + } // Required fields release.version = reqRelease.version; @@ -96,7 +127,7 @@ public static Release createFromRequest(CreateRelease reqRelease) { release.yb_type = YbType.valueOf(reqRelease.yb_type); // Optional fields - release.releaseTag = reqRelease.release_tag; + release.releaseTag = encodeReleaseTag(reqRelease.release_tag); if (reqRelease.release_date != null) { try { release.releaseDate = Date.from(Instant.ofEpochSecond(reqRelease.release_date)); @@ -138,8 +169,12 @@ public static Release getOrBadRequest(UUID releaseUUID) { } public static Release getByVersion(String version) { - // TODO: Need to map between version and tag. - return find.query().where().eq("version", version).findOne(); + return Release.getByVersion(version, null); + } + + public static Release getByVersion(String version, String tag) { + tag = encodeReleaseTag(tag); + return find.query().where().eq("version", version).eq("release_tag", tag).findOne(); } public void addArtifact(ReleaseArtifact artifact) { @@ -176,10 +211,20 @@ public ReleaseArtifact getKubernetesArtifact() { } public void setReleaseTag(String tag) { - this.releaseTag = tag; + this.releaseTag = encodeReleaseTag(tag); save(); } + public String getReleaseTag() { + return decodeReleaseTag(this.releaseTag); + } + + // Mainly used for test validation, please use `getReleaseTag()` outside of unit tests. + @VisibleForTesting + public String getRawReleaseTag() { + return releaseTag; + } + public void setReleaseDate(Date date) { this.releaseDate = date; save(); @@ -237,4 +282,21 @@ public Set getUniverses() { String formattedVersion = this.version; return Universe.universeDetailsIfReleaseExists(formattedVersion); } + + private static String encodeReleaseTag(String releaseTag) { + if (releaseTag == null) { + return NULL_CONSTANT; + } else if (releaseTag.equals(NULL_CONSTANT)) { + throw new PlatformServiceException( + BAD_REQUEST, "cannot set release tag to " + Release.NULL_CONSTANT); + } + return releaseTag; + } + + private static String decodeReleaseTag(String releaseTag) { + if (releaseTag == null || releaseTag.equals(NULL_CONSTANT)) { + return null; + } + return releaseTag; + } } diff --git a/managed/src/main/java/db/migration/default_/common/R__Release_Metadata_Migration.java b/managed/src/main/java/db/migration/default_/common/R__Release_Metadata_Migration.java index 54e612b3f5e7..6db9c0810dd5 100644 --- a/managed/src/main/java/db/migration/default_/common/R__Release_Metadata_Migration.java +++ b/managed/src/main/java/db/migration/default_/common/R__Release_Metadata_Migration.java @@ -2,6 +2,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import io.ebean.DB; +import io.ebean.SqlRow; import jakarta.persistence.PersistenceException; import java.sql.Connection; import java.sql.SQLException; @@ -25,10 +26,10 @@ public Integer getChecksum() { int codeChecksum = 82918231; // Change me if you want to force migration to run MurmurHash3 murmurHash3 = new MurmurHash3(); try { - String jsonStr = + SqlRow row = DB.sqlQuery("SELECT value FROM yugaware_property WHERE name='SoftwareReleases'") - .findOne() - .getString("value"); + .findOne(); + String jsonStr = row == null ? "" : row.getString("value"); return murmurHash3.stringHash(jsonStr, codeChecksum); } catch (PersistenceException e) { log.warn("failed to query yugaware property: " + e.getLocalizedMessage()); diff --git a/managed/src/main/resources/db/migration/default_/postgres/V341__Release_Version_Tag_Unique.sql b/managed/src/main/resources/db/migration/default_/postgres/V341__Release_Version_Tag_Unique.sql new file mode 100644 index 000000000000..00a56e82ea82 --- /dev/null +++ b/managed/src/main/resources/db/migration/default_/postgres/V341__Release_Version_Tag_Unique.sql @@ -0,0 +1,6 @@ + +UPDATE release + SET release_tag='NULL-VALUE-DO-NOT-USE-AS-INPUT' + WHERE release_tag is null; +ALTER TABLE release + ADD CONSTRAINT version_tag_unique UNIQUE (version, release_tag); \ No newline at end of file diff --git a/managed/src/test/java/com/yugabyte/yw/models/ReleasesTest.java b/managed/src/test/java/com/yugabyte/yw/models/ReleasesTest.java index f292266befe1..9acedffaf69c 100644 --- a/managed/src/test/java/com/yugabyte/yw/models/ReleasesTest.java +++ b/managed/src/test/java/com/yugabyte/yw/models/ReleasesTest.java @@ -1,6 +1,8 @@ package com.yugabyte.yw.models; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; import com.yugabyte.yw.cloud.PublicCloudConstants; import com.yugabyte.yw.cloud.PublicCloudConstants.Architecture; @@ -21,6 +23,12 @@ public void testCreateAndGet() { assertEquals(foundRelease.getReleaseUUID(), createdRelease.getReleaseUUID()); assertEquals(foundRelease.getVersion(), createdRelease.getVersion()); assertEquals(foundRelease.getReleaseType(), createdRelease.getReleaseType()); + + assertThrows( + PlatformServiceException.class, () -> Release.create(UUID.randomUUID(), "1.2.3", "LTS")); + Release release2 = Release.create(UUID.randomUUID(), "1.2.3", "LTS", "tag1"); + Release foundRelease2 = Release.get(release2.getReleaseUUID()); + assertEquals("tag1", foundRelease2.getReleaseTag()); } @Test(expected = PlatformServiceException.class) @@ -97,4 +105,13 @@ public void testAddingK8S() { release.addArtifact(art1); assertEquals(Release.ReleaseState.ACTIVE, release.getState()); } + + @Test + public void testTagRenaming() { + Release release = Release.create("1.2.3.4", "LTS"); + assertNull(release.getReleaseTag()); + assertEquals(Release.NULL_CONSTANT, release.getRawReleaseTag()); + assertThrows( + PlatformServiceException.class, () -> release.setReleaseTag(Release.NULL_CONSTANT)); + } }