From db4121e06396854c1daff999c5f4372c7d5c572d Mon Sep 17 00:00:00 2001 From: Scott Wicken <1562170+swicken@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:05:27 -0400 Subject: [PATCH 1/7] feat(storage): add feature-gated S3 storage layer for shared asset storage First slice of the S3 asset storage work. Adds the FEATURE_FLAG_S3_ASSET_STORAGE startup flag (read once per process), chain-compatible S3, filesystem and database persistence operations, content-addressed S3 blob storage, shared extracted metadata, STS credential support, and SigV4/region/pagination handling for static push publishing. Everything is inert while the flag is off, which is the default. --- docs/testing/BINARY_S3_STORAGE.md | 97 ++++ dotCMS/pom.xml | 4 + .../publishing/storage/AWSS3Storage.java | 106 ++++- .../publishing/storage/Storage.java | 20 + .../AmazonS3StoragePersistenceAPIImpl.java | 413 +++++++++++++++++- .../dotcms/storage/AssetStorageFeature.java | 48 ++ .../ChainableStoragePersistenceAPI.java | 185 +++++++- .../DataBaseStoragePersistenceAPIImpl.java | 19 +- .../com/dotcms/storage/FileStorageAPI.java | 24 + .../dotcms/storage/FileStorageAPIImpl.java | 40 +- .../FileSystemStoragePersistenceAPIImpl.java | 170 ++++++- .../storage/S3ContentAddressedStorage.java | 194 ++++++++ .../storage/SharedExtractedMetadata.java | 51 +++ .../dotcms/storage/StoragePersistenceAPI.java | 40 ++ .../main/java/com/dotcms/tika/TikaUtils.java | 7 + .../java/com/dotmarketing/util/Config.java | 5 + .../resources/dotmarketing-config.properties | 10 + .../storage/AssetStorageFeatureLatchTest.java | 46 ++ .../storage/AssetStorageFeatureTest.java | 242 ++++++++++ .../dotcms/storage/BinaryS3StorageTest.java | 161 +++++++ .../storage/S3StorageConfigurationTest.java | 128 ++++++ 21 files changed, 1974 insertions(+), 36 deletions(-) create mode 100644 docs/testing/BINARY_S3_STORAGE.md create mode 100644 dotCMS/src/main/java/com/dotcms/storage/AssetStorageFeature.java create mode 100644 dotCMS/src/main/java/com/dotcms/storage/S3ContentAddressedStorage.java create mode 100644 dotCMS/src/main/java/com/dotcms/storage/SharedExtractedMetadata.java create mode 100644 dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureLatchTest.java create mode 100644 dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureTest.java create mode 100644 dotCMS/src/test/java/com/dotcms/storage/BinaryS3StorageTest.java create mode 100644 dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java diff --git a/docs/testing/BINARY_S3_STORAGE.md b/docs/testing/BINARY_S3_STORAGE.md new file mode 100644 index 000000000000..f46533c3fad8 --- /dev/null +++ b/docs/testing/BINARY_S3_STORAGE.md @@ -0,0 +1,97 @@ +# S3 asset storage + +S3 asset storage lets dotCMS keep binary assets and their metadata durably in S3, with the +local asset directory acting as a cache. It is opt-in and off by default. With the flag off, +the filesystem/NFS behavior is unchanged. + +This page describes what is in the code today. It grows as the remaining parts land. + +## Feature flag + +Set `FEATURE_FLAG_S3_ASSET_STORAGE=true` (Docker: `DOT_FEATURE_FLAG_S3_ASSET_STORAGE=true`) +in the environment or `dotmarketing-config.properties`, then restart every node. The default +is `false`. + +The flag is read once per process (`AssetStorageFeature.isEnabled()`) and then kept, because +several storage objects capture the mode when they are built. A value set only in the system +table may not be seen at first read, and a runtime change is ignored until restart. In-memory +overrides through `Config.setProperty` do re-read it, which is how tests switch modes. Tests +that mock `Config` statically must call `AssetStorageFeature.reset()` themselves. + +## Storage layer behavior with the flag on + +The storage chain (`ChainableStoragePersistenceAPI`) and its providers change as follows: + +- Writes publish to every durable provider before the new local copy becomes visible, and a + failed remote write keeps the previous local contents. +- Reads restore a missing local copy from S3. A miss is not cached as a 404, because another + node can upload the same key at any time. +- Deletes keep the local copy until all durable providers accept the deletion, so an + in-flight restore on the same chain cannot bring the object back. +- Storage and database query failures propagate instead of being reported as a missing + object. +- Providers can list objects under a prefix, verify that a durable copy has the same + contents (`hasDurableCopy`), and copy an existing file or object to durable storage without + overwriting a conflicting one (`backfillFile`, `backfillObject`). + +Two building blocks are included for the slices that follow. `S3ContentAddressedStorage` +stores one immutable copy of each set of bytes at +`asset-blobs/sha256/////`. +`SharedExtractedMetadata` caches byte-derived Tika extraction at +`extracted-metadata//.json`. Nothing calls either one yet. + +## Credentials and region + +With the flag on, leave both `storage.file-metadata.s3.access-key` and +`storage.file-metadata.s3.secret-access-key` unset to use the AWS SDK default credential +provider chain. The configured bucket region and endpoint are kept in that mode. A partial +key pair fails initialization. The AWS STS module is packaged so role-based providers, +including web identity, work. + +Without a custom endpoint, a configured region selects the regional AWS endpoint, and an +absent region falls back to the SDK region provider chain. A custom endpoint +(`storage.file-metadata.s3.endpoint`) must be an absolute HTTP(S) URL and requires +`storage.file-metadata.s3.bucket-region` for request signing. Invalid endpoint configuration +fails instead of silently selecting AWS. With the flag off, configuration behaves as before. + +## Sharing one bucket across installations + +With the flag on, `storage.file-metadata.s3.namespace` (Docker: +`DOT_STORAGE_FILE_METADATA_S3_NAMESPACE`) separates the keys of installations that share a +bucket. Owned keys become `asset-namespaces///`; shared +extracted metadata stays at the bucket root so identical bytes are extracted once. Use the same +value for all nodes in one cluster and distinct values for independent installations. Values +are 1 to 64 letters, digits, underscores or hyphens, starting with a letter or digit. Empty is +the default and keeps the existing layout. + +Set the namespace before the installation first stores assets. Changing it later does not +migrate existing records. It separates application keys only; it does not restrict what the +bucket credentials can access. + +## Static push publishing + +Enabling the flag also changes static push publishing to S3 (`AWSS3Storage`). Clients sign +with SigV4, resolve the endpoint from the region when no endpoint is configured, and list +every page of objects instead of only the first 1,000. + +## Running the checks + +The S3 checks use the real filesystem provider, chain and AWS adapter against a disposable +MinIO bucket. `BinaryS3StorageTest` is skipped unless `s3.test.endpoint` is set; the other +tests always run. The credentials below are disposable local test values. + +```sh +docker run -d --rm --name binary-s3-test \ + -p 127.0.0.1:19002:9000 \ + -e MINIO_ROOT_USER=binary-storage-test \ + -e MINIO_ROOT_PASSWORD=binary-storage-test \ + minio/minio:latest server /data + +./mvnw test -pl :dotcms-core -Dmaven.build.cache.enabled=false \ + -Dtest=AssetStorageFeatureTest,AssetStorageFeatureLatchTest,S3StorageConfigurationTest,BinaryS3StorageTest \ + -Ds3.test.endpoint=http://127.0.0.1:19002 + +docker stop binary-s3-test +``` + +CI does not yet provide the MinIO service, so `BinaryS3StorageTest` does not run there. diff --git a/dotCMS/pom.xml b/dotCMS/pom.xml index 47ef2d90e573..1798a2727f91 100644 --- a/dotCMS/pom.xml +++ b/dotCMS/pom.xml @@ -763,6 +763,10 @@ com.amazonaws aws-java-sdk-s3 + + com.amazonaws + aws-java-sdk-sts + software.amazon.awssdk sqs diff --git a/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/AWSS3Storage.java b/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/AWSS3Storage.java index 0b8734efd769..2ce26d19cbeb 100644 --- a/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/AWSS3Storage.java +++ b/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/AWSS3Storage.java @@ -29,6 +29,9 @@ import java.io.ByteArrayInputStream; import java.io.File; import java.io.InputStream; +import java.io.IOException; +import java.nio.file.Files; +import java.util.ArrayList; import java.util.List; import java.util.concurrent.Executors; @@ -48,6 +51,11 @@ public AWSS3Storage(DefaultAWSCredentialsProviderChain credentialsProviderChain) this (getAmazonS3Client(credentialsProviderChain , null, DEFAULT_S3_REGION)); } + public AWSS3Storage(final AWSCredentialsProvider credentialsProvider, final String endpoint, + final String region) { + this(getAmazonS3Client(credentialsProvider, endpoint, region)); + } + public AWSS3Storage(final AWSS3Configuration configuration) { this(getAmazonS3Client(configuration.getAccessKey(), configuration.getSecretKey(), configuration.getEndPoint(), configuration.getRegion())); @@ -63,14 +71,18 @@ private static AmazonS3 getAmazonS3Client(final AWSCredentialsProvider credentia final String endPoint, final String region) { - return AmazonS3ClientBuilder.standard() + final AmazonS3ClientBuilder builder = AmazonS3ClientBuilder.standard() .withCredentials(credentialsProvider) - .withClientConfiguration(getClientConfiguration()) - .withEndpointConfiguration( + .withClientConfiguration(getClientConfiguration()); + if (!com.dotcms.storage.AssetStorageFeature.isEnabled() || UtilMethods.isSet(endPoint)) { + builder.withEndpointConfiguration( new AwsClientBuilder.EndpointConfiguration( UtilMethods.isSet(endPoint) ? endPoint : "s3.amazonaws.com", - region)) - .build(); + region)); + } else if (UtilMethods.isSet(region)) { + builder.withRegion(region); + } + return builder.build(); } private AWSS3Storage(AmazonS3 s3client) { @@ -86,7 +98,7 @@ private AWSS3Storage(AmazonS3 s3client) { private static ClientConfiguration getClientConfiguration(){ ClientConfiguration conf = new ClientConfiguration(); - conf.setSignerOverride("S3SignerType"); + conf.setSignerOverride(com.dotcms.storage.AssetStorageFeature.isEnabled() ? "AWSS3V4SignerType" : "S3SignerType"); return conf; } @@ -184,7 +196,17 @@ public MultipleFileUpload uploadFolder(final String bucketName, final String fol public ObjectListing listObjects(String bucketName, String folderPath) throws DotRuntimeException { try { ListObjectsRequest lor = new ListObjectsRequest().withBucketName(bucketName).withPrefix(folderPath); - return s3client.listObjects(lor); + final ObjectListing result = s3client.listObjects(lor); + if (!com.dotcms.storage.AssetStorageFeature.isEnabled()) { + return result; + } + ObjectListing page = result; + while (page.isTruncated()) { + page = s3client.listNextBatchOfObjects(page); + result.getObjectSummaries().addAll(page.getObjectSummaries()); + } + result.setTruncated(false); + return result; } catch (AmazonServiceException ase) { throw new DotRuntimeException("Caught an error from Amazon S3: request made but was rejected", ase); } catch (AmazonClientException ace) { @@ -233,6 +255,76 @@ public Upload uploadFile(final PutObjectRequest putObjectRequest) } } + @Override + public S3Object getObject(final String bucket, final String key) { + try { + return s3client.getObject(bucket, key); + } catch (AmazonS3Exception failure) { + if (failure.getStatusCode() == 404 && "NoSuchKey".equals(failure.getErrorCode())) return null; + throw failure; + } + } + + @Override + public String uploadFileIfMatch(final String bucket, final String key, final File file, final String etag) { + if (!com.dotcms.storage.AssetStorageFeature.isEnabled()) throw new IllegalStateException("S3 asset storage is disabled"); + final var request = new PutObjectRequest(bucket, key, file); + request.putCustomRequestHeader(etag == null ? "If-None-Match" : "If-Match", etag == null ? "*" : "\"" + etag + "\""); + try { + return s3client.putObject(request).getETag(); + } catch (AmazonS3Exception failure) { + if (failure.getStatusCode() == 412 || failure.getStatusCode() == 409 + || (etag != null && failure.getStatusCode() == 404 && "NoSuchKey".equals(failure.getErrorCode()))) return null; + throw failure; + } + } + + /** Creates an object without replacing an existing key, including multipart completion. */ + @Override + public void uploadFileIfAbsent(final String bucket, final String key, final File file) { + if (!com.dotcms.storage.AssetStorageFeature.isEnabled()) { + throw new IllegalStateException("S3 asset storage is disabled"); + } + if (file.length() <= DEFAULT_STATIC_PUSH_MULTIPART_UPLOAD_THRESHOLD) { + final PutObjectRequest request = new PutObjectRequest(bucket, key, file); + request.putCustomRequestHeader("If-None-Match", "*"); + s3client.putObject(request); + return; + } + // The bundled TransferManager does not carry custom headers to multipart completion. + final String uploadId = s3client.initiateMultipartUpload(new InitiateMultipartUploadRequest(bucket, key)).getUploadId(); + try { + final List parts = new ArrayList<>(); + final long length = file.length(); + final long partSize = Math.max(8L * 1024 * 1024, (length + 9999) / 10000); + for (long offset = 0; offset < length; offset += partSize) { + parts.add(s3client.uploadPart(new UploadPartRequest().withBucketName(bucket).withKey(key) + .withUploadId(uploadId).withPartNumber(parts.size() + 1).withFile(file) + .withFileOffset(offset).withPartSize(Math.min(partSize, length - offset))).getPartETag()); + } + final CompleteMultipartUploadRequest request = new CompleteMultipartUploadRequest(bucket, key, uploadId, parts); + request.putCustomRequestHeader("If-None-Match", "*"); + s3client.completeMultipartUpload(request); + } catch (RuntimeException failure) { + try { + s3client.abortMultipartUpload(new AbortMultipartUploadRequest(bucket, key, uploadId)); + } catch (RuntimeException abortFailure) { + failure.addSuppressed(abortFailure); + } + throw failure; + } + } + + /** Verifies opaque/encrypted/multipart ETags by reading the actual object bytes. */ + @Override + public boolean fileContentsMatch(final String bucket, final String key, final File file) throws IOException { + try (final S3Object object = s3client.getObject(bucket, key); + final InputStream local = Files.newInputStream(file.toPath())) { + return object.getObjectMetadata().getContentLength() == file.length() + && org.apache.commons.io.IOUtils.contentEquals(object.getObjectContent(), local); + } + } + @Override public Download downloadFile(final String bucketName, final String filePath, File file) throws DotRuntimeException { diff --git a/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/Storage.java b/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/Storage.java index c34cf97ce572..3aa911badf9d 100644 --- a/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/Storage.java +++ b/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/Storage.java @@ -91,6 +91,26 @@ MultipleFileUpload uploadFolder(final String bucketName, final String folderPath */ Upload uploadFile(final PutObjectRequest putObjectRequest) throws DotRuntimeException; + /** Creates an object without replacing an existing key. Unsupported implementations must fail. */ + default void uploadFileIfAbsent(final String bucketName, final String key, final File file) { + throw new UnsupportedOperationException("Conditional uploads are not supported"); + } + + /** Reads bytes and their version from the same S3 response. Caller closes the response. */ + default com.amazonaws.services.s3.model.S3Object getObject(final String bucket, final String key) { + throw new UnsupportedOperationException("Versioned reads are not supported"); + } + + /** Writes a small record only if its ETag still matches; null expects absence. */ + default String uploadFileIfMatch(final String bucket, final String key, final File file, final String etag) { + throw new UnsupportedOperationException("Conditional writes are not supported"); + } + + /** Compares object bytes when an ETag cannot verify file contents. */ + default boolean fileContentsMatch(final String bucketName, final String key, final File file) throws java.io.IOException { + return false; + } + /** * Async method: Schedules a new transfer to download data from Amazon S3 and save it to the specified file. */ diff --git a/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java b/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java index c85092c29e39..5f9145d80b10 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java @@ -2,6 +2,8 @@ import com.amazonaws.auth.DefaultAWSCredentialsProviderChain; import com.amazonaws.services.s3.model.Bucket; +import com.amazonaws.services.s3.model.PutObjectRequest; +import org.apache.commons.codec.digest.DigestUtils; import com.amazonaws.services.s3.transfer.Download; import com.amazonaws.services.s3.transfer.Transfer; import com.amazonaws.services.s3.transfer.Upload; @@ -47,8 +49,8 @@ import java.util.function.Function; import java.util.stream.Collectors; -import static com.liferay.util.StringPool.BLANK; import static com.liferay.util.StringPool.FORWARD_SLASH; +import static com.liferay.util.StringPool.BLANK; /** * Provides a Metadata Provider implementation that uses AWS S3 to persist the metadata files. @@ -71,6 +73,8 @@ enum PathEncryptionMode { } private final Storage storage; + private final String namespace = configuredNamespace(); + private final FileRepositoryManager fileRepositoryManager = this.getFileRepository(); private MessageDigest sha256; private final Set groups = ConcurrentHashMap.newKeySet(); @@ -84,13 +88,29 @@ enum PathEncryptionMode { public static final String AWS_S3_ACCESS_KEY_PROP = "storage.file-metadata.s3.access-key"; public static final String AWS_S3_SECRET_ACCESS_KEY_PROP = "storage.file-metadata.s3.secret-access-key"; public static final String AWS_S3_PATH_ENCRYPTION_MODE_PROP = "storage.file-metadata.s3.path-encryption-mode"; + public static final String AWS_S3_ENDPOINT_PROP = "storage.file-metadata.s3.endpoint"; + public static final String AWS_S3_NAMESPACE_PROP = "storage.file-metadata.s3.namespace"; + + private static String configuredNamespace() { + if (!AssetStorageFeature.isEnabled()) return BLANK; + final String value = Config.getStringProperty(AWS_S3_NAMESPACE_PROP, BLANK); + if (!value.isEmpty() && !value.matches("[A-Za-z0-9][A-Za-z0-9_-]{0,63}")) { + throw new DotRuntimeException("S3 namespace must be empty or 1-64 letters, digits, underscores or hyphens, starting with a letter or digit"); + } + return value; + } + + private String groupKey(final String group) { + return !AssetStorageFeature.isEnabled() || namespace.isEmpty() || SharedExtractedMetadata.GROUP.equals(group) + ? group : "asset-namespaces/" + namespace + FORWARD_SLASH + group; + } + + private static final String METADATA_GROUP_NAME = Config.getStringProperty( + StoragePersistenceProvider.METADATA_GROUP_NAME, FileMetadataAPI.DOT_METADATA); private static final String S3_BASE_STORAGE_FILE_REPO_TYPE = Config.getStringProperty( "S3_STORAGE_FILE_REPO_TYPE", FileRepositoryManager.TEMP_REPO).toUpperCase(); - private static final String METADATA_GROUP_NAME = Config - .getStringProperty(StoragePersistenceProvider.METADATA_GROUP_NAME, FileMetadataAPI.DOT_METADATA); - /** * Returns the file repository manager based on the configuration. * @@ -116,19 +136,64 @@ public AmazonS3StoragePersistenceAPIImpl() { this.pathEncryptionMode = PathEncryptionMode.valueOf(Config.getStringProperty(AWS_S3_PATH_ENCRYPTION_MODE_PROP, PathEncryptionMode.SHA256.name())); this.sha256 = Try.of(() -> MessageDigest.getInstance(Encryptor.SHA256_ALGORITHM)).getOrElseThrow(e -> new DotRuntimeException(e.getMessage(), e)); + final String endpoint = AssetStorageFeature.isEnabled() ? Config.getStringProperty(AWS_S3_ENDPOINT_PROP, null) : null; + if (AssetStorageFeature.isEnabled() + && UtilMethods.isSet(accessKey) != UtilMethods.isSet(secretAccessKey)) { + throw new DotRuntimeException("Configure both S3 access key and secret key, or leave both unset to use the AWS credential provider chain"); + } + if (AssetStorageFeature.isEnabled() && UtilMethods.isSet(endpoint)) { + try { + final var uri = java.net.URI.create(endpoint); + if (uri.getHost() == null || !("http".equalsIgnoreCase(uri.getScheme()) + || "https".equalsIgnoreCase(uri.getScheme()))) { + throw new IllegalArgumentException("Expected an absolute HTTP(S) URL"); + } + } catch (IllegalArgumentException invalid) { + throw new DotRuntimeException("Invalid S3 endpoint: configure an absolute HTTP(S) URL", invalid); + } + if (!UtilMethods.isSet(region)) { + throw new DotRuntimeException("A custom S3 endpoint requires storage.file-metadata.s3.bucket-region for request signing"); + } + } this.storage = !UtilMethods.isSet(accessKey) || !UtilMethods.isSet(secretAccessKey) ? - new AWSS3Storage(new DefaultAWSCredentialsProviderChain()) : - new AWSS3Storage(new AWSS3Configuration.Builder().accessKey(accessKey).secretKey(secretAccessKey).endPoint(null).region(region).build()); + (AssetStorageFeature.isEnabled() + ? new AWSS3Storage(new DefaultAWSCredentialsProviderChain(), endpoint, region) + : new AWSS3Storage(new DefaultAWSCredentialsProviderChain())) : + new AWSS3Storage(new AWSS3Configuration.Builder().accessKey(accessKey).secretKey(secretAccessKey).endPoint(endpoint).region(region).build()); } @SuppressWarnings("unused") public AmazonS3StoragePersistenceAPIImpl(final Storage storage) { + this(storage, Config.getStringProperty(AWS_S3_BUCKET_NAME_PROP, null), PathEncryptionMode.NONE); + } + + AmazonS3StoragePersistenceAPIImpl(final Storage storage, final String bucketName, + final PathEncryptionMode pathEncryptionMode) { this.storage = storage; + this.bucketName = bucketName; + this.pathEncryptionMode = pathEncryptionMode; + this.lockManager = DotConcurrentFactory.getInstance().getIdentifierStripedLock(); + this.sha256 = Try.of(() -> MessageDigest.getInstance(Encryptor.SHA256_ALGORITHM)) + .getOrElseThrow(DotRuntimeException::new); + } + + /** + * Creates an S3 provider with {@link PathEncryptionMode#NONE} — preserves the directory + * structure in S3 keys instead of hashing paths with SHA-256. Required for binary asset + * storage where prefix-based listing must work (SHA-256 hashing breaks prefix semantics). + * + * @return a new S3 provider configured with plain (unhashed) paths + */ + public static AmazonS3StoragePersistenceAPIImpl withPlainPaths() { + final AmazonS3StoragePersistenceAPIImpl impl = new AmazonS3StoragePersistenceAPIImpl(); + impl.pathEncryptionMode = PathEncryptionMode.NONE; + return impl; } @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public boolean existsGroup(final String groupName) throws DotDataException { + if (!AssetStorageFeature.isEnabled()) { if (this.groups.contains(groupName)) { return true; } @@ -144,12 +209,42 @@ public boolean existsGroup(final String groupName) throws DotDataException { } this.groups.add(groupName); return objectExists; + + } + + if (this.groups.contains(groupName)) { + return true; + } + boolean objectExists = this.storage.existsBucket(this.bucketName); + if (objectExists) { + objectExists = !this.storage.listObjects(this.bucketName, + groupKey(groupName) + FORWARD_SLASH).getObjectSummaries().isEmpty(); + if (!objectExists) { + Logger.debug(this, () -> String.format("Group '%s' does not exist", groupName)); + } + } else { + Logger.debug(this, () -> String.format("Bucket '%s' does not exist", this.bucketName)); + } + if (objectExists) { + this.groups.add(groupName); + } + return objectExists; } @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public boolean existsObject(final String groupName, final String objectPath) throws DotDataException { final String correctedPath = transformReadPath(groupName, objectPath); + if (AssetStorageFeature.isEnabled()) { + try { + final var objects = this.storage.listObjects(this.bucketName, correctedPath); + final String directory = correctedPath.endsWith(FORWARD_SLASH) ? correctedPath : correctedPath + FORWARD_SLASH; + return objects != null && objects.getObjectSummaries().stream().anyMatch(object -> + object.getKey().equals(correctedPath) || object.getKey().startsWith(directory)); + } catch (RuntimeException e) { + throw new DotDataException("Unable to check S3 object " + correctedPath, e); + } + } final boolean exists = this.storage.existsBucket(this.bucketName) && !this.storage.listObjects(this.bucketName, correctedPath).getObjectSummaries().isEmpty(); Logger.debug(this, () -> String.format("Object '%s' in group '%s' exists? %s", correctedPath, groupName, exists)); @@ -161,7 +256,7 @@ public boolean existsObject(final String groupName, final String objectPath) thr public boolean createGroup(final String groupName) throws DotDataException { if (!this.existsGroup(groupName)) { Logger.debug(this, () -> String.format("Creating group with name '%s'", groupName)); - this.storage.createFolder(this.bucketName, groupName); + this.storage.createFolder(this.bucketName, groupKey(groupName)); } this.groups.add(groupName); return true; @@ -177,6 +272,17 @@ public boolean createGroup(final String groupName, final Map ext @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public int deleteGroup(final String groupName) throws DotDataException { + if (AssetStorageFeature.isEnabled()) { + try { + for (final var object : storage.listObjects(bucketName, groupKey(groupName) + FORWARD_SLASH).getObjectSummaries()) { + storage.deleteFile(bucketName, object.getKey()); + } + groups.remove(groupName); + return 0; + } catch (RuntimeException failure) { + throw new DotDataException("Unable to delete S3 group " + groupName, failure); + } + } this.storage.deleteFolder(this.bucketName, groupName); return 0; } @@ -184,14 +290,25 @@ public int deleteGroup(final String groupName) throws DotDataException { @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public boolean deleteObjectAndReferences(final String groupName, final String path) throws DotDataException { + if (!AssetStorageFeature.isEnabled()) { this.storage.deleteFile(this.bucketName, groupName + path); return true; + + } + + this.storage.deleteFile(this.bucketName, transformReadPath(groupName, path)); + return true; } @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public boolean deleteObjectReference(final String groupName, final String path) throws DotDataException { + if (!AssetStorageFeature.isEnabled()) { return this.deleteObjectAndReferences(this.bucketName, groupName + path); + + } + + return this.deleteObjectAndReferences(groupName, path); } @Override @@ -200,10 +317,36 @@ public List listGroups() { return this.storage.listBuckets().stream().map(Bucket::getName).collect(Collectors.toList()); } + @Override + @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) + public List listObjectPaths(final String groupName, + final String pathPrefix) throws DotDataException { + + final String prefix = transformReadPath(groupName, pathPrefix); + final String s3Prefix = prefix.endsWith(FORWARD_SLASH) ? prefix : prefix + FORWARD_SLASH; + final com.amazonaws.services.s3.model.ObjectListing listing = + this.storage.listObjects(this.bucketName, s3Prefix); + + if (listing == null || listing.getObjectSummaries().isEmpty()) { + return List.of(); + } + + final String groupPrefix = groupKey(groupName) + FORWARD_SLASH; + return listing.getObjectSummaries().stream() + .map(com.amazonaws.services.s3.model.S3ObjectSummary::getKey) + .filter(key -> key.startsWith(groupPrefix) && !key.endsWith(FORWARD_SLASH)) + .map(key -> AssetStorageFeature.isEnabled() && pathEncryptionMode == PathEncryptionMode.SHA256 + && pathPrefix.endsWith(FORWARD_SLASH) + ? pathPrefix + key.substring(s3Prefix.length()) + : key.substring(groupPrefix.length())) + .collect(Collectors.toList()); + } + @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public Object pushFile(final String groupName, final String path, final File file, final Map extraMeta) throws DotDataException { + if (!AssetStorageFeature.isEnabled()) { final String pathForS3 = transformWritePath(groupName, path, file.getName()); final Upload upload = this.storage.uploadFile(this.bucketName, pathForS3, file); @@ -225,13 +368,38 @@ public Object pushFile(final String groupName, final String path, final File fil throw new DotRuntimeException(String.format("Failed to push file '%s' to S3 group " + "'%s': %s", path, groupName, ExceptionUtil.getErrorMessage(e)), e); } + + } + + final String pathForS3 = transformReadPath(groupName, path); + final Upload upload = this.storage.uploadFile(new PutObjectRequest(this.bucketName, pathForS3, file)); + try { + return lockManager.tryLock("s3_" + groupName + path, () -> { + + Logger.debug(this, () -> String.format("Pushing file '%s' to group '%s' with " + + "path '%s' [ %s ]", file.getName(), groupName, pathForS3, path)); + final UploadResult result = + Try.of(upload::waitForUploadResult).getOrElseThrow(e -> new DotDataException(e.getMessage(), e)); + Logger.debug(this, () -> String.format("File '%s' in group '%s' with path '%s' " + + "[ %s ] was pushed successfully!", file.getName(), groupName, + pathForS3, path)); + return result.getETag(); + + } + ); + } catch (final Throwable e) { + throw new DotRuntimeException(String.format("Failed to push file '%s' to S3 group " + + "'%s': %s", path, groupName, ExceptionUtil.getErrorMessage(e)), e); + } } @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public Object pushObject(final String groupName, final String path, final ObjectWriterDelegate writerDelegate, final Serializable object, final Map extraMeta) throws DotDataException { - final File file = new File(ConfigUtils.getAssetTempPath() + path); + final File file = AssetStorageFeature.isEnabled() + ? Try.of(() -> Files.createTempFile("s3-metadata-", ".tmp").toFile()).getOrElseThrow(DotDataException::new) + : new File(ConfigUtils.getAssetTempPath() + path); try { this.createTempFile(writerDelegate, object, file); return this.pushFile(groupName, path, file, extraMeta); @@ -272,17 +440,34 @@ private void createTempFile(final ObjectWriterDelegate writerDelegate, @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public Future pushFileAsync(final String groupName, final String path, final File file, final Map extraMeta) { + if (AssetStorageFeature.isEnabled()) { + return DotConcurrentFactory.getInstance().getSubmitter(STORAGE_POOL).submit(() -> this.pushFile(groupName, path, file, extraMeta)); + } + + if (!AssetStorageFeature.isEnabled()) { final String pathForS3 = transformWritePath(groupName, path, file.getName()); Logger.debug(this, () -> String.format("Async pushing file '%s' to group '%s' with path " + "'%s' [ %s ]", file.getName(), groupName, pathForS3, path)); final Upload upload = this.storage.uploadFile(this.bucketName, pathForS3, file); return new UploadFuture<>(upload, file); + + } + + final String pathForS3 = transformReadPath(groupName, path); + Logger.debug(this, () -> String.format("Async pushing file '%s' to group '%s' with path " + + "'%s' [ %s ]", file.getName(), groupName, pathForS3, path)); + final Upload upload = this.storage.uploadFile(new PutObjectRequest(this.bucketName, pathForS3, file)); + return new UploadFuture<>(upload, file); } @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public Future pushObjectAsync(final String bucketName, final String path, final ObjectWriterDelegate writerDelegate, final Serializable object, final Map extraMeta) { + if (AssetStorageFeature.isEnabled()) { + return DotConcurrentFactory.getInstance().getSubmitter(STORAGE_POOL).submit(() -> this.pushObject(bucketName, path, writerDelegate, object, extraMeta)); + } + final File file = new File(ConfigUtils.getAssetPath() + path); return this.pushFileAsync(bucketName, path, file, extraMeta); } @@ -290,6 +475,32 @@ public Future pushObjectAsync(final String bucketName, final String path @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public File pullFile(final String groupName, final String path) throws DotDataException { + if (AssetStorageFeature.isEnabled()) { + final File download = fileRepositoryManager.getOrCreateFile(path); + if (download == null) { + throw new DotDataException("Unable to allocate an S3 download file"); + } + try { + storage.downloadFile(bucketName, transformReadPath(groupName, path), download).waitForCompletion(); + return download; + } catch (Exception e) { + releaseRetrievedFile(download); + for (Throwable cause = e; cause != null; cause = cause.getCause()) { + if (cause instanceof com.amazonaws.services.s3.model.AmazonS3Exception + && ((com.amazonaws.services.s3.model.AmazonS3Exception) cause).getStatusCode() == 404) { + final String code = ((com.amazonaws.services.s3.model.AmazonS3Exception) cause).getErrorCode(); + if ("NoSuchKey".equals(code) + || (!"NoSuchBucket".equals(code) && storage.existsBucket(bucketName))) { + return null; + } + } + } + if (e instanceof InterruptedException) { + Thread.currentThread().interrupt(); + } + throw new DotDataException("Unable to retrieve S3 object " + groupName + "/" + path, e); + } + } final File file = fileRepositoryManager.getOrCreateFile(path); final String pathForS3 = transformReadPath(groupName, path); Logger.debug(this, () -> String.format("Pulling file '%s' from group '%s' with path '%s' " + @@ -304,6 +515,19 @@ public File pullFile(final String groupName, final String path) throws DotDataEx @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public Object pullObject(final String groupName, final String path, final ObjectReaderDelegate readerDelegate) throws DotDataException { + if (AssetStorageFeature.isEnabled()) { + final File file = pullFile(groupName, path); + if (file == null) { + return null; + } + try (InputStream input = Files.newInputStream(file.toPath())) { + return readerDelegate.read(input); + } catch (IOException e) { + throw new DotDataException("Unable to read downloaded object " + path, e); + } finally { + releaseRetrievedFile(file); + } + } Object object = null; final File file = pullFile(groupName, path); @@ -318,9 +542,71 @@ public Object pullObject(final String groupName, final String path, final Object return object; } + /** Value and version are from one response; modified is the S3 server timestamp. */ + public record ObjectSnapshot(String path, Object value, String version, long modified) { } + + public ObjectSnapshot readObjectSnapshot(final String group, final String path, + final ObjectReaderDelegate reader) throws DotDataException { + if (!AssetStorageFeature.isEnabled()) throw new DotDataException("S3 asset storage is disabled"); + try (var object = storage.getObject(bucketName, transformReadPath(group, path))) { + if (object == null) return null; + return new ObjectSnapshot(path, reader.read(object.getObjectContent()), + object.getObjectMetadata().getETag(), object.getObjectMetadata().getLastModified().getTime()); + } catch (Exception failure) { + throw new DotDataException("Unable to read S3 object version " + path, failure); + } + } + + public List listObjectSnapshots(final String group, final String prefix) throws DotDataException { + if (!AssetStorageFeature.isEnabled() || pathEncryptionMode != PathEncryptionMode.NONE) { + throw new DotDataException("Version listing requires enabled S3 plain paths"); + } + final String start = transformReadPath(group, prefix); + try { + return storage.listObjects(bucketName, start.endsWith("/") ? start : start + "/") + .getObjectSummaries().stream().filter(object -> !object.getKey().endsWith("/")) + .map(object -> new ObjectSnapshot(object.getKey().substring(groupKey(group).length() + 1), null, + object.getETag(), object.getLastModified().getTime())).toList(); + } catch (RuntimeException failure) { + throw new DotDataException("Unable to list S3 object versions " + prefix, failure); + } + } + + /** Returns the new version, or null on a concurrent change; never retries a stale write. */ + public String writeObjectIfMatch(final String group, final String path, final Serializable value, + final String version) throws DotDataException { + if (!AssetStorageFeature.isEnabled()) throw new DotDataException("S3 asset storage is disabled"); + try { + final var stage = Files.createTempFile("s3-record-", ".json"); + try { + writeToFile(new JsonWriterDelegate(), value, stage.toFile()); + return storage.uploadFileIfMatch(bucketName, transformReadPath(group, path), stage.toFile(), version); + } finally { + Files.deleteIfExists(stage); + } + } catch (Exception failure) { + throw new DotDataException("Unable to conditionally write S3 record " + path, failure); + } + } + + @Override + public void releaseRetrievedFile(final File file) throws DotDataException { + if (AssetStorageFeature.isEnabled() && fileRepositoryManager instanceof com.dotcms.storage.repository.TempFileRepositoryManager) { + try { + Files.deleteIfExists(file.toPath()); + } catch (IOException e) { + throw new DotDataException("Unable to remove S3 download staging file " + file, e); + } + } + } + @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public Future pullFileAsync(final String groupName, final String path) { + if (AssetStorageFeature.isEnabled()) { + return DotConcurrentFactory.getInstance().getSubmitter(STORAGE_POOL).submit(() -> this.pullFile(groupName, path)); + } + final File file = fileRepositoryManager.getOrCreateFile(path); final String pathForS3 = transformReadPath(groupName, path); Logger.debug(this, () -> String.format("Async pulling file '%s' from group '%s' with path '%s' [ %s ]", file.getName(), groupName, pathForS3, path)); @@ -331,6 +617,10 @@ public Future pullFileAsync(final String groupName, final String path) { @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public Future pullObjectAsync(final String groupName, final String path, final ObjectReaderDelegate readerDelegate) { + if (AssetStorageFeature.isEnabled()) { + return DotConcurrentFactory.getInstance().getSubmitter(STORAGE_POOL).submit(() -> this.pullObject(groupName, path, readerDelegate)); + } + final File file = fileRepositoryManager.getOrCreateFile(path); final Download download = this.storage.downloadFile(groupName, path, file); final Function toObjectFunction = aFile -> { @@ -367,6 +657,7 @@ public Future pullObjectAsync(final String groupName, final String path, * @return The transformed read path. */ private String transformReadPath(final String groupName, final String path) { + if (!AssetStorageFeature.isEnabled()) { String s3Path; final String fileName = path.lastIndexOf(FORWARD_SLASH) > 0 ? path.substring(path.lastIndexOf(FORWARD_SLASH) + 1) : path; @@ -378,6 +669,20 @@ private String transformReadPath(final String groupName, final String path) { s3Path = groupName + FORWARD_SLASH + correctedPath + fileName; } return s3Path; + + } + + final String normalized = path.startsWith(FORWARD_SLASH) ? path.substring(1) : path; + if (this.pathEncryptionMode != PathEncryptionMode.SHA256) { + return groupKey(groupName) + FORWARD_SLASH + normalized; + } + final int slash = normalized.lastIndexOf(FORWARD_SLASH); + final String directory = normalized.substring(0, slash + 1); + final String fileName = normalized.substring(slash + 1); + final String hash = EncryptorFactory.getInstance().getEncryptor() + .encryptString(directory, AssetStorageFeature.isEnabled() + ? newSha256() : this.sha256); + return groupKey(groupName) + FORWARD_SLASH + hash + FORWARD_SLASH + fileName; } /** @@ -415,6 +720,98 @@ private String transformWritePath(final String groupName, final String path, return s3Path; } + private static MessageDigest newSha256() { + return Try.of(() -> MessageDigest.getInstance(Encryptor.SHA256_ALGORITHM)) + .getOrElseThrow(DotRuntimeException::new); + } + + @Override + @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) + public boolean hasDurableCopy(final String groupName, final String path, + final File file) throws DotDataException { + if (!AssetStorageFeature.isEnabled()) { + return false; + } + final String key = transformReadPath(groupName, path); + try (final InputStream input = Files.newInputStream(file.toPath())) { + final String md5 = DigestUtils.md5Hex(input); + for (final var object : storage.listObjects(bucketName, key).getObjectSummaries()) { + if (key.equals(object.getKey()) && object.getSize() == file.length()) { + return md5.equalsIgnoreCase(object.getETag()) || storage.fileContentsMatch(bucketName, key, file); + } + } + return false; + } catch (final IOException e) { + throw new DotDataException("Failed to verify durable copy of " + path, e); + } + } + + @Override + public boolean backfillFile(final String groupName, final String path, final File file) throws DotDataException { + if (!AssetStorageFeature.isEnabled()) { + return false; + } + if (!Files.isRegularFile(file.toPath(), java.nio.file.LinkOption.NOFOLLOW_LINKS)) { + throw new DotDataException("Backfill requires a regular source file: " + file); + } + try { + if (hasDurableCopy(groupName, path, file)) { + return true; + } + try { + storage.uploadFileIfAbsent(bucketName, transformReadPath(groupName, path), file); + } catch (com.amazonaws.services.s3.model.AmazonS3Exception conflict) { + if (conflict.getStatusCode() != 412) { + throw conflict; + } + } + if (!hasDurableCopy(groupName, path, file)) { + throw new DotDataException("S3 backfill conflicts with the existing object: " + groupName + "/" + path); + } + return true; + } catch (RuntimeException failure) { + throw new DotDataException("Unable to backfill S3 object: " + groupName + "/" + path, failure); + } + } + + @Override + public boolean backfillObject(final String groupName, final String path, final ObjectWriterDelegate writer, + final ObjectReaderDelegate reader, final Serializable object) throws DotDataException { + if (!AssetStorageFeature.isEnabled()) { + return false; + } + final File file = Try.of(() -> Files.createTempFile("s3-backfill-", ".tmp").toFile()) + .getOrElseThrow(DotDataException::new); + try { + createTempFile(writer, object, file); + final Object expected; + try (final InputStream input = Files.newInputStream(file.toPath())) { + expected = reader.read(input); + } + if (expected == null) { + throw new DotDataException("Cannot backfill a null metadata object: " + path); + } + if (java.util.Objects.equals(expected, pullObject(groupName, path, reader))) { + return true; + } + try { + storage.uploadFileIfAbsent(bucketName, transformReadPath(groupName, path), file); + } catch (com.amazonaws.services.s3.model.AmazonS3Exception conflict) { + if (conflict.getStatusCode() != 412) { + throw conflict; + } + } + if (!java.util.Objects.equals(expected, pullObject(groupName, path, reader))) { + throw new DotDataException("S3 backfill conflicts with the existing metadata: " + path); + } + return true; + } catch (IOException | RuntimeException failure) { + throw new DotDataException("Unable to backfill metadata: " + path, failure); + } finally { + file.delete(); + } + } + /** * This is a special version of the {@link Future} class that allows you to asynchronously * upload a file to S3 and get the ETag of the uploaded file as a result. diff --git a/dotCMS/src/main/java/com/dotcms/storage/AssetStorageFeature.java b/dotCMS/src/main/java/com/dotcms/storage/AssetStorageFeature.java new file mode 100644 index 000000000000..01fcd69b9b98 --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/storage/AssetStorageFeature.java @@ -0,0 +1,48 @@ +package com.dotcms.storage; + +import com.dotmarketing.util.Config; +import com.dotmarketing.util.Logger; + +/** + * Startup configuration for the opt-in S3 asset lifecycle. Disabled preserves filesystem/NFS behavior. + * + *

