Repository navigation
Conversation
github-actions
Bot
added
Area : Backend
|
Claude finished @fabrizzio-dotCMS's task in 3m 11s —— View job Code Review — S3 storage layer (PR #37770)I traced every new branch in the storage providers and confirmed each is gated on Below is my recheck of the open findings plus a couple of smaller observations. New Issues
Existing
Resolved
Verdict: No new blocking bugs. The one pre-existing 🟠 High (stale local copy after a durable write succeeds) is the item I'd most want resolved before merge; the remaining 🟡 items are non-blocking but several (409 handling, compressor exceptions, MinIO CI coverage) are cheap and would meaningfully harden the flag-on path. · |
nollymar
added
the
PR : dotbot review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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( |
There was a problem hiding this comment.
🟡 [P2] SharedExtractedMetadata.java:35 guard null extractorVersion before building shared cache key
Current code:
final String configuration = org.apache.commons.codec.digest.DigestUtils.sha256Hex(
"tika:" + extractorVersion + ":schema:" + schemaVersion + ":text-limit:" + textLimit);Problem: TikaUtils.extractorVersion() (TikaUtils.java:672) returns null when OSGi/Tika is uninitialized, so the key becomes "tika:null:..." and unknown parser bundles share one cached entry, violating the class's own "unknown implementations must not share extraction caches" contract. Dead until PR 2 wires it.
Fix:
if (extractorVersion == null || extractorVersion.isBlank()) return extract.apply(source);Assumption: a PR-2 caller passes TikaUtils.extractorVersion() through unchanged. What to verify: no caller can reach get() with a null version and expect a correct per-parser cache.
…orage 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.
…nceAPI and route remote storage through the provider
…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 was not reachable from docs/README.md, which fails the docs reachability check.
801072a to
aa4cc7a
Compare
| 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( |
There was a problem hiding this comment.
SharedExtractedMetadata.java:35 null extractorVersion collapses distinct parser bundles into one shared cache key
Current code:
final String configuration = org.apache.commons.codec.digest.DigestUtils.sha256Hex(
"tika:" + extractorVersion + ":schema:" + schemaVersion + ":text-limit:" + textLimit);Problem: TikaUtils.extractorVersion() (TikaUtils.java:672) returns null when OSGi/Tika is not initialized, so the key becomes "tika:null:..." and unrelated parser bundles share one cached extraction, violating the class's own "unknown implementations must not share extraction caches" contract.
Fix:
if (extractorVersion == null) {
return extract.apply(source);
}
final String configuration = org.apache.commons.codec.digest.DigestUtils.sha256Hex(
"tika:" + extractorVersion + ":schema:" + schemaVersion + ":text-limit:" + textLimit);
| if (assetStorageFlag) { | ||
| // ponytail: only in-memory overrides re-read the latched flag; reloads and the system table stay latched. | ||
| com.dotcms.storage.AssetStorageFeature.reset(); | ||
| } |
There was a problem hiding this comment.
Config.java:800 drop stray 'ponytail:' token from comment
Current code:
// ponytail: only in-memory overrides re-read the latched flag; reloads and the system table stay latched.Problem: The comment begins with an accidental leftover word that obscures its meaning.
Fix:
// Only in-memory overrides re-read the latched flag; reloads and the system table stay latched.| 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( |
There was a problem hiding this comment.
SharedExtractedMetadata.java:34 guard null extractorVersion before building shared cache key
Current code:
final String configuration = org.apache.commons.codec.digest.DigestUtils.sha256Hex(
"tika:" + extractorVersion + ":schema:" + schemaVersion + ":text-limit:" + textLimit);Problem: TikaUtils.extractorVersion() (TikaUtils.java:672) returns null when OSGi/Tika is uninitialized, so the key becomes "tika:null:..." and unrelated parser bundles share one cached extraction, violating the class's own "unknown implementations must not share extraction caches" contract. Dead until PR 2 wires it.
Fix:
if (extractorVersion == null || extractorVersion.isBlank()) return extract.apply(source);
final String configuration = org.apache.commons.codec.digest.DigestUtils.sha256Hex(
"tika:" + extractorVersion + ":schema:" + schemaVersion + ":text-limit:" + textLimit);
fabrizzio-dotCMS
left a comment
fabrizzio-dotCMS
left a comment
There was a problem hiding this comment.
Reviewed the full production diff. The design matches the stack doc, and every new branch is gated, so flag-off behavior matches main.
Two things I'd like addressed before merge:
- A local write failing after S3 accepted leaves a stale local copy that keeps being served and is never evicted (comment on
ChainableStoragePersistenceAPI.pushFile). - The shared
IdentifierStripedLockheld across S3 uploads inAmazonS3StoragePersistenceAPIImpl.pushFile: harmless here, but once bundles go through it in #37774 it can fail unrelated content saves.
The rest are a verification-cost improvement in S3ContentAddressedStorage, CI coverage against a real S3 (MinIO through the existing docker-maven-plugin setup), lock hardening suggestions, a few documentation requests (target deployment, namespace, environment cloning), one question on key case, and minor items.
| * node half-enabled. Set it through the environment or properties file and restart.</p> | ||
| */ | ||
| public final class AssetStorageFeature { | ||
| public static final String FLAG = "FEATURE_FLAG_S3_ASSET_STORAGE"; |
There was a problem hiding this comment.
FEATURE_FLAG_S3_ASSET_STORAGE should be declared in com.dotcms.featureflag.FeatureFlagName like the other flags (with a Javadoc noting it is read once per process and needs a restart), and AssetStorageFeature.FLAG should reference it, the same way IndexConfigHelper.FLAG_KEY references FeatureFlagName.FEATURE_FLAG_OPEN_SEARCH_PHASE. That keeps the flag discoverable where people look for flags.
public static final String FLAG = FeatureFlagName.FEATURE_FLAG_S3_ASSET_STORAGE;| @@ -0,0 +1,129 @@ | |||
| # S3 asset storage | |||
|
|
|||
| S3 asset storage lets dotCMS keep binary assets and their metadata durably in S3, with the | |||
There was a problem hiding this comment.
Could this page state the deployment this feature targets? Reading the parent issue, the goal is to drop the shared NFS volume: S3 holds the only durable copy and is what nodes share, and each node keeps a bounded, disposable local cache (#37868, "scale out without a shared NFS volume"). The page doesn't say that, and a reader can easily conclude the opposite: that the flag adds S3 on top of an existing NFS asset directory, which only adds a layer (and eviction on a shared NFS directory is called out as unsafe later in the stack).
Two things would help:
- Say explicitly that the intended setup is a node-local asset directory per node, and whether running with the flag on over a shared NFS directory is supported, discouraged or untested.
- Say whether serving directly from S3 (streaming, or range requests without a full download to the local cache) is planned, or out of scope. Today a cold read downloads the whole object before serving, including for range requests on large files.
| private final List<StoragePersistenceAPI> storagePersistenceAPIList; | ||
| private final ObjectWriterDelegate defaultWriterDelegate; | ||
| private final Chainable404StorageCache cache; | ||
| private final com.google.common.util.concurrent.Striped<java.util.concurrent.locks.Lock> assetLocks = |
There was a problem hiding this comment.
Question on key case. The lock key is groupName + "/" + path, case-sensitive, and S3 keys keep their case (transformReadPath never lowercases with the flag on). The filesystem provider, though, lowercases every path (FileSystemStoragePersistenceAPIImpl.normalizePath). So two keys that differ only in case (.../File.json and .../file.json) are two S3 objects and take two different locks, but share one local file. The restore/delete ordering this lock guarantees doesn't hold for that pair, and the local cache can return one key's bytes for the other.
In this PR that only reaches the metadata chain, where keys come from inodes and field names, so it may be impossible in practice. If so, could that be stated here? Otherwise, deriving the lock key with the same normalization the local provider applies would close it. (1b keeps case locally for binary-assets/generated-assets, but dotmetadata stays lowercased.)
| private final ObjectWriterDelegate defaultWriterDelegate; | ||
| private final Chainable404StorageCache cache; | ||
| private final com.google.common.util.concurrent.Striped<java.util.concurrent.locks.Lock> assetLocks = | ||
| com.google.common.util.concurrent.Striped.lock(256); |
There was a problem hiding this comment.
Nit: the package names are spelled inline here instead of imported.
|
|
||
| if (AssetStorageFeature.isEnabled()) { | ||
| final var lock = assetLocks.get(groupName + "/" + path); | ||
| lock.lock(); |
There was a problem hiding this comment.
Two suggestions on the per-key lock, same pattern in all five flag-on branches (145, 254, 288, 375, 472):
-
Make
assetLocksstatic. The lock protects a key, not an instance, but each chain instance has its own stripes. Today each group is owned by one effectively-singleton chain (metadata viaStoragePersistenceProvider, binaries viaBinaryAssetStorageAPIImplin 1b, bundles in 4), so it works by discipline.StoragePersistenceProvider.forceInitialize()rebuilds the chain, after which an in-flight operation on the old instance and a new one on the new instance no longer exclude each other. A process-wide field removes that dependency at almost no cost (raise the stripe count if needed). Worth a comment that code holding one of these locks must never call into another chain, since there is no timeout. -
Bound the wait.
lock()waits forever and is not interruptible, and the critical section does real I/O: S3 transfers (the SDK only cuts after 50 s without data; a slow transfer that keeps progressing has no cap) and local writes, which on a hard-mounted NFS directory can block indefinitely. One stuck holder then parks every thread that needs a key on that stripe, which can exhaust request threads.tryLockwith a generous timeout (minutes) that fails the operation with a clear error would keep the correctness guarantee (it never proceeds without the lock) while making a stuck holder visible.
if (!lock.tryLock(LOCK_WAIT_MINUTES, TimeUnit.MINUTES)) {
throw new DotDataException("Timed out waiting for storage key " + groupName + "/" + path);
}| try { | ||
| storage.uploadFileIfAbsent(bucketName, transformReadPath(groupName, path), file); | ||
| } catch (com.amazonaws.services.s3.model.AmazonS3Exception conflict) { | ||
| if (conflict.getStatusCode() != 412) { |
There was a problem hiding this comment.
Minor: here (and in backfillObject, line 856) only 412 counts as "someone else wrote first", while S3ContentAddressedStorage.preconditionFailure and AWSS3Storage.uploadFileIfMatch also accept 409. AWS returns 409 ConditionalRequestConflict when two conditional writes to the same key race. With a 409 the backfill throws instead of falling through to the hasDurableCopy re-verification below, so the batch fails and relies on the job retry. Treating 409 like 412 here (or sharing one preconditionFailure helper) would make the three paths consistent.
| } | ||
| final String key = transformReadPath(groupName, path); | ||
| try (final InputStream input = Files.newInputStream(file.toPath())) { | ||
| final String md5 = DigestUtils.md5Hex(input); |
There was a problem hiding this comment.
Nit: the MD5 reads the whole local file before we know S3 has the key or that the size matches; when either fails the answer is false without any hash. For backfill of large files not yet in S3 that is a full read for nothing. Moving the digest after the listFirstObject and size check doesn't change the result.
final var object = storage.listFirstObject(bucketName, key);
if (object == null || !key.equals(object.getKey()) || object.getSize() != file.length()) {
return false;
}
try (InputStream input = Files.newInputStream(file.toPath())) {
return DigestUtils.md5Hex(input).equalsIgnoreCase(object.getETag())
|| storage.fileContentsMatch(bucketName, key, file);
}| } | ||
| consumeReference(published); | ||
| } | ||
| if (!matches(ownerKey, snapshot.toFile())) throw new DotDataException("Shared asset bytes were not verified after publication"); |
There was a problem hiding this comment.
Each store downloads the full blob twice to verify it: blobMatches after the conditional upload (line 84) and matches here, which also re-hashes the local snapshot. A new blob costs one upload plus two full downloads; a deduplicated one costs two full downloads. From #37772 this runs inside the check-in transaction (storeRevision under ESContentletAPIImpl.checkin), with the contentlet row locked, so a 1 GB binary keeps the transaction open for the upload plus 2 GB of reads.
Only one of those comparisons protects against something:
- After an upload that succeeded, the SDK has already validated the transferred bytes, so re-reading the blob adds nothing. The byte comparison matters when the blob already existed (412/409 on
uploadFileIfAbsent, orblobMatchestrue on entry), because then we don't know who wrote it. matchesat the end repeats that same blob comparison. What it adds is the reference header check, which thepublishedblock just above already does.
Suggestion: compare blob bytes only when the blob pre-existed, and end with the reference check only. A new blob then needs no download and a deduplicated one needs one, with the same guarantees.
| import static org.mockito.Mockito.*; | ||
|
|
||
| /** Real filesystem + AWS adapter + S3 server. See docs/testing/BINARY_S3_STORAGE.md. */ | ||
| @EnabledIfSystemProperty(named = "s3.test.endpoint", matches = ".+") |
There was a problem hiding this comment.
This class is the only test in the PR that talks to a real S3 API, and CI never sets s3.test.endpoint, so it is always skipped there. The rest of the suite (notably AssetStorageFeatureTest) runs in CI with mocks and fakes, which covers the chain and provider logic well, but the behavior that depends on a real server never runs in CI: If-None-Match/If-Match handling, listObjects pagination, and the real 404 NoSuchKey/409/412 responses that pullFile, uploadFileIfAbsent and uploadFileIfMatch map. Per the PR description, this is also the only PR in the stack that gets the full CI run.
Suggestion: add MinIO as one more entry in the docker-maven-plugin imagesMap in parent/pom.xml, next to database and opensearch, and pass -Ds3.test.endpoint plus the bucket properties to the test runs. That follows the pattern already used for Postgres and OpenSearch, works the same locally and in CI, and would let this class and the flag-on integration tests (-Ds3.cms.enabled=true) run on every PR of the stack. Untested paths in this PR that would benefit: S3ContentAddressedStorage, the multipart branch of uploadFileIfAbsent (above 32 MiB), and a local write failing after S3 accepted.
| return value; | ||
| } | ||
|
|
||
| private String groupKey(final String group) { |
There was a problem hiding this comment.
Docs suggestion on the namespace. The prefix is added here, while what the database stores (e.g. storageKey from #37772) is the key without it, so the configured value has to stay the same for an installation's lifetime and be identical on every node that shares its database. If a node reads a different value (or none), its writes and reads land under another prefix and content looks missing on that node.
dotmarketing-config.properties already warns that changing it "requires migration and separate local cache roots". It would help to say in BINARY_S3_STORAGE.md:
- every node of one installation (one database) must use the same value;
- like the flag, it shouldn't be set only in the system table:
Configgives an environment variable precedence over the system table, and if this provider is built before the system table source is initialized, it falls back to the properties file (empty) and keeps that value until restart, sincenamespaceis final; - how to clone an environment. Copying production's database into staging doesn't work on its own: with its own namespace (needed so staging's cleanup jobs don't delete production's objects), staging's rows point at keys that only exist under production's prefix, so it sees no binaries; with the same namespace, staging's deletions remove production's binaries. The supported path is starter export/import (feat(storage): add S3 binary backfill, starter export/import and integrity repair #37773), which publishes the assets under the target installation's namespace before the import commits.
Optional, if you think it's worth it: record the namespace in use on first enable (e.g. a database row) and refuse to start when the configured value differs.
|
dotbot code review:
No new actionable bugs were found in the current changes, but 1 prior unresolved dotbot finding still applies, so the patch remains incorrect. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · meta/muse-spark-1.3 · medium |
spec.md:456 resolve FR-012 contradiction between 260px minimum and shrink-to-fitPosted as a general PR comment because the referenced file is not part of this PR's diff. Current code: Problem: A card that "shrinks to fit" below 260px contradicts "MUST be at least 260px wide" in the same requirement; an implementer cannot satisfy both. Fix: |
|
dotbot code review:
1 new actionable finding was identified in the current changes, and 1 prior unresolved dotbot finding still applies, so the patch remains incorrect. The incremental delta adds only the Content Drive grid-view specification; no production code changed since the previously reviewed head. The spec is consistent with the existing keybindings spec (#32591) except for a minor wording contradiction in FR-012, a P3 spec-edit item. The prior Config.java comment typo remains unfixed in the branch and is carried forward. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · ~z-ai/glm-latest · medium |
|
Pull Request Unsafe to Rollback!!!
|
jcastro-dotcms
left a comment
jcastro-dotcms
left a comment
There was a problem hiding this comment.
Thanks for the detailed write-up and the docs, they made this a lot easier to review. I've been doing the QA and code review of this PR focused on one question: does dotCMS behave exactly like main with the flag off?
Short answer: yes, with one documented exception. I traced every changed method in the chain, filesystem, database and S3 providers, FileStorageAPIImpl, and the static push publishing classes, and with the flag off each one falls back to main's code. I also ran the tests locally on the PR head:
- the PR's own unit tests: 24 passed, 2 skipped (the MinIO ones, see the doc comment);
PublishingEndPointFactoryTest: 5 passed;- the existing integration tests that cover the changed code, with the flag off:
FileStorageAPITest,StoragePersistenceAPITestandFileMetadataAPITestall pass.PublishingEndPointTesthas 2 failures, but they fail the same way on main when the class runs on its own (the endpoint's catch block callsPortalUtil.getUser().getUserId()with no request on the thread). That's a pre-existing test-isolation issue unrelated to this PR, and it passes in CI insideMainSuite1a.
I agree with the points Fabrizzio raised, especially the stale local copy after S3 accepts a write, and the shared IdentifierStripedLock held during uploads. The inline comments below cover only what his review doesn't already mention:
- The STS module is the only flag-off behavior change, and I'd like it documented and in the release notes (
dotCMS/pom.xml). - With the flag on and
S3_STORAGE_FILE_REPO_TYPEset toLOCAL/HASH_LOCAL, concurrent downloads of the same key collide and downloaded files are never cleaned up (AmazonS3StoragePersistenceAPIImpl). - The MinIO image in the doc can't be pulled, so
BinaryS3StorageTestcan't run anywhere (BINARY_S3_STORAGE.md). - Two smaller ones in
Config: a leftoverponytail:in a comment, andConfignow depending on the storage feature.
I'm requesting changes for 1–3 together with Fabrizzio's two blocking items. 4 is minor.
The pre-merge manual test plan for the flag-off scenarios will follow as a separate comment on this PR.
| 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. |
There was a problem hiding this comment.
Small one: this comment starts with the word ponytail:, which doesn't mean anything in this context and looks like text left over from an earlier draft or a note-to-self. The rest of the sentence is accurate and useful, so it's only the leading token that should go.
It's harmless at runtime, but Config is one of the most-read classes in the codebase, and a reader will stop and wonder whether ponytail refers to some mechanism they should know about. Here's the same comment without it:
| // 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. |
| */ | ||
| public static void setProperty(String key, Object value) { | ||
| if (props != null) { | ||
| final boolean assetStorageFlag = com.dotcms.storage.AssetStorageFeature.FLAG.equals(key); |
There was a problem hiding this comment.
This is about which class depends on which, not about whether it works (it does).
Config is one of the lowest-level utilities we have: it lives in com.dotmarketing.util and pretty much everything else depends on it. With this change, setProperty now checks for one specific feature's key and calls com.dotcms.storage.AssetStorageFeature.reset() directly, written fully qualified in two places. So the generic configuration class now knows about the S3 storage feature, which is the opposite of the usual direction.
Why it matters beyond style:
- If another flag later needs the same "read once, reset in tests" behavior, the natural move is to add another
ifhere, andConfigslowly collects feature-specific special cases. - Someone reading
Config.setPropertyhas no hint of why storage code appears there unless they already know the latching design.
A couple of options, in order of preference:
- Give
Configa small, generic hook for in-memory overrides (for example, a way to register a callback for a given key), and haveAssetStorageFeatureregister its own reset.Configthen stays feature-agnostic. - If that's more than you want for this PR, keep it as is but import the class and add a one-line comment explaining that the S3 flag is latched at first read, and that this reset only exists so tests can switch modes.
Not a blocker on its own, but I'd like to see one of the two.
| </dependency> | ||
| <dependency> | ||
| <groupId>com.amazonaws</groupId> | ||
| <artifactId>aws-java-sdk-sts</artifactId> |
There was a problem hiding this comment.
I want to flag this one specifically, because it's the only change in the PR that reaches installations running with the flag off, and the goal of this review is that flag-off behaves exactly like main.
What I verified. Adding the STS module changes how the AWS SDK v1 resolves credentials when no access key and secret are configured. NoWebIdentityCredentialsProviderChain handles this well for dotCMS's own code: the three places where dotCMS falls back to the default chain (the S3 metadata provider, static push publishing, and the static push endpoint validation) use a chain without the web-identity step while the flag is off. Those three keep resolving credentials exactly as they did on main.
What still changes with the flag off (the PR description mentions both, but only briefly):
- AWS profiles that use
role_arn. On main, when the SDK reaches a profile withrole_arn, it tries to load STS by reflection, fails with "To use assume role profiles the aws-java-sdk-sts module must be on the class path" (I confirmed the message in the SDK jar), and quietly moves on to the next provider, usually the EC2/ECS instance role. With STS packaged, the role is now actually assumed. So the identity dotCMS uses for S3 can change after an upgrade, with no configuration change on the customer's side. If the assumed role and the instance role have different S3 permissions, static push publishing or the S3 metadata provider could start failing with AccessDenied, or start succeeding where they used to fail. - Plugins that build their own
DefaultAWSCredentialsProviderChainwith the SDK classes dotCMS provides. In an EKS pod with a web identity token, those plugins now get the pod's role instead of the node's role.
What it doesn't affect: installations with static keys configured (static keys always win before these steps), SDK v2 clients such as the SQS code (v2 has its own separate STS module), and stored data. Rolling back just removes the jar and returns to the old resolution, so there's no rollback-safety concern.
What I'm asking for: the risk is low, and both setups are uncommon in our Docker/Kubernetes deployments. But it's the one flag-off difference, and if it ever bites someone it will look like an unrelated permissions problem right after an upgrade. Could you:
- add a short note in
BINARY_S3_STORAGE.mdunder "Behavior with the flag off", and - make sure it goes into the release notes for the release that ships this PR?
That way support has something to point to if a customer reports S3 AccessDenied after upgrading.
| @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); |
There was a problem hiding this comment.
This is a flag-on issue that only appears with a non-default setting, but it's easy to miss.
With the flag on, pullFile downloads the object into fileRepositoryManager.getOrCreateFile(path). Which file that is depends on S3_STORAGE_FILE_REPO_TYPE:
- With the default (
TEMP), each call gets its own temporary file, andreleaseRetrievedFile(line 641) deletes it afterwards. That works correctly. - With
LOCALorHASH_LOCAL, the file is a fixed path derived from the key. Two problems follow from that.
1. Concurrent downloads of the same key collide. If two threads on the same node call pullFile for the same key at the same time, both download into the same file at once and can produce a mixed or truncated result. The chain's assetLocks prevents this when the call comes through ChainableStoragePersistenceAPI, but this method has no lock of its own. So anything that calls the S3 provider directly (the remoteObjectStorage() users later in the stack), or two separate chain instances, aren't protected.
2. Downloaded files are never cleaned up. releaseRetrievedFile only deletes when the repo is a TempFileRepositoryManager. With LOCAL/HASH_LOCAL, after the chain restores an object into the filesystem provider, the downloaded file stays where it is. Every restored asset ends up stored twice on the node's disk: once in the asset directory and once in the repo directory. For a feature whose point is a bounded local cache, that works against the goal, and eviction (in 1b) won't know about the second copy.
Suggestion: with the flag on, always download to a unique temporary file (Files.createTempFile, the same approach pushObject already takes when the flag is on), whatever the repo type, and always delete it in releaseRetrievedFile. If LOCAL/HASH_LOCAL aren't meant to be supported with the flag on, the alternative is to reject them at startup with a clear message and say so in BINARY_S3_STORAGE.md, so nobody runs into this in production.
| -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 |
There was a problem hiding this comment.
I tried to follow this section to run the MinIO tests locally during QA, and couldn't, because the image can't be pulled:
docker pull minio/minio:latestfails withpull access denied for minio/minio, repository does not exist or may require 'docker login'docker pull quay.io/minio/minio:latest(MinIO's own registry) fails with401 UNAUTHORIZED
Other Docker Hub images pull fine on the same machine, so it's this image specifically, not a general Docker or network problem.
Why it matters: BinaryS3StorageTest is the only test in this PR that talks to a real S3 API, and CI already skips it because there's no endpoint there. If developers can't run it locally either, nobody exercises it: the conditional writes (If-None-Match / If-Match), the real 404/409/412 handling, pagination, and S3ContentAddressedStorage against a real server. As a result, my local run of the PR's unit tests had to skip both tests in that class.
What I'm asking for:
- Point the doc at an image that can actually be pulled, ideally pinned to a specific tag or digest so it doesn't silently change. A dotCMS-mirrored image would be ideal.
- Use that same image for the
docker-maven-pluginentry Fabrizzio suggested in hisBinaryS3StorageTestcomment, so the tests run identically in CI and locally. - If MinIO is replaced with another S3-compatible server, it needs to support conditional PUTs (
If-None-Match: *andIf-Match), because these tests depend on them.
Once there's a working image I'll re-run the 2 skipped tests and report back on this PR.
This branch has not been deployed
4 participants
Refs #37868
Proposed Changes
This is the foundation for storing binary assets durably in S3, with the local asset directory as a cache. It adds the flag and the storage-layer behavior the later PRs build on. Content binaries are not stored through S3 until 2; with the flag on, the only paths this PR changes for running code are metadata reads and writes through
FileStorageAPI(storage failures now propagate instead of reading as missing) and static push publishing.AssetStorageFeaturereadsFEATURE_FLAG_S3_ASSET_STORAGEonce per process and keeps the value, because several storage objects capture the mode when they are built. It is set through the environment ordotmarketing-config.propertiesand needs a restart; a runtime change is ignored.Config.setPropertyresets the cached value so tests can switch modes. The first read logs at INFO only when the flag is on.ChainableStoragePersistenceAPIand the filesystem, database and S3 providers): writes publish to durable providers before the new local copy becomes visible, reads restore missing local copies from S3 without caching the miss, deletes keep the local copy until every durable provider accepts, and storage or database failures propagate instead of reading as a missing object. Restores take the same per-key lock as writes and deletes. A zero-length or truncated local metadata file is replaced from S3 when S3 holds a readable copy; otherwise the read fails and the file is kept, so the metadata is never treated as absent and regenerated. Providers gain prefix listing, durable-copy verification and non-overwriting backfill; S3 existence checks list at most one key.storage.file-metadata.s3.namespaceto separate installations that share a bucket.S3ContentAddressedStorage(one immutable blob per set of bytes, used from 1b) andSharedExtractedMetadata(shared Tika extraction, used from 2).AWSS3Storage) signs with SigV4, uses the bucket's own region (looked up by the SDK when none is configured), and lists every page of objects, but only when the flag is on.docs/testing/BINARY_S3_STORAGE.mddescribes the current behavior and how to run the checks. Later PRs add their own sections.Behavior with the flag off
Unchanged from main, with one documented difference in AWS credential resolution. Every new branch in the providers is gated on the flag, and the new configuration keys are ignored.
The STS module is now on the classpath. Where dotCMS itself falls back to the AWS default chain (static push publishing, its endpoint validation and the S3 metadata provider), it uses
NoWebIdentityCredentialsProviderChainwhile the flag is off: the SDK's default chain without the web-identity step, which always failed over on main. So a pod with a web-identity token keeps the identity it had on main. Two cases still see the module: a plugin that builds its ownDefaultAWSCredentialsProviderChain, and an AWS profile that usesrole_arn, which used to fail over to the next provider and now assumes the role.Review fixes
The last commit on this branch (
fix(storage): keep flag-off AWS credential resolution and harden the flag-on storage layer) addresses a full review of this PR:UnreadableStoredObjectException).Storage.listFirstObjectreplaces whole-group listings inexistsGroup,existsObjectandhasDurableCopy.us-west-2.pushFilestarts its upload inside its lock, the filesystem prefix listing resolves its prefix the way writes do, and unreachable flag checks are removed.asset-blobs/andextracted-metadata/are shared across namespaces.Deliberately not in this PR
The binary asset API and eviction (1b). Anything that routes content, metadata or other subsystems through S3 (2 to 6). The job-processor registration filter moves to 2, where the first S3 job processors arrive.
Checklist
NoWebIdentityCredentialsProviderChainTest. The MinIO tests skip in CI because CI provides no S3 endpoint (-Ds3.test.endpoint).Additional Info
This is the only PR in the stack that gets the full PR CI run, because that workflow only runs for PRs based on main. Rollback: merging with the flag off is safe to roll back. With the flag on and a namespace configured, metadata keys move under
asset-namespaces/..., which an older release does not read; the one-way content change starts in 2.