The flag is read once per process and then kept. Several storage objects capture the mode when + * they are built, so changing it on a running node (system table, properties reload) would leave that + * node half-enabled. Set it through the environment or properties file and restart.

+ */ +public final class AssetStorageFeature { + public static final String FLAG = "FEATURE_FLAG_S3_ASSET_STORAGE"; + + private static volatile Boolean enabled; + + private AssetStorageFeature() { } + + /** + * Returns whether the S3 asset lifecycle is enabled for this process. The first call reads the + * configuration; later calls return that value. + * + * @return {@code true} when the feature flag was on at first read + */ + public static boolean isEnabled() { + Boolean value = enabled; + if (value == null) { + synchronized (AssetStorageFeature.class) { + if (enabled == null) { + enabled = Config.getBooleanProperty(FLAG, false); + Logger.info(AssetStorageFeature.class, "S3 asset storage " + (enabled ? "enabled" : "disabled")); + } + value = enabled; + } + } + return value; + } + + /** + * Forgets the value read at first use, so the next {@link #isEnabled()} reads the configuration + * again. Called by {@link Config#setProperty(String, Object)} for in-memory overrides of the flag, + * which tests use to switch modes. + */ + public static void reset() { + enabled = null; + } +} diff --git a/dotCMS/src/main/java/com/dotcms/storage/ChainableStoragePersistenceAPI.java b/dotCMS/src/main/java/com/dotcms/storage/ChainableStoragePersistenceAPI.java index 6fdc21038759..475fec497656 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/ChainableStoragePersistenceAPI.java +++ b/dotCMS/src/main/java/com/dotcms/storage/ChainableStoragePersistenceAPI.java @@ -34,6 +34,8 @@ public class ChainableStoragePersistenceAPI implements StoragePersistenceAPI { private final List storagePersistenceAPIList; private final ObjectWriterDelegate defaultWriterDelegate; private final Chainable404StorageCache cache; + private final com.google.common.util.concurrent.Striped assetLocks = + com.google.common.util.concurrent.Striped.lock(256); private static final String SUBMITTER_NAME = Config.getStringProperty("COMPOSITE_STORAGE_SUBMITTER_NAME", "SubmitterCompositeStoragePersistenceAPI"); @@ -67,6 +69,18 @@ public boolean existsGroup(final String groupName) { @Override public boolean existsObject(final String groupName, final String objectPath) { + if (AssetStorageFeature.isEnabled()) { + for (StoragePersistenceAPI storage : storagePersistenceAPIList) { + try { + if (storage.existsObject(groupName, objectPath)) { + return true; + } + } catch (DotDataException e) { + throw new DotRuntimeException("Unable to check stored object " + objectPath, e); + } + } + return false; + } if (this.cache.is404(groupName, objectPath)) { return false; } @@ -125,6 +139,22 @@ public int deleteGroup(final String groupName) throws DotDataException { @Override public boolean deleteObjectAndReferences(final String groupName, final String path) throws DotDataException { + if (AssetStorageFeature.isEnabled()) { + final var lock = assetLocks.get(groupName + "/" + path); + lock.lock(); + try { + // Keep the local copy until all durable providers accept the deletion. + boolean deleted = !storagePersistenceAPIList.isEmpty(); + for (int i = storagePersistenceAPIList.size() - 1; i >= 0; i--) { + deleted &= storagePersistenceAPIList.get(i).deleteObjectAndReferences(groupName, path); + } + cache.remove(groupName, path); + return deleted; + } finally { + lock.unlock(); + } + } + boolean deleted = !this.storagePersistenceAPIList.isEmpty(); for(final StoragePersistenceAPI storage : this.storagePersistenceAPIList) { @@ -167,10 +197,73 @@ public List listGroups() throws DotDataException { return List.copyOf(uniqueGroups); } + @Override + public List listObjectPaths(final String groupName, + final String pathPrefix) throws DotDataException { + + final Set paths = new LinkedHashSet<>(); + for (final StoragePersistenceAPI storage : storagePersistenceAPIList) { + paths.addAll(storage.listObjectPaths(groupName, pathPrefix)); + } + return List.copyOf(paths); + } + + @Override + public boolean hasDurableCopy(final String groupName, final String path, + final File file) throws DotDataException { + for (final StoragePersistenceAPI storage : storagePersistenceAPIList) { + if (storage.hasDurableCopy(groupName, path, file)) { + return true; + } + } + return false; + } + + @Override + public boolean backfillFile(final String groupName, final String path, final File file) throws DotDataException { + if (!AssetStorageFeature.isEnabled()) { + return false; + } + boolean durable = false; + for (final StoragePersistenceAPI storage : storagePersistenceAPIList) { + durable |= storage.backfillFile(groupName, path, file); + } + return durable; + } + + @Override + public boolean backfillObject(final String groupName, final String path, final ObjectWriterDelegate writer, + final ObjectReaderDelegate reader, final Serializable object) throws DotDataException { + if (!AssetStorageFeature.isEnabled()) { + return false; + } + boolean durable = false; + for (final StoragePersistenceAPI storage : storagePersistenceAPIList) { + durable |= storage.backfillObject(groupName, path, writer, reader, object); + } + return durable; + } + @Override public Object pushFile(final String groupName, final String path, final File file, final Map extraMeta) throws DotDataException { + if (AssetStorageFeature.isEnabled()) { + final var lock = assetLocks.get(groupName + "/" + path); + lock.lock(); + try { + Object result = null; + // Publish durably before exposing the new local cache contents. + for (int i = storagePersistenceAPIList.size() - 1; i >= 0; i--) { + result = storagePersistenceAPIList.get(i).pushFile(groupName, path, file, extraMeta); + } + cache.remove(groupName, path); + return result; + } finally { + lock.unlock(); + } + } + Object object = null; for(final StoragePersistenceAPI storage : this.storagePersistenceAPIList) { @@ -189,6 +282,21 @@ public Object pushFile(final String groupName, final String path, final File fil public Object pushObject(final String groupName, final String path, final ObjectWriterDelegate writerDelegate, final Serializable objectIn, final Map extraMeta) throws DotDataException { + if (AssetStorageFeature.isEnabled()) { + final var lock = assetLocks.get(groupName + "/" + path); + lock.lock(); + try { + Object result = null; + for (int i = storagePersistenceAPIList.size() - 1; i >= 0; i--) { + result = storagePersistenceAPIList.get(i).pushObject(groupName, path, writerDelegate, objectIn, extraMeta); + } + cache.remove(groupName, path); + return result; + } finally { + lock.unlock(); + } + } + Object objectToReturn = null; for(final StoragePersistenceAPI storage : this.storagePersistenceAPIList) { @@ -206,6 +314,10 @@ public Object pushObject(final String groupName, final String path, @Override public Future pushFileAsync(final String groupName, final String path, final File file, final Map extraMeta) { + if (AssetStorageFeature.isEnabled()) { + return DotConcurrentFactory.getInstance().getSubmitter(STORAGE_POOL).submit(() -> this.pushFile(groupName, path, file, extraMeta)); + } + final List> futures = new ArrayList<>(); for(final StoragePersistenceAPI storage : this.storagePersistenceAPIList) { @@ -230,6 +342,10 @@ public Future pushFileAsync(final String groupName, final String path, f public Future pushObjectAsync(final String bucketName, final String path, final ObjectWriterDelegate writerDelegate, final Serializable object, final Map extraMeta) { + if (AssetStorageFeature.isEnabled()) { + return DotConcurrentFactory.getInstance().getSubmitter(STORAGE_POOL).submit(() -> this.pushObject(bucketName, path, writerDelegate, object, extraMeta)); + } + final List> futures = new ArrayList<>(); for(final StoragePersistenceAPI storage : this.storagePersistenceAPIList) { @@ -251,7 +367,40 @@ public Future pushObjectAsync(final String bucketName, final String path } @Override - public File pullFile(final String groupName, final String path) { + public File pullFile(final String groupName, final String path) throws DotDataException { + + if (AssetStorageFeature.isEnabled()) { + final var lock = assetLocks.get(groupName + "/" + path); + lock.lock(); + try { + final List missing = new ArrayList<>(); + for (StoragePersistenceAPI storage : storagePersistenceAPIList) { + final File file = storage.pullFile(groupName, path); + if (file == null) { + missing.add(storage); + continue; + } + if (missing.isEmpty()) { + return file; + } + try { + for (StoragePersistenceAPI cacheStorage : missing) { + if (!cacheStorage.existsGroup(groupName)) { + cacheStorage.createGroup(groupName); + } + cacheStorage.pushFile(groupName, path, file, Map.of()); + } + return missing.get(0).pullFile(groupName, path); + } finally { + storage.releaseRetrievedFile(file); + } + } + // Absence is not cached: another node can upload this key at any time. + return null; + } finally { + lock.unlock(); + } + } if (this.cache.is404(groupName, path)) { @@ -302,6 +451,32 @@ public Object pullObject(final String groupName, final String path, final ObjectReaderDelegate readerDelegate) { + if (AssetStorageFeature.isEnabled()) { + final List missing = new ArrayList<>(); + try { + for (StoragePersistenceAPI storage : storagePersistenceAPIList) { + final Object object = storage.pullObject(groupName, path, readerDelegate); + if (object == null) { + missing.add(storage); + } else { + if (object instanceof Serializable) { + for (StoragePersistenceAPI cacheStorage : missing) { + if (!cacheStorage.existsGroup(groupName)) { + cacheStorage.createGroup(groupName); + } + cacheStorage.pushObject(groupName, path, defaultWriterDelegate, + (Serializable) object, Map.of()); + } + } + return object; + } + } + return null; + } catch (DotDataException e) { + throw new DotRuntimeException("Unable to retrieve stored object " + path, e); + } + } + if (this.cache.is404(groupName, path)) { return null; @@ -347,6 +522,10 @@ private void populateMissStorageChain(final String groupName, final String path, @Override public Future pullFileAsync(final String groupName, final String path) { + if (AssetStorageFeature.isEnabled()) { + return DotConcurrentFactory.getInstance().getSubmitter(STORAGE_POOL).submit(() -> this.pullFile(groupName, path)); + } + if (this.cache.is404(groupName, path)) { @@ -368,6 +547,10 @@ public Future pullFileAsync(final String groupName, final String path) { @Override public Future pullObjectAsync(final String groupName, final String path, final ObjectReaderDelegate readerDelegate) { + if (AssetStorageFeature.isEnabled()) { + return DotConcurrentFactory.getInstance().getSubmitter(STORAGE_POOL).submit(() -> this.pullObject(groupName, path, readerDelegate)); + } + if (this.cache.is404(groupName, path)) { diff --git a/dotCMS/src/main/java/com/dotcms/storage/DataBaseStoragePersistenceAPIImpl.java b/dotCMS/src/main/java/com/dotcms/storage/DataBaseStoragePersistenceAPIImpl.java index 8a2f48c4a847..441858c09630 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/DataBaseStoragePersistenceAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/storage/DataBaseStoragePersistenceAPIImpl.java @@ -680,11 +680,11 @@ public File pullFile(final String groupName, final String path) throws DotDataEx final MutableObject file = new MutableObject<>(null); try (Connection connection = getConnection()) { - final List> storageResult = Try.of(() -> - new DotConnect() - .setSQL("SELECT hash FROM storage WHERE group_name = ? AND path = ?") - .addParam(groupNameLC).addParam(pathLC) - .loadObjectResults(connection)).getOrElse(Collections::emptyList); + final var query = Try.of(() -> new DotConnect() + .setSQL("SELECT hash FROM storage WHERE group_name = ? AND path = ?") + .addParam(groupNameLC).addParam(pathLC).loadObjectResults(connection)); + final List> storageResult = AssetStorageFeature.isEnabled() + ? query.getOrElseThrow(DotDataException::new) : query.getOrElse(Collections::emptyList); if (!storageResult.isEmpty()) { @@ -696,6 +696,9 @@ public File pullFile(final String groupName, final String path) throws DotDataEx file.setValue(this.createJoinFile(objectHash, connection)); } } else { + if (AssetStorageFeature.isEnabled()) { + return null; + } throw new DoesNotExistException( "The storage, group: " + groupName + ", path: " + path + " does not exists"); @@ -770,8 +773,7 @@ public Object pullObject(final String groupName, final String path, final File file = pullFile(groupName, path); if (null != file) { - object = - Try.of(() -> { + final var read = Try.of(() -> { try (InputStream inputStream = file instanceof BinaryFileWrapper? new ByteArrayInputStream(BinaryFileWrapper.class.cast(file).getBufferByte()): Files.newInputStream(file.toPath())) { @@ -779,7 +781,8 @@ public Object pullObject(final String groupName, final String path, return readerDelegate.read(inputStream); } } - ).getOrNull(); + ); + object = AssetStorageFeature.isEnabled() ? read.getOrElseThrow(DotDataException::new) : read.getOrNull(); } return object; diff --git a/dotCMS/src/main/java/com/dotcms/storage/FileStorageAPI.java b/dotCMS/src/main/java/com/dotcms/storage/FileStorageAPI.java index 5830a2f61449..ad0390f8cc27 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/FileStorageAPI.java +++ b/dotCMS/src/main/java/com/dotcms/storage/FileStorageAPI.java @@ -97,6 +97,30 @@ default Map generateMetaData(final Supplier binary, */ Map retrieveMetaData(final FetchMetadataParams requestMetaData) throws DotDataException; + /** + * Reads stored metadata without cache projection or derived UI fields. Only the S3 asset + * lifecycle calls this. + * + * @param storageKey the metadata location + * @return the stored metadata, or {@code null} if absent + * @throws DotDataException if the metadata cannot be read + */ + default Map retrieveRawMetaData(StorageKey storageKey) throws DotDataException { + throw new UnsupportedOperationException("S3 asset storage requires the default FileStorageAPI"); + } + + /** + * Copies existing metadata to durable storage without overwriting a conflicting object. Only the + * S3 asset lifecycle calls this. + * + * @param storageKey the metadata location + * @return {@code false} if no metadata exists at the key + * @throws DotDataException if the copy fails + */ + default boolean backfillMetadata(StorageKey storageKey) throws DotDataException { + throw new UnsupportedOperationException("S3 asset storage requires the default FileStorageAPI"); + } + /** * Deletes all related metadata for the given contentlet diff --git a/dotCMS/src/main/java/com/dotcms/storage/FileStorageAPIImpl.java b/dotCMS/src/main/java/com/dotcms/storage/FileStorageAPIImpl.java index 2c2ea7f8e59d..896a60324d25 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/FileStorageAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/storage/FileStorageAPIImpl.java @@ -158,6 +158,9 @@ private Map generateFullMetaData(final File binary, } catch (Exception e) { + if (AssetStorageFeature.isEnabled() && e instanceof com.dotmarketing.exception.DotRuntimeException) { + throw (com.dotmarketing.exception.DotRuntimeException) e; + } Logger.error(FileStorageAPIImpl.class, "Exception generating full metadata", e); return Collections.emptyMap(); } @@ -211,7 +214,8 @@ public Map generateMetaData(final Supplier binary, // Assume the metadata object is there and read it directly — a stat-then-read pattern // doubles the filesystem round-trips, and on network-backed storage every extra stat is // a chance to hang (issue #36498). When the override just removed it, don't bother. - final boolean skipRead = configuration.isOverride() && configuration.isStore(); + final boolean skipRead = configuration.isOverride() + && (configuration.isStore() || AssetStorageFeature.isEnabled()); Map metadataMap = skipRead ? null : tryRetrieveMetaData(storageKey, storage); final boolean objectExists = metadataMap != null; @@ -237,15 +241,18 @@ public Map generateMetaData(final Supplier binary, /** * Reads the metadata object assuming it exists, returning {@code null} when it is absent, - * empty or unreadable so the caller regenerates it from the binary instead. This replaces - * the exists-then-read pattern: one read attempt instead of several stats plus a read. + * or empty so the caller can regenerate it. Enabled storage failures propagate; disabled + * mode retains its unreadable-as-missing fallback. Reads do not add a stat before opening. */ private Map tryRetrieveMetaData(final StorageKey storageKey, - final StoragePersistenceAPI storage) { + final StoragePersistenceAPI storage) throws DotDataException { try { final Map metadataMap = retrieveMetaData(storageKey, storage); return null == metadataMap || metadataMap.isEmpty() ? null : metadataMap; } catch (final Exception absentOrUnreadable) { + if (AssetStorageFeature.isEnabled()) { + throw new DotDataException("Unable to read metadata at " + storageKey.getPath(), absentOrUnreadable); + } Logger.debug(FileStorageAPIImpl.class, () -> "No readable metadata at path: " + storageKey.getPath() + " — it will be regenerated. Reason: " @@ -419,6 +426,10 @@ private void checkOverride(final StoragePersistenceAPI storage, * @param override {@code true} if the file must be overridden, {@code false} otherwise. */ private void checkOverride(final StoragePersistenceAPI storage, final StorageKey storageKey, final boolean override) throws DotDataException { + if (AssetStorageFeature.isEnabled()) { + // Providers replace a completed object; keep the prior value if generation/upload fails. + return; + } if (override && storage.existsObject(storageKey.getGroup(), storageKey.getPath())) { try { storage.deleteObjectAndReferences(storageKey.getGroup(), storageKey.getPath()); @@ -467,6 +478,27 @@ public Map retrieveMetaData(final FetchMetadataParams requ return metadataMap; } + @Override + public Map retrieveRawMetaData(final StorageKey storageKey) throws DotDataException { + return tryRetrieveMetaData(storageKey, persistenceProvider.getStorage(storageKey.getStorage())); + } + + @Override + public boolean backfillMetadata(final StorageKey storageKey) throws DotDataException { + if (!AssetStorageFeature.isEnabled()) { + return false; + } + final Map metadata = retrieveRawMetaData(storageKey); + if (metadata == null) { + return false; + } + if (!persistenceProvider.getStorage(storageKey.getStorage()).backfillObject(storageKey.getGroup(), + storageKey.getPath(), objectWriterDelegate, objectReaderDelegate, (Serializable) metadata)) { + throw new DotDataException("Metadata backfill requires a durable provider in the configured metadata chain"); + } + return true; + } + private Map fetchAndCacheMetadata(FetchMetadataParams requestMetaData, String cacheKey) throws DotDataException { final StorageKey storageKey = requestMetaData.getStorageKey(); diff --git a/dotCMS/src/main/java/com/dotcms/storage/FileSystemStoragePersistenceAPIImpl.java b/dotCMS/src/main/java/com/dotcms/storage/FileSystemStoragePersistenceAPIImpl.java index 5dd05f066f79..f069d64f57fe 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/FileSystemStoragePersistenceAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/storage/FileSystemStoragePersistenceAPIImpl.java @@ -46,6 +46,8 @@ */ public class FileSystemStoragePersistenceAPIImpl implements StoragePersistenceAPI { + public static final String GROUP_DIRECTORY = "groupDirectory"; + private static final String DEFAULT_ROOT = "root"; private static final String THE_BUCKET_NAME_S_DOES_NOT_HAVE_ANY_FILE_MAPPED = "The bucketName: `%s`, does not have any files mapped"; private static final String STORAGE_POOL = "StoragePool"; @@ -75,7 +77,7 @@ public class FileSystemStoragePersistenceAPIImpl implements StoragePersistenceAP * @param groupName {@link String} bucket name * @param folder {@link File} */ - void addGroupMapping(final String groupName, final File folder) { + public void addGroupMapping(final String groupName, final File folder) { if (!folder.isDirectory() || !folder.exists() || !folder.canWrite()) { throw new IllegalArgumentException(String.format( @@ -86,6 +88,17 @@ void addGroupMapping(final String groupName, final File folder) { Logger.debug(this, () -> String.format("Registering new group '%s' mapped to folder '%s' ",groupName, folder)); } + /** + * Maps a storage key to its path on disk. Keys are lowercased, as they always have been. + * + * @param groupName the group the key belongs to + * @param path the storage key + * @return the path to use under the group's folder + */ + private String normalizePath(final String groupName, final String path) { + return path.toLowerCase(); + } + @Override public boolean existsGroup(final String groupName) throws DotDataException{ final String groupNameLC = groupName.toLowerCase(); @@ -101,7 +114,7 @@ public boolean existsObject(final String groupName, final String path) throws Do } final File groupDir = groups.get(groupNameLC); try { - return Paths.get(groupDir.getCanonicalPath(), path.toLowerCase()).toFile().exists(); + return Paths.get(groupDir.getCanonicalPath(), normalizePath(groupName, path)).toFile().exists(); }catch(IOException e){ throw new DotDataException(e.getMessage(), e); } @@ -114,8 +127,20 @@ public boolean createGroup(final String groupName) throws DotDataException { @Override public boolean createGroup(final String groupName, final Map extraOptions) throws DotDataException { + if (extraOptions != null && extraOptions.get(GROUP_DIRECTORY) instanceof File) { + addGroupMapping(groupName, (File) extraOptions.get(GROUP_DIRECTORY)); + return true; + } final String groupNameLC = groupName.toLowerCase(); final File groupNamePath = this.groups.get(groupNameLC); + if (AssetStorageFeature.isEnabled() && groupNamePath != null) { + try { + Files.createDirectories(groupNamePath.toPath()); + return true; + } catch (IOException e) { + throw new DotDataException("Unable to create mapped storage directory " + groupNamePath, e); + } + } if (null != groupNamePath && groupNamePath.exists() && groupNamePath.isAbsolute()) { return true; } @@ -148,7 +173,7 @@ public int deleteGroup(final String groupName) throws DotDataException { @Override public boolean deleteObjectAndReferences(final String groupName, final String path) throws DotDataException { - return new File(groups.get(groupName.toLowerCase()), path.toLowerCase()).delete(); + return new File(groups.get(groupName.toLowerCase()), normalizePath(groupName, path)).delete(); } @Override @@ -179,8 +204,25 @@ public Object pushFile(final String groupName, if (null != file && file.exists() && file.canRead() && groupDir.canWrite()) { try { - final File destBucketFile = Paths.get(groupDir.getCanonicalPath(), path.toLowerCase()).toFile(); - FileUtils.copyFile(file, destBucketFile); + final File destBucketFile = Paths.get(groupDir.getCanonicalPath(), normalizePath(groupName, path)).toFile(); + if (!AssetStorageFeature.isEnabled()) { + FileUtils.copyFile(file, destBucketFile); + } else if (!file.getCanonicalFile().equals(destBucketFile.getCanonicalFile())) { + final Path destination = destBucketFile.toPath(); + Files.createDirectories(destination.getParent()); + final Path staged = Files.createTempFile(destination.getParent(), ".restore-", ".tmp"); + try { + Files.copy(file.toPath(), staged, java.nio.file.StandardCopyOption.REPLACE_EXISTING); + try { + Files.move(staged, destination, java.nio.file.StandardCopyOption.ATOMIC_MOVE, + java.nio.file.StandardCopyOption.REPLACE_EXISTING); + } catch (java.nio.file.AtomicMoveNotSupportedException e) { + Files.move(staged, destination, java.nio.file.StandardCopyOption.REPLACE_EXISTING); + } + } finally { + Files.deleteIfExists(staged); + } + } } catch (IOException e) { Logger.error(FileSystemStoragePersistenceAPIImpl.class, e.getMessage(), e); throw new DotDataException(e.getMessage(), e); @@ -214,7 +256,27 @@ public Object pushObject(final String groupName, final String path, return lockManager.tryLock("fs_" + groupName + path, () -> { - final File destBucketFile = Paths.get(groupDir.getCanonicalPath(),path.toLowerCase()).toFile(); + final File destBucketFile = Paths.get(groupDir.getCanonicalPath(),normalizePath(groupName, path)).toFile(); + + if (AssetStorageFeature.isEnabled()) { + final Path destination = destBucketFile.toPath(); + Files.createDirectories(destination.getParent()); + final Path staged = Files.createTempFile(destination.getParent(), ".metadata-", ".tmp"); + try { + try (OutputStream outputStream = FileUtil.createOutputStream(staged, contentMetadataCompressor.get())) { + writerDelegate.write(outputStream, object); + } + try { + Files.move(staged, destination, java.nio.file.StandardCopyOption.ATOMIC_MOVE, + java.nio.file.StandardCopyOption.REPLACE_EXISTING); + } catch (java.nio.file.AtomicMoveNotSupportedException e) { + Files.move(staged, destination, java.nio.file.StandardCopyOption.REPLACE_EXISTING); + } + return true; + } finally { + Files.deleteIfExists(staged); + } + } // someone else already wrote the file meanwhile waiting for the lock // so, I do not need to write it again @@ -284,6 +346,24 @@ public Future pushObjectAsync(final String groupName, final String path, @Override public File pullFile(final String groupName, final String path) throws DotDataException { + if (AssetStorageFeature.isEnabled()) { + final File groupDir = groups.get(groupName.toLowerCase()); + if (groupDir == null) { + return null; + } + try { + final Path file = Paths.get(groupDir.getCanonicalPath(), normalizePath(groupName, path)); + if (!Files.readAttributes(file, java.nio.file.attribute.BasicFileAttributes.class).isRegularFile()) { + throw new IOException("Stored binary is not a regular file: " + file); + } + return file.toFile(); + } catch (NoSuchFileException absent) { + return null; + } catch (IOException e) { + throw new DotDataException("Unable to read stored binary " + path, e); + } + } + if (!this.existsGroup(groupName)) { throw new IllegalArgumentException(String.format( @@ -293,10 +373,13 @@ public File pullFile(final String groupName, final String path) throws DotDataEx try { final File groupDir = groups.get(groupName.toLowerCase()); if (groupDir.canRead()) { - final File destBucketFile = Paths.get(groupDir.getCanonicalPath(), path.toLowerCase()).toFile(); + final File destBucketFile = Paths.get(groupDir.getCanonicalPath(), normalizePath(groupName, path)).toFile(); if (destBucketFile.exists()) { clientFile = destBucketFile; } else { + if (AssetStorageFeature.isEnabled()) { + return null; + } throw new IllegalArgumentException( "The group: " + destBucketFile + ", does not exists."); } @@ -315,6 +398,24 @@ public File pullFile(final String groupName, final String path) throws DotDataEx public Object pullObject(final String groupName, final String path, final ObjectReaderDelegate readerDelegate) throws DotDataException { + if (AssetStorageFeature.isEnabled()) { + final File groupDir = groups.get(groupName.toLowerCase()); + if (groupDir == null) { + return null; + } + try { + final Path file = Paths.get(groupDir.getCanonicalPath(), normalizePath(groupName, path)); + final String compressor = Config.getStringProperty("CONTENT_METADATA_COMPRESSOR", "none"); + try (InputStream input = FileUtil.createInputStream(file, compressor)) { + return readerDelegate.read(input); + } + } catch (NoSuchFileException absent) { + return null; + } catch (IOException e) { + throw new DotDataException("Unable to read stored object " + path, e); + } + } + Object object; if (!this.existsGroup(groupName)) { @@ -328,7 +429,7 @@ public Object pullObject(final String groupName, final String path, File file = null; try { - file = Paths.get(groupDir.getCanonicalPath(), path.toLowerCase()).toFile(); + file = Paths.get(groupDir.getCanonicalPath(), normalizePath(groupName, path)).toFile(); // Assume the file is there and open it directly — a failed open reports absence // without the extra stat of an exists() pre-check, which is a hang risk on // network-backed storage (issue #36498). @@ -336,6 +437,9 @@ public Object pullObject(final String groupName, final String path, try (InputStream input = FileUtil.createInputStream(file.toPath(), compressor)) { object = readerDelegate.read(input); } catch (final FileNotFoundException | NoSuchFileException absent) { + if (AssetStorageFeature.isEnabled()) { + return null; + } throw new IllegalArgumentException("The file: " + path + ", does not exists."); } } catch (MismatchedInputException e) { @@ -427,6 +531,56 @@ private void countFiles(final File dirPath, final MutableInt count) { } } + @Override + public List listObjectPaths(final String groupName, + final String pathPrefix) throws DotDataException { + + final String groupNameLC = groupName.toLowerCase(); + if (!existsGroup(groupNameLC)) { + return List.of(); + } + + final File groupDir = groups.get(groupNameLC); + final Path prefixDir; + try { + prefixDir = Paths.get(groupDir.getCanonicalPath(), pathPrefix); + } catch (final IOException e) { + throw new DotDataException( + "Failed to resolve prefix path for group '" + groupName + "': " + e.getMessage(), e); + } + + // Path traversal guard: ensure resolved path stays within group root + try { + if (!prefixDir.toRealPath().startsWith(groupDir.toPath().toRealPath())) { + throw new IllegalArgumentException( + "Path prefix escapes group root: " + pathPrefix); + } + } catch (final java.nio.file.NoSuchFileException e) { + // Directory does not exist — no objects to list + return List.of(); + } catch (final IOException e) { + throw new DotDataException( + "Failed to validate prefix path for group '" + groupName + "': " + e.getMessage(), e); + } + + if (!prefixDir.toFile().exists() || !prefixDir.toFile().isDirectory()) { + return List.of(); + } + + try (final Stream walk = Files.walk(prefixDir)) { + final Path groupRoot = groupDir.getCanonicalFile().toPath(); + return walk + .filter(Files::isRegularFile) + .map(groupRoot::relativize) + .map(Path::toString) + .collect(java.util.stream.Collectors.toList()); + } catch (final IOException e) { + throw new DotDataException( + "Failed to list objects under prefix '" + pathPrefix + + "' in group '" + groupName + "': " + e.getMessage(), e); + } + } + @Override public Iterable toIterable(final String group) { diff --git a/dotCMS/src/main/java/com/dotcms/storage/S3ContentAddressedStorage.java b/dotCMS/src/main/java/com/dotcms/storage/S3ContentAddressedStorage.java new file mode 100644 index 000000000000..d9def8ea94b1 --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/storage/S3ContentAddressedStorage.java @@ -0,0 +1,194 @@ +package com.dotcms.storage; + +import com.amazonaws.services.s3.model.ObjectMetadata; +import com.amazonaws.services.s3.model.PutObjectRequest; +import com.dotcms.enterprise.publishing.storage.Storage; +import com.dotmarketing.exception.DotDataException; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.security.DigestInputStream; +import java.security.MessageDigest; +import java.util.HexFormat; +import org.apache.commons.io.IOUtils; + +/** Immutable shared bytes, addressed by SHA-256, behind ordinary content-owned S3 references. */ +final class S3ContentAddressedStorage { + static final String BLOB_PREFIX = "asset-blobs/sha256/"; + static final String HASH_HEADER = "dotcms-blob-sha256"; + private final Storage storage; + private final String bucket; + + S3ContentAddressedStorage(Storage storage, String bucket) { + this.storage = storage; + this.bucket = bucket; + } + + static String blobKey(String hash) { + if (hash == null || !hash.matches("[a-f0-9]{64}")) throw new IllegalArgumentException("Invalid asset SHA-256"); + return BLOB_PREFIX + hash.substring(0, 2) + "/" + hash.substring(2, 4) + "/" + + hash.substring(4, 6) + "/" + hash.substring(6, 8) + "/" + hash; + } + + private static MessageDigest digest() { + try { return MessageDigest.getInstance("SHA-256"); } + catch (java.security.NoSuchAlgorithmException impossible) { throw new IllegalStateException(impossible); } + } + + static String hash(File file) throws IOException { + final var digest = digest(); + try (var input = new DigestInputStream(Files.newInputStream(file.toPath()), digest)) { + input.transferTo(java.io.OutputStream.nullOutputStream()); + } + return HexFormat.of().formatHex(digest.digest()); + } + + String store(String ownerKey, File source, boolean backfill) throws DotDataException { + Path snapshot = null; + Path reference = null; + try { + // Hash and upload the same private snapshot, even if a caller subsequently edits its file. + snapshot = Files.createTempFile("s3-blob-", ".tmp"); + final var digest = digest(); + try (var input = new DigestInputStream(Files.newInputStream(source.toPath()), digest); + var output = Files.newOutputStream(snapshot)) { input.transferTo(output); } + final String hash = HexFormat.of().formatHex(digest.digest()); + final String blob = blobKey(hash); + String previousVersion = null; + if (backfill) { + try (var owner = storage.getObject(bucket, ownerKey)) { + if (owner != null) { + previousVersion = owner.getObjectMetadata().getETag(); + final String storedHash = owner.getObjectMetadata().getUserMetaDataOf(HASH_HEADER); + if (storedHash != null) { + consumeReference(owner); + if (!hash.equals(storedHash)) throw new DotDataException("Backfill conflicts with an existing asset reference"); + if (blobMatches(blob, snapshot.toFile())) return hash; + } else { + try (var local = Files.newInputStream(snapshot)) { + if (!IOUtils.contentEquals(local, owner.getObjectContent())) { + throw new DotDataException("Backfill conflicts with an existing raw asset"); + } + } + } + } + } + } + if (!blobMatches(blob, snapshot.toFile())) { + try { storage.uploadFileIfAbsent(bucket, blob, snapshot.toFile()); } + catch (RuntimeException conflict) { + if (!preconditionFailure(conflict)) throw conflict; + } + if (!blobMatches(blob, snapshot.toFile())) { + throw new DotDataException("Shared asset bytes do not match their SHA-256 key"); + } + } + reference = Files.createTempFile("s3-asset-reference-", ".json"); + Files.writeString(reference, "{\"sha256\":\"" + hash + "\",\"length\":" + Files.size(snapshot) + "}"); + final var metadata = new ObjectMetadata(); + metadata.setContentType("application/vnd.dotcms.asset-reference+json"); + metadata.addUserMetadata(HASH_HEADER, hash); + final var request = new PutObjectRequest(bucket, ownerKey, reference.toFile()).withMetadata(metadata); + if (backfill) { + request.putCustomRequestHeader(previousVersion == null ? "If-None-Match" : "If-Match", + previousVersion == null ? "*" : "\"" + previousVersion + "\""); + } + try { storage.uploadFile(request).waitForCompletion(); } + catch (Exception conflict) { + if (!backfill || !preconditionFailure(conflict)) throw conflict; + } + try (var published = storage.getObject(bucket, ownerKey)) { + if (published == null || !hash.equals(published.getObjectMetadata().getUserMetaDataOf(HASH_HEADER))) { + throw new DotDataException("Asset reference publication was not verified"); + } + consumeReference(published); + } + if (!matches(ownerKey, snapshot.toFile())) throw new DotDataException("Shared asset bytes were not verified after publication"); + return hash; + } catch (Exception failure) { + if (failure instanceof InterruptedException) Thread.currentThread().interrupt(); + throw new DotDataException("Unable to store shared asset " + ownerKey, failure); + } finally { + remove(snapshot); + remove(reference); + } + } + + private static boolean preconditionFailure(Throwable failure) { + for (Throwable cause = failure; cause != null; cause = cause.getCause()) { + if (cause instanceof com.amazonaws.services.s3.model.AmazonS3Exception s3 + && (s3.getStatusCode() == 412 || s3.getStatusCode() == 409)) return true; + } + return false; + } + + /** Read old raw objects too; only an explicit S3 metadata marker denotes a reference. */ + boolean retrieve(String ownerKey, File destination) throws IOException, DotDataException { + try (var owner = storage.getObject(bucket, ownerKey)) { + if (owner == null) return false; + final String hash = owner.getObjectMetadata().getUserMetaDataOf(HASH_HEADER); + if (hash == null) { + try (var output = Files.newOutputStream(destination.toPath())) { owner.getObjectContent().transferTo(output); } + return true; + } + consumeReference(owner); + try (var blob = openBlob(hash)) { + if (blob == null) throw new DotDataException("Asset reference points to a missing shared blob"); + final var digest = digest(); + try (var input = new DigestInputStream(blob.getObjectContent(), digest); + var output = Files.newOutputStream(destination.toPath())) { input.transferTo(output); } + if (!hash.equals(HexFormat.of().formatHex(digest.digest()))) { + throw new DotDataException("Downloaded asset does not match its SHA-256 reference"); + } + } + return true; + } + } + + boolean matches(String ownerKey, File file) throws IOException { + try (var owner = storage.getObject(bucket, ownerKey)) { + if (owner == null) return false; + final String hash = owner.getObjectMetadata().getUserMetaDataOf(HASH_HEADER); + if (hash == null) { + try (InputStream local = Files.newInputStream(file.toPath())) { + return IOUtils.contentEquals(local, owner.getObjectContent()); + } + } + consumeReference(owner); + if (!hash.equals(hash(file))) return false; + try (var blob = openBlob(hash); var local = Files.newInputStream(file.toPath())) { + return blob != null && IOUtils.contentEquals(local, blob.getObjectContent()); + } + } + } + + private com.amazonaws.services.s3.model.S3Object openBlob(String hash) { + final var blob = storage.getObject(bucket, blobKey(hash)); + // Existing one-level blobs remain readable; only absence permits fallback. + return blob != null ? blob : storage.getObject(bucket, BLOB_PREFIX + hash.substring(0, 2) + "/" + hash); + } + + private boolean blobMatches(String key, File file) throws IOException { + try (var object = storage.getObject(bucket, key)) { + if (object == null) return false; + try (var input = Files.newInputStream(file.toPath())) { + return object.getObjectMetadata().getContentLength() == file.length() + && IOUtils.contentEquals(input, object.getObjectContent()); + } + } + } + + private static void consumeReference(com.amazonaws.services.s3.model.S3Object owner) throws IOException { + if (owner.getObjectMetadata().getContentLength() > 256) throw new IOException("Invalid asset reference size"); + owner.getObjectContent().transferTo(java.io.OutputStream.nullOutputStream()); + } + + private static void remove(Path path) { + if (path == null) return; + try { Files.deleteIfExists(path); } + catch (IOException failure) { com.dotmarketing.util.Logger.warn(S3ContentAddressedStorage.class, + "Unable to remove shared asset staging file", failure); } + } +} diff --git a/dotCMS/src/main/java/com/dotcms/storage/SharedExtractedMetadata.java b/dotCMS/src/main/java/com/dotcms/storage/SharedExtractedMetadata.java new file mode 100644 index 000000000000..251596318dd5 --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/storage/SharedExtractedMetadata.java @@ -0,0 +1,51 @@ +package com.dotcms.storage; + +import com.dotmarketing.exception.DotDataException; +import java.io.File; +import java.io.Serializable; +import java.nio.file.Files; +import java.nio.file.StandardCopyOption; +import java.util.HashMap; +import java.util.Map; +import java.util.TreeMap; +import java.util.function.Function; + +/** Only byte-derived extraction goes here; filenames and editorial metadata are merged afterwards. */ +final class SharedExtractedMetadata { + static final String GROUP = "extracted-metadata"; + private final AmazonS3StoragePersistenceAPIImpl storage; + + private static class Holder { + private static final SharedExtractedMetadata INSTANCE = new SharedExtractedMetadata( + AmazonS3StoragePersistenceAPIImpl.withPlainPaths()); + } + + static SharedExtractedMetadata getInstance() { return Holder.INSTANCE; } + SharedExtractedMetadata(AmazonS3StoragePersistenceAPIImpl storage) { this.storage = storage; } + + @SuppressWarnings("unchecked") + Map get(File source, String extractorVersion, int schemaVersion, int textLimit, + Function> extract) throws Exception { + if (!AssetStorageFeature.isEnabled()) return extract.apply(source); + final var snapshot = Files.createTempFile("asset-extraction-", ".tmp"); + try { + Files.copy(source.toPath(), snapshot, StandardCopyOption.REPLACE_EXISTING); + final String hash = S3ContentAddressedStorage.hash(snapshot.toFile()); + final String configuration = org.apache.commons.codec.digest.DigestUtils.sha256Hex( + "tika:" + extractorVersion + ":schema:" + schemaVersion + ":text-limit:" + textLimit); + final String key = hash + "/" + configuration + ".json"; + final var reader = new JsonReaderDelegate<>(Map.class); + final Map existing = (Map) storage.pullObject(GROUP, key, reader); + if (existing != null) return new HashMap<>(existing); + final Map extracted = extract.apply(snapshot.toFile()); + // Tika adds size on failures too; only a completed extraction includes its content field. + if (!extracted.containsKey(com.dotmarketing.portlets.fileassets.business.FileAssetAPI.CONTENT_FIELD)) return extracted; + storage.createGroup(GROUP); + storage.writeObjectIfMatch(GROUP, key, new TreeMap<>(extracted), null); + // Concurrent extraction may differ in incidental parser diagnostics. Use the first published result. + final Map published = (Map) storage.pullObject(GROUP, key, reader); + if (published == null) throw new DotDataException("Extracted metadata publication was not verified"); + return new HashMap<>(published); + } finally { Files.deleteIfExists(snapshot); } + } +} diff --git a/dotCMS/src/main/java/com/dotcms/storage/StoragePersistenceAPI.java b/dotCMS/src/main/java/com/dotcms/storage/StoragePersistenceAPI.java index a6e4fa4735e3..7316043b0dbf 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/StoragePersistenceAPI.java +++ b/dotCMS/src/main/java/com/dotcms/storage/StoragePersistenceAPI.java @@ -151,6 +151,10 @@ Object pushObject(final String groupName, final String path, final ObjectWriterD */ File pullFile (final String groupName, final String path) throws DotDataException; + /** Releases provider-owned download staging after the caller has copied or consumed it. */ + default void releaseRetrievedFile(final File file) throws DotDataException { } + + /** * Pull a stream from the storage and read as an object * @@ -196,6 +200,42 @@ default Iterable toIterable(String group) { return new EmptyIterable<>(); } + /** + * Lists object paths under a given prefix within a group. Returns paths relative to the + * group root. For example, if objects exist at {@code a/b/inode/field/file.pdf}, calling + * with prefix {@code a/b/inode} returns {@code ["a/b/inode/field/file.pdf"]}. + * + *

Default implementation returns an empty list, which is safe for providers that do not + * support prefix-based listing.

+ * + * @param groupName The group (bucket/folder) to list within. + * @param pathPrefix The path prefix to filter objects by. + * @return A list of relative object paths matching the prefix. + * @throws DotDataException If a storage error occurs during listing. + */ + default List listObjectPaths(final String groupName, + final String pathPrefix) throws DotDataException { + return List.of(); + } + + /** Returns true only when a durable provider verifies the same file contents. */ + default boolean hasDurableCopy(final String groupName, final String path, + final File file) throws DotDataException { + return false; + } + + /** Copies to durable storage without overwriting a conflicting object. False means unsupported. */ + default boolean backfillFile(final String groupName, final String path, + final File file) throws DotDataException { + return false; + } + + /** Backfills an object using its reader to compare existing contents independently of serialization order. */ + default boolean backfillObject(final String groupName, final String path, final ObjectWriterDelegate writer, + final ObjectReaderDelegate reader, final Serializable object) throws DotDataException { + return false; + } + /** * Takes the information from the object and writes it to the specified file using the Writer * Delegate to do so. This way, there's no in-memory loading. diff --git a/dotCMS/src/main/java/com/dotcms/tika/TikaUtils.java b/dotCMS/src/main/java/com/dotcms/tika/TikaUtils.java index 2c80ece1526f..15449e6b8626 100644 --- a/dotCMS/src/main/java/com/dotcms/tika/TikaUtils.java +++ b/dotCMS/src/main/java/com/dotcms/tika/TikaUtils.java @@ -668,6 +668,13 @@ public String detect(File file) throws IOException { return this.tikaService.detect(file); } + /** Version of the actual loaded parser bundle; unknown implementations must not share extraction caches. */ + public String extractorVersion() { + if (!osgiInitialized || tikaService == null) return null; + final var bundle = org.osgi.framework.FrameworkUtil.getBundle(tikaService.getClass()); + return bundle == null ? null : bundle.getSymbolicName() + ":" + bundle.getVersion(); + } + private Map buildMetaDataMap() { Map metaMap = new HashMap<>(); diff --git a/dotCMS/src/main/java/com/dotmarketing/util/Config.java b/dotCMS/src/main/java/com/dotmarketing/util/Config.java index 08f6e70b296d..6445b4d088a1 100644 --- a/dotCMS/src/main/java/com/dotmarketing/util/Config.java +++ b/dotCMS/src/main/java/com/dotmarketing/util/Config.java @@ -787,12 +787,17 @@ public static boolean getBooleanProperty(String name, boolean defaultVal) { */ public static void setProperty(String key, Object value) { if (props != null) { + final boolean assetStorageFlag = com.dotcms.storage.AssetStorageFeature.FLAG.equals(key); if(props.containsKey(envKey(key))) { key = envKey(key); } trackOverrides(key, value); Logger.info(Config.class, "Setting property: " + key + " to " + value); props.setProperty(key, value); + if (assetStorageFlag) { + // ponytail: only in-memory overrides re-read the latched flag; reloads and the system table stay latched. + com.dotcms.storage.AssetStorageFeature.reset(); + } } } diff --git a/dotCMS/src/main/resources/dotmarketing-config.properties b/dotCMS/src/main/resources/dotmarketing-config.properties index 507c90d6f131..a298abb757bc 100644 --- a/dotCMS/src/main/resources/dotmarketing-config.properties +++ b/dotCMS/src/main/resources/dotmarketing-config.properties @@ -915,6 +915,16 @@ CONTENT_EDITOR2_ENABLED=true ## Localizations Enhancements LOCALIZATION_ENHANCEMENTS_ENABLED=false +# Opt-in S3 asset lifecycle. Requires S3 configuration and a restart: the value is read +# once at startup, so set it here or in the environment, not the system table. +# When false, existing filesystem/NFS paths and cleanup remain in use. +FEATURE_FLAG_S3_ASSET_STORAGE=false +# Optional stable deployment namespace for owned S3 assets/metadata. Cluster nodes +# share a namespace; independent installations sharing a bucket use distinct values. +# Empty preserves existing keys. SHA blobs and extracted metadata remain bucket-shared. +# Set before first use; changing it requires migration and separate local cache roots. +storage.file-metadata.s3.namespace= + ## UVE Toggle Lock FEATURE_FLAG_UVE_TOGGLE_LOCK=false diff --git a/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureLatchTest.java b/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureLatchTest.java new file mode 100644 index 000000000000..722bd390ad81 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureLatchTest.java @@ -0,0 +1,46 @@ +package com.dotcms.storage; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.mockStatic; + +import com.dotmarketing.util.Config; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; + +class AssetStorageFeatureLatchTest { + private String previous; + + @BeforeEach + void remember() { + previous = Config.getStringProperty(AssetStorageFeature.FLAG, null); + } + + @AfterEach + void restore() { + Config.setProperty(AssetStorageFeature.FLAG, previous); + } + + @Test + void runtimeConfigurationChangesAreIgnoredUntilReset() { + Config.setProperty(AssetStorageFeature.FLAG, true); + assertTrue(AssetStorageFeature.isEnabled()); + try (MockedStatic config = mockStatic(Config.class)) { + config.when(() -> Config.getBooleanProperty(AssetStorageFeature.FLAG, false)).thenReturn(false); + assertTrue(AssetStorageFeature.isEnabled(), "A running node must keep the mode it started with"); + AssetStorageFeature.reset(); + assertFalse(AssetStorageFeature.isEnabled()); + } finally { + AssetStorageFeature.reset(); + } + } + + @Test + void inMemoryOverridesSwitchTheMode() { + Config.setProperty(AssetStorageFeature.FLAG, true); + assertTrue(AssetStorageFeature.isEnabled()); + Config.setProperty(AssetStorageFeature.FLAG, false); + assertFalse(AssetStorageFeature.isEnabled()); + } +} diff --git a/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureTest.java b/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureTest.java new file mode 100644 index 000000000000..fa49fa14917b --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureTest.java @@ -0,0 +1,242 @@ +package com.dotcms.storage; + +import com.dotmarketing.exception.DotDataException; +import com.dotmarketing.util.Config; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import java.io.File; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Map; +import java.util.concurrent.*; +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; + +class AssetStorageFeatureTest { + @TempDir Path root; + private String previousFlag; + private String previousRoot; + private static final String GROUP = "binary-assets"; + private static final String KEY = "a/b/abc123/HeroImage/MyFile.PNG"; + + @BeforeEach + void configure() { + previousFlag = Config.getStringProperty(AssetStorageFeature.FLAG, null); + previousRoot = Config.getStringProperty("ROOT_GROUP_FOLDER_PATH", null); + Config.setProperty(AssetStorageFeature.FLAG, true); + Config.setProperty("ROOT_GROUP_FOLDER_PATH", root.toString()); + } + + @AfterEach + void restore() { + Config.setProperty(AssetStorageFeature.FLAG, previousFlag); + Config.setProperty("ROOT_GROUP_FOLDER_PATH", previousRoot); + } + + private FileSystemStoragePersistenceAPIImpl filesystem() { + var fs = new FileSystemStoragePersistenceAPIImpl(); + fs.addGroupMapping(GROUP, root.toFile()); + return fs; + } + + private ChainableStoragePersistenceAPI chain(StoragePersistenceAPI... providers) { + return new ChainableStoragePersistenceAPI(new JsonWriterDelegate(), List.of(providers), + mock(Chainable404StorageCache.class)); + } + + @Test + void databaseQueryFailuresAreNotReportedAsMissingObjectsWhenEnabled() throws Exception { + final var connection = mock(java.sql.Connection.class); + final var queryFailure = new DotDataException("database query failed", new java.sql.SQLException("offline")); + final var database = new DataBaseStoragePersistenceAPIImpl() { + @Override + protected java.sql.Connection getConnection() { return connection; } + }; + try (var queries = mockConstruction(com.dotmarketing.common.db.DotConnect.class, (query, context) -> { + when(query.setSQL(anyString())).thenReturn(query); + when(query.addParam(anyString())).thenReturn(query); + when(query.loadObjectResults(connection)).thenThrow(queryFailure); + })) { + final DotDataException failure = assertThrows(DotDataException.class, + () -> database.pullFile("metadata", "/file.json")); + assertInstanceOf(java.sql.SQLException.class, + org.apache.commons.lang3.exception.ExceptionUtils.getRootCause(failure)); + Config.setProperty(AssetStorageFeature.FLAG, false); + final DotDataException legacy = assertThrows(DotDataException.class, + () -> database.pullFile("metadata", "/file.json")); + assertTrue(legacy.getCause() instanceof com.dotmarketing.exception.DoesNotExistException); + } + } + + @Test + void filesystemMetadataReplacementKeepsPriorValueDuringAndAfterFailedSerialization() throws Exception { + final var fs = filesystem(); + final var reader = new JsonReaderDelegate<>(String.class); + fs.pushObject(GROUP, KEY, new JsonWriterDelegate(), "Original", Map.of()); + final var writing = new CountDownLatch(1); + final var release = new CountDownLatch(1); + final var executor = Executors.newSingleThreadExecutor(); + try { + final var failed = executor.submit(() -> fs.pushObject(GROUP, KEY, (out, value) -> { + out.write(123); + writing.countDown(); + try { + if (!release.await(10, TimeUnit.SECONDS)) { + throw new java.io.IOException("writer was not released"); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new java.io.IOException(e); + } + throw new java.io.IOException("injected serialization failure"); + }, "Partial", Map.of())); + assertTrue(writing.await(5, TimeUnit.SECONDS)); + assertEquals("Original", fs.pullObject(GROUP, KEY, reader)); + release.countDown(); + assertThrows(ExecutionException.class, () -> failed.get(5, TimeUnit.SECONDS)); + assertEquals("Original", fs.pullObject(GROUP, KEY, reader)); + fs.pushObject(GROUP, KEY, new JsonWriterDelegate(), "Replacement", Map.of()); + assertEquals("Replacement", fs.pullObject(GROUP, KEY, reader)); + try (var files = Files.walk(root)) { + assertFalse(files.anyMatch(file -> file.getFileName().toString().startsWith(".metadata-"))); + } + Config.setProperty(AssetStorageFeature.FLAG, false); + final String legacyKey = "legacy-metadata.json"; + fs.pushObject(GROUP, legacyKey, new JsonWriterDelegate(), "Legacy", Map.of()); + fs.pushObject(GROUP, legacyKey, new JsonWriterDelegate(), "Ignored", Map.of()); + assertEquals("Legacy", fs.pullObject(GROUP, legacyKey, reader), "Disabled object writes retain legacy behavior"); + } finally { + release.countDown(); + executor.shutdownNow(); + } + } + + @Test + void disabledFilesystemProviderKeepsLegacyPathAndMissingFileBehavior() throws Exception { + Config.setProperty(AssetStorageFeature.FLAG, false); + var storage = filesystem(); + File source = Files.writeString(root.resolve("upload.tmp"), "legacy contents").toFile(); + storage.pushFile(GROUP, KEY, source, Map.of()); + Path legacyPath = root.resolve(KEY.toLowerCase()); + assertEquals("legacy contents", Files.readString(legacyPath)); + assertEquals(legacyPath.toFile().getCanonicalFile(), storage.pullFile(GROUP, KEY)); + assertTrue(storage.existsObject(GROUP, KEY)); + assertTrue(storage.deleteObjectAndReferences(GROUP, KEY)); + assertFalse(Files.exists(legacyPath)); + assertThrows(IllegalArgumentException.class, () -> storage.pullFile(GROUP, KEY)); + } + + @Test + void storageOutagePropagatesAndRecoveryBypassesNegativeCache() throws Exception { + var provider = mock(StoragePersistenceAPI.class); + File file = Files.writeString(root.resolve("remote"), "restored").toFile(); + when(provider.pullFile(GROUP, KEY)).thenThrow(new DotDataException("S3 outage")).thenReturn(file); + var cache = mock(Chainable404StorageCache.class); + when(cache.is404(anyString(), anyString())).thenReturn(true); + var chain = new ChainableStoragePersistenceAPI(new JsonWriterDelegate(), List.of(provider), cache); + assertThrows(DotDataException.class, () -> chain.pullFile(GROUP, KEY)); + assertSame(file, chain.pullFile(GROUP, KEY)); + verifyNoInteractions(cache); + } + + @Test + void failedRemoteWriteKeepsPreviousLocalContents() throws Exception { + var fs = filesystem(); + Path destination = root.resolve(KEY); + Files.createDirectories(destination.getParent()); + Files.writeString(destination, "previous version"); + File upload = Files.writeString(root.resolve("upload"), "replacement").toFile(); + var remote = mock(StoragePersistenceAPI.class); + when(remote.pushFile(eq(GROUP), eq(KEY), any(), any())).thenThrow(new DotDataException("upload failed")); + assertThrows(DotDataException.class, () -> chain(fs, remote).pushFile(GROUP, KEY, upload, Map.of())); + assertEquals("previous version", Files.readString(destination)); + } + + @Test + void asyncWriteReportsRemoteFailureAndDoesNotPublishLocalReplacement() throws Exception { + var fs = filesystem(); + Path destination = root.resolve(KEY); + Files.createDirectories(destination.getParent()); + Files.writeString(destination, "previous version"); + File upload = Files.writeString(root.resolve("upload"), "replacement").toFile(); + var remote = mock(StoragePersistenceAPI.class); + when(remote.pushFile(eq(GROUP), eq(KEY), any(), any())).thenThrow(new DotDataException("upload failed")); + var result = chain(fs, remote).pushFileAsync(GROUP, KEY, upload, Map.of()); + assertInstanceOf(DotDataException.class, + assertThrows(ExecutionException.class, () -> result.get(5, TimeUnit.SECONDS)).getCause()); + assertEquals("previous version", Files.readString(destination)); + } + + @Test + void failedRestoreStillReleasesDownload() throws Exception { + var fs = mock(StoragePersistenceAPI.class); + var remote = mock(StoragePersistenceAPI.class); + File download = Files.writeString(root.resolve("download.tmp"), "remote").toFile(); + when(remote.pullFile(GROUP, KEY)).thenReturn(download); + when(fs.pushFile(eq(GROUP), eq(KEY), any(), any())).thenThrow(new DotDataException("disk full")); + assertThrows(DotDataException.class, () -> chain(fs, remote).pullFile(GROUP, KEY)); + verify(remote).releaseRetrievedFile(download); + } + + @Test + void deleteCannotBeUndoneBySameChainInflightRestore() throws Exception { + var fs = filesystem(); + var remote = mock(StoragePersistenceAPI.class); + File snapshot = Files.writeString(root.resolve("download.tmp"), "remote").toFile(); + var fetching = new CountDownLatch(1); + var finishFetch = new CountDownLatch(1); + when(remote.pullFile(GROUP, KEY)).thenAnswer(call -> { + fetching.countDown(); + assertTrue(finishFetch.await(10, TimeUnit.SECONDS)); + return snapshot; + }); + var chain = chain(fs, remote); + try (var executor = Executors.newFixedThreadPool(2)) { + var read = executor.submit(() -> chain.pullFile(GROUP, KEY)); + assertTrue(fetching.await(5, TimeUnit.SECONDS)); + var deleting = new CountDownLatch(1); + var delete = executor.submit(() -> { + deleting.countDown(); + return chain.deleteObjectAndReferences(GROUP, KEY); + }); + assertTrue(deleting.await(5, TimeUnit.SECONDS)); + try { + assertThrows(TimeoutException.class, () -> delete.get(100, TimeUnit.MILLISECONDS)); + } finally { + finishFetch.countDown(); + } + read.get(5, TimeUnit.SECONDS); + delete.get(5, TimeUnit.SECONDS); + assertFalse(Files.exists(root.resolve(KEY))); + } + } + + @Test + void metadataKeysAreStableAcrossConcurrentCalls() throws Exception { + var adapter = new AmazonS3StoragePersistenceAPIImpl( + mock(com.dotcms.enterprise.publishing.storage.AWSS3Storage.class), "test", + AmazonS3StoragePersistenceAPIImpl.PathEncryptionMode.SHA256); + var transform = AmazonS3StoragePersistenceAPIImpl.class.getDeclaredMethod("transformReadPath", String.class, String.class); + transform.setAccessible(true); + var start = new CountDownLatch(1); + try (var executor = Executors.newFixedThreadPool(8)) { + var futures = new java.util.ArrayList>(); + for (int i = 0; i < 8; i++) { + String directory = "a/b/inode" + i + "/HeroImage/"; + String expected = "dotmetadata/" + org.apache.commons.codec.digest.DigestUtils.sha256Hex(directory) + "/asset-metadata.json"; + futures.add(executor.submit(() -> { + start.await(); + for (int j = 0; j < 1000; j++) { + assertEquals(expected, transform.invoke(adapter, "dotmetadata", "/" + directory + "asset-metadata.json")); + } + return null; + })); + } + start.countDown(); + for (var future : futures) future.get(20, TimeUnit.SECONDS); + } + } +} diff --git a/dotCMS/src/test/java/com/dotcms/storage/BinaryS3StorageTest.java b/dotCMS/src/test/java/com/dotcms/storage/BinaryS3StorageTest.java new file mode 100644 index 000000000000..7d4b97bee72e --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/storage/BinaryS3StorageTest.java @@ -0,0 +1,161 @@ +package com.dotcms.storage; + +import com.amazonaws.auth.AWSStaticCredentialsProvider; +import com.amazonaws.auth.BasicAWSCredentials; +import com.amazonaws.client.builder.AwsClientBuilder; +import com.amazonaws.services.s3.AmazonS3; +import com.amazonaws.services.s3.AmazonS3ClientBuilder; +import com.dotcms.enterprise.publishing.staticpublishing.AWSS3Configuration; +import com.dotcms.enterprise.publishing.storage.AWSS3Storage; +import com.dotmarketing.util.Config; +import com.dotmarketing.util.ConfigUtils; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledIfSystemProperty; +import org.junit.jupiter.api.io.TempDir; +import org.mockito.MockedStatic; + +import java.io.File; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Map; +import java.util.UUID; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; + +/** Real filesystem + AWS adapter + S3 server. See docs/testing/BINARY_S3_STORAGE.md. */ +@EnabledIfSystemProperty(named = "s3.test.endpoint", matches = ".+") +class BinaryS3StorageTest { + private String previousFeatureFlag; + + @org.junit.jupiter.api.BeforeEach + void enableS3FeatureForTest() { + previousFeatureFlag = Config.getStringProperty(com.dotcms.storage.AssetStorageFeature.FLAG, null); + Config.setProperty(com.dotcms.storage.AssetStorageFeature.FLAG, true); + } + + @org.junit.jupiter.api.AfterEach + void restoreS3FeatureAfterTest() { + Config.setProperty(com.dotcms.storage.AssetStorageFeature.FLAG, previousFeatureFlag); + } + + private static final String CREDENTIAL = "binary-storage-test"; + @TempDir Path root; + private AmazonS3 client; + private AWSS3Storage storage; + private AmazonS3StoragePersistenceAPIImpl s3; + private FileSystemStoragePersistenceAPIImpl fs; + private MockedStatic configUtils; + private String bucket; + private boolean versioned; + private String previousRoot; + + @BeforeEach + void setUp() { + final String endpoint = System.getProperty("s3.test.endpoint"); + client = AmazonS3ClientBuilder.standard() + .withEndpointConfiguration(new AwsClientBuilder.EndpointConfiguration(endpoint, "us-east-1")) + .withPathStyleAccessEnabled(true) + .withCredentials(new AWSStaticCredentialsProvider(new BasicAWSCredentials(CREDENTIAL, CREDENTIAL))) + .build(); + bucket = "binary-test-" + UUID.randomUUID(); + client.createBucket(bucket); + storage = new AWSS3Storage(new AWSS3Configuration.Builder() + .accessKey(CREDENTIAL).secretKey(CREDENTIAL).endPoint(endpoint).region("us-east-1").build()); + s3 = new AmazonS3StoragePersistenceAPIImpl(storage, bucket, + AmazonS3StoragePersistenceAPIImpl.PathEncryptionMode.NONE); + previousRoot = Config.getStringProperty("ROOT_GROUP_FOLDER_PATH", null); + Config.setProperty("ROOT_GROUP_FOLDER_PATH", root.toString()); + fs = new FileSystemStoragePersistenceAPIImpl(); + configUtils = mockStatic(ConfigUtils.class); + configUtils.when(ConfigUtils::getAssetPath).thenReturn(root.toString()); + configUtils.when(ConfigUtils::getDotGeneratedPath).thenReturn(root.resolve("dotGenerated").toString()); + configUtils.when(ConfigUtils::getBundlePath).thenReturn(root.resolve("bundles").toString()); + } + + @AfterEach + void tearDown() { + if (configUtils != null) { + configUtils.close(); + } + Config.setProperty("ROOT_GROUP_FOLDER_PATH", previousRoot); + if (storage != null) { + storage.shutdownTransferManager(); + } + if (client != null) { + if (bucket != null) { + if (versioned) { + var versions = client.listVersions(bucket, ""); + while (true) { + versions.getVersionSummaries().forEach(version -> + client.deleteVersion(bucket, version.getKey(), version.getVersionId())); + if (!versions.isTruncated()) { + break; + } + versions = client.listNextBatchOfVersions(versions); + } + } + client.listObjectsV2(bucket).getObjectSummaries() + .forEach(object -> client.deleteObject(bucket, object.getKey())); + client.deleteBucket(bucket); + } + client.shutdown(); + } + } + + public static class CountingResizeImageFilter extends com.dotmarketing.image.filter.ResizeImageFilter { + static int generations; + @Override + protected String getFilterName() { return "resize"; } + @Override + public File runFilter(final File file, final Map parameters) { + generations++; + return super.runFilter(file, parameters); + } + } + + @Test + void s3ObjectSerializationUsesIndependentStagingAndCleansUpFailures() throws Exception { + final var staged = new java.util.concurrent.CopyOnWriteArrayList(); + final var backing = spy(storage); + doAnswer(call -> { + final com.amazonaws.services.s3.model.PutObjectRequest request = call.getArgument(0); + staged.add(request.getFile()); + return storage.uploadFile(request); + }).when(backing).uploadFile(any(com.amazonaws.services.s3.model.PutObjectRequest.class)); + final var adapter = new AmazonS3StoragePersistenceAPIImpl(backing, bucket, + AmazonS3StoragePersistenceAPIImpl.PathEncryptionMode.NONE); + for (final String value : List.of("first", "second")) { + adapter.pushObject("metadata", "/same.json", new JsonWriterDelegate(), value, Map.of()); + } + assertEquals(2, staged.size()); + assertNotEquals(staged.get(0), staged.get(1), "Independent requests must not share a staging file"); + assertTrue(staged.stream().noneMatch(File::exists)); + assertEquals("second", adapter.pullObject("metadata", "/same.json", new JsonReaderDelegate<>(String.class))); + assertThrows(com.dotmarketing.exception.DotDataException.class, + () -> adapter.pushObject("metadata", "/same.json", (out, value) -> { + out.write(123); + throw new java.io.IOException("injected serialization failure"); + }, "partial", Map.of())); + assertEquals("second", adapter.pullObject("metadata", "/same.json", new JsonReaderDelegate<>(String.class))); + } + + @Test + void slashPrefixedMetadataKeysStillRoundTripWithHashing() throws Exception { + final AmazonS3StoragePersistenceAPIImpl metadata = new AmazonS3StoragePersistenceAPIImpl( + storage, bucket, AmazonS3StoragePersistenceAPIImpl.PathEncryptionMode.SHA256); + final File source = Files.writeString(root.resolve("upload.tmp"), "metadata").toFile(); + final String path = "/a/b/repeated.json/repeated.json"; + metadata.pushFile("metadata", path, source, Map.of()); + final String expectedKey = "metadata/" + org.apache.commons.codec.digest.DigestUtils + .sha256Hex("a/b/repeated.json/") + "/repeated.json"; + assertTrue(client.doesObjectExist(bucket, expectedKey)); + assertEquals(List.of(path), metadata.listObjectPaths("metadata", "/a/b/repeated.json/")); + assertEquals("metadata", Files.readString(metadata.pullFile("metadata", path).toPath())); + metadata.deleteObjectReference("metadata", path); + assertFalse(client.doesObjectExist(bucket, expectedKey)); + } +} diff --git a/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java b/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java new file mode 100644 index 000000000000..a9e3d976ae53 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java @@ -0,0 +1,128 @@ +package com.dotcms.storage; + +import com.amazonaws.services.s3.AmazonS3; +import com.dotcms.enterprise.publishing.staticpublishing.AWSS3Configuration; +import com.dotcms.enterprise.publishing.storage.AWSS3Storage; +import com.dotmarketing.exception.DotRuntimeException; +import com.dotmarketing.util.Config; +import java.util.HashMap; +import java.util.Map; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import static org.junit.jupiter.api.Assertions.*; + +class S3StorageConfigurationTest { + private final Map previous = new HashMap<>(); + + @BeforeEach + void configure() { + for (String key : new String[] {AssetStorageFeature.FLAG, + AmazonS3StoragePersistenceAPIImpl.AWS_S3_ACCESS_KEY_PROP, + AmazonS3StoragePersistenceAPIImpl.AWS_S3_SECRET_ACCESS_KEY_PROP, + AmazonS3StoragePersistenceAPIImpl.AWS_S3_REGION_PROP, + AmazonS3StoragePersistenceAPIImpl.AWS_S3_NAMESPACE_PROP, + AmazonS3StoragePersistenceAPIImpl.AWS_S3_ENDPOINT_PROP}) { + previous.put(key, Config.getStringProperty(key, null)); + Config.setProperty(key, null); + } + Config.setProperty(AssetStorageFeature.FLAG, true); + Config.setProperty(AmazonS3StoragePersistenceAPIImpl.AWS_S3_REGION_PROP, "eu-west-1"); + } + + @AfterEach + void restore() { previous.forEach(Config::setProperty); } + + @Test + void defaultCredentialsRetainRegionAndEndpointAndDisabledConfigurationStaysLegacy() throws Exception { + var provider = new AmazonS3StoragePersistenceAPIImpl(); + var storage = storage(provider); + try { + assertEquals("eu-west-1", client(storage).getRegionName()); + assertTrue(client(storage).getUrl("test-bucket", "file").getHost().contains("eu-west-1")); + } finally { storage.shutdownTransferManager(); } + + Config.setProperty(AmazonS3StoragePersistenceAPIImpl.AWS_S3_ENDPOINT_PROP, "http://127.0.0.1:19077"); + storage = storage(new AmazonS3StoragePersistenceAPIImpl()); + try { + assertEquals("eu-west-1", client(storage).getRegionName()); + assertEquals("127.0.0.1", client(storage).getUrl("test-bucket", "file").getHost()); + assertEquals(19077, client(storage).getUrl("test-bucket", "file").getPort()); + } finally { storage.shutdownTransferManager(); } + + Config.setProperty(AssetStorageFeature.FLAG, false); + storage = storage(new AmazonS3StoragePersistenceAPIImpl()); + try { + assertEquals("test-bucket.s3.amazonaws.com", client(storage).getUrl("test-bucket", "file").getHost()); + } finally { storage.shutdownTransferManager(); } + } + + @Test + void partialExplicitCredentialsFailBeforeFallingBackToIam() throws Exception { + for (String key : new String[] {AmazonS3StoragePersistenceAPIImpl.AWS_S3_ACCESS_KEY_PROP, + AmazonS3StoragePersistenceAPIImpl.AWS_S3_SECRET_ACCESS_KEY_PROP}) { + Config.setProperty(key, "incomplete-credential"); + assertThrows(DotRuntimeException.class, AmazonS3StoragePersistenceAPIImpl::new); + Config.setProperty(key, null); + } + Config.setProperty(AssetStorageFeature.FLAG, false); + Config.setProperty(AmazonS3StoragePersistenceAPIImpl.AWS_S3_ACCESS_KEY_PROP, "legacy-partial-key"); + final var storage = storage(new AmazonS3StoragePersistenceAPIImpl()); + try { assertEquals("test-bucket.s3.amazonaws.com", client(storage).getUrl("test-bucket", "file").getHost()); } + finally { storage.shutdownTransferManager(); } + } + + @Test + void customEndpointNeedsAValidUrlAndSigningRegion() throws Exception { + for (String endpoint : new String[] {"not a URL", "ftp://127.0.0.1", "http:///missing-host"}) { + Config.setProperty(AmazonS3StoragePersistenceAPIImpl.AWS_S3_ENDPOINT_PROP, endpoint); + assertThrows(DotRuntimeException.class, AmazonS3StoragePersistenceAPIImpl::new); + } + Config.setProperty(AmazonS3StoragePersistenceAPIImpl.AWS_S3_ENDPOINT_PROP, "http://127.0.0.1:19077"); + Config.setProperty(AmazonS3StoragePersistenceAPIImpl.AWS_S3_REGION_PROP, null); + assertThrows(DotRuntimeException.class, AmazonS3StoragePersistenceAPIImpl::new); + + Config.setProperty(AssetStorageFeature.FLAG, false); + Config.setProperty(AmazonS3StoragePersistenceAPIImpl.AWS_S3_ENDPOINT_PROP, "not a URL"); + final var storage = storage(new AmazonS3StoragePersistenceAPIImpl()); + try { assertEquals("test-bucket.s3.amazonaws.com", client(storage).getUrl("test-bucket", "file").getHost()); } + finally { storage.shutdownTransferManager(); } + } + + @Test + void namespaceValidationIsGatedAndRunsBeforeConnecting() throws Exception { + for (String namespace : new String[] {"../other", "/absolute", "nested/path", "a.b", "a b", "x".repeat(65)}) { + Config.setProperty(AmazonS3StoragePersistenceAPIImpl.AWS_S3_NAMESPACE_PROP, namespace); + assertThrows(DotRuntimeException.class, AmazonS3StoragePersistenceAPIImpl::new, namespace); + } + Config.setProperty(AssetStorageFeature.FLAG, false); + final var storage = storage(new AmazonS3StoragePersistenceAPIImpl()); + try { assertEquals("test-bucket.s3.amazonaws.com", client(storage).getUrl("test-bucket", "file").getHost()); } + finally { storage.shutdownTransferManager(); } + } + + @Test + void publishingEndpointUsesItsOwnConfiguration() throws Exception { + Config.setProperty(AmazonS3StoragePersistenceAPIImpl.AWS_S3_ENDPOINT_PROP, "http://127.0.0.1:19077"); + final var storage = new AWSS3Storage(new AWSS3Configuration.Builder() + .accessKey("publisher-key").secretKey("publisher-secret") + .region("ap-southeast-2").endPoint("http://127.0.0.1:19078").build()); + try { + assertEquals("ap-southeast-2", client(storage).getRegionName()); + assertEquals(19078, client(storage).getUrl("test-bucket", "file").getPort()); + } finally { storage.shutdownTransferManager(); } + } + + // Inspect the real SDK configuration without making network requests or exposing production getters. + static AWSS3Storage storage(AmazonS3StoragePersistenceAPIImpl provider) throws Exception { + final var field = AmazonS3StoragePersistenceAPIImpl.class.getDeclaredField("storage"); + field.setAccessible(true); + return (AWSS3Storage) field.get(provider); + } + + private static AmazonS3 client(AWSS3Storage storage) throws Exception { + final var field = AWSS3Storage.class.getDeclaredField("s3client"); + field.setAccessible(true); + return (AmazonS3) field.get(storage); + } +} From f88d1717fc96e44c0db7b631d6af2196b28464d5 Mon Sep 17 00:00:00 2001 From: Scott Wicken <1562170+swicken@users.noreply.github.com> Date: Tue, 29 Sep 2026 13:07:35 -0400 Subject: [PATCH 2/7] refactor(storage): put versioned-object operations on StoragePersistenceAPI and route remote storage through the provider --- .../AmazonS3StoragePersistenceAPIImpl.java | 5 +- .../com/dotcms/storage/ObjectSnapshot.java | 13 +++++ .../storage/SharedExtractedMetadata.java | 6 +-- .../dotcms/storage/StoragePersistenceAPI.java | 47 +++++++++++++++++++ .../storage/StoragePersistenceProvider.java | 22 +++++++++ .../VersionedObjectStorageDefaultsTest.java | 29 ++++++++++++ 6 files changed, 117 insertions(+), 5 deletions(-) create mode 100644 dotCMS/src/main/java/com/dotcms/storage/ObjectSnapshot.java create mode 100644 dotCMS/src/test/java/com/dotcms/storage/VersionedObjectStorageDefaultsTest.java diff --git a/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java b/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java index 5f9145d80b10..55fa3ee981b0 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java @@ -543,8 +543,7 @@ public Object pullObject(final String groupName, final String path, final Object } /** Value and version are from one response; modified is the S3 server timestamp. */ - public record ObjectSnapshot(String path, Object value, String version, long modified) { } - + @Override public ObjectSnapshot readObjectSnapshot(final String group, final String path, final ObjectReaderDelegate reader) throws DotDataException { if (!AssetStorageFeature.isEnabled()) throw new DotDataException("S3 asset storage is disabled"); @@ -557,6 +556,7 @@ public ObjectSnapshot readObjectSnapshot(final String group, final String path, } } + @Override public List listObjectSnapshots(final String group, final String prefix) throws DotDataException { if (!AssetStorageFeature.isEnabled() || pathEncryptionMode != PathEncryptionMode.NONE) { throw new DotDataException("Version listing requires enabled S3 plain paths"); @@ -573,6 +573,7 @@ public List listObjectSnapshots(final String group, final String } /** Returns the new version, or null on a concurrent change; never retries a stale write. */ + @Override public String writeObjectIfMatch(final String group, final String path, final Serializable value, final String version) throws DotDataException { if (!AssetStorageFeature.isEnabled()) throw new DotDataException("S3 asset storage is disabled"); diff --git a/dotCMS/src/main/java/com/dotcms/storage/ObjectSnapshot.java b/dotCMS/src/main/java/com/dotcms/storage/ObjectSnapshot.java new file mode 100644 index 000000000000..a008a2382987 --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/storage/ObjectSnapshot.java @@ -0,0 +1,13 @@ +package com.dotcms.storage; + +/** + * One stored object as seen by a single read or listing: its path, value, version and the store's own + * timestamp. The value and version come from the same response, so the version can be passed to + * {@link StoragePersistenceAPI#writeObjectIfMatch} to change exactly what was read. + * + * @param path the object's path within its group + * @param value the deserialized value, or {@code null} for a listing entry + * @param version the store's version tag for this exact content (an S3 ETag) + * @param modified the store's last-modified time in milliseconds, independent of node clocks + */ +public record ObjectSnapshot(String path, Object value, String version, long modified) { } diff --git a/dotCMS/src/main/java/com/dotcms/storage/SharedExtractedMetadata.java b/dotCMS/src/main/java/com/dotcms/storage/SharedExtractedMetadata.java index 251596318dd5..099571bc95ad 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/SharedExtractedMetadata.java +++ b/dotCMS/src/main/java/com/dotcms/storage/SharedExtractedMetadata.java @@ -13,15 +13,15 @@ /** Only byte-derived extraction goes here; filenames and editorial metadata are merged afterwards. */ final class SharedExtractedMetadata { static final String GROUP = "extracted-metadata"; - private final AmazonS3StoragePersistenceAPIImpl storage; + private final StoragePersistenceAPI storage; private static class Holder { private static final SharedExtractedMetadata INSTANCE = new SharedExtractedMetadata( - AmazonS3StoragePersistenceAPIImpl.withPlainPaths()); + StoragePersistenceProvider.remoteObjectStorage()); } static SharedExtractedMetadata getInstance() { return Holder.INSTANCE; } - SharedExtractedMetadata(AmazonS3StoragePersistenceAPIImpl storage) { this.storage = storage; } + SharedExtractedMetadata(StoragePersistenceAPI storage) { this.storage = storage; } @SuppressWarnings("unchecked") Map get(File source, String extractorVersion, int schemaVersion, int textLimit, diff --git a/dotCMS/src/main/java/com/dotcms/storage/StoragePersistenceAPI.java b/dotCMS/src/main/java/com/dotcms/storage/StoragePersistenceAPI.java index 7316043b0dbf..48dc341bb0c2 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/StoragePersistenceAPI.java +++ b/dotCMS/src/main/java/com/dotcms/storage/StoragePersistenceAPI.java @@ -236,6 +236,53 @@ default boolean backfillObject(final String groupName, final String path, final return false; } + /** + * Reads an object together with its version and the store's timestamp, from one response. + * + * @param groupName the group to read from + * @param path the object's path + * @param reader deserializes the stored value + * @return the snapshot, or {@code null} if the object does not exist + * @throws DotDataException if the read fails, or the store does not support versioned objects + */ + default ObjectSnapshot readObjectSnapshot(final String groupName, final String path, + final ObjectReaderDelegate reader) throws DotDataException { + throw unsupportedVersionedObjects(); + } + + /** + * Lists the objects under a prefix with their versions and the store's timestamps, without values. + * + * @param groupName the group to list + * @param prefix the path prefix + * @return one snapshot per object, each with a {@code null} value + * @throws DotDataException if the listing fails, or the store does not support versioned objects + */ + default List listObjectSnapshots(final String groupName, final String prefix) + throws DotDataException { + throw unsupportedVersionedObjects(); + } + + /** + * Writes an object only if it is still at the given version, or only if it is absent when the + * version is {@code null}. Concurrent writers use this to detect each other instead of overwriting. + * + * @param groupName the group to write to + * @param path the object's path + * @param value the value to store + * @param version the expected current version, or {@code null} to require that the object is absent + * @return the new version, or {@code null} if the object changed concurrently + * @throws DotDataException if the write fails, or the store does not support versioned objects + */ + default String writeObjectIfMatch(final String groupName, final String path, final Serializable value, + final String version) throws DotDataException { + throw unsupportedVersionedObjects(); + } + + private DotDataException unsupportedVersionedObjects() { + return new DotDataException(getClass().getSimpleName() + " does not support versioned objects"); + } + /** * Takes the information from the object and writes it to the specified file using the Writer * Delegate to do so. This way, there's no in-memory loading. diff --git a/dotCMS/src/main/java/com/dotcms/storage/StoragePersistenceProvider.java b/dotCMS/src/main/java/com/dotcms/storage/StoragePersistenceProvider.java index 26f14d6845f8..0fbe7bc0fdf7 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/StoragePersistenceProvider.java +++ b/dotCMS/src/main/java/com/dotcms/storage/StoragePersistenceProvider.java @@ -185,6 +185,28 @@ public void forceInitialize(){ storagePersistenceInstances.clear(); } + /** + * Returns the durable remote object store with plain, listable keys, for features that keep their + * own records there (temporary uploads, WebDAV staging, recovery archives, publishing bundles and + * shared extraction). Today this is always S3; callers depend on {@link StoragePersistenceAPI} so + * another remote provider can be added here. + * + * @return a new store instance using plain paths + */ + public static StoragePersistenceAPI remoteObjectStorage() { + return AmazonS3StoragePersistenceAPIImpl.withPlainPaths(); + } + + /** + * Returns the storage type of the durable remote store, for code that routes through + * {@link StorageKey} or {@link #getStorage(StorageType)}. + * + * @return the remote storage type, today always {@link StorageType#S3} + */ + public static StorageType remoteStorageType() { + return StorageType.S3; + } + public enum INSTANCE { INSTANCE; diff --git a/dotCMS/src/test/java/com/dotcms/storage/VersionedObjectStorageDefaultsTest.java b/dotCMS/src/test/java/com/dotcms/storage/VersionedObjectStorageDefaultsTest.java new file mode 100644 index 000000000000..de4967cc4af7 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/storage/VersionedObjectStorageDefaultsTest.java @@ -0,0 +1,29 @@ +package com.dotcms.storage; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; + +import com.dotmarketing.exception.DotDataException; +import java.util.HashMap; +import org.junit.jupiter.api.Test; + +/** + * Versioned-object operations are part of the storage contract so callers depend on the interface, + * not the S3 implementation. A store that cannot provide them must refuse clearly rather than + * pretend: callers rely on these operations for concurrency safety. + */ +class VersionedObjectStorageDefaultsTest { + @Test + void storesWithoutVersionedObjectsRefuseEachOperationWithAClearError() { + final StoragePersistenceAPI store = mock(StoragePersistenceAPI.class, CALLS_REAL_METHODS); + final var write = assertThrows(DotDataException.class, + () -> store.writeObjectIfMatch("group", "path", new HashMap(), null)); + final var read = assertThrows(DotDataException.class, + () -> store.readObjectSnapshot("group", "path", new JsonReaderDelegate<>(java.util.Map.class))); + final var list = assertThrows(DotDataException.class, + () -> store.listObjectSnapshots("group", "prefix/")); + for (DotDataException refused : java.util.List.of(write, read, list)) { + assertTrue(refused.getMessage().contains("does not support versioned objects"), refused.getMessage()); + } + } +} From 0720aa289c48b5115cdf1602adff32b657bbc35b Mon Sep 17 00:00:00 2001 From: Scott Wicken <1562170+swicken@users.noreply.github.com> Date: Fri, 2 Oct 2026 10:55:45 -0400 Subject: [PATCH 3/7] fix(storage): keep flag-off AWS credential resolution and harden the flag-on storage layer With the flag off, every place dotCMS falls back to the AWS default credential chain (static push publishing, endpoint validation and the S3 metadata provider) now uses NoWebIdentityCredentialsProviderChain, the SDK default chain without its web identity step. Before the STS module was packaged that step always failed over, so a pod with a web identity token keeps the identity it used before. With the flag on the standard chain is unchanged. AssetStorageFeature logs at INFO only when the flag is on; with it off the first read logs at debug, so a flag-off node writes no new INFO line. The chain's pullObject now restores missing local copies under the same per-key lock as writes and deletes, so a restore can no longer overwrite a newer write or bring back a deleted object. A zero-length or truncated local metadata file is now replaced from S3 with the flag on: the filesystem provider reports it as UnreadableStoredObjectException and keeps it, and the chain overwrites it with a readable durable copy. Without one the read fails as before, so the metadata is never treated as absent and regenerated. Storage gains listFirstObject, a one-key listing. The S3 provider uses it for existsGroup, existsObject and hasDurableCopy instead of listing every page under the prefix. With the flag on, static push and metadata clients without a region or custom endpoint use the global endpoint and no signer override, so the SDK looks up the bucket's region and signs SigV4 for it instead of failing to find a region. The credential-chain constructor is no longer pinned to us-west-2 with the flag on. The filesystem listObjectPaths resolves its prefix through normalizePath, as writes do. The S3 provider's pushFile starts its upload inside its per-key lock, so a lock timeout no longer leaves an upload running and pushes of one key on a node are ordered. Unreachable flag checks in the S3 and filesystem providers are removed, and the doc now describes flag-off credential behavior, the region handling, the corrupt-cache and locking behavior, and that both asset-blobs/ and extracted-metadata/ are shared across namespaces. --- docs/testing/BINARY_S3_STORAGE.md | 50 +++++++-- .../staticpublishing/AWSS3Publisher.java | 5 +- .../publishing/storage/AWSS3Storage.java | 68 +++++++++++- .../publishing/storage/Storage.java | 19 ++++ .../bean/impl/AWSS3PublishingEndPoint.java | 11 +- .../AmazonS3StoragePersistenceAPIImpl.java | 103 ++++++++++++++---- .../dotcms/storage/AssetStorageFeature.java | 10 +- .../ChainableStoragePersistenceAPI.java | 34 +++++- .../FileSystemStoragePersistenceAPIImpl.java | 47 ++++++-- ...NoWebIdentityCredentialsProviderChain.java | 76 +++++++++++++ .../UnreadableStoredObjectException.java | 22 ++++ .../storage/AssetStorageFeatureTest.java | 97 +++++++++++++++++ ...bIdentityCredentialsProviderChainTest.java | 52 +++++++++ .../storage/S3StorageConfigurationTest.java | 31 ++++++ 14 files changed, 577 insertions(+), 48 deletions(-) create mode 100644 dotCMS/src/main/java/com/dotcms/storage/NoWebIdentityCredentialsProviderChain.java create mode 100644 dotCMS/src/main/java/com/dotcms/storage/UnreadableStoredObjectException.java create mode 100644 dotCMS/src/test/java/com/dotcms/storage/NoWebIdentityCredentialsProviderChainTest.java diff --git a/docs/testing/BINARY_S3_STORAGE.md b/docs/testing/BINARY_S3_STORAGE.md index f46533c3fad8..07724c9a7678 100644 --- a/docs/testing/BINARY_S3_STORAGE.md +++ b/docs/testing/BINARY_S3_STORAGE.md @@ -6,6 +6,23 @@ the filesystem/NFS behavior is unchanged. This page describes what is in the code today. It grows as the remaining parts land. +## Behavior with the flag off + +With the flag off, the storage chain, its providers, metadata and static push publishing run +the same code paths as before. Two details are worth knowing: + +- This feature packages the AWS STS module (`aws-java-sdk-sts`). Before, the web identity step + of the SDK's default credential chain always failed for lack of that module, and the chain + moved on to the profile files and then the container or EC2 instance role. With the flag off, + every place where dotCMS falls back to the default chain (static push publishing, endpoint + validation and the S3 metadata provider) uses `NoWebIdentityCredentialsProviderChain`, which + is that chain without the web identity step, so a pod with a web identity token keeps using + the identity it used before. Two differences remain because the module is on the classpath: + a profile in the AWS config files that assumes a role (`role_arn`) now resolves instead of + failing over to the instance role, and an OSGi plugin that builds its own + `DefaultAWSCredentialsProviderChain` now gets the web identity step. +- The flag is logged at INFO only when it is on. With it off, the first read logs at debug. + ## Feature flag Set `FEATURE_FLAG_S3_ASSET_STORAGE=true` (Docker: `DOT_FEATURE_FLAG_S3_ASSET_STORAGE=true`) @@ -25,7 +42,12 @@ The storage chain (`ChainableStoragePersistenceAPI`) and its providers change as - Writes publish to every durable provider before the new local copy becomes visible, and a failed remote write keeps the previous local contents. - Reads restore a missing local copy from S3. A miss is not cached as a 404, because another - node can upload the same key at any time. + node can upload the same key at any time. Object reads restore under the same per-key lock + as writes and deletes, so a restore cannot overwrite a newer write on the same chain. +- A zero-length or truncated local metadata file is a read failure, not a miss. When S3 holds a + readable copy, the chain replaces the local file with it under the per-key lock. Otherwise the + read fails and the local file is kept, so the metadata is never treated as absent and + regenerated, which would drop custom attributes such as a focal point. - Deletes keep the local copy until all durable providers accept the deletion, so an in-flight restore on the same chain cannot bring the object back. - Storage and database query failures propagate instead of being reported as a missing @@ -33,6 +55,9 @@ The storage chain (`ChainableStoragePersistenceAPI`) and its providers change as - Providers can list objects under a prefix, verify that a durable copy has the same contents (`hasDurableCopy`), and copy an existing file or object to durable storage without overwriting a conflicting one (`backfillFile`, `backfillObject`). +- S3 existence checks (`existsGroup`, `existsObject`, `hasDurableCopy`) list at most one key + per lookup. Only listings that need every key (`listObjectPaths`, `listObjectSnapshots`, + `deleteGroup` and static push) follow every page. Two building blocks are included for the slices that follow. `S3ContentAddressedStorage` stores one immutable copy of each set of bytes at @@ -46,10 +71,11 @@ With the flag on, leave both `storage.file-metadata.s3.access-key` and `storage.file-metadata.s3.secret-access-key` unset to use the AWS SDK default credential provider chain. The configured bucket region and endpoint are kept in that mode. A partial key pair fails initialization. The AWS STS module is packaged so role-based providers, -including web identity, work. +including web identity, work with the flag on. -Without a custom endpoint, a configured region selects the regional AWS endpoint, and an -absent region falls back to the SDK region provider chain. A custom endpoint +Without a custom endpoint, a configured region selects the regional AWS endpoint. With no +region, the client uses the global endpoint and the SDK looks up the bucket's region on first +use. Requests are signed with SigV4, the SDK's default S3 signer. A custom endpoint (`storage.file-metadata.s3.endpoint`) must be an absolute HTTP(S) URL and requires `storage.file-metadata.s3.bucket-region` for request signing. Invalid endpoint configuration fails instead of silently selecting AWS. With the flag off, configuration behaves as before. @@ -58,8 +84,10 @@ fails instead of silently selecting AWS. With the flag off, configuration behave With the flag on, `storage.file-metadata.s3.namespace` (Docker: `DOT_STORAGE_FILE_METADATA_S3_NAMESPACE`) separates the keys of installations that share a -bucket. Owned keys become `asset-namespaces///`; shared -extracted metadata stays at the bucket root so identical bytes are extracted once. Use the same +bucket. Owned keys become `asset-namespaces///`. Two prefixes +stay at the bucket root and are shared by every namespace: content-addressed blobs +(`asset-blobs/`), so identical bytes are stored once, and shared extracted metadata +(`extracted-metadata/`), so identical bytes are extracted once. Use the same value for all nodes in one cluster and distinct values for independent installations. Values are 1 to 64 letters, digits, underscores or hyphens, starting with a letter or digit. Empty is the default and keeps the existing layout. @@ -71,8 +99,12 @@ bucket credentials can access. ## Static push publishing Enabling the flag also changes static push publishing to S3 (`AWSS3Storage`). Clients sign -with SigV4, resolve the endpoint from the region when no endpoint is configured, and list -every page of objects instead of only the first 1,000. +with SigV4 and list every page of objects instead of only the first 1,000. Without a custom +endpoint, a configured region selects that region's endpoint; with no region, the client uses +the global endpoint and the SDK looks up the bucket's region, so a region is not required. +Endpoints that use the default credential chain are not pinned to a region either. Their +configured endpoint and region are still not passed to the client on that path, as before, so +an S3-compatible custom endpoint needs a key and secret. ## Running the checks @@ -88,7 +120,7 @@ docker run -d --rm --name binary-s3-test \ minio/minio:latest server /data ./mvnw test -pl :dotcms-core -Dmaven.build.cache.enabled=false \ - -Dtest=AssetStorageFeatureTest,AssetStorageFeatureLatchTest,S3StorageConfigurationTest,BinaryS3StorageTest \ + -Dtest=AssetStorageFeatureTest,AssetStorageFeatureLatchTest,S3StorageConfigurationTest,NoWebIdentityCredentialsProviderChainTest,BinaryS3StorageTest \ -Ds3.test.endpoint=http://127.0.0.1:19002 docker stop binary-s3-test diff --git a/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/staticpublishing/AWSS3Publisher.java b/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/staticpublishing/AWSS3Publisher.java index 5c592cd84f8e..a9018c117814 100644 --- a/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/staticpublishing/AWSS3Publisher.java +++ b/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/staticpublishing/AWSS3Publisher.java @@ -20,6 +20,7 @@ import com.dotcms.publisher.environment.business.EnvironmentAPI; import com.dotcms.publisher.pusher.PushUtils; import com.dotcms.publisher.util.PusheableAsset; +import com.dotcms.storage.NoWebIdentityCredentialsProviderChain; import com.dotcms.publishing.*; import com.dotcms.system.event.local.business.LocalSystemEventsAPI; import com.dotcms.system.event.local.type.pushpublish.EndpointFailureDetail; @@ -507,6 +508,8 @@ private String updateStatusFailedToSend(PublishAuditHistory currentStatusHistory /** * Creates and returns an AWSS3EndPointPublisher. * Uses the default AWS credentials provider chain if the tokenProp and/or secretProp are not set. + * With S3 asset storage off, that chain leaves out the web identity step, as it did before the + * STS module was packaged (see {@link NoWebIdentityCredentialsProviderChain#defaultChain()}). * Uses the provided tokenProp and secretProp if they are both set. * * @param tokenProp - the AWS credentials token key @@ -519,7 +522,7 @@ private AWSS3EndPointPublisher getAWSS3EndPointPublisher(final String tokenProp, final String secretProp, final String endPoint, final String region) { AWSS3EndPointPublisher endPointPublisher; if (!UtilMethods.isSet(tokenProp) || !UtilMethods.isSet(secretProp)) { - DefaultAWSCredentialsProviderChain creds = new DefaultAWSCredentialsProviderChain(); + DefaultAWSCredentialsProviderChain creds = NoWebIdentityCredentialsProviderChain.defaultChain(); endPointPublisher = new AWSS3EndPointPublisher(creds); } else { AWSS3Configuration awss3Configuration = diff --git a/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/AWSS3Storage.java b/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/AWSS3Storage.java index 2ce26d19cbeb..04ef257bdea9 100644 --- a/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/AWSS3Storage.java +++ b/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/AWSS3Storage.java @@ -47,10 +47,28 @@ public class AWSS3Storage implements Storage { private final AmazonS3 s3client; private final TransferManager transferManager; + /** + * Creates a storage client that authenticates through the given AWS default credential chain + * and uses the global S3 endpoint. With S3 asset storage off, requests are signed for + * {@code us-west-2} as before. With it on, no region is pinned, so the SDK looks up each + * bucket's region on first use and signs for that region. + * + * @param credentialsProviderChain the credential chain to authenticate with + */ public AWSS3Storage(DefaultAWSCredentialsProviderChain credentialsProviderChain) { - this (getAmazonS3Client(credentialsProviderChain , null, DEFAULT_S3_REGION)); + this (getAmazonS3Client(credentialsProviderChain , null, + com.dotcms.storage.AssetStorageFeature.isEnabled() ? null : DEFAULT_S3_REGION)); } + /** + * Creates a storage client from any credential provider, with an optional custom endpoint and + * region. See {@link #getAmazonS3Client(AWSCredentialsProvider, String, String)} for how the two + * are applied. + * + * @param credentialsProvider the credentials to authenticate with + * @param endpoint a custom S3-compatible endpoint, or {@code null} for AWS + * @param region the bucket region, or {@code null} + */ public AWSS3Storage(final AWSCredentialsProvider credentialsProvider, final String endpoint, final String region) { this(getAmazonS3Client(credentialsProvider, endpoint, region)); @@ -67,6 +85,18 @@ private static AmazonS3 getAmazonS3Client(final String accessKey, , endPoint, region); } + /** + * Builds the S3 client. With S3 asset storage off, the client always uses the configured + * endpoint, or the global endpoint {@code s3.amazonaws.com}, with the given signing region, as + * before. With it on, a custom endpoint is used with the given signing region, a region alone + * selects that region's AWS endpoint, and with neither the client uses the global endpoint and + * the SDK looks up each bucket's region on first use instead of requiring one. + * + * @param credentialsProvider the credentials to authenticate with + * @param endPoint a custom endpoint, or {@code null} + * @param region the signing region, or {@code null} + * @return the configured client + */ private static AmazonS3 getAmazonS3Client(final AWSCredentialsProvider credentialsProvider, final String endPoint, final String region) { @@ -74,7 +104,8 @@ private static AmazonS3 getAmazonS3Client(final AWSCredentialsProvider credentia final AmazonS3ClientBuilder builder = AmazonS3ClientBuilder.standard() .withCredentials(credentialsProvider) .withClientConfiguration(getClientConfiguration()); - if (!com.dotcms.storage.AssetStorageFeature.isEnabled() || UtilMethods.isSet(endPoint)) { + if (!com.dotcms.storage.AssetStorageFeature.isEnabled() || UtilMethods.isSet(endPoint) + || !UtilMethods.isSet(region)) { builder.withEndpointConfiguration( new AwsClientBuilder.EndpointConfiguration( UtilMethods.isSet(endPoint) ? endPoint : "s3.amazonaws.com", @@ -96,9 +127,19 @@ private AWSS3Storage(AmazonS3 s3client) { } + /** + * Returns the client configuration. With S3 asset storage off, requests keep the legacy + * {@code S3SignerType} override. With it on, no signer is forced: the SDK's default S3 signer is + * already SigV4, and an explicit override would stop the SDK from looking up a bucket's region + * when the client has none configured. + * + * @return a new client configuration + */ private static ClientConfiguration getClientConfiguration(){ ClientConfiguration conf = new ClientConfiguration(); - conf.setSignerOverride(com.dotcms.storage.AssetStorageFeature.isEnabled() ? "AWSS3V4SignerType" : "S3SignerType"); + if (!com.dotcms.storage.AssetStorageFeature.isEnabled()) { + conf.setSignerOverride("S3SignerType"); + } return conf; } @@ -214,6 +255,27 @@ public ObjectListing listObjects(String bucketName, String folderPath) throws Do } } + /** + * Lists at most one key under the prefix, without following further pages. + * + * @param bucketName the bucket to look in + * @param prefix the key prefix + * @return the first matching object summary in key order, or {@code null} + * @throws DotRuntimeException if S3 rejects the request or the client fails + */ + @Override + public S3ObjectSummary listFirstObject(final String bucketName, final String prefix) throws DotRuntimeException { + try { + final List objects = s3client.listObjects(new ListObjectsRequest() + .withBucketName(bucketName).withPrefix(prefix).withMaxKeys(1)).getObjectSummaries(); + return objects.isEmpty() ? null : objects.get(0); + } catch (AmazonServiceException ase) { + throw new DotRuntimeException("Caught an error from Amazon S3: request made but was rejected", ase); + } catch (AmazonClientException ace) { + throw new DotRuntimeException("Caught an error from Amazon S3: client encountered an internal error", ace); + } + } + @Override public void deleteFolder(final String bucketName, final String folderName) throws DotRuntimeException { try { diff --git a/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/Storage.java b/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/Storage.java index 3aa911badf9d..71d66f5b0444 100644 --- a/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/Storage.java +++ b/dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/storage/Storage.java @@ -14,6 +14,7 @@ import com.amazonaws.services.s3.model.ObjectListing; import com.amazonaws.services.s3.model.Owner; import com.amazonaws.services.s3.model.PutObjectRequest; +import com.amazonaws.services.s3.model.S3ObjectSummary; import com.amazonaws.services.s3.transfer.Download; import com.amazonaws.services.s3.transfer.MultipleFileUpload; import com.amazonaws.services.s3.transfer.ObjectMetadataProvider; @@ -126,6 +127,24 @@ default boolean fileContentsMatch(final String bucketName, final String key, fin */ ObjectListing listObjects(final String bucketName, final String folderPath) throws DotRuntimeException; + /** + * Returns the first object, in key order, whose key starts with the prefix, or {@code null} when + * there is none. S3 lists keys in ascending order, so when an object's key equals the prefix it + * is the one returned. Use this instead of {@link #listObjects(String, String)} when only + * existence matters: it asks for one key instead of every page under the prefix. The default + * implementation filters a full listing so existing implementations keep working. + * + * @param bucketName the bucket to look in + * @param prefix the key prefix + * @return the first matching object summary, or {@code null} + * @throws DotRuntimeException if the listing fails + */ + default S3ObjectSummary listFirstObject(final String bucketName, + final String prefix) throws DotRuntimeException { + final List objects = listObjects(bucketName, prefix).getObjectSummaries(); + return objects.isEmpty() ? null : objects.get(0); + } + /** * Performs a head bucket operation on the requested bucket name. This operation is useful to determine if a bucket exists and you have permission to access it. */ diff --git a/dotCMS/src/main/java/com/dotcms/publisher/endpoint/bean/impl/AWSS3PublishingEndPoint.java b/dotCMS/src/main/java/com/dotcms/publisher/endpoint/bean/impl/AWSS3PublishingEndPoint.java index 6f824e11ee25..11d7ccd3800e 100644 --- a/dotCMS/src/main/java/com/dotcms/publisher/endpoint/bean/impl/AWSS3PublishingEndPoint.java +++ b/dotCMS/src/main/java/com/dotcms/publisher/endpoint/bean/impl/AWSS3PublishingEndPoint.java @@ -10,6 +10,7 @@ import com.dotcms.enterprise.publishing.staticpublishing.AWSS3Publisher; import com.dotcms.enterprise.publishing.staticpublishing.EndPointPublisherConnectionException; import com.dotcms.publisher.endpoint.bean.PublishingEndPoint; +import com.dotcms.storage.NoWebIdentityCredentialsProviderChain; import com.amazonaws.auth.DefaultAWSCredentialsProviderChain; import com.dotmarketing.cms.factories.PublicEncryptionFactory; import com.dotmarketing.exception.DotRuntimeException; @@ -54,6 +55,14 @@ public Class getPublisher() { return AWSS3Publisher.class; }// getPublisher. + /** + * Checks that the endpoint's AWS S3 properties are present and that dotCMS can reach the + * bucket with them. Without a token and secret, the connection test uses the AWS default + * credential chain, which leaves out web identity while S3 asset storage is off (see + * {@link NoWebIdentityCredentialsProviderChain#defaultChain()}). + * + * @throws PublishingEndPointValidationException if a required property is missing or invalid + */ @Override public void validatePublishingEndPoint() throws PublishingEndPointValidationException { @@ -94,7 +103,7 @@ public void validatePublishingEndPoint() throws PublishingEndPointValidationExce if (!UtilMethods.isSet(token) || !UtilMethods.isSet(secret)) { // Validate DefaultAWSCredentialsProviderChain configuration - DefaultAWSCredentialsProviderChain creds = new DefaultAWSCredentialsProviderChain(); + DefaultAWSCredentialsProviderChain creds = NoWebIdentityCredentialsProviderChain.defaultChain(); new AWSS3EndPointPublisher(creds).checkConnectSuccessfully(bucketValidationName); } else { diff --git a/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java b/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java index 55fa3ee981b0..a6962be81335 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java @@ -1,6 +1,5 @@ package com.dotcms.storage; -import com.amazonaws.auth.DefaultAWSCredentialsProviderChain; import com.amazonaws.services.s3.model.Bucket; import com.amazonaws.services.s3.model.PutObjectRequest; import org.apache.commons.codec.digest.DigestUtils; @@ -126,6 +125,15 @@ private FileRepositoryManager getFileRepository() { } } + /** + * Creates the provider from the {@code storage.file-metadata.s3.*} configuration. Without both + * keys it authenticates through the AWS default credential chain, which leaves out web identity + * while S3 asset storage is off (see {@link NoWebIdentityCredentialsProviderChain#defaultChain()}). + * With the flag on, an invalid namespace, a partial key pair, an invalid custom endpoint, or a + * custom endpoint without a region fails here. + * + * @throws DotRuntimeException if the flag is on and the configuration is invalid + */ public AmazonS3StoragePersistenceAPIImpl() { this.lockManager = DotConcurrentFactory.getInstance().getIdentifierStripedLock(); // todo: this should be from an app, just need to pass the host name fallback to system @@ -157,8 +165,8 @@ public AmazonS3StoragePersistenceAPIImpl() { } this.storage = !UtilMethods.isSet(accessKey) || !UtilMethods.isSet(secretAccessKey) ? (AssetStorageFeature.isEnabled() - ? new AWSS3Storage(new DefaultAWSCredentialsProviderChain(), endpoint, region) - : new AWSS3Storage(new DefaultAWSCredentialsProviderChain())) : + ? new AWSS3Storage(NoWebIdentityCredentialsProviderChain.defaultChain(), endpoint, region) + : new AWSS3Storage(NoWebIdentityCredentialsProviderChain.defaultChain())) : new AWSS3Storage(new AWSS3Configuration.Builder().accessKey(accessKey).secretKey(secretAccessKey).endPoint(endpoint).region(region).build()); } @@ -190,6 +198,14 @@ public static AmazonS3StoragePersistenceAPIImpl withPlainPaths() { return impl; } + /** + * Reports whether the group holds at least one object in the bucket. With S3 asset storage on, + * this lists at most one key under the group, and only a positive answer is remembered. + * + * @param groupName the group to check + * @return {@code true} if the bucket exists and the group has an object + * @throws DotDataException if the check fails + */ @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public boolean existsGroup(final String groupName) throws DotDataException { @@ -217,8 +233,8 @@ public boolean existsGroup(final String groupName) throws DotDataException { } boolean objectExists = this.storage.existsBucket(this.bucketName); if (objectExists) { - objectExists = !this.storage.listObjects(this.bucketName, - groupKey(groupName) + FORWARD_SLASH).getObjectSummaries().isEmpty(); + objectExists = this.storage.listFirstObject(this.bucketName, + groupKey(groupName) + FORWARD_SLASH) != null; if (!objectExists) { Logger.debug(this, () -> String.format("Group '%s' does not exist", groupName)); } @@ -231,16 +247,31 @@ public boolean existsGroup(final String groupName) throws DotDataException { return objectExists; } + /** + * Reports whether an object exists at the path. With S3 asset storage off, any key that starts + * with the path counts. With it on, the exact key or a key beneath the path as a folder counts, + * and each lookup lists at most one key instead of every key under the path. + * + * @param groupName the group to look in + * @param objectPath the object's path + * @return {@code true} if the object, or something beneath it, exists + * @throws DotDataException if the flag is on and the check fails + */ @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public boolean existsObject(final String groupName, final String objectPath) throws DotDataException { final String correctedPath = transformReadPath(groupName, objectPath); if (AssetStorageFeature.isEnabled()) { try { - final var objects = this.storage.listObjects(this.bucketName, correctedPath); + // An exact key sorts before every longer key under the same prefix, so one key is enough + // to see it. Otherwise a second one-key lookup checks for objects stored beneath the path. + final var first = this.storage.listFirstObject(this.bucketName, correctedPath); + if (first == null) { + return false; + } final String directory = correctedPath.endsWith(FORWARD_SLASH) ? correctedPath : correctedPath + FORWARD_SLASH; - return objects != null && objects.getObjectSummaries().stream().anyMatch(object -> - object.getKey().equals(correctedPath) || object.getKey().startsWith(directory)); + return first.getKey().equals(correctedPath) || first.getKey().startsWith(directory) + || this.storage.listFirstObject(this.bucketName, directory) != null; } catch (RuntimeException e) { throw new DotDataException("Unable to check S3 object " + correctedPath, e); } @@ -342,6 +373,18 @@ public List listObjectPaths(final String groupName, .collect(Collectors.toList()); } + /** + * Uploads a file to the group and waits for the upload to finish. With S3 asset storage on, the + * upload starts only after this node's per-key lock is held. + * + * @param groupName the group to write to + * @param path the object's path + * @param file the file to upload + * @param extraMeta not used by this provider + * @return the uploaded object's ETag + * @throws DotDataException declared by the interface; a failed upload or lock wait is thrown as + * a {@link DotRuntimeException} + */ @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public Object pushFile(final String groupName, final String path, final File file, @@ -372,10 +415,12 @@ public Object pushFile(final String groupName, final String path, final File fil } final String pathForS3 = transformReadPath(groupName, path); - final Upload upload = this.storage.uploadFile(new PutObjectRequest(this.bucketName, pathForS3, file)); try { return lockManager.tryLock("s3_" + groupName + path, () -> { + // Start the upload under the lock so concurrent pushes of one key on this node are + // ordered, and a lock timeout leaves nothing uploading in the background. + final Upload upload = this.storage.uploadFile(new PutObjectRequest(this.bucketName, pathForS3, file)); Logger.debug(this, () -> String.format("Pushing file '%s' to group '%s' with " + "path '%s' [ %s ]", file.getName(), groupName, pathForS3, path)); final UploadResult result = @@ -437,6 +482,16 @@ private void createTempFile(final ObjectWriterDelegate writerDelegate, writeToFile(writerDelegate, object, file); } + /** + * Uploads a file in the background. With S3 asset storage on, this runs {@link #pushFile} on the + * storage pool; with it off, it starts the legacy transfer directly. + * + * @param groupName the group to write to + * @param path the object's path + * @param file the file to upload + * @param extraMeta not used by this provider + * @return a future for the uploaded object's ETag + */ @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public Future pushFileAsync(final String groupName, final String path, final File file, final Map extraMeta) { @@ -444,20 +499,11 @@ public Future pushFileAsync(final String groupName, final String path, f return DotConcurrentFactory.getInstance().getSubmitter(STORAGE_POOL).submit(() -> this.pushFile(groupName, path, file, extraMeta)); } - if (!AssetStorageFeature.isEnabled()) { final String pathForS3 = transformWritePath(groupName, path, file.getName()); Logger.debug(this, () -> String.format("Async pushing file '%s' to group '%s' with path " + "'%s' [ %s ]", file.getName(), groupName, pathForS3, path)); final Upload upload = this.storage.uploadFile(this.bucketName, pathForS3, file); return new UploadFuture<>(upload, file); - - } - - final String pathForS3 = transformReadPath(groupName, path); - Logger.debug(this, () -> String.format("Async pushing file '%s' to group '%s' with path " + - "'%s' [ %s ]", file.getName(), groupName, pathForS3, path)); - final Upload upload = this.storage.uploadFile(new PutObjectRequest(this.bucketName, pathForS3, file)); - return new UploadFuture<>(upload, file); } @Override @@ -681,8 +727,7 @@ private String transformReadPath(final String groupName, final String path) { final String directory = normalized.substring(0, slash + 1); final String fileName = normalized.substring(slash + 1); final String hash = EncryptorFactory.getInstance().getEncryptor() - .encryptString(directory, AssetStorageFeature.isEnabled() - ? newSha256() : this.sha256); + .encryptString(directory, newSha256()); return groupKey(groupName) + FORWARD_SLASH + hash + FORWARD_SLASH + fileName; } @@ -726,6 +771,17 @@ private static MessageDigest newSha256() { .getOrElseThrow(DotRuntimeException::new); } + /** + * Reports whether S3 holds an object at the path with the same bytes as the file. The size must + * match, and then either the ETag equals the file's MD5 or the stored bytes are compared. Only + * one key is listed, because an exact key sorts first under its own prefix. + * + * @param groupName the group to look in + * @param path the object's path + * @param file the local file to compare with + * @return {@code true} if the flag is on and S3 holds the same contents + * @throws DotDataException if the file cannot be read or the comparison fails + */ @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public boolean hasDurableCopy(final String groupName, final String path, @@ -736,10 +792,9 @@ public boolean hasDurableCopy(final String groupName, final String path, final String key = transformReadPath(groupName, path); try (final InputStream input = Files.newInputStream(file.toPath())) { final String md5 = DigestUtils.md5Hex(input); - for (final var object : storage.listObjects(bucketName, key).getObjectSummaries()) { - if (key.equals(object.getKey()) && object.getSize() == file.length()) { - return md5.equalsIgnoreCase(object.getETag()) || storage.fileContentsMatch(bucketName, key, file); - } + final var object = storage.listFirstObject(bucketName, key); + if (object != null && key.equals(object.getKey()) && object.getSize() == file.length()) { + return md5.equalsIgnoreCase(object.getETag()) || storage.fileContentsMatch(bucketName, key, file); } return false; } catch (final IOException e) { diff --git a/dotCMS/src/main/java/com/dotcms/storage/AssetStorageFeature.java b/dotCMS/src/main/java/com/dotcms/storage/AssetStorageFeature.java index 01fcd69b9b98..3b746872819f 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/AssetStorageFeature.java +++ b/dotCMS/src/main/java/com/dotcms/storage/AssetStorageFeature.java @@ -19,7 +19,9 @@ private AssetStorageFeature() { } /** * Returns whether the S3 asset lifecycle is enabled for this process. The first call reads the - * configuration; later calls return that value. + * configuration; later calls return that value. The first read logs at INFO only when the flag + * is on, so a node with the flag off writes no new INFO line compared with releases without + * this feature. * * @return {@code true} when the feature flag was on at first read */ @@ -29,7 +31,11 @@ public static boolean isEnabled() { synchronized (AssetStorageFeature.class) { if (enabled == null) { enabled = Config.getBooleanProperty(FLAG, false); - Logger.info(AssetStorageFeature.class, "S3 asset storage " + (enabled ? "enabled" : "disabled")); + if (enabled) { + Logger.info(AssetStorageFeature.class, "S3 asset storage enabled"); + } else { + Logger.debug(AssetStorageFeature.class, "S3 asset storage disabled"); + } } value = enabled; } diff --git a/dotCMS/src/main/java/com/dotcms/storage/ChainableStoragePersistenceAPI.java b/dotCMS/src/main/java/com/dotcms/storage/ChainableStoragePersistenceAPI.java index 475fec497656..d3a8c48563b1 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/ChainableStoragePersistenceAPI.java +++ b/dotCMS/src/main/java/com/dotcms/storage/ChainableStoragePersistenceAPI.java @@ -4,6 +4,7 @@ import com.dotmarketing.business.CacheLocator; import com.dotmarketing.exception.DotDataException; import com.dotmarketing.exception.DotRuntimeException; +import com.dotmarketing.util.Logger; import com.dotmarketing.util.Config; import com.google.common.annotations.VisibleForTesting; import io.vavr.control.Try; @@ -446,6 +447,19 @@ private void populateMissStorageChain(final String groupName, final String path, } } + /** + * Reads an object from the first provider that has it. With S3 asset storage on, providers + * earlier in the chain that missed it are refilled with the value, under the same per-key lock + * that writes and deletes take, so a restore cannot overwrite a newer write or bring back a + * deleted object. A provider whose copy cannot be deserialized is refilled the same way when a + * later provider holds a readable copy; if none does, the read fails rather than reporting the + * object as absent. Read failures propagate instead of being reported as absence. + * + * @param groupName the group to read from + * @param path the object's path + * @param readerDelegate deserializes the stored value + * @return the object, or {@code null} if no provider has it + */ @Override public Object pullObject(final String groupName, final String path, @@ -453,12 +467,25 @@ public Object pullObject(final String groupName, if (AssetStorageFeature.isEnabled()) { final List missing = new ArrayList<>(); + UnreadableStoredObjectException unreadable = null; + final var lock = assetLocks.get(groupName + "/" + path); + lock.lock(); try { for (StoragePersistenceAPI storage : storagePersistenceAPIList) { - final Object object = storage.pullObject(groupName, path, readerDelegate); + Object object; + try { + object = storage.pullObject(groupName, path, readerDelegate); + } catch (final UnreadableStoredObjectException e) { + unreadable = e; + object = null; + } if (object == null) { missing.add(storage); } else { + if (unreadable != null) { + Logger.warn(this, "Replacing an unreadable copy of (" + groupName + '|' + path + + ") from a later storage provider: " + unreadable.getMessage()); + } if (object instanceof Serializable) { for (StoragePersistenceAPI cacheStorage : missing) { if (!cacheStorage.existsGroup(groupName)) { @@ -471,9 +498,14 @@ public Object pullObject(final String groupName, return object; } } + if (unreadable != null) { + throw unreadable; + } return null; } catch (DotDataException e) { throw new DotRuntimeException("Unable to retrieve stored object " + path, e); + } finally { + lock.unlock(); } } diff --git a/dotCMS/src/main/java/com/dotcms/storage/FileSystemStoragePersistenceAPIImpl.java b/dotCMS/src/main/java/com/dotcms/storage/FileSystemStoragePersistenceAPIImpl.java index f069d64f57fe..8443b7f164ce 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/FileSystemStoragePersistenceAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/storage/FileSystemStoragePersistenceAPIImpl.java @@ -343,6 +343,17 @@ public Future pushObjectAsync(final String groupName, final String path, ); } + /** + * Returns the stored file for the path. With S3 asset storage on, an unmapped group or a missing + * file returns {@code null} so the chain can try the next provider, and a path that is not a + * regular file fails. With it off, a missing group or file throws + * {@link IllegalArgumentException} as before. + * + * @param groupName the group to read from + * @param path the object's path + * @return the stored file, or {@code null} when the flag is on and it is absent + * @throws DotDataException if the file cannot be read + */ @Override public File pullFile(final String groupName, final String path) throws DotDataException { @@ -377,9 +388,6 @@ public File pullFile(final String groupName, final String path) throws DotDataEx if (destBucketFile.exists()) { clientFile = destBucketFile; } else { - if (AssetStorageFeature.isEnabled()) { - return null; - } throw new IllegalArgumentException( "The group: " + destBucketFile + ", does not exists."); } @@ -394,6 +402,21 @@ public File pullFile(final String groupName, final String path) throws DotDataEx return clientFile; } + /** + * Reads and deserializes the stored object for the path. With S3 asset storage on, an unmapped + * group or a missing file returns {@code null}. A zero-length or truncated file is kept and + * reported as {@link UnreadableStoredObjectException}, so the chain can replace it from durable + * storage and nothing treats it as absent and regenerates it. Other read failures throw. + * With the flag off, a corrupt file is deleted and an empty map is returned, and a missing group + * or file throws {@link IllegalArgumentException}, as before. + * + * @param groupName the group to read from + * @param path the object's path + * @param readerDelegate deserializes the stored value + * @return the object, or {@code null} when the flag is on and the file is absent + * @throws UnreadableStoredObjectException if the flag is on and the file cannot be deserialized + * @throws DotDataException if the flag is on and the file cannot be read + */ @Override public Object pullObject(final String groupName, final String path, final ObjectReaderDelegate readerDelegate) throws DotDataException { @@ -411,6 +434,9 @@ public Object pullObject(final String groupName, final String path, } } catch (NoSuchFileException absent) { return null; + } catch (com.fasterxml.jackson.core.JsonProcessingException | java.io.EOFException corrupt) { + // Keep the file: the chain replaces it from durable storage, or the read fails. + throw new UnreadableStoredObjectException("Unable to read stored object " + path, corrupt); } catch (IOException e) { throw new DotDataException("Unable to read stored object " + path, e); } @@ -437,9 +463,6 @@ public Object pullObject(final String groupName, final String path, try (InputStream input = FileUtil.createInputStream(file.toPath(), compressor)) { object = readerDelegate.read(input); } catch (final FileNotFoundException | NoSuchFileException absent) { - if (AssetStorageFeature.isEnabled()) { - return null; - } throw new IllegalArgumentException("The file: " + path + ", does not exists."); } } catch (MismatchedInputException e) { @@ -531,6 +554,15 @@ private void countFiles(final File dirPath, final MutableInt count) { } } + /** + * Lists the stored files under a prefix, relative to the group folder. The prefix is mapped to + * disk the same way keys are when they are written, so a prefix in any letter case finds them. + * + * @param groupName the group to list + * @param pathPrefix the key prefix + * @return the matching paths as stored on disk, or an empty list + * @throws DotDataException if the folder cannot be resolved or walked + */ @Override public List listObjectPaths(final String groupName, final String pathPrefix) throws DotDataException { @@ -543,7 +575,8 @@ public List listObjectPaths(final String groupName, final File groupDir = groups.get(groupNameLC); final Path prefixDir; try { - prefixDir = Paths.get(groupDir.getCanonicalPath(), pathPrefix); + // Keys are stored through normalizePath, so the prefix must be resolved the same way. + prefixDir = Paths.get(groupDir.getCanonicalPath(), normalizePath(groupName, pathPrefix)); } catch (final IOException e) { throw new DotDataException( "Failed to resolve prefix path for group '" + groupName + "': " + e.getMessage(), e); diff --git a/dotCMS/src/main/java/com/dotcms/storage/NoWebIdentityCredentialsProviderChain.java b/dotCMS/src/main/java/com/dotcms/storage/NoWebIdentityCredentialsProviderChain.java new file mode 100644 index 000000000000..36ad2466a326 --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/storage/NoWebIdentityCredentialsProviderChain.java @@ -0,0 +1,76 @@ +package com.dotcms.storage; + +import com.amazonaws.auth.AWSCredentials; +import com.amazonaws.auth.AWSCredentialsProvider; +import com.amazonaws.auth.AWSCredentialsProviderChain; +import com.amazonaws.auth.DefaultAWSCredentialsProviderChain; +import com.amazonaws.auth.EC2ContainerCredentialsProviderWrapper; +import com.amazonaws.auth.EnvironmentVariableCredentialsProvider; +import com.amazonaws.auth.SystemPropertiesCredentialsProvider; +import com.amazonaws.auth.profile.ProfileCredentialsProvider; +import java.util.List; + +/** + * The AWS SDK default credential chain without its web identity step: environment variables, Java + * system properties, the shared profile files, then the container or EC2 instance role, in the + * SDK's own order. + * + *

Before the S3 asset storage feature, dotCMS did not package the AWS STS module, so the web + * identity step of {@link DefaultAWSCredentialsProviderChain} always failed and the chain moved on to + * the next provider. With STS packaged, that step can now succeed, for example in a Kubernetes pod + * that has a web identity token. With the feature flag off this chain keeps the earlier resolution. + * It extends {@link DefaultAWSCredentialsProviderChain} so the public constructors that take that + * type keep their signatures.

+ */ +public final class NoWebIdentityCredentialsProviderChain extends DefaultAWSCredentialsProviderChain { + + private final List providers = List.of( + new EnvironmentVariableCredentialsProvider(), + new SystemPropertiesCredentialsProvider(), + new ProfileCredentialsProvider(), + new EC2ContainerCredentialsProviderWrapper()); + private final AWSCredentialsProviderChain delegate = new AWSCredentialsProviderChain(providers); + + private NoWebIdentityCredentialsProviderChain() { } + + /** + * Returns the credential chain to use where dotCMS falls back to the AWS default chain. With the + * S3 asset storage flag on, this is the standard SDK chain, including web identity. With the flag + * off, it is the chain without web identity, which resolves credentials the way dotCMS did before + * the STS module was packaged. + * + * @return a new credential chain for the current mode + */ + public static DefaultAWSCredentialsProviderChain defaultChain() { + return AssetStorageFeature.isEnabled() + ? new DefaultAWSCredentialsProviderChain() + : new NoWebIdentityCredentialsProviderChain(); + } + + /** + * Resolves credentials from the first provider in this chain that supplies them. + * + * @return the resolved credentials + */ + @Override + public AWSCredentials getCredentials() { + return delegate.getCredentials(); + } + + /** + * Asks every provider in this chain to refresh its credentials. + */ + @Override + public void refresh() { + delegate.refresh(); + } + + /** + * Returns the providers this chain consults, in order, so tests can check its composition. + * + * @return the providers in the order they are tried + */ + List providers() { + return providers; + } +} diff --git a/dotCMS/src/main/java/com/dotcms/storage/UnreadableStoredObjectException.java b/dotCMS/src/main/java/com/dotcms/storage/UnreadableStoredObjectException.java new file mode 100644 index 000000000000..da1ae439861d --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/storage/UnreadableStoredObjectException.java @@ -0,0 +1,22 @@ +package com.dotcms.storage; + +import com.dotmarketing.exception.DotDataException; + +/** + * Thrown by a storage provider when a stored object exists but cannot be deserialized, for example + * a zero-length or truncated local metadata file. It is a read failure, not absence, so the object + * is never treated as missing and regenerated. {@link ChainableStoragePersistenceAPI} uses it to + * replace the unreadable copy from a later provider that holds a readable one. + */ +public class UnreadableStoredObjectException extends DotDataException { + + /** + * Creates the exception. + * + * @param message describes the unreadable object + * @param cause the deserialization failure + */ + public UnreadableStoredObjectException(final String message, final Throwable cause) { + super(message, cause); + } +} diff --git a/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureTest.java b/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureTest.java index fa49fa14917b..21dfa0dc32b0 100644 --- a/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureTest.java +++ b/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureTest.java @@ -1,6 +1,7 @@ package com.dotcms.storage; import com.dotmarketing.exception.DotDataException; +import com.dotmarketing.exception.DotRuntimeException; import com.dotmarketing.util.Config; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -214,6 +215,102 @@ void deleteCannotBeUndoneBySameChainInflightRestore() throws Exception { } } + @Test + void deleteCannotBeUndoneBySameChainInflightObjectRestore() throws Exception { + var fs = filesystem(); + var remote = mock(StoragePersistenceAPI.class); + var reader = new JsonReaderDelegate<>(String.class); + var fetching = new CountDownLatch(1); + var finishFetch = new CountDownLatch(1); + when(remote.pullObject(GROUP, KEY, reader)).thenAnswer(call -> { + fetching.countDown(); + assertTrue(finishFetch.await(10, TimeUnit.SECONDS)); + return "remote"; + }); + var chain = chain(fs, remote); + try (var executor = Executors.newFixedThreadPool(2)) { + var read = executor.submit(() -> chain.pullObject(GROUP, KEY, reader)); + assertTrue(fetching.await(5, TimeUnit.SECONDS)); + var deleting = new CountDownLatch(1); + var delete = executor.submit(() -> { + deleting.countDown(); + return chain.deleteObjectAndReferences(GROUP, KEY); + }); + assertTrue(deleting.await(5, TimeUnit.SECONDS)); + try { + assertThrows(TimeoutException.class, () -> delete.get(100, TimeUnit.MILLISECONDS), + "A delete must wait for the restore of the same key"); + } finally { + finishFetch.countDown(); + } + assertEquals("remote", read.get(5, TimeUnit.SECONDS)); + delete.get(5, TimeUnit.SECONDS); + assertFalse(Files.exists(root.resolve(KEY.toLowerCase())), "The deleted object must not come back locally"); + } + } + + @Test + void unreadableLocalMetadataIsReplacedFromADurableCopyAndOtherwiseFails() throws Exception { + // Metadata keeps lowercased keys in every slice; the binary groups later become case-preserving. + final String group = "dotmetadata"; + var fs = new FileSystemStoragePersistenceAPIImpl(); + fs.addGroupMapping(group, root.toFile()); + var reader = new JsonReaderDelegate<>(String.class); + Path local = root.resolve(KEY.toLowerCase()); + Files.createDirectories(local.getParent()); + Files.write(local, new byte[0]); + var remote = mock(StoragePersistenceAPI.class); + when(remote.pullObject(group, KEY, reader)).thenReturn("durable"); + + assertEquals("durable", chain(fs, remote).pullObject(group, KEY, reader)); + assertEquals("durable", fs.pullObject(group, KEY, reader), "The local copy is replaced from S3"); + + Files.writeString(local, "\"dura"); + assertThrows(UnreadableStoredObjectException.class, () -> fs.pullObject(group, KEY, reader), + "A truncated local copy is a read failure, not absence"); + var empty = mock(StoragePersistenceAPI.class); + assertThrows(DotRuntimeException.class, () -> chain(fs, empty).pullObject(group, KEY, reader), + "Without a readable durable copy the read fails instead of reporting the object as absent"); + assertEquals("\"dura", Files.readString(local), "An unreadable copy without a replacement is kept"); + } + + @Test + void filesystemListingFindsKeysWhateverTheCaseOfThePrefix() throws Exception { + // Metadata keeps lowercased keys in every slice; the binary groups later become case-preserving. + final String group = "dotmetadata"; + var fs = new FileSystemStoragePersistenceAPIImpl(); + fs.addGroupMapping(group, root.toFile()); + File source = Files.writeString(root.resolve("upload.tmp"), "contents").toFile(); + fs.pushFile(group, KEY, source, Map.of()); + assertEquals(List.of(KEY.toLowerCase()), fs.listObjectPaths(group, "a/b/ABC123")); + } + + @Test + void s3ExistenceChecksListOneKeyInsteadOfTheWholePrefix() throws Exception { + var storage = mock(com.dotcms.enterprise.publishing.storage.Storage.class); + var adapter = new AmazonS3StoragePersistenceAPIImpl(storage, "bucket", + AmazonS3StoragePersistenceAPIImpl.PathEncryptionMode.NONE); + when(storage.existsBucket("bucket")).thenReturn(true); + when(storage.listFirstObject("bucket", GROUP + "/")).thenReturn(summary(GROUP + "/x")); + assertTrue(adapter.existsGroup(GROUP)); + + String key = GROUP + "/a/b/file"; + when(storage.listFirstObject("bucket", key)).thenReturn(summary(key + ".bak")); + assertFalse(adapter.existsObject(GROUP, "/a/b/file"), "A sibling key that shares the prefix does not count"); + when(storage.listFirstObject("bucket", key + "/")).thenReturn(summary(key + "/child")); + assertTrue(adapter.existsObject(GROUP, "/a/b/file"), "An object beneath the path counts"); + when(storage.listFirstObject("bucket", key)).thenReturn(summary(key)); + assertTrue(adapter.existsObject(GROUP, "/a/b/file")); + + verify(storage, never()).listObjects(anyString(), anyString()); + } + + private static com.amazonaws.services.s3.model.S3ObjectSummary summary(final String key) { + var summary = new com.amazonaws.services.s3.model.S3ObjectSummary(); + summary.setKey(key); + return summary; + } + @Test void metadataKeysAreStableAcrossConcurrentCalls() throws Exception { var adapter = new AmazonS3StoragePersistenceAPIImpl( diff --git a/dotCMS/src/test/java/com/dotcms/storage/NoWebIdentityCredentialsProviderChainTest.java b/dotCMS/src/test/java/com/dotcms/storage/NoWebIdentityCredentialsProviderChainTest.java new file mode 100644 index 000000000000..ec214946da8b --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/storage/NoWebIdentityCredentialsProviderChainTest.java @@ -0,0 +1,52 @@ +package com.dotcms.storage; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertSame; + +import com.amazonaws.auth.DefaultAWSCredentialsProviderChain; +import com.amazonaws.auth.EC2ContainerCredentialsProviderWrapper; +import com.amazonaws.auth.EnvironmentVariableCredentialsProvider; +import com.amazonaws.auth.SystemPropertiesCredentialsProvider; +import com.amazonaws.auth.profile.ProfileCredentialsProvider; +import com.dotmarketing.util.Config; +import java.util.List; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +class NoWebIdentityCredentialsProviderChainTest { + private String previousFlag; + + @BeforeEach + void remember() { + previousFlag = Config.getStringProperty(AssetStorageFeature.FLAG, null); + } + + @AfterEach + void restore() { + Config.setProperty(AssetStorageFeature.FLAG, previousFlag); + } + + @Test + void disabledFlagResolvesCredentialsWithoutTheWebIdentityStep() { + Config.setProperty(AssetStorageFeature.FLAG, false); + final DefaultAWSCredentialsProviderChain chain = NoWebIdentityCredentialsProviderChain.defaultChain(); + final var legacy = assertInstanceOf(NoWebIdentityCredentialsProviderChain.class, chain); + // SDK 1.12.488 orders the default chain as environment, system properties, web identity, + // profile, container or instance role. Without STS on the classpath the web identity step + // always failed over, so the effective chain is the other four in the same order. + assertEquals(List.of(EnvironmentVariableCredentialsProvider.class, + SystemPropertiesCredentialsProvider.class, + ProfileCredentialsProvider.class, + EC2ContainerCredentialsProviderWrapper.class), + legacy.providers().stream().map(Object::getClass).toList()); + } + + @Test + void enabledFlagUsesTheStandardSdkChain() { + Config.setProperty(AssetStorageFeature.FLAG, true); + assertSame(DefaultAWSCredentialsProviderChain.class, + NoWebIdentityCredentialsProviderChain.defaultChain().getClass()); + } +} diff --git a/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java b/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java index a9e3d976ae53..eba855768d7b 100644 --- a/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java +++ b/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java @@ -6,6 +6,7 @@ import com.dotmarketing.exception.DotRuntimeException; import com.dotmarketing.util.Config; import java.util.HashMap; +import java.util.List; import java.util.Map; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -113,6 +114,36 @@ void publishingEndpointUsesItsOwnConfiguration() throws Exception { } finally { storage.shutdownTransferManager(); } } + @Test + void staticPushClientsWithoutARegionUseTheGlobalEndpointWithRegionLookup() throws Exception { + // Keys without a region or endpoint used to fail with "Unable to find a region", and the + // credential-chain path was pinned to us-west-2. Both now use the global endpoint, with no + // signer override, so the SDK can look up each bucket's region and sign SigV4 for it. + var keysOnly = new AWSS3Storage(new AWSS3Configuration.Builder() + .accessKey("publisher-key").secretKey("publisher-secret").build()); + var credentialChain = new AWSS3Storage(NoWebIdentityCredentialsProviderChain.defaultChain()); + try { + for (var storage : List.of(keysOnly, credentialChain)) { + assertEquals("test-bucket.s3.amazonaws.com", client(storage).getUrl("test-bucket", "file").getHost()); + assertNull(signerOverride(storage)); + } + } finally { + keysOnly.shutdownTransferManager(); + credentialChain.shutdownTransferManager(); + } + + Config.setProperty(AssetStorageFeature.FLAG, false); + var legacy = new AWSS3Storage(NoWebIdentityCredentialsProviderChain.defaultChain()); + try { + assertEquals("test-bucket.s3.amazonaws.com", client(legacy).getUrl("test-bucket", "file").getHost()); + assertEquals("S3SignerType", signerOverride(legacy)); + } finally { legacy.shutdownTransferManager(); } + } + + private static String signerOverride(AWSS3Storage storage) throws Exception { + return ((com.amazonaws.AmazonWebServiceClient) client(storage)).getClientConfiguration().getSignerOverride(); + } + // Inspect the real SDK configuration without making network requests or exposing production getters. static AWSS3Storage storage(AmazonS3StoragePersistenceAPIImpl provider) throws Exception { final var field = AmazonS3StoragePersistenceAPIImpl.class.getDeclaredField("storage"); From aa4cc7a4b80e81a7b51b9f436c64a04191bfe792 Mon Sep 17 00:00:00 2001 From: Scott Wicken <1562170+swicken@users.noreply.github.com> Date: Fri, 2 Oct 2026 12:31:09 -0400 Subject: [PATCH 4/7] docs(storage): link the S3 asset storage doc from the docs index docs/testing/BINARY_S3_STORAGE.md was not reachable from docs/README.md, which fails the docs reachability check. --- docs/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/README.md b/docs/README.md index 2ea4d671c946..6e94709ac2ad 100644 --- a/docs/README.md +++ b/docs/README.md @@ -77,6 +77,7 @@ Descriptions say **when to load** the doc, not what it contains. | [E2E_TESTS.md](testing/E2E_TESTS.md) | Working on the Playwright suite in `core-web/apps/dotcms-ui-e2e` | | [API_TESTING.md](testing/API_TESTING.md) | Writing API tests — Postman (`dotcms-postman/`) and Karate (`test-karate/`) | | [PERFORMANCE_TESTS.md](testing/PERFORMANCE_TESTS.md) | Load testing with JMeter (`test-jmeter/`) or the Kubernetes analytics suite | +| [BINARY_S3_STORAGE.md](testing/BINARY_S3_STORAGE.md) | Working on S3 asset storage (`FEATURE_FLAG_S3_ASSET_STORAGE`): what changes with the flag on and off, and how to run its MinIO and integration checks | | [test-cases/README.md](test-cases/README.md) | Looking for the manual/feature test-suite index | ## CLI (`tools/dotcms-cli`) From 5bbdcb629b1c5b02bfa41fb0a4d3d663849343cd Mon Sep 17 00:00:00 2001 From: swicken <1562170+swicken@users.noreply.github.com> Date: Tue, 13 Oct 2026 08:05:53 -0400 Subject: [PATCH 5/7] Update dotCMS/src/main/java/com/dotmarketing/util/Config.java Co-authored-by: Jose Castro --- dotCMS/src/main/java/com/dotmarketing/util/Config.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dotCMS/src/main/java/com/dotmarketing/util/Config.java b/dotCMS/src/main/java/com/dotmarketing/util/Config.java index 6445b4d088a1..6a9a92e45be3 100644 --- a/dotCMS/src/main/java/com/dotmarketing/util/Config.java +++ b/dotCMS/src/main/java/com/dotmarketing/util/Config.java @@ -795,7 +795,7 @@ public static void setProperty(String key, Object value) { Logger.info(Config.class, "Setting property: " + key + " to " + value); props.setProperty(key, value); if (assetStorageFlag) { - // ponytail: only in-memory overrides re-read the latched flag; reloads and the system table stay latched. + // Only in-memory overrides re-read the latched flag; reloads and the system table stay latched. com.dotcms.storage.AssetStorageFeature.reset(); } } From d87902b9a081464d53f46ef01062340bcc860cd4 Mon Sep 17 00:00:00 2001 From: Scott Wicken <1562170+swicken@users.noreply.github.com> Date: Tue, 13 Oct 2026 09:49:02 -0400 Subject: [PATCH 6/7] fix(storage): address review findings on the S3 storage layer Chain - When a provider fails after a later provider accepted a write, drop the now-stale copies so the next read restores the new value from S3. - Make the per-key locks process-wide, lowercase the lock key to match the filesystem provider, and bound the wait (15 min) instead of waiting forever. S3 provider - Flag-on pushFile no longer holds the shared identifier lock during uploads. - Flag-on downloads always go to their own temp file and are always deleted, whatever S3_STORAGE_FILE_REPO_TYPE says. - Backfill treats 409 ConditionalRequestConflict like 412. - hasDurableCopy hashes the local file only after the key and size match. - S3ContentAddressedStorage.store downloads the blob only when someone else wrote it, and verifies just the reference at the end. Other - Filesystem metadata reads treat any decode failure (bad gzip/bzip2 header) as unreadable so the chain replaces the file from S3. - SharedExtractedMetadata skips the shared cache when the extractor version is unknown. - FEATURE_FLAG_S3_ASSET_STORAGE is declared in FeatureFlagName, and Config gets a generic onOverride hook so it no longer references the storage code. - BINARY_S3_STORAGE.md: target deployment, namespace rules and environment cloning, the one-way nature of enabling the flag, release-note callout for STS, and a pullable pinned MinIO image. --- docs/testing/BINARY_S3_STORAGE.md | 69 +++++++- .../dotcms/featureflag/FeatureFlagName.java | 12 ++ .../AmazonS3StoragePersistenceAPIImpl.java | 105 ++++++++---- .../dotcms/storage/AssetStorageFeature.java | 13 +- .../ChainableStoragePersistenceAPI.java | 151 ++++++++++++++++-- .../FileSystemStoragePersistenceAPIImpl.java | 31 ++-- .../storage/S3ContentAddressedStorage.java | 37 ++++- .../storage/SharedExtractedMetadata.java | 15 ++ .../java/com/dotmarketing/util/Config.java | 31 +++- .../storage/AssetStorageFeatureLatchTest.java | 19 +++ .../storage/AssetStorageFeatureTest.java | 104 ++++++++++++ .../S3ContentAddressedStorageTest.java | 112 +++++++++++++ .../storage/S3StorageConfigurationTest.java | 107 +++++++++++++ .../storage/SharedExtractedMetadataTest.java | 54 +++++++ 14 files changed, 778 insertions(+), 82 deletions(-) create mode 100644 dotCMS/src/test/java/com/dotcms/storage/S3ContentAddressedStorageTest.java create mode 100644 dotCMS/src/test/java/com/dotcms/storage/SharedExtractedMetadataTest.java diff --git a/docs/testing/BINARY_S3_STORAGE.md b/docs/testing/BINARY_S3_STORAGE.md index 07724c9a7678..e062467d85cd 100644 --- a/docs/testing/BINARY_S3_STORAGE.md +++ b/docs/testing/BINARY_S3_STORAGE.md @@ -6,6 +6,23 @@ the filesystem/NFS behavior is unchanged. This page describes what is in the code today. It grows as the remaining parts land. +## Target deployment + +The feature exists so a cluster can scale out without a shared NFS volume (#37868). S3 holds +the only durable copy of each asset and is what the nodes share. Each node keeps its own +node-local asset directory as a disposable cache: a missing local file is restored from S3 on +the next read. The intended setup is one node-local asset directory per node. Cache eviction, +which keeps that directory bounded, arrives in a later PR of this stack. + +Running with the flag on over a shared NFS asset directory is not the target and is not +supported. Eviction, once it lands, deletes local files on the assumption that only its own +node uses them, so on a shared directory one node would remove files that other nodes are +serving. + +Serving directly from S3 is out of scope today. A read that misses the local cache downloads +the whole object to the local directory before serving it, including a range request on a +large file. Nothing streams from S3 or reads part of an object. + ## Behavior with the flag off With the flag off, the storage chain, its providers, metadata and static push publishing run @@ -23,6 +40,9 @@ the same code paths as before. Two details are worth knowing: `DefaultAWSCredentialsProviderChain` now gets the web identity step. - The flag is logged at INFO only when it is on. With it off, the first read logs at debug. +The release notes for the release that ships this feature should call out the STS packaging +change, because it can change which credentials an existing setup resolves. + ## Feature flag Set `FEATURE_FLAG_S3_ASSET_STORAGE=true` (Docker: `DOT_FEATURE_FLAG_S3_ASSET_STORAGE=true`) @@ -35,6 +55,15 @@ table may not be seen at first read, and a runtime change is ignored until resta overrides through `Config.setProperty` do re-read it, which is how tests switch modes. Tests that mock `Config` statically must call `AssetStorageFeature.reset()` themselves. +Turning the flag on is a one-way operational decision. With a namespace set (see below), the +objects written with the flag on live under `asset-namespaces//`, which a release +without this feature does not read. The flag-off path of this release ignores the namespace +too, so turning the flag off again hides those objects in the same way. The later slices that +store binaries in S3 add a layout that an older release cannot read either: a JSON reference at +the owner key and the bytes under `asset-blobs/sha256/...`. Once assets are written that way, +do not roll back below the release that ships this feature without first migrating those +assets back to the previous layout. + ## Storage layer behavior with the flag on The storage chain (`ChainableStoragePersistenceAPI`) and its providers change as follows: @@ -87,15 +116,36 @@ With the flag on, `storage.file-metadata.s3.namespace` (Docker: bucket. Owned keys become `asset-namespaces///`. Two prefixes stay at the bucket root and are shared by every namespace: content-addressed blobs (`asset-blobs/`), so identical bytes are stored once, and shared extracted metadata -(`extracted-metadata/`), so identical bytes are extracted once. Use the same -value for all nodes in one cluster and distinct values for independent installations. Values -are 1 to 64 letters, digits, underscores or hyphens, starting with a letter or digit. Empty is -the default and keeps the existing layout. +(`extracted-metadata/`), so identical bytes are extracted once. Independent installations +use distinct values. Values are 1 to 64 letters, digits, underscores or hyphens, starting with +a letter or digit. Empty is the default and keeps the existing layout. + +Every node of one installation, meaning every node that uses the same database, must use the +same value for the life of the installation. The S3 provider adds the prefix when it talks to +S3, and the database stores keys without it (the later slices that store binaries record a +`storageKey` per binary, also without the prefix). A node with a different value, or none, +reads and writes under another prefix, so content looks missing on that node. + +Set the namespace the same way as the flag, in the environment or +`dotmarketing-config.properties`, not only in the system table. `Config` gives a `DOT_` +environment variable precedence. Otherwise a system-table value overrides the properties file, +but only after the system-table source has been initialized during startup. The provider reads +the namespace once, when it is built, and keeps it. If it is built before that point, a value +set only in the system table is not seen, and a later change in the table is ignored until +restart. Set the namespace before the installation first stores assets. Changing it later does not migrate existing records. It separates application keys only; it does not restrict what the bucket credentials can access. +Copying one installation's database into another does not work on its own. Suppose staging +gets a copy of production's database. If staging uses its own namespace, which it needs so its +cleanup jobs cannot delete production's objects, its rows point at keys that exist only under +production's prefix, and staging sees no binaries. If staging uses production's namespace, +staging's deletions remove production's binaries. The supported path is starter export and +import, which lands in #37773: it publishes the assets under the target installation's +namespace before the import commits. + ## Static push publishing Enabling the flag also changes static push publishing to S3 (`AWSS3Storage`). Clients sign @@ -112,12 +162,20 @@ The S3 checks use the real filesystem provider, chain and AWS adapter against a MinIO bucket. `BinaryS3StorageTest` is skipped unless `s3.test.endpoint` is set; the other tests always run. The credentials below are disposable local test values. +MinIO no longer publishes community images, so `minio/minio:latest` can no longer be pulled. +The command uses Chainguard's free MinIO image instead. Its free tier publishes only the +`latest` tag, so the command pins the multi-architecture digest that `latest` resolved to on +2026-10-13; update the digest when you need a newer build. Any S3-compatible server can stand +in, but it must support conditional PUTs (`If-None-Match: *` and `If-Match`), because the +backfill operations and the content-addressed store rely on them. + ```sh docker run -d --rm --name binary-s3-test \ -p 127.0.0.1:19002:9000 \ -e MINIO_ROOT_USER=binary-storage-test \ -e MINIO_ROOT_PASSWORD=binary-storage-test \ - minio/minio:latest server /data + cgr.dev/chainguard/minio@sha256:dc455cdd4db6e390b1b26514f15ff37681a777bf8dab41aac445a0259f751e56 \ + server /data ./mvnw test -pl :dotcms-core -Dmaven.build.cache.enabled=false \ -Dtest=AssetStorageFeatureTest,AssetStorageFeatureLatchTest,S3StorageConfigurationTest,NoWebIdentityCredentialsProviderChainTest,BinaryS3StorageTest \ @@ -127,3 +185,4 @@ docker stop binary-s3-test ``` CI does not yet provide the MinIO service, so `BinaryS3StorageTest` does not run there. +Wiring MinIO into the CI build is planned before the feature is enabled for customers. diff --git a/dotCMS/src/main/java/com/dotcms/featureflag/FeatureFlagName.java b/dotCMS/src/main/java/com/dotcms/featureflag/FeatureFlagName.java index 0850b21b6c97..55a7f1e8fa48 100644 --- a/dotCMS/src/main/java/com/dotcms/featureflag/FeatureFlagName.java +++ b/dotCMS/src/main/java/com/dotcms/featureflag/FeatureFlagName.java @@ -147,4 +147,16 @@ public interface FeatureFlagName { * customer data has been migrated to CAEM. */ String FEATURE_FLAG_CAEM_EXPERIMENT_RESULTS = "FEATURE_FLAG_CAEM_EXPERIMENT_RESULTS"; + + /** + * Stores binary assets in S3 instead of the filesystem/NFS asset tree. Off by default. + * + *

Read once per process and then kept, so changing it in the properties file or the system + * table needs a restart. In-memory overrides made with + * {@link com.dotmarketing.util.Config#setProperty(String, Object)}, which tests use to switch + * modes, are re-read.

+ * + * @see com.dotcms.storage.AssetStorageFeature + */ + String FEATURE_FLAG_S3_ASSET_STORAGE = "FEATURE_FLAG_S3_ASSET_STORAGE"; } diff --git a/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java b/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java index a6962be81335..41c61c87a3e2 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java @@ -374,16 +374,18 @@ public List listObjectPaths(final String groupName, } /** - * Uploads a file to the group and waits for the upload to finish. With S3 asset storage on, the - * upload starts only after this node's per-key lock is held. + * Uploads a file to the group and waits for the upload to finish. With S3 asset storage on, no + * lock is taken: the process-wide identifier lock is shared with content saves, and holding it + * for a long upload would make those saves time out. This provider does not order concurrent + * writes of the same key; callers that need ordering must serialize them. * * @param groupName the group to write to * @param path the object's path * @param file the file to upload * @param extraMeta not used by this provider * @return the uploaded object's ETag - * @throws DotDataException declared by the interface; a failed upload or lock wait is thrown as - * a {@link DotRuntimeException} + * @throws DotDataException declared by the interface; a failed upload, or with the flag off a + * failed lock wait, is thrown as a {@link DotRuntimeException} */ @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) @@ -416,22 +418,15 @@ public Object pushFile(final String groupName, final String path, final File fil final String pathForS3 = transformReadPath(groupName, path); try { - return lockManager.tryLock("s3_" + groupName + path, () -> { - - // Start the upload under the lock so concurrent pushes of one key on this node are - // ordered, and a lock timeout leaves nothing uploading in the background. - final Upload upload = this.storage.uploadFile(new PutObjectRequest(this.bucketName, pathForS3, file)); - Logger.debug(this, () -> String.format("Pushing file '%s' to group '%s' with " + - "path '%s' [ %s ]", file.getName(), groupName, pathForS3, path)); - final UploadResult result = - Try.of(upload::waitForUploadResult).getOrElseThrow(e -> new DotDataException(e.getMessage(), e)); - Logger.debug(this, () -> String.format("File '%s' in group '%s' with path '%s' " + - "[ %s ] was pushed successfully!", file.getName(), groupName, - pathForS3, path)); - return result.getETag(); - - } - ); + final Upload upload = this.storage.uploadFile(new PutObjectRequest(this.bucketName, pathForS3, file)); + Logger.debug(this, () -> String.format("Pushing file '%s' to group '%s' with " + + "path '%s' [ %s ]", file.getName(), groupName, pathForS3, path)); + final UploadResult result = + Try.of(upload::waitForUploadResult).getOrElseThrow(e -> new DotDataException(e.getMessage(), e)); + Logger.debug(this, () -> String.format("File '%s' in group '%s' with path '%s' " + + "[ %s ] was pushed successfully!", file.getName(), groupName, + pathForS3, path)); + return result.getETag(); } catch (final Throwable e) { throw new DotRuntimeException(String.format("Failed to push file '%s' to S3 group " + "'%s': %s", path, groupName, ExceptionUtil.getErrorMessage(e)), e); @@ -518,14 +513,23 @@ public Future pushObjectAsync(final String bucketName, final String path return this.pushFileAsync(bucketName, path, file, extraMeta); } + /** + * Downloads the object at the path to a local file and waits for the download to finish. With S3 + * asset storage on, every call downloads to its own new temporary file, whatever + * {@code S3_STORAGE_FILE_REPO_TYPE} says, so concurrent downloads of one key never share a file; + * the caller hands it back through {@link #releaseRetrievedFile(File)} once it is done with it. + * + * @param groupName the group to read from + * @param path the object's path + * @return the downloaded file, or with the flag on {@code null} when the object does not exist + * @throws DotDataException if the download fails + */ @Override @EnterpriseFeature(licenseLevel = LicenseLevel.PLATFORM, errorMsg = INVALID_LICENSE) public File pullFile(final String groupName, final String path) throws DotDataException { if (AssetStorageFeature.isEnabled()) { - final File download = fileRepositoryManager.getOrCreateFile(path); - if (download == null) { - throw new DotDataException("Unable to allocate an S3 download file"); - } + final File download = Try.of(() -> Files.createTempFile("s3-download-", ".tmp").toFile()) + .getOrElseThrow(e -> new DotDataException("Unable to allocate an S3 download file", e)); try { storage.downloadFile(bucketName, transformReadPath(groupName, path), download).waitForCompletion(); return download; @@ -636,9 +640,16 @@ public String writeObjectIfMatch(final String group, final String path, final Se } } + /** + * Deletes a file returned by {@link #pullFile(String, String)} once the caller has copied or read + * it. With S3 asset storage off this does nothing, as before. + * + * @param file the downloaded file + * @throws DotDataException if the flag is on and the file cannot be deleted + */ @Override public void releaseRetrievedFile(final File file) throws DotDataException { - if (AssetStorageFeature.isEnabled() && fileRepositoryManager instanceof com.dotcms.storage.repository.TempFileRepositoryManager) { + if (AssetStorageFeature.isEnabled()) { try { Files.deleteIfExists(file.toPath()); } catch (IOException e) { @@ -790,18 +801,29 @@ public boolean hasDurableCopy(final String groupName, final String path, return false; } final String key = transformReadPath(groupName, path); - try (final InputStream input = Files.newInputStream(file.toPath())) { - final String md5 = DigestUtils.md5Hex(input); - final var object = storage.listFirstObject(bucketName, key); - if (object != null && key.equals(object.getKey()) && object.getSize() == file.length()) { - return md5.equalsIgnoreCase(object.getETag()) || storage.fileContentsMatch(bucketName, key, file); - } + final var object = storage.listFirstObject(bucketName, key); + if (object == null || !key.equals(object.getKey()) || object.getSize() != file.length()) { return false; + } + try (final InputStream input = Files.newInputStream(file.toPath())) { + return DigestUtils.md5Hex(input).equalsIgnoreCase(object.getETag()) + || storage.fileContentsMatch(bucketName, key, file); } catch (final IOException e) { throw new DotDataException("Failed to verify durable copy of " + path, e); } } + /** + * Copies a local file to S3 only if the key is empty. When another writer creates the key first + * (S3 answers 412, or 409 when the two conditional writes raced), the stored object is checked + * instead and counts as a backfill if it holds the same bytes. + * + * @param groupName the group to write to + * @param path the object's path + * @param file the local file to copy + * @return {@code true} once S3 holds the same bytes; {@code false} if the flag is off + * @throws DotDataException if the upload fails or S3 holds different bytes at the key + */ @Override public boolean backfillFile(final String groupName, final String path, final File file) throws DotDataException { if (!AssetStorageFeature.isEnabled()) { @@ -816,8 +838,8 @@ public boolean backfillFile(final String groupName, final String path, final Fil } try { storage.uploadFileIfAbsent(bucketName, transformReadPath(groupName, path), file); - } catch (com.amazonaws.services.s3.model.AmazonS3Exception conflict) { - if (conflict.getStatusCode() != 412) { + } catch (RuntimeException conflict) { + if (!S3ContentAddressedStorage.preconditionFailure(conflict)) { throw conflict; } } @@ -830,6 +852,19 @@ public boolean backfillFile(final String groupName, final String path, final Fil } } + /** + * Writes a serialized object to S3 only if the key is empty. When another writer creates the key + * first (S3 answers 412, or 409 when the two conditional writes raced), the stored object is read + * back instead and counts as a backfill if it reads as an equal object. + * + * @param groupName the group to write to + * @param path the object's path + * @param writer serializes the object + * @param reader reads the stored object back for comparison + * @param object the object to write + * @return {@code true} once S3 holds an equal object; {@code false} if the flag is off + * @throws DotDataException if the write fails or S3 holds a different object at the key + */ @Override public boolean backfillObject(final String groupName, final String path, final ObjectWriterDelegate writer, final ObjectReaderDelegate reader, final Serializable object) throws DotDataException { @@ -852,8 +887,8 @@ public boolean backfillObject(final String groupName, final String path, final O } try { storage.uploadFileIfAbsent(bucketName, transformReadPath(groupName, path), file); - } catch (com.amazonaws.services.s3.model.AmazonS3Exception conflict) { - if (conflict.getStatusCode() != 412) { + } catch (RuntimeException conflict) { + if (!S3ContentAddressedStorage.preconditionFailure(conflict)) { throw conflict; } } diff --git a/dotCMS/src/main/java/com/dotcms/storage/AssetStorageFeature.java b/dotCMS/src/main/java/com/dotcms/storage/AssetStorageFeature.java index 3b746872819f..24c6b30eebb6 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/AssetStorageFeature.java +++ b/dotCMS/src/main/java/com/dotcms/storage/AssetStorageFeature.java @@ -1,5 +1,6 @@ package com.dotcms.storage; +import com.dotcms.featureflag.FeatureFlagName; import com.dotmarketing.util.Config; import com.dotmarketing.util.Logger; @@ -11,10 +12,15 @@ * node half-enabled. Set it through the environment or properties file and restart.

*/ public final class AssetStorageFeature { - public static final String FLAG = "FEATURE_FLAG_S3_ASSET_STORAGE"; + public static final String FLAG = FeatureFlagName.FEATURE_FLAG_S3_ASSET_STORAGE; private static volatile Boolean enabled; + static { + // Registered before the first isEnabled() can latch, since that call loads this class. + Config.onOverride(FLAG, AssetStorageFeature::reset); + } + private AssetStorageFeature() { } /** @@ -45,8 +51,9 @@ public static boolean isEnabled() { /** * Forgets the value read at first use, so the next {@link #isEnabled()} reads the configuration - * again. Called by {@link Config#setProperty(String, Object)} for in-memory overrides of the flag, - * which tests use to switch modes. + * again. Registered with {@link Config#onOverride(String, Runnable)}, so in-memory overrides of the + * flag made with {@link Config#setProperty(String, Object)}, which tests use to switch modes, are + * re-read. Property reloads and the system table do not trigger it. */ public static void reset() { enabled = null; diff --git a/dotCMS/src/main/java/com/dotcms/storage/ChainableStoragePersistenceAPI.java b/dotCMS/src/main/java/com/dotcms/storage/ChainableStoragePersistenceAPI.java index d3a8c48563b1..a119aeb749a8 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/ChainableStoragePersistenceAPI.java +++ b/dotCMS/src/main/java/com/dotcms/storage/ChainableStoragePersistenceAPI.java @@ -7,6 +7,7 @@ import com.dotmarketing.util.Logger; import com.dotmarketing.util.Config; import com.google.common.annotations.VisibleForTesting; +import com.google.common.util.concurrent.Striped; import io.vavr.control.Try; import java.io.File; @@ -14,10 +15,13 @@ import java.util.ArrayList; import java.util.LinkedHashSet; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.Lock; /** * This API implements a chainable pattern that encapsulates a list of {@link StoragePersistenceAPI} @@ -35,8 +39,21 @@ public class ChainableStoragePersistenceAPI implements StoragePersistenceAPI { private final List storagePersistenceAPIList; private final ObjectWriterDelegate defaultWriterDelegate; private final Chainable404StorageCache cache; - private final com.google.common.util.concurrent.Striped assetLocks = - com.google.common.util.concurrent.Striped.lock(256); + + /** + * Per-key locks used with S3 asset storage on, so reads, writes and deletes of one key do not + * interleave. They are shared by the whole process because they protect the key, not a chain: + * re-initializing the storage provider builds a new chain while the old one may still be in use. + * Each stripe is a {@link java.util.concurrent.locks.ReentrantLock}, so a thread can take the + * lock of a key it already holds. Code holding one of these locks must not take another one + * (for example by calling another locked operation of a chain for a different key): two keys + * can map to different stripes, and two threads taking them in opposite order deadlock until + * {@link #ASSET_LOCK_TIMEOUT_MINUTES} runs out. + */ + private static final Striped ASSET_LOCKS = Striped.lock(256); + + /** How long to wait for a key's lock. Generous because large binaries are transferred under it. */ + private static final long ASSET_LOCK_TIMEOUT_MINUTES = 15; private static final String SUBMITTER_NAME = Config.getStringProperty("COMPOSITE_STORAGE_SUBMITTER_NAME", "SubmitterCompositeStoragePersistenceAPI"); @@ -62,6 +79,55 @@ public Chainable404StorageCache getCache() { return cache; } + /** + * Takes the lock of a storage key, waiting at most {@link #ASSET_LOCK_TIMEOUT_MINUTES}. The key + * is lowercased because the filesystem provider stores keys that differ only by case in the same + * file; variants that a provider keeps apart merely wait for each other. + * + * @param groupName the group the key belongs to + * @param path the storage key + * @return the held lock, which the caller must unlock + * @throws DotDataException if the wait times out or the thread is interrupted + */ + private static Lock lockKey(final String groupName, final String path) throws DotDataException { + final Lock lock = ASSET_LOCKS.get((groupName + "/" + path).toLowerCase(Locale.ROOT)); + try { + if (!lock.tryLock(ASSET_LOCK_TIMEOUT_MINUTES, TimeUnit.MINUTES)) { + throw new DotDataException("Timed out waiting for storage key " + groupName + "/" + path); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new DotDataException("Interrupted waiting for storage key " + groupName + "/" + path, e); + } + return lock; + } + + /** + * Called when the provider at {@code failedIndex} rejects a write. Writes go from the last + * provider to the first, so every provider after it already holds the new value while it and + * the providers before it still hold the previous one, and reads, which start at the first + * provider, would keep serving that stale copy. Deletes those copies, best effort, so the next + * read restores the new value from a later provider. Does nothing when the last provider, the + * first one written, failed, because then nothing has changed. + * + * @param groupName the group of the key being written + * @param path the key being written + * @param failedIndex the position in the chain of the provider whose write failed + */ + private void dropCopiesOlderThanAcceptedWrite(final String groupName, final String path, + final int failedIndex) { + if (failedIndex == storagePersistenceAPIList.size() - 1) { + return; + } + for (int i = failedIndex; i >= 0; i--) { + final StoragePersistenceAPI storage = storagePersistenceAPIList.get(i); + Try.run(() -> storage.deleteObjectAndReferences(groupName, path)) + .onFailure(e -> Logger.error(this, "Stale copy of " + groupName + "/" + path + + " left in " + storage + "; reads may serve it", e)); + } + cache.remove(groupName, path); + } + @Override public boolean existsGroup(final String groupName) { @@ -137,12 +203,21 @@ public int deleteGroup(final String groupName) throws DotDataException { return deleteCount; } + /** + * Deletes the object from every provider. With S3 asset storage on, this happens under the key's + * lock, from the last provider to the first, so the local copy goes only after the durable + * providers accepted the deletion. + * + * @param groupName the group of the object + * @param path the object's path + * @return {@code true} if every provider reported the object deleted + * @throws DotDataException if a provider fails, or the key's lock cannot be taken in time + */ @Override public boolean deleteObjectAndReferences(final String groupName, final String path) throws DotDataException { if (AssetStorageFeature.isEnabled()) { - final var lock = assetLocks.get(groupName + "/" + path); - lock.lock(); + final Lock lock = lockKey(groupName, path); try { // Keep the local copy until all durable providers accept the deletion. boolean deleted = !storagePersistenceAPIList.isEmpty(); @@ -245,18 +320,36 @@ public boolean backfillObject(final String groupName, final String path, final O return durable; } + /** + * Stores the file in every provider. With S3 asset storage on, this happens under the key's lock, + * from the last provider to the first, so durable storage holds the file before the local copy + * changes. If a provider fails after a later one accepted the file, the now-stale copies of it + * and the providers before it are deleted, so the next read restores the new file, and the + * failure is rethrown. + * + * @param groupName the group to store into + * @param path the file's key + * @param file the file to store + * @param extraMeta extra metadata for providers that keep it + * @return the result of the first provider in the chain + * @throws DotDataException if a provider fails, or the key's lock cannot be taken in time + */ @Override public Object pushFile(final String groupName, final String path, final File file, final Map extraMeta) throws DotDataException { if (AssetStorageFeature.isEnabled()) { - final var lock = assetLocks.get(groupName + "/" + path); - lock.lock(); + final Lock lock = lockKey(groupName, path); try { Object result = null; // Publish durably before exposing the new local cache contents. for (int i = storagePersistenceAPIList.size() - 1; i >= 0; i--) { - result = storagePersistenceAPIList.get(i).pushFile(groupName, path, file, extraMeta); + try { + result = storagePersistenceAPIList.get(i).pushFile(groupName, path, file, extraMeta); + } catch (DotDataException | RuntimeException failure) { + dropCopiesOlderThanAcceptedWrite(groupName, path, i); + throw failure; + } } cache.remove(groupName, path); return result; @@ -279,17 +372,34 @@ public Object pushFile(final String groupName, final String path, final File fil return object; } + /** + * Stores the object in every provider. With S3 asset storage on, this behaves like + * {@link #pushFile(String, String, File, Map)}: under the key's lock, durable providers first, + * and stale copies are deleted when a provider fails after a later one accepted the object. + * + * @param groupName the group to store into + * @param path the object's key + * @param writerDelegate serializes the object + * @param objectIn the object to store + * @param extraMeta extra metadata for providers that keep it + * @return the result of the first provider in the chain + * @throws DotDataException if a provider fails, or the key's lock cannot be taken in time + */ @Override public Object pushObject(final String groupName, final String path, final ObjectWriterDelegate writerDelegate, final Serializable objectIn, final Map extraMeta) throws DotDataException { if (AssetStorageFeature.isEnabled()) { - final var lock = assetLocks.get(groupName + "/" + path); - lock.lock(); + final Lock lock = lockKey(groupName, path); try { Object result = null; for (int i = storagePersistenceAPIList.size() - 1; i >= 0; i--) { - result = storagePersistenceAPIList.get(i).pushObject(groupName, path, writerDelegate, objectIn, extraMeta); + try { + result = storagePersistenceAPIList.get(i).pushObject(groupName, path, writerDelegate, objectIn, extraMeta); + } catch (DotDataException | RuntimeException failure) { + dropCopiesOlderThanAcceptedWrite(groupName, path, i); + throw failure; + } } cache.remove(groupName, path); return result; @@ -367,12 +477,21 @@ public Future pushObjectAsync(final String bucketName, final String path return futures.get(0); // well we return the first one at least } + /** + * Returns the file from the first provider that has it. With S3 asset storage on, this happens + * under the key's lock, providers before the one that had it are refilled with the file, and + * read failures propagate instead of being reported as absence. + * + * @param groupName the group to read from + * @param path the file's key + * @return the file, or {@code null} if no provider has it + * @throws DotDataException if a provider fails, or the key's lock cannot be taken in time + */ @Override public File pullFile(final String groupName, final String path) throws DotDataException { if (AssetStorageFeature.isEnabled()) { - final var lock = assetLocks.get(groupName + "/" + path); - lock.lock(); + final Lock lock = lockKey(groupName, path); try { final List missing = new ArrayList<>(); for (StoragePersistenceAPI storage : storagePersistenceAPIList) { @@ -468,8 +587,12 @@ public Object pullObject(final String groupName, if (AssetStorageFeature.isEnabled()) { final List missing = new ArrayList<>(); UnreadableStoredObjectException unreadable = null; - final var lock = assetLocks.get(groupName + "/" + path); - lock.lock(); + final Lock lock; + try { + lock = lockKey(groupName, path); + } catch (DotDataException e) { + throw new DotRuntimeException("Unable to retrieve stored object " + path, e); + } try { for (StoragePersistenceAPI storage : storagePersistenceAPIList) { Object object; diff --git a/dotCMS/src/main/java/com/dotcms/storage/FileSystemStoragePersistenceAPIImpl.java b/dotCMS/src/main/java/com/dotcms/storage/FileSystemStoragePersistenceAPIImpl.java index 8443b7f164ce..d78f8bbddb19 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/FileSystemStoragePersistenceAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/storage/FileSystemStoragePersistenceAPIImpl.java @@ -23,6 +23,7 @@ import java.io.InputStream; import java.io.OutputStream; import java.io.Serializable; +import java.nio.file.FileSystemException; import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; @@ -404,9 +405,11 @@ public File pullFile(final String groupName, final String path) throws DotDataEx /** * Reads and deserializes the stored object for the path. With S3 asset storage on, an unmapped - * group or a missing file returns {@code null}. A zero-length or truncated file is kept and - * reported as {@link UnreadableStoredObjectException}, so the chain can replace it from durable - * storage and nothing treats it as absent and regenerates it. Other read failures throw. + * group or a missing file returns {@code null}. A file that opens but cannot be decoded, such as + * a zero-length or truncated file or one not in the configured {@code CONTENT_METADATA_COMPRESSOR} + * format, is kept and reported as {@link UnreadableStoredObjectException}, so the chain can + * replace it from durable storage and nothing treats it as absent and regenerates it. A file + * that cannot be opened throws {@link DotDataException}. * With the flag off, a corrupt file is deleted and an empty map is returned, and a missing group * or file throws {@link IllegalArgumentException}, as before. * @@ -426,19 +429,23 @@ public Object pullObject(final String groupName, final String path, if (groupDir == null) { return null; } + final Path file; try { - final Path file = Paths.get(groupDir.getCanonicalPath(), normalizePath(groupName, path)); - final String compressor = Config.getStringProperty("CONTENT_METADATA_COMPRESSOR", "none"); - try (InputStream input = FileUtil.createInputStream(file, compressor)) { - return readerDelegate.read(input); - } + file = Paths.get(groupDir.getCanonicalPath(), normalizePath(groupName, path)); + } catch (IOException e) { + throw new DotDataException("Unable to read stored object " + path, e); + } + try (InputStream input = FileUtil.createInputStream(file, contentMetadataCompressor.get())) { + return readerDelegate.read(input); } catch (NoSuchFileException absent) { return null; - } catch (com.fasterxml.jackson.core.JsonProcessingException | java.io.EOFException corrupt) { - // Keep the file: the chain replaces it from durable storage, or the read fails. - throw new UnreadableStoredObjectException("Unable to read stored object " + path, corrupt); - } catch (IOException e) { + } catch (FileSystemException e) { + // Opening the file failed (permissions, too many open files, ...): an I/O problem, not corruption. throw new DotDataException("Unable to read stored object " + path, e); + } catch (IOException corrupt) { + // The file opened but could not be decoded (bad compression header, truncated or invalid + // contents). Keep it: the chain replaces it from durable storage, or the read fails. + throw new UnreadableStoredObjectException("Unable to read stored object " + path, corrupt); } } diff --git a/dotCMS/src/main/java/com/dotcms/storage/S3ContentAddressedStorage.java b/dotCMS/src/main/java/com/dotcms/storage/S3ContentAddressedStorage.java index d9def8ea94b1..a20c95f07ecc 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/S3ContentAddressedStorage.java +++ b/dotCMS/src/main/java/com/dotcms/storage/S3ContentAddressedStorage.java @@ -45,6 +45,19 @@ static String hash(File file) throws IOException { return HexFormat.of().formatHex(digest.digest()); } + /** + * Stores the file's bytes once under their SHA-256 and writes a small reference to them at the + * owner's key. A new blob is uploaded with a create-only write and is not downloaded again. A + * blob that already existed, or that another writer created first, is downloaded once and + * compared byte for byte. The method finishes by reading back the reference it published. + * + * @param ownerKey the content-owned key that will hold the reference + * @param source the file to store; a private copy is hashed and uploaded + * @param backfill {@code true} to convert an existing raw object or keep an existing matching + * reference instead of overwriting it, failing if either conflicts + * @return the SHA-256 of the stored bytes + * @throws DotDataException if the bytes or the reference cannot be stored or verified + */ String store(String ownerKey, File source, boolean backfill) throws DotDataException { Path snapshot = null; Path reference = null; @@ -76,14 +89,19 @@ String store(String ownerKey, File source, boolean backfill) throws DotDataExcep } } } - if (!blobMatches(blob, snapshot.toFile())) { + // The SDK checks the MD5 of each upload request against S3's response, and the key is the + // SHA-256 of the snapshot we sent, so only a blob someone else wrote needs a full comparison. + final var existing = storage.listFirstObject(bucket, blob); + boolean writtenElsewhere = existing != null && blob.equals(existing.getKey()); + if (!writtenElsewhere) { try { storage.uploadFileIfAbsent(bucket, blob, snapshot.toFile()); } catch (RuntimeException conflict) { if (!preconditionFailure(conflict)) throw conflict; + writtenElsewhere = true; } - if (!blobMatches(blob, snapshot.toFile())) { - throw new DotDataException("Shared asset bytes do not match their SHA-256 key"); - } + } + if (writtenElsewhere && !blobMatches(blob, snapshot.toFile())) { + throw new DotDataException("Shared asset bytes do not match their SHA-256 key"); } reference = Files.createTempFile("s3-asset-reference-", ".json"); Files.writeString(reference, "{\"sha256\":\"" + hash + "\",\"length\":" + Files.size(snapshot) + "}"); @@ -105,7 +123,6 @@ String store(String ownerKey, File source, boolean backfill) throws DotDataExcep } consumeReference(published); } - if (!matches(ownerKey, snapshot.toFile())) throw new DotDataException("Shared asset bytes were not verified after publication"); return hash; } catch (Exception failure) { if (failure instanceof InterruptedException) Thread.currentThread().interrupt(); @@ -116,7 +133,15 @@ String store(String ownerKey, File source, boolean backfill) throws DotDataExcep } } - private static boolean preconditionFailure(Throwable failure) { + /** + * Reports whether a failed S3 write was a conditional write that lost to an existing object: + * HTTP 412 when the condition was false, or 409 {@code ConditionalRequestConflict} when two + * conditional writes of one key raced. Causes are checked too, since callers may wrap the error. + * + * @param failure the error thrown by the write + * @return {@code true} if the write was refused because another write got there first + */ + static boolean preconditionFailure(Throwable failure) { for (Throwable cause = failure; cause != null; cause = cause.getCause()) { if (cause instanceof com.amazonaws.services.s3.model.AmazonS3Exception s3 && (s3.getStatusCode() == 412 || s3.getStatusCode() == 409)) return true; diff --git a/dotCMS/src/main/java/com/dotcms/storage/SharedExtractedMetadata.java b/dotCMS/src/main/java/com/dotcms/storage/SharedExtractedMetadata.java index 099571bc95ad..0004cfa4afc3 100644 --- a/dotCMS/src/main/java/com/dotcms/storage/SharedExtractedMetadata.java +++ b/dotCMS/src/main/java/com/dotcms/storage/SharedExtractedMetadata.java @@ -23,10 +23,25 @@ private static class Holder { static SharedExtractedMetadata getInstance() { return Holder.INSTANCE; } SharedExtractedMetadata(StoragePersistenceAPI storage) { this.storage = storage; } + /** + * Returns the byte-derived metadata for a file, sharing one extraction across every copy of the + * same bytes when S3 asset storage is on. Extracts directly, without the shared cache, when the + * feature is off or the extractor version is unknown, because unknown parser bundles must not + * share one cached result. + * + * @param source the file to extract from + * @param extractorVersion the Tika bundle version, or {@code null} when it cannot be determined + * @param schemaVersion the metadata schema version, part of the cache key + * @param textLimit the maximum extracted text length, part of the cache key + * @param extract the extraction to run on a cache miss + * @return the extracted metadata, as a mutable map + * @throws Exception when copying, hashing, or publishing the extraction fails + */ @SuppressWarnings("unchecked") Map get(File source, String extractorVersion, int schemaVersion, int textLimit, Function> extract) throws Exception { if (!AssetStorageFeature.isEnabled()) return extract.apply(source); + if (extractorVersion == null || extractorVersion.isBlank()) return extract.apply(source); final var snapshot = Files.createTempFile("asset-extraction-", ".tmp"); try { Files.copy(source.toPath(), snapshot, StandardCopyOption.REPLACE_EXISTING); diff --git a/dotCMS/src/main/java/com/dotmarketing/util/Config.java b/dotCMS/src/main/java/com/dotmarketing/util/Config.java index 6a9a92e45be3..4a3c73359f8a 100644 --- a/dotCMS/src/main/java/com/dotmarketing/util/Config.java +++ b/dotCMS/src/main/java/com/dotmarketing/util/Config.java @@ -27,6 +27,7 @@ import java.nio.file.Files; import java.util.*; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicBoolean; import java.util.stream.Collectors; @@ -53,6 +54,8 @@ public class Config { public static final Map testOverrideTracker = new ConcurrentHashMap<>(); + private static final Map> overrideCallbacks = new ConcurrentHashMap<>(); + private static final Set environmentSetKeys = ConcurrentHashMap.newKeySet(); private static SystemTableConfigSource systemTableConfigSource = null; @@ -782,25 +785,39 @@ public static boolean getBooleanProperty(String name, boolean defaultVal) { } /** - * @param key - * @param value + * Overrides a property in memory. When the {@code DOT_}-prefixed form of the key is already + * present, that form is overridden instead. Afterwards, runs the callbacks registered with + * {@link #onOverride(String, Runnable)} for the key exactly as it was passed in. + * + * @param key the property name + * @param value the new value */ public static void setProperty(String key, Object value) { if (props != null) { - final boolean assetStorageFlag = com.dotcms.storage.AssetStorageFeature.FLAG.equals(key); + final List callbacks = overrideCallbacks.getOrDefault(key, List.of()); if(props.containsKey(envKey(key))) { key = envKey(key); } trackOverrides(key, value); Logger.info(Config.class, "Setting property: " + key + " to " + value); props.setProperty(key, value); - if (assetStorageFlag) { - // Only in-memory overrides re-read the latched flag; reloads and the system table stay latched. - com.dotcms.storage.AssetStorageFeature.reset(); - } + callbacks.forEach(Runnable::run); } } + /** + * Registers a callback that runs after {@link #setProperty(String, Object)} overrides the given + * key in memory. Lets code that caches a property, for example a flag read once per process, + * forget that value when a test switches it. Property file reloads and system table changes do + * not run the callback. + * + * @param key the property name, as callers pass it to {@code setProperty} + * @param callback the action to run after each in-memory override of that key + */ + public static void onOverride(final String key, final Runnable callback) { + overrideCallbacks.computeIfAbsent(key, k -> new CopyOnWriteArrayList<>()).add(callback); + } + private static void trackOverrides(String key, Object value) { if (props.getBoolean(USE_CONFIG_TEST_OVERRIDE_TRACKER, false)) { if (value == null) { diff --git a/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureLatchTest.java b/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureLatchTest.java index 722bd390ad81..62e76aa17bcc 100644 --- a/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureLatchTest.java +++ b/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureLatchTest.java @@ -4,6 +4,8 @@ import static org.mockito.Mockito.mockStatic; import com.dotmarketing.util.Config; +import java.util.ArrayList; +import java.util.List; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -43,4 +45,21 @@ void inMemoryOverridesSwitchTheMode() { Config.setProperty(AssetStorageFeature.FLAG, false); assertFalse(AssetStorageFeature.isEnabled()); } + + @Test + void overrideCallbacksRunOnlyForTheirKeyAfterTheValueIsSet() { + final String key = "ASSET_STORAGE_LATCH_TEST_HOOKED_KEY"; + final String otherKey = "ASSET_STORAGE_LATCH_TEST_OTHER_KEY"; + final List seen = new ArrayList<>(); + Config.onOverride(key, () -> seen.add(Config.getStringProperty(key, null))); + try { + Config.setProperty(otherKey, "ignored"); + assertTrue(seen.isEmpty(), "Overriding another key must not run the callback"); + Config.setProperty(key, "updated"); + assertEquals(List.of("updated"), seen, "The callback runs once and sees the new value"); + } finally { + Config.setProperty(key, null); + Config.setProperty(otherKey, null); + } + } } diff --git a/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureTest.java b/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureTest.java index 21dfa0dc32b0..eda8870e58b0 100644 --- a/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureTest.java +++ b/dotCMS/src/test/java/com/dotcms/storage/AssetStorageFeatureTest.java @@ -156,6 +156,47 @@ void failedRemoteWriteKeepsPreviousLocalContents() throws Exception { assertEquals("previous version", Files.readString(destination)); } + @Test + void failedLocalWriteAfterDurableWriteServesTheDurableFile() throws Exception { + var local = spy(filesystem()); + Path destination = root.resolve(KEY.toLowerCase()); + Files.createDirectories(destination.getParent()); + Files.writeString(destination, "previous version"); + File upload = Files.writeString(root.resolve("upload"), "replacement").toFile(); + File download = Files.writeString(root.resolve("download.tmp"), "replacement").toFile(); + var remote = mock(StoragePersistenceAPI.class); + when(remote.pullFile(GROUP, KEY)).thenReturn(download); + doThrow(new DotDataException("disk full")).doCallRealMethod() + .when(local).pushFile(eq(GROUP), eq(KEY), any(), any()); + var chain = chain(local, remote); + + assertThrows(DotDataException.class, () -> chain.pushFile(GROUP, KEY, upload, Map.of()), + "The local failure reaches the caller"); + verify(remote).pushFile(GROUP, KEY, upload, Map.of()); + assertEquals("replacement", Files.readString(chain.pullFile(GROUP, KEY).toPath()), + "The next read restores the durable copy instead of serving the stale local one"); + } + + @Test + void failedLocalObjectWriteAfterDurableWriteServesTheDurableValue() throws Exception { + var local = spy(filesystem()); + var reader = new JsonReaderDelegate<>(String.class); + local.pushObject(GROUP, KEY, new JsonWriterDelegate(), "Original", Map.of()); + var remote = mock(StoragePersistenceAPI.class); + when(remote.pullObject(GROUP, KEY, reader)).thenReturn("Replacement"); + // The filesystem provider reports object write failures as unchecked exceptions. + doThrow(new RuntimeException("disk full")).doCallRealMethod() + .when(local).pushObject(eq(GROUP), eq(KEY), any(), any(), any()); + var chain = chain(local, remote); + + assertThrows(RuntimeException.class, + () -> chain.pushObject(GROUP, KEY, new JsonWriterDelegate(), "Replacement", Map.of()), + "The local failure reaches the caller"); + assertEquals("Replacement", chain.pullObject(GROUP, KEY, reader), + "The next read returns the durable value instead of the stale local one"); + assertEquals("Replacement", local.pullObject(GROUP, KEY, reader), "The local copy is restored"); + } + @Test void asyncWriteReportsRemoteFailureAndDoesNotPublishLocalReplacement() throws Exception { var fs = filesystem(); @@ -249,6 +290,42 @@ void deleteCannotBeUndoneBySameChainInflightObjectRestore() throws Exception { } } + @Test + void sameKeyIsLockedAcrossChainInstancesAndLetterCase() throws Exception { + var remote = mock(StoragePersistenceAPI.class); + File snapshot = Files.writeString(root.resolve("download.tmp"), "remote").toFile(); + var fetching = new CountDownLatch(1); + var finishFetch = new CountDownLatch(1); + when(remote.pullFile(GROUP, KEY)).thenAnswer(call -> { + fetching.countDown(); + assertTrue(finishFetch.await(10, TimeUnit.SECONDS)); + return snapshot; + }); + // Re-initializing the storage provider builds a new chain while the old one may still be in use. + var readingChain = chain(filesystem(), remote); + var deletingChain = chain(filesystem(), remote); + try (var executor = Executors.newFixedThreadPool(2)) { + var read = executor.submit(() -> readingChain.pullFile(GROUP, KEY)); + assertTrue(fetching.await(5, TimeUnit.SECONDS)); + var deleting = new CountDownLatch(1); + // The filesystem keeps one file for both spellings, so they must share a lock. + var delete = executor.submit(() -> { + deleting.countDown(); + return deletingChain.deleteObjectAndReferences(GROUP, KEY.toUpperCase()); + }); + assertTrue(deleting.await(5, TimeUnit.SECONDS)); + try { + assertThrows(TimeoutException.class, () -> delete.get(100, TimeUnit.MILLISECONDS), + "A delete through another chain, spelled differently, must wait for the restore"); + } finally { + finishFetch.countDown(); + } + read.get(5, TimeUnit.SECONDS); + delete.get(5, TimeUnit.SECONDS); + assertFalse(Files.exists(root.resolve(KEY.toLowerCase())), "The deleted file must not come back locally"); + } + } + @Test void unreadableLocalMetadataIsReplacedFromADurableCopyAndOtherwiseFails() throws Exception { // Metadata keeps lowercased keys in every slice; the binary groups later become case-preserving. @@ -274,6 +351,33 @@ void unreadableLocalMetadataIsReplacedFromADurableCopyAndOtherwiseFails() throws assertEquals("\"dura", Files.readString(local), "An unreadable copy without a replacement is kept"); } + @Test + void localMetadataNotInTheConfiguredCompressionIsReplacedFromADurableCopy() throws Exception { + final String group = "dotmetadata"; + final String previousCompressor = Config.getStringProperty("CONTENT_METADATA_COMPRESSOR", null); + var reader = new JsonReaderDelegate<>(String.class); + Path local = root.resolve(KEY.toLowerCase()); + Files.createDirectories(local.getParent()); + try { + for (String compressor : List.of("gzip", "bzip2")) { + Config.setProperty("CONTENT_METADATA_COMPRESSOR", compressor); + // The provider reads the compressor once, so it is created after setting it. + var fs = new FileSystemStoragePersistenceAPIImpl(); + fs.addGroupMapping(group, root.toFile()); + Files.writeString(local, "\"durable\""); + var remote = mock(StoragePersistenceAPI.class); + when(remote.pullObject(group, KEY, reader)).thenReturn("durable"); + + assertEquals("durable", chain(fs, remote).pullObject(group, KEY, reader), + "A " + compressor + " file that cannot be decoded is replaced, not a failed read"); + assertEquals("durable", fs.pullObject(group, KEY, reader), + "The local copy is replaced in the " + compressor + " format"); + } + } finally { + Config.setProperty("CONTENT_METADATA_COMPRESSOR", previousCompressor); + } + } + @Test void filesystemListingFindsKeysWhateverTheCaseOfThePrefix() throws Exception { // Metadata keeps lowercased keys in every slice; the binary groups later become case-preserving. diff --git a/dotCMS/src/test/java/com/dotcms/storage/S3ContentAddressedStorageTest.java b/dotCMS/src/test/java/com/dotcms/storage/S3ContentAddressedStorageTest.java new file mode 100644 index 000000000000..d951061b62d3 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/storage/S3ContentAddressedStorageTest.java @@ -0,0 +1,112 @@ +package com.dotcms.storage; + +import com.amazonaws.services.s3.model.AmazonS3Exception; +import com.amazonaws.services.s3.model.ObjectMetadata; +import com.amazonaws.services.s3.model.PutObjectRequest; +import com.amazonaws.services.s3.model.S3Object; +import com.amazonaws.services.s3.model.S3ObjectSummary; +import com.amazonaws.services.s3.transfer.Upload; +import com.dotcms.enterprise.publishing.storage.Storage; +import com.dotmarketing.exception.DotDataException; +import java.io.ByteArrayInputStream; +import java.io.File; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import org.apache.commons.codec.digest.DigestUtils; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.*; + +/** Store verifies shared bytes with at most one full blob download. */ +class S3ContentAddressedStorageTest { + private static final String BUCKET = "bucket"; + private static final String OWNER = "binary-assets/a/b/file"; + private static final byte[] BYTES = "shared asset bytes".getBytes(StandardCharsets.UTF_8); + private static final String HASH = DigestUtils.sha256Hex(BYTES); + private static final String BLOB = S3ContentAddressedStorage.blobKey(HASH); + + @TempDir Path root; + private Storage storage; + private File source; + + @BeforeEach + void setUp() throws Exception { + source = Files.write(root.resolve("source"), BYTES).toFile(); + storage = mock(Storage.class); + when(storage.uploadFile(any(PutObjectRequest.class))).thenReturn(mock(Upload.class)); + when(storage.getObject(BUCKET, OWNER)).thenAnswer(call -> + object("{}".getBytes(StandardCharsets.UTF_8), HASH)); + } + + @Test + void storingANewBlobDoesNotDownloadIt() throws Exception { + final var uploaded = new java.util.concurrent.atomic.AtomicBoolean(); + doAnswer(call -> { uploaded.set(true); return null; }) + .when(storage).uploadFileIfAbsent(eq(BUCKET), eq(BLOB), any(File.class)); + when(storage.getObject(BUCKET, BLOB)).thenAnswer(call -> uploaded.get() ? object(BYTES, null) : null); + + assertEquals(HASH, new S3ContentAddressedStorage(storage, BUCKET).store(OWNER, source, false)); + verify(storage).uploadFileIfAbsent(eq(BUCKET), eq(BLOB), any(File.class)); + verify(storage, never()).getObject(BUCKET, BLOB); + } + + @Test + void storingAnExistingBlobComparesItsBytesOnce() throws Exception { + when(storage.listFirstObject(BUCKET, BLOB)).thenReturn(summary()); + when(storage.getObject(BUCKET, BLOB)).thenAnswer(call -> object(BYTES, null)); + + assertEquals(HASH, new S3ContentAddressedStorage(storage, BUCKET).store(OWNER, source, false)); + verify(storage, times(1)).getObject(BUCKET, BLOB); + verify(storage, never()).uploadFileIfAbsent(any(), any(), any()); + } + + @Test + void storingABlobWrittenConcurrentlyByAnotherNodeComparesItsBytesOnce() throws Exception { + // The blob is absent when listed, then another node's conditional write wins the race. + final var written = new java.util.concurrent.atomic.AtomicBoolean(); + final var conflict = new AmazonS3Exception("ConditionalRequestConflict"); + conflict.setStatusCode(409); + doAnswer(call -> { written.set(true); throw conflict; }) + .when(storage).uploadFileIfAbsent(eq(BUCKET), eq(BLOB), any(File.class)); + when(storage.getObject(BUCKET, BLOB)).thenAnswer(call -> written.get() ? object(BYTES, null) : null); + + assertEquals(HASH, new S3ContentAddressedStorage(storage, BUCKET).store(OWNER, source, false)); + verify(storage, times(1)).getObject(BUCKET, BLOB); + } + + @Test + void anExistingBlobWithDifferentBytesFailsWithoutPublishingAReference() throws Exception { + final byte[] other = BYTES.clone(); + other[0] ^= 1; + when(storage.listFirstObject(BUCKET, BLOB)).thenReturn(summary()); + when(storage.getObject(BUCKET, BLOB)).thenAnswer(call -> object(other, null)); + + assertThrows(DotDataException.class, + () -> new S3ContentAddressedStorage(storage, BUCKET).store(OWNER, source, false)); + verify(storage, never()).uploadFile(any(PutObjectRequest.class)); + } + + private static S3ObjectSummary summary() { + final var summary = new S3ObjectSummary(); + summary.setKey(BLOB); + summary.setSize(BYTES.length); + return summary; + } + + private static S3Object object(final byte[] bytes, final String hashHeader) { + final var metadata = new ObjectMetadata(); + metadata.setContentLength(bytes.length); + if (hashHeader != null) { + metadata.addUserMetadata(S3ContentAddressedStorage.HASH_HEADER, hashHeader); + } + final var object = new S3Object(); + object.setObjectMetadata(metadata); + object.setObjectContent(new ByteArrayInputStream(bytes)); + return object; + } +} diff --git a/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java b/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java index eba855768d7b..254fe84ab38f 100644 --- a/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java +++ b/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java @@ -1,17 +1,39 @@ package com.dotcms.storage; import com.amazonaws.services.s3.AmazonS3; +import com.amazonaws.services.s3.model.AmazonS3Exception; +import com.amazonaws.services.s3.model.PutObjectRequest; +import com.amazonaws.services.s3.model.S3ObjectSummary; +import com.amazonaws.services.s3.transfer.Download; +import com.amazonaws.services.s3.transfer.Upload; +import com.amazonaws.services.s3.transfer.model.UploadResult; +import com.dotcms.concurrent.DotConcurrentFactory; import com.dotcms.enterprise.publishing.staticpublishing.AWSS3Configuration; import com.dotcms.enterprise.publishing.storage.AWSS3Storage; +import com.dotcms.enterprise.publishing.storage.Storage; +import com.dotcms.storage.repository.LocalFileRepositoryManager; import com.dotmarketing.exception.DotRuntimeException; import com.dotmarketing.util.Config; +import java.io.File; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import org.apache.commons.codec.digest.DigestUtils; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; class S3StorageConfigurationTest { private final Map previous = new HashMap<>(); @@ -140,6 +162,91 @@ void staticPushClientsWithoutARegionUseTheGlobalEndpointWithRegionLookup() throw } finally { legacy.shutdownTransferManager(); } } + @Test + void enabledUploadsDoNotHoldTheSharedIdentifierLock(@TempDir Path root) throws Exception { + // The identifier lock is shared with content saves; an upload must not make them time out. + final var storage = mock(Storage.class); + final var upload = mock(Upload.class); + final var result = new UploadResult(); + result.setETag("etag"); + when(storage.uploadFile(any(PutObjectRequest.class))).thenReturn(upload); + final var provider = new AmazonS3StoragePersistenceAPIImpl(storage, "bucket", + AmazonS3StoragePersistenceAPIImpl.PathEncryptionMode.NONE); + final File file = Files.writeString(root.resolve("upload.tmp"), "contents").toFile(); + try (var executor = Executors.newSingleThreadExecutor()) { + when(upload.waitForUploadResult()).thenAnswer(call -> executor.submit(() -> { + try { + return DotConcurrentFactory.getInstance().getIdentifierStripedLock() + .tryLock("s3_binary-assets/a/b/file", () -> result); + } catch (Throwable lockTimeout) { + throw new Exception(lockTimeout); + } + }).get(10, TimeUnit.SECONDS)); + assertEquals("etag", provider.pushFile("binary-assets", "/a/b/file", file, Map.of())); + } + } + + @Test + void enabledDownloadsUseTheirOwnFileWhateverTheRepositoryAndAreDeletedOnRelease(@TempDir Path root) throws Exception { + final var storage = mock(Storage.class); + when(storage.downloadFile(eq("bucket"), eq("binary-assets/a/b/file"), any(File.class))).thenAnswer(call -> { + Files.writeString(call.getArgument(2).toPath(), "remote"); + return mock(Download.class); + }); + final var provider = new AmazonS3StoragePersistenceAPIImpl(storage, "bucket", + AmazonS3StoragePersistenceAPIImpl.PathEncryptionMode.NONE); + // S3_STORAGE_FILE_REPO_TYPE=LOCAL maps every download of a key to one fixed path. + final Path repository = root.resolve("local-repo"); + Files.createDirectories(repository.resolve("a/b")); + final var field = AmazonS3StoragePersistenceAPIImpl.class.getDeclaredField("fileRepositoryManager"); + field.setAccessible(true); + field.set(provider, new LocalFileRepositoryManager(repository.toString()) { }); + + final File first = provider.pullFile("binary-assets", "/a/b/file"); + final File second = provider.pullFile("binary-assets", "/a/b/file"); + assertNotEquals(first, second, "Concurrent downloads of one key must not share a file"); + assertEquals("remote", Files.readString(first.toPath())); + assertEquals("remote", Files.readString(second.toPath())); + provider.releaseRetrievedFile(first); + provider.releaseRetrievedFile(second); + assertFalse(first.exists()); + assertFalse(second.exists()); + try (var files = Files.walk(repository)) { + assertEquals(0, files.filter(Files::isRegularFile).count(), "Downloads are not kept in the local repository"); + } + } + + @Test + void backfillTreatsAConflictingConcurrentWriteLikeAnExistingObject(@TempDir Path root) throws Exception { + // AWS answers 409 ConditionalRequestConflict, not 412, when two conditional writes race. + final var conflict = new AmazonS3Exception("ConditionalRequestConflict"); + conflict.setStatusCode(409); + final var storage = mock(Storage.class); + doThrow(conflict).when(storage).uploadFileIfAbsent(eq("bucket"), anyString(), any(File.class)); + final var provider = new AmazonS3StoragePersistenceAPIImpl(storage, "bucket", + AmazonS3StoragePersistenceAPIImpl.PathEncryptionMode.NONE); + + final File file = Files.writeString(root.resolve("source"), "contents").toFile(); + final var written = new S3ObjectSummary(); + written.setKey("binary-assets/a/b/file"); + written.setSize(file.length()); + written.setETag(DigestUtils.md5Hex("contents")); + when(storage.listFirstObject("bucket", "binary-assets/a/b/file")).thenReturn(null, written); + assertTrue(provider.backfillFile("binary-assets", "/a/b/file", file)); + + final var missing = new AmazonS3Exception("missing"); + missing.setStatusCode(404); + missing.setErrorCode("NoSuchKey"); + when(storage.downloadFile(eq("bucket"), eq("dotmetadata/a/b/meta.json"), any(File.class))) + .thenThrow(missing) + .thenAnswer(call -> { + Files.writeString(call.getArgument(2).toPath(), "\"value\""); + return mock(Download.class); + }); + assertTrue(provider.backfillObject("dotmetadata", "/a/b/meta.json", new JsonWriterDelegate(), + new JsonReaderDelegate<>(String.class), "value")); + } + private static String signerOverride(AWSS3Storage storage) throws Exception { return ((com.amazonaws.AmazonWebServiceClient) client(storage)).getClientConfiguration().getSignerOverride(); } diff --git a/dotCMS/src/test/java/com/dotcms/storage/SharedExtractedMetadataTest.java b/dotCMS/src/test/java/com/dotcms/storage/SharedExtractedMetadataTest.java new file mode 100644 index 000000000000..345712eff2ed --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/storage/SharedExtractedMetadataTest.java @@ -0,0 +1,54 @@ +package com.dotcms.storage; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoInteractions; + +import com.dotmarketing.util.Config; +import java.io.File; +import java.io.Serializable; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +class SharedExtractedMetadataTest { + @TempDir Path root; + private String previousFlag; + + @BeforeEach + void enable() { + previousFlag = Config.getStringProperty(AssetStorageFeature.FLAG, null); + Config.setProperty(AssetStorageFeature.FLAG, true); + } + + @AfterEach + void restore() { + Config.setProperty(AssetStorageFeature.FLAG, previousFlag); + } + + @Test + void unknownExtractorVersionExtractsWithoutTheSharedCache() throws Exception { + assertTrue(AssetStorageFeature.isEnabled()); + final File source = Files.writeString(root.resolve("asset.txt"), "same bytes").toFile(); + final StoragePersistenceAPI storage = mock(StoragePersistenceAPI.class); + final List extractedFrom = new ArrayList<>(); + final Map extracted = Map.of("content", "same bytes"); + + final Map result = new SharedExtractedMetadata(storage) + .get(source, null, 1, 100, file -> { + extractedFrom.add(file); + return extracted; + }); + + assertEquals(extracted, result); + assertEquals(List.of(source), extractedFrom, "Extraction must run on the source, not a cached copy"); + verifyNoInteractions(storage); + } +} From 7af5fa528363be5c7a47b68e3ee79176cbc3755a Mon Sep 17 00:00:00 2001 From: Scott Wicken <1562170+swicken@users.noreply.github.com> Date: Tue, 13 Oct 2026 10:51:23 -0400 Subject: [PATCH 7/7] test(storage): exercise the plain S3 key path with the metadata group --- .../storage/S3StorageConfigurationTest.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java b/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java index 254fe84ab38f..4451c753b7f1 100644 --- a/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java +++ b/dotCMS/src/test/java/com/dotcms/storage/S3StorageConfigurationTest.java @@ -177,19 +177,19 @@ void enabledUploadsDoNotHoldTheSharedIdentifierLock(@TempDir Path root) throws E when(upload.waitForUploadResult()).thenAnswer(call -> executor.submit(() -> { try { return DotConcurrentFactory.getInstance().getIdentifierStripedLock() - .tryLock("s3_binary-assets/a/b/file", () -> result); + .tryLock("s3_dotmetadata/a/b/file", () -> result); } catch (Throwable lockTimeout) { throw new Exception(lockTimeout); } }).get(10, TimeUnit.SECONDS)); - assertEquals("etag", provider.pushFile("binary-assets", "/a/b/file", file, Map.of())); + assertEquals("etag", provider.pushFile("dotmetadata", "/a/b/file", file, Map.of())); } } @Test void enabledDownloadsUseTheirOwnFileWhateverTheRepositoryAndAreDeletedOnRelease(@TempDir Path root) throws Exception { final var storage = mock(Storage.class); - when(storage.downloadFile(eq("bucket"), eq("binary-assets/a/b/file"), any(File.class))).thenAnswer(call -> { + when(storage.downloadFile(eq("bucket"), eq("dotmetadata/a/b/file"), any(File.class))).thenAnswer(call -> { Files.writeString(call.getArgument(2).toPath(), "remote"); return mock(Download.class); }); @@ -202,8 +202,8 @@ void enabledDownloadsUseTheirOwnFileWhateverTheRepositoryAndAreDeletedOnRelease( field.setAccessible(true); field.set(provider, new LocalFileRepositoryManager(repository.toString()) { }); - final File first = provider.pullFile("binary-assets", "/a/b/file"); - final File second = provider.pullFile("binary-assets", "/a/b/file"); + final File first = provider.pullFile("dotmetadata", "/a/b/file"); + final File second = provider.pullFile("dotmetadata", "/a/b/file"); assertNotEquals(first, second, "Concurrent downloads of one key must not share a file"); assertEquals("remote", Files.readString(first.toPath())); assertEquals("remote", Files.readString(second.toPath())); @@ -228,11 +228,11 @@ void backfillTreatsAConflictingConcurrentWriteLikeAnExistingObject(@TempDir Path final File file = Files.writeString(root.resolve("source"), "contents").toFile(); final var written = new S3ObjectSummary(); - written.setKey("binary-assets/a/b/file"); + written.setKey("dotmetadata/a/b/file"); written.setSize(file.length()); written.setETag(DigestUtils.md5Hex("contents")); - when(storage.listFirstObject("bucket", "binary-assets/a/b/file")).thenReturn(null, written); - assertTrue(provider.backfillFile("binary-assets", "/a/b/file", file)); + when(storage.listFirstObject("bucket", "dotmetadata/a/b/file")).thenReturn(null, written); + assertTrue(provider.backfillFile("dotmetadata", "/a/b/file", file)); final var missing = new AmazonS3Exception("missing"); missing.setStatusCode(404);