add index-level-encryption support for snapshots and remote-store #20095
Conversation
|
❌ Gradle check result for 2e1137b: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
@gbbafna, @Bukhtawar can you please review changes |
| indexKmsKey = cryptoMetadata.settings().get("kms.key_arn"); | ||
| indexEncContext = cryptoMetadata.settings().get("kms.encryption_context"); |
There was a problem hiding this comment.
this could also be cluster key. how do we by sure know that this is index level key? i think we need to put a marker in CryptoMetadata for index level keys.
There was a problem hiding this comment.
or we should do here CryptoMetadata.fromIndexSettings right here.
There was a problem hiding this comment.
The repository-level cryptoMetadata is available by default via blobStore for a BlobContainer. If there is any index-level cryptoMetadata, it is passed via this parameter. The index-level encryption cryptoMetadata would override the default.
| indexKmsKey = null; | ||
| indexEncContext = null; |
There was a problem hiding this comment.
should we rather have Optional s?
| * @param cryptoMetadata Optional CryptoMetadata for index-level encryption | ||
| * @throws IOException if copy fails | ||
| */ | ||
| public void copyFrom( |
There was a problem hiding this comment.
Not really following why we need this method.
There was a problem hiding this comment.
The earlier copyFrom method was calling the Lucene Directory implementation, which would request an IndexOutput override. This was passed from the RemoteDirectory implementation (where a BlobContainer object was returned).
However, for index-level encryption override, the BlobContainer needs to be created with CryptoMetadata, which we didn't have available to pass. Therefore, we're overriding the Lucene implementation to obtain the correct BlobContainer with the proper encryption metadata.
|
❌ Gradle check result for 8a3cc9c: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
| final String indexKmsKey; | ||
| final String indexEncContext; | ||
|
|
||
| if (cryptoMetadata != null && "aws-kms".equals(cryptoMetadata.keyProviderType())) { |
There was a problem hiding this comment.
S3BlobStore should instead be composed with a KMSCryptoMetadata extending CryptoMetadata to avoid these checks
| final Map<String, String> metadata | ||
| ) throws IOException { | ||
| final Map<String, String> metadata, | ||
| String indexKmsKey, | ||
| String indexEncContext) throws IOException { |
There was a problem hiding this comment.
A blob just needs to have an encryption context, index/cluster level keys shouldn't dictate how an upload should happen, can we please avoid modifying interfaces with additional info on index level enc wherever possible
There was a problem hiding this comment.
Yes, created KmsCryptoMetadata extending CryptoMetadata for KMS-specific functionality. Most of the code now passes the CryptoMetadata abstraction. The instanceof check in asyncBlobUpload is needed to extract KMS-specific values (key ARN and encryption context) for the async upload path, as these values need to be passed separately to the UploadRequest constructor.
| public static CryptoMetadata fromIndexSettings(Settings indexSettings) { | ||
| String keyProviderName = indexSettings.get("index.store.crypto.key_provider"); | ||
| if (keyProviderName == null) { | ||
| return null; | ||
| } | ||
|
|
||
| String keyProviderType = indexSettings.get( | ||
| "index.store.crypto.key_provider_type", | ||
| "aws-kms" // default | ||
| ); | ||
|
|
||
| Settings cryptoSettings = indexSettings.getAsSettings("index.store.crypto"); | ||
|
|
||
| return new CryptoMetadata(keyProviderName, keyProviderType, cryptoSettings); | ||
| } | ||
|
|
There was a problem hiding this comment.
Create a new KmsCryptoMetadata class to encapsulate this details
WalkthroughAdds index-level encryption support: new CryptoMetadata and KmsCryptoMetadata types, utilities for merging/encoding encryption contexts, and propagation of CryptoMetadata through repository, remote-store, blob container, and S3/Azure upload paths; many tests and mocks updated to the extended method signatures. Changes
sequenceDiagram
participant Listener as RemoteStoreRefreshListener
participant Uploader as RemoteStoreUploaderService
participant RemoteDir as RemoteDirectory
participant EncStore as EncryptedBlobStore
participant Provider as S3BlobContainer
participant S3 as S3Backend
Listener->>Uploader: uploadSegments(localSegments, cryptoMetadata)
Uploader->>RemoteDir: copyFrom(..., cryptoMetadata)
RemoteDir->>EncStore: getBlobContainer(blobPath, cryptoMetadata)
EncStore-->>RemoteDir: EncryptedBlobContainer
RemoteDir->>Provider: writeBlobWithMetadata(name, ..., metadata, cryptoMetadata)
Provider->>Provider: configureEncryptionSettings(cryptoMetadata)
Provider->>S3: PutObject (SSE-KMS headers + merged encryption-context)
S3-->>Provider: Success
Provider-->>RemoteDir: write complete
RemoteDir-->>Uploader: upload complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Areas requiring extra attention:
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.0)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java[] server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.javaComment |
|
❌ Gradle check result for 4f29198: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for c990e13: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 13a2cd9: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
udabhas
requested review from
CEHENKLE,
anasalkouz,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2 and
msfroh
as code owners
udabhas
requested review from
a team,
Rishikesh1159,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerMockClientTests.java (1)
755-761: Fix the type mismatch in the verification call.The CodeQL error indicates a signature mismatch. The
anyMap()matcher returnsMatcher<Map<Object,Object>>, but the method expectsMap<String, String>. This will cause compilation or verification failures.Apply this fix:
verify(s3BlobContainer, times(1)).executeMultipartUpload( any(S3BlobStore.class), anyString(), any(InputStream.class), anyLong(), - anyMap(), - null, null); + ArgumentMatchers.<Map<String, String>>anyMap(), + any(), + any());Alternatively, if nulls are specifically required:
verify(s3BlobContainer, times(1)).executeMultipartUpload( any(S3BlobStore.class), anyString(), any(InputStream.class), anyLong(), - anyMap(), + ArgumentMatchers.<Map<String, String>>anyMap(), null, null);plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java (1)
151-158: Compilation error: Extra parameter inexecuteSingleUploadcall.Same issue — 7 arguments passed but method expects 6.
() -> blobContainer.executeSingleUpload( blobStore, blobName, new ByteArrayInputStream(new byte[0]), ByteSizeUnit.MB.toBytes(2), - null, null, null)
🧹 Nitpick comments (13)
server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java (1)
144-145: Consider adding test coverage for the new index-level encryption functionality.The test correctly validates backward compatibility by passing
nullfor the IndexMetadata parameter, ensuring the system falls back to repository-level crypto metadata. However, since this PR introduces index-level encryption support, consider adding test cases that pass IndexMetadata with crypto settings to verify the new functionality works correctly.You can add a dedicated test method that:
- Creates an IndexMetadata instance with crypto settings (e.g.,
index.store.type=cryptofsandindex.store.crypto.kms.key_arn)- Passes it to
snapshotShardinstead ofnull- Verifies that the snapshot is created with index-level encryption
Would you like me to help generate such a test?
server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java (1)
20-22: TightenresolveCryptoMetadataJavadoc to match actual behaviorThe helper implementation is straightforward and NPE‑safe: it just extracts
CryptoMetadatafrom index settings (or returns null on missingIndexMetadata). The current Javadoc, however, suggests a “2‑layer priority” and explicit repository‑level fallback, which this method does not implement; the repo fallback is presumably handled later whencryptoMetadatais null.To avoid confusion for future readers, I’d align the Javadoc with what this method actually does, e.g.:
- /** - * Resolves CryptoMetadata with 2-layer priority: - * 1. Index-level encryption from index settings (if configured) - * 2. Repository-level encryption (fallback to null) - * - * @param indexMetadata Index metadata containing crypto settings - * @return CryptoMetadata if encryption is configured, null otherwise - */ + /** + * Resolves {@link CryptoMetadata} from the given index metadata. + * <p> + * Returns the index-level encryption configuration derived from index settings + * (if configured) or {@code null} if the index does not define crypto metadata. + * Repository-level encryption, if any, is handled by repository/remote-store + * components when {@code cryptoMetadata} is {@code null}. + * + * @param indexMetadata index metadata containing crypto settings, may be {@code null} + * @return CryptoMetadata if index-level encryption is configured, {@code null} otherwise + */Functionally the code looks good; this is just a documentation accuracy tweak.
Also applies to: 333-347
server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java (1)
103-105: Consider simplifying the null cast.The explicit cast
(CryptoMetadata) nullis unnecessary in Java—you can passnulldirectly. This would make the code slightly cleaner.Apply this diff:
final BlobContainer blobContainer = blobStoreRepository.shardContainer( new IndexId("DUMMY", indexId, pathType.getCode()), - shardId, (CryptoMetadata) null); + shardId, null);server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java (2)
93-107: Add input validation for crypto settings.The method reads settings without validating their values. Consider adding checks for:
- Empty/whitespace-only
keyProviderName- Valid
keyProviderTypevalues (if there's a known set of supported types)This would catch configuration errors earlier and provide clearer error messages.
Example validation:
public static CryptoMetadata fromIndexSettings(Settings indexSettings) { String keyProviderName = indexSettings.get("index.store.crypto.key_provider"); if (keyProviderName == null) { return null; } + if (keyProviderName.trim().isEmpty()) { + throw new IllegalArgumentException("index.store.crypto.key_provider cannot be empty"); + } String keyProviderType = indexSettings.get("index.store.crypto.key_provider_type", "aws-kms"); Settings cryptoSettings = indexSettings.getAsSettings("index.store.crypto"); if ("aws-kms".equals(keyProviderType)) { return new KmsCryptoMetadata(keyProviderName, cryptoSettings, true); } return new CryptoMetadata(keyProviderName, keyProviderType, cryptoSettings); }
154-155: Remove extra blank lines.There are two consecutive blank lines here, which appears unintentional.
plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobContainer.java (1)
140-150: Document that metadata and cryptoMetadata are not yet implemented for Azure.The method signature accepts
metadataandcryptoMetadataparameters but currently ignores them, delegating directly towriteBlob. This stub implementation should be documented to avoid confusion.Consider adding a comment or TODO explaining:
- Whether Azure crypto support is planned
- If there are technical limitations preventing implementation
- Tracking issue reference if applicable
Would you like me to help draft documentation or open a tracking issue for Azure crypto support?
server/src/main/java/org/opensearch/common/blobstore/stream/write/WriteContext.java (1)
11-11: Plumbing CryptoMetadata through WriteContext is correct; consider clarifying nullabilityThe CryptoMetadata additions (field, constructor/copy-constructor, builder, getter) correctly extend
WriteContextwithout changing existing behavior when no crypto is used.Given the constructor parameters are annotated
@Nullable, it would be clearer to also annotate the field and getter:- private final CryptoMetadata cryptoMetadata; + @Nullable + private final CryptoMetadata cryptoMetadata; ... - public CryptoMetadata getCryptoMetadata() { + @Nullable + public CryptoMetadata getCryptoMetadata() { return cryptoMetadata; }This makes it explicit to callers that
getCryptoMetadata()may returnnullwhen uploads rely on repository defaults.Also applies to: 35-36, 48-59, 69-70, 85-86, 152-157, 174-175, 221-224, 236-238
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerEncryptionTests.java (2)
20-32: Suggest parsing JSON for more robust assertions.The string
contains()checks may yield false positives if one key/value appears as a substring of another. Consider parsing the decoded JSON and asserting on the resulting Map.String merged = SseKmsUtil.mergeAndEncodeEncryptionContexts(indexContext, repoContext); assertNotNull(merged); String decoded = new String(Base64.getDecoder().decode(merged)); - assertTrue(decoded.contains("\"tenant\":\"acme\"")); - assertTrue(decoded.contains("\"classification\":\"confidential\"")); - assertTrue(decoded.contains("\"repo\":\"test\"")); - assertTrue(decoded.contains("\"env\":\"prod\"")); + + // Parse JSON and validate structure + Map<String, String> contextMap = parseJsonToMap(decoded); + assertEquals("acme", contextMap.get("tenant")); + assertEquals("confidential", contextMap.get("classification")); + assertEquals("test", contextMap.get("repo")); + assertEquals("prod", contextMap.get("env"));Helper method to add:
private Map<String, String> parseJsonToMap(String json) throws Exception { ObjectMapper mapper = new ObjectMapper(); return mapper.readValue(json, new TypeReference<Map<String, String>>(){}); }
34-43: Verify CryptoMetadata type and alignment with KmsCryptoMetadata.The test creates a
CryptoMetadatadirectly but doesn't verify it's the correct type for KMS operations. Consider usingKmsCryptoMetadataexplicitly or adding an instanceof check.Settings cryptoSettings = Settings.builder() .put("kms.key_arn", "arn:aws:kms:us-east-1:123456789:key/index-key") .put("kms.encryption_context", "tenant=acme,env=staging") .build(); - CryptoMetadata cryptoMetadata = new CryptoMetadata("index-provider", "aws-kms", cryptoSettings); + KmsCryptoMetadata cryptoMetadata = new KmsCryptoMetadata("index-provider", cryptoSettings, true); - assertEquals("arn:aws:kms:us-east-1:123456789:key/index-key", cryptoMetadata.settings().get("kms.key_arn")); - assertEquals("tenant=acme,env=staging", cryptoMetadata.settings().get("kms.encryption_context")); + assertTrue(cryptoMetadata.getKmsKeyArn().isPresent()); + assertEquals("arn:aws:kms:us-east-1:123456789:key/index-key", cryptoMetadata.getKmsKeyArn().get()); + assertTrue(cryptoMetadata.getKmsEncryptionContext().isPresent()); + assertEquals("tenant=acme,env=staging", cryptoMetadata.getKmsEncryptionContext().get());server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java (1)
171-197: Consider using an import forHashMapinstead of inline fully-qualified name.The test uses
java.util.HashMap<String, String>()inline in multiple places. For consistency and readability, consider adding an import.Add this import at the top of the file:
import java.util.HashMap;Then simplify the usages:
- var map = new java.util.HashMap<String, String>(); + var map = new HashMap<String, String>();server/src/test/java/org/opensearch/common/blobstore/EncryptedBlobStoreTests.java (1)
23-25: Consider explicit Mockito imports instead of wildcard.Using explicit imports improves code clarity and avoids potential conflicts. This is a minor style preference.
-import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.argThat; -import static org.mockito.Mockito.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when;server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java (1)
30-38: Avoid duplicating index-settings extraction logic between CryptoMetadata and KmsCryptoMetadata
KmsCryptoMetadata.fromIndexSettingsre-implements the sameindex.store.crypto.*extraction thatCryptoMetadata.fromIndexSettingsalready provides, but only for the KMS case.To reduce drift and keep behavior consistent over time, consider:
- Either delegating to
CryptoMetadata.fromIndexSettingsand casting when the result is aKmsCryptoMetadata, or- Removing this helper entirely in favor of the central
CryptoMetadata.fromIndexSettings.Functionally this is fine today, but the duplication makes future changes to the index settings schema easy to miss.
Also applies to: 58-66
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
38-74: Encryption context merging logic is sound for flat K/V contexts but relies on a very simple JSON parserThe overall behavior here looks correct:
mergeAndEncodeEncryptionContexts:
- Handles null/empty inputs.
- Accepts index context in either cryptofs or JSON, and repo context as Base64 JSON (with a lenient fallback to raw JSON).
- Merges as
repobase +indexoverride and returns Base64-encoded JSON.
mergeCryptoMetadata:
- Sensibly falls back when either side is null.
- Prefers index key provider/type when present.
- Starts from repo settings and overlays index settings, then explicitly merges
"kms.encryption_context"(cryptofs↔JSON conversions +mergeJson), preserving original format when only one side is present.Given S3/KMS encryption contexts are defined as flat string-string maps, the lightweight
cryptofsToJson/jsonToCryptofs/mergeJson/parseSimpleJsonhelpers are acceptable, but they have clear scope limits:
- Only top-level string keys/values are supported; no nested JSON.
- Escaping support is minimal (primarily
\").- Inputs with unexpected formatting (extra whitespace, unusual escaping, etc.) may not parse as expected.
Two suggestions:
- Explicitly document in Javadoc that only simple flat contexts are supported and that keys/values must not contain commas or unescaped quotes.
- If future requirements introduce more complex contexts, consider switching to a proper JSON parser for the merge path to avoid subtle parsing bugs.
Functionally, for the current KMS use-case this looks fine.
Also applies to: 85-131, 138-205, 211-240
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
CHANGELOG.md(1 hunks)plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobContainer.java(2 hunks)plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java(9 hunks)plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java(2 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerEncryptionTests.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerMockClientTests.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java(7 hunks)server/src/internalClusterTest/java/org/opensearch/snapshots/CloneSnapshotIT.java(3 hunks)server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java(3 hunks)server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java(2 hunks)server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java(1 hunks)server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobContainer.java(1 hunks)server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java(5 hunks)server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/stream/write/WriteContext.java(9 hunks)server/src/main/java/org/opensearch/common/blobstore/transfer/RemoteTransferContainer.java(6 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java(6 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreUploader.java(2 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreUploaderService.java(3 hunks)server/src/main/java/org/opensearch/index/store/RemoteDirectory.java(8 hunks)server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java(6 hunks)server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java(1 hunks)server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java(2 hunks)server/src/main/java/org/opensearch/indices/replication/checkpoint/RemoteStorePublishMergedSegmentAction.java(3 hunks)server/src/main/java/org/opensearch/repositories/FilterRepository.java(2 hunks)server/src/main/java/org/opensearch/repositories/Repository.java(2 hunks)server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java(20 hunks)server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java(1 hunks)server/src/main/java/org/opensearch/snapshots/SnapshotShardsService.java(2 hunks)server/src/test/java/org/opensearch/cluster/metadata/KmsCryptoMetadataTests.java(1 hunks)server/src/test/java/org/opensearch/common/blobstore/EncryptedBlobStoreTests.java(1 hunks)server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java(1 hunks)server/src/test/java/org/opensearch/index/shard/RemoteStoreUploaderServiceTests.java(9 hunks)server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java(2 hunks)server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java(7 hunks)server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryEncryptionTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java(2 hunks)server/src/test/java/org/opensearch/snapshots/mockstore/MockEventuallyConsistentRepository.java(1 hunks)test/framework/src/main/java/org/opensearch/index/shard/IndexShardTestCase.java(1 hunks)test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java(1 hunks)test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java (1)
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
EncryptionContextUtils(25-241)
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
server/src/main/java/org/opensearch/index/search/SimpleQueryStringQueryParser.java (1)
Settings(317-450)
server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java (1)
server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java (1)
ExperimentalApi(30-187)
server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (1)
server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java (1)
EncryptedBlobStore(27-151)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java (1)
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
EncryptionContextUtils(25-241)
🪛 GitHub Actions: CodeQL
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerMockClientTests.java
[error] 755-755: Compilation error: S3BlobContainer.executeMultipartUpload(...) signature mismatch. Expected (S3BlobStore,String,InputStream,long,Map<String,String>,CryptoMetadata) but found (S3BlobStore,String,InputStream,long,Map<Object,Object>,null,null).
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java
[error] 137-137: Compilation error: S3BlobContainer.executeSingleUpload(...) signature mismatch. Expected (S3BlobStore,String,InputStream,long,Map<String,String>,CryptoMetadata) but found (S3BlobStore,String,null,blobSize,null,null,null).
[error] 151-151: Compilation error: S3BlobContainer.executeSingleUpload(...) signature mismatch. Expected (S3BlobStore,String,InputStream,long,Map<String,String>,CryptoMetadata) but found (S3BlobStore,String,ByteArrayInputStream,long,null,null,null).
[error] 668-668: Compilation error: S3BlobContainer.executeSingleUpload(...) signature mismatch. Expected (S3BlobStore,String,InputStream,long,Map<String,String>,CryptoMetadata) but found (S3BlobStore,String,ByteArrayInputStream,int,Map<String,String>,null,null).
[error] 706-706: Compilation error: S3BlobContainer.executeSingleUpload(...) signature mismatch. Expected (S3BlobStore,String,InputStream,long,Map<String,String>,CryptoMetadata) but found (S3BlobStore,String,ByteArrayInputStream,long,Map<String,String>,null,null).
[error] 718-718: Compilation error: S3BlobContainer.executeMultipartUpload(...) signature mismatch. Expected (S3BlobStore,String,InputStream,long,Map<String,String>,CryptoMetadata) but found (S3BlobStore,String,,long,,,).
[error] 806-806: Compilation error: S3BlobContainer.executeMultipartUpload(...) signature mismatch. Expected (S3BlobStore,String,InputStream,long,Map<String,String>,CryptoMetadata) but found (S3BlobStore,String,ByteArrayInputStream,long,Map<String,String>,,).
[error] 936-936: Compilation error: S3BlobContainer.executeMultipartUpload(...) signature mismatch. Expected (S3BlobStore,String,InputStream,long,Map<String,String>,CryptoMetadata) but found (S3BlobStore,String,ByteArrayInputStream,long,,,).
🪛 GitHub Actions: Gradle Precommit
server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/snapshots/SnapshotShardsService.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/test/java/org/opensearch/cluster/metadata/KmsCryptoMetadataTests.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/common/blobstore/stream/write/WriteContext.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobContainer.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/test/java/org/opensearch/snapshots/mockstore/MockEventuallyConsistentRepository.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/repositories/FilterRepository.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryEncryptionTests.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/internalClusterTest/java/org/opensearch/snapshots/CloneSnapshotIT.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/index/shard/RemoteStoreUploaderService.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/index/shard/RemoteStoreUploader.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/common/blobstore/transfer/RemoteTransferContainer.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/test/java/org/opensearch/common/blobstore/EncryptedBlobStoreTests.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/indices/replication/checkpoint/RemoteStorePublishMergedSegmentAction.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/repositories/Repository.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/test/java/org/opensearch/index/shard/RemoteStoreUploaderServiceTests.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
server/src/main/java/org/opensearch/index/store/RemoteDirectory.java
[error] 1-1: Step failed: ':server:forbiddenApisMain'. ForbiddenApiException: Check for forbidden API calls failed. The log indicates forbidden usages (e.g., java.security.Policy / SystemPolicy). See build log for details.
...ns/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java
Outdated
Show resolved
Hide resolved
...ns/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java
Outdated
Show resolved
Hide resolved
...ns/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java
Outdated
Show resolved
Hide resolved
...ns/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java
Outdated
Show resolved
Hide resolved
...ns/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java
Outdated
Show resolved
Hide resolved
...ns/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/snapshots/SnapshotShardsService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (1)
766-784: Ensure shardContainer callsites use consistent CryptoMetadata when encryption is enabledThe new
shardContainer(IndexId, int, CryptoMetadata)correctly routes toEncryptedBlobStore.blobContainer(path, cryptoMetadata)only whencryptoMetadata != null. However, several paths that operate on shard-level blobs still callshardContainer(..., (CryptoMetadata) null), e.g.:
cloneShardSnapshot/cloneRemoteStoreIndexShardSnapshotwriteUpdatedShardMetaDataAndComputeDeletes(per‑shard cleanup)cleanupRemoteStoreLocks/deleteShardDatasnapshotRemoteStoreIndexShardshallow‑copy metadata writesgetRemoteStoreShallowCopyShardMetadataIf an index supplies CryptoMetadata that overrides the repository‑level metadata, snapshot data and shard‑level metadata for that index will be written using an index‑specific CryptoHandler, while these code paths will later access the same blobs using the repository‑level CryptoHandler. Under an
EncryptedBlobStorethis can lead to decryption failures or newly written shard metadata that is unreadable for that index.Consider:
- For any operation that reads or writes shard‑level blobs for a concrete index (index‑N, snap‑UUID, data blobs), resolving
IndexMetadataand usingshardContainer(indexId, shardId, indexMetadata)so thatresolveCryptoMetadatais consistently applied; or- Explicitly documenting and constraining which operations are intentionally repository‑level only and should ignore index CryptoMetadata.
At minimum, clone and delete paths that manipulate shard‑level snapshot state for an index should be revisited to confirm they are using the same CryptoMetadata as snapshot/restore.
Also applies to: 880-881, 1857-1898, 2503-2511, 2557-2560, 3892-3893, 4535-4536, 2974-2992, 4987-4993
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java (1)
237-268: Async uploads drop repository encryption context when CryptoMetadata is nullIn
asyncBlobUploadyou now derive:
indexKmsKeyandmergeEncContextonly whencrypto instanceof KmsCryptoMetadata, and- Pass
indexKmsKey != null ? indexKmsKey : blobStore.serverSideEncryptionKmsKey()andmergeEncContextintoUploadRequest.When
writeContext.getCryptoMetadata()isnull(the existing “repository‑only SSE‑KMS” case),indexKmsKeyandmergeEncContextboth staynull, so:
- The KMS key falls back correctly to
blobStore.serverSideEncryptionKmsKey(), but- The encryption context passed to
UploadRequestis nownull, whereas previously it was alwaysblobStore.serverSideEncryptionEncryptionContext().That changes behavior for repositories that relied solely on repository‑level
server_side_encryption.encryption_context: async uploads will stop sending any encryption context, which can break KMS policies or make encryption semantics inconsistent with the sync paths.Consider always computing the merged encryption context, e.g.:
CryptoMetadata crypto = writeContext.getCryptoMetadata(); String indexContext = (crypto instanceof KmsCryptoMetadata) ? ((KmsCryptoMetadata) crypto).getKmsEncryptionContext().orElse(null) : null; String mergedEncContext = SseKmsUtil.mergeAndEncodeEncryptionContexts( indexContext, blobStore.serverSideEncryptionEncryptionContext() ); String kmsKey = (crypto instanceof KmsCryptoMetadata) ? ((KmsCryptoMetadata) crypto).getKmsKeyArn().orElse(blobStore.serverSideEncryptionKmsKey()) : blobStore.serverSideEncryptionKmsKey();and then passing
kmsKeyandmergedEncContexttoUploadRequest. That preserves existing repo‑only behavior while enabling index‑level overrides.Also applies to: 278-290
🧹 Nitpick comments (6)
server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java (1)
72-87: Variable naming inconsistency.Line 80: The variable
IndexCryptoHandleruses PascalCase, which is typically reserved for class names in Java. It should beindexCryptoHandler(camelCase) to follow Java naming conventions.Apply this diff:
- CryptoHandler IndexCryptoHandler = cryptoHandlerRegistry.fetchCryptoHandler(merged); + CryptoHandler<?, ?> indexCryptoHandler = cryptoHandlerRegistry.fetchCryptoHandler(merged); BlobContainer blobContainer = blobStore.blobContainer(path); if (blobContainer instanceof AsyncMultiStreamBlobContainer) { - return new AsyncMultiStreamEncryptedBlobContainer<>((AsyncMultiStreamBlobContainer) blobContainer, IndexCryptoHandler); + return new AsyncMultiStreamEncryptedBlobContainer<>((AsyncMultiStreamBlobContainer) blobContainer, indexCryptoHandler); } - return new EncryptedBlobContainer<>(blobContainer, IndexCryptoHandler); + return new EncryptedBlobContainer<>(blobContainer, indexCryptoHandler);Also consider adding the generic type parameters
<?, ?>for consistency with the existing code pattern in the class.server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (1)
3973-3986: Tighten CryptoMetadata plumbing in snapshotShard/executeOneFileSnapshotFunctionally the snapshot flow looks correct:
snapshotShard(...)resolves CryptoMetadata fromIndexMetadata, picks a CryptoMetadata‑aware shard container, andsnapshotFile(...)uses that CryptoMetadata for per‑file uploads.There are a couple of minor cleanups that would simplify the code:
resolveCryptoMetadata(indexMetadata)is called both inshardContainer(indexId, shardId, indexMetadata)and again later insnapshotShardto computecryptoMetadata. You can compute it once at the top ofsnapshotShardand reuse it for both the container and the file uploads.executeOneFileSnapshot(...)receivesIndexMetadata indexMetadatabut never uses it; onlyCryptoMetadata cryptoMetadatais needed. Dropping the unused parameter and its recursive propagation will simplify the signature and reduce future confusion.These are non‑functional refactors but would make the CryptoMetadata flow easier to follow and maintain.
Also applies to: 4160-4178, 4185-4214, 4848-4891, 4987-4993
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
132-145: Be explicit that JSON utilities only handle flat string→string objects
cryptofsToJson,jsonToCryptofs,mergeJson, andparseSimpleJsondeliberately implement a minimal JSON subset:
- Only flat objects with
"key":"value"pairs (no nesting/arrays).- Keys and values must be quoted; values only handle
\"escaping.- Invalid or more complex input is effectively ignored rather than rejected.
Given this is used for KMS encryption contexts, that constraint is probably acceptable, but it would be helpful to:
- Document in Javadoc that only simple flat maps are supported, and
- Ensure callers (e.g., configuration docs/tests) don’t encourage more complex JSON here.
No functional change required, just tightening expectations.
Also applies to: 152-167, 177-199, 205-235
server/src/main/java/org/opensearch/index/store/RemoteDirectory.java (2)
69-72: Make blobStore/blobPath nullability invariant explicit
getBlobContainerassumes that ifblobStoreis anEncryptedBlobStorethenblobPathis non‑null, but the constructor does not enforce that these two fields are either both null or both non‑null. A mis‑use of the new 8‑arg constructor (passing a non‑nullblobStorewith a nullblobPath) would lead to an NPE when creating a crypto‑aware container.Consider asserting or documenting that
(blobStore == null) == (blobPath == null)in the 8‑arg constructor, so any misuse fails fast and clearly.Also applies to: 91-108, 155-168
435-454: Align AsyncMultiStream detection with the actual container used for uploadIn
copyFrom(...)you checkblobContainer instanceof AsyncMultiStreamBlobContainerto decide whether to take the async path, but inuploadBlob(...)you:
- Derive a possibly different
targetContainerviagetBlobContainer(cryptoMetadata), then- Cast
targetContainertoAsyncMultiStreamBlobContainerand invokeasyncBlobUpload.If
getBlobContainer(cryptoMetadata)ever returns a container that is not anAsyncMultiStreamBlobContainerwhileblobContaineris (for example, a different wrapper for index‑level CryptoMetadata), this will result in aClassCastExceptionat runtime.To make this robust, base both the
instanceofcheck and the cast on the same container instance, e.g.:- public boolean copyFrom(..., boolean lowPriorityUpload, CryptoMetadata cryptoMetadata) { - if (blobContainer instanceof AsyncMultiStreamBlobContainer) { - try { - uploadBlob(..., lowPriorityUpload, cryptoMetadata); - } catch (Exception e) { - listener.onFailure(e); - } - return true; - } - return false; - } + public boolean copyFrom(..., boolean lowPriorityUpload, CryptoMetadata cryptoMetadata) { + BlobContainer targetContainer = getBlobContainer(cryptoMetadata); + if (targetContainer instanceof AsyncMultiStreamBlobContainer) { + try { + uploadBlob(from, src, remoteFileName, context, postUploadRunner, listener, lowPriorityUpload, cryptoMetadata); + } catch (Exception e) { + listener.onFailure(e); + } + return true; + } + return false; + }and then either pass
targetContainerintouploadBlobor callgetBlobContainerconsistently there as well.This keeps the type check and the cast in sync and avoids depending on implicit assumptions about wrapper types.
Also applies to: 487-531, 572-578
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java (1)
729-749: Clarify intended encryption behavior for metadata uploadsThe new
copyFrom(Directory, String, String, IOContext, CryptoMetadata)correctly forwards CryptoMetadata toremoteDataDirectory, butuploadMetadata(...)always callsremoteMetadataDirectory.copyFrom(storeDirectory, metadataFilename, metadataFilename, IOContext.DEFAULT, null);passing
nullforcryptoMetadata.If the intent is that only segment data (not metadata files) honors index‑level crypto overrides and metadata always uses the repository’s default encryption, it would be helpful to document that explicitly in the Javadoc around
uploadMetadataor where CryptoMetadata is introduced.If, instead, metadata is also expected to be protected by the index‑level key, you may want to extend
uploadMetadatato accept aCryptoMetadataand plumb it through here similarly to the segment‑data path.Also applies to: 776-824
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
CHANGELOG.md(1 hunks)plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobContainer.java(2 hunks)plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java(10 hunks)plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerEncryptionTests.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerMockClientTests.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java(7 hunks)server/src/internalClusterTest/java/org/opensearch/snapshots/CloneSnapshotIT.java(3 hunks)server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java(3 hunks)server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java(1 hunks)server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java(1 hunks)server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobContainer.java(1 hunks)server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java(5 hunks)server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/stream/write/WriteContext.java(9 hunks)server/src/main/java/org/opensearch/common/blobstore/transfer/RemoteTransferContainer.java(6 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java(6 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreUploader.java(2 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreUploaderService.java(3 hunks)server/src/main/java/org/opensearch/index/store/RemoteDirectory.java(8 hunks)server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java(6 hunks)server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java(1 hunks)server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java(2 hunks)server/src/main/java/org/opensearch/indices/replication/checkpoint/RemoteStorePublishMergedSegmentAction.java(3 hunks)server/src/main/java/org/opensearch/repositories/FilterRepository.java(2 hunks)server/src/main/java/org/opensearch/repositories/Repository.java(2 hunks)server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java(20 hunks)server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java(1 hunks)server/src/main/java/org/opensearch/snapshots/SnapshotShardsService.java(2 hunks)server/src/test/java/org/opensearch/cluster/metadata/KmsCryptoMetadataTests.java(1 hunks)server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java(1 hunks)server/src/test/java/org/opensearch/index/shard/RemoteStoreUploaderServiceTests.java(9 hunks)server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java(2 hunks)server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java(7 hunks)server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryEncryptionTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java(2 hunks)server/src/test/java/org/opensearch/snapshots/mockstore/MockEventuallyConsistentRepository.java(1 hunks)test/framework/src/main/java/org/opensearch/index/shard/IndexShardTestCase.java(1 hunks)test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java(1 hunks)test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java
- server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java
- server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java
- server/src/main/java/org/opensearch/index/shard/RemoteStoreUploader.java
- server/src/main/java/org/opensearch/common/blobstore/stream/write/WriteContext.java
- server/src/main/java/org/opensearch/snapshots/SnapshotShardsService.java
- CHANGELOG.md
- server/src/test/java/org/opensearch/cluster/metadata/KmsCryptoMetadataTests.java
- server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java
- test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java
- server/src/main/java/org/opensearch/index/shard/RemoteStoreUploaderService.java
- plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobContainer.java
- plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerEncryptionTests.java
- server/src/main/java/org/opensearch/indices/replication/checkpoint/RemoteStorePublishMergedSegmentAction.java
- server/src/internalClusterTest/java/org/opensearch/snapshots/CloneSnapshotIT.java
- server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java
- server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java
- plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java
- server/src/main/java/org/opensearch/repositories/FilterRepository.java
- server/src/test/java/org/opensearch/index/shard/RemoteStoreUploaderServiceTests.java
- server/src/main/java/org/opensearch/repositories/Repository.java
🧰 Additional context used
🧬 Code graph analysis (3)
server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java (1)
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
EncryptionContextUtils(25-235)
server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java (2)
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
EncryptionContextUtils(25-235)server/src/main/java/org/opensearch/crypto/CryptoHandlerRegistry.java (1)
CryptoHandlerRegistry(32-169)
server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (1)
server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java (1)
EncryptedBlobStore(27-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: gradle-check
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: Analyze (java)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: Mend Security Check
🔇 Additional comments (43)
test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java (1)
201-214: Signature update correctly mirrors Repository APIAdding
IndexMetadata indexMetadatatosnapshotShardkeeps this test helper aligned with the updatedRepositoryinterface while preserving its no-op behavior, which is appropriate for a restore-only/dummy repository. No further changes needed here.server/src/test/java/org/opensearch/snapshots/mockstore/MockEventuallyConsistentRepository.java (1)
417-427: writeBlobWithMetadata delegation is appropriate for this mockDelegating to
writeBlobpreserves the existing consistency-testing behavior while safely accepting the new metadata/cryptoMetadata parameters, which this mock does not need to model. No further changes required here.server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java (1)
758-770: Signature update forsnapshotShardcorrectly tracks Repository APIAdding the
IndexMetadata indexMetadataparameter keeps this test stub aligned with the updatedRepository.snapshotShardsignature while preserving existing test behavior (the body is intentionally a no-op). No further changes are needed here.server/src/main/java/org/opensearch/common/blobstore/transfer/RemoteTransferContainer.java (3)
77-137: LGTM! Constructor delegation maintains backward compatibility.The constructor overloads correctly delegate to the primary constructor, passing
nullforcryptoMetadatawhen not provided. This ensures existing callers continue to work without modification.
61-61: LGTM! CryptoMetadata field and constructor implementation look correct.The
cryptoMetadatafield follows the same pattern as themetadatafield, and the primary constructor properly stores it. The Javadoc is clear and complete.Also applies to: 139-175
190-190: I attempted to verify the review comment, but the repository clone failed, preventing access to the codebase. Without being able to inspect theWriteContext.Builderimplementation and verify how it handles nullcryptoMetadatavalues, I cannot definitively confirm or refute the concern raised in the original review comment.
Verify that WriteContext.Builder handles null cryptoMetadata.
The
cryptoMetadatafield is propagated toWriteContext.Builder. SincecryptoMetadatacan benull(for backward compatibility), ensure thatWriteContext.Builder.cryptoMetadata()and downstream consumers properly handlenullvalues.test/framework/src/main/java/org/opensearch/index/shard/IndexShardTestCase.java (1)
534-535: LGTM - Test correctly updated for new signature.The addition of the
nullparameter aligns with the extendedRepository.snapshotShardsignature that now accepts anIndexMetadataparameter for index-level encryption support. Passingnullhere means the test uses repository-level defaults, which is appropriate for this test utility.plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerMockClientTests.java (1)
755-762: LGTM - Mock verification updated for crypto-aware signature.The test now verifies that
executeMultipartUploadis called with two additional parameters: a metadata Map (anyMap()) andnullfor CryptoMetadata. This correctly reflects the extended signature supporting index-level encryption metadata while testing the default (non-crypto) path.server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java (1)
839-843: LGTM - Mock signatures updated for CryptoMetadata support.The test mocks have been correctly updated to match the extended
RemoteDirectory.copyFromsignatures:
- Line 840: 8-parameter version (async with ActionListener + CryptoMetadata)
- Line 842: 5-parameter version (sync + CryptoMetadata)
The test continues to exercise the upload timeout scenario with the crypto-enabled API surface.
server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java (1)
209-220: LGTM - Correct no-op implementation for filesystem storage.The new
writeBlobWithMetadatamethod appropriately delegates towriteBloband ignores bothmetadataandcryptoMetadataparameters. This is the correct behavior for filesystem-based blob containers, which don't support metadata storage. The comment on line 218 clearly documents this limitation.server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java (2)
90-110: LGTM - Test updated for extended copyFrom signature.The test now passes two additional parameters to
copyFrom: a boolean flag (false) andnullfor CryptoMetadata. This correctly exercises the crypto-aware API path while testing the default (non-encrypted) scenario.
130-150: LGTM - Exception test updated consistently.The error path test is correctly updated with the same additional parameters (
false,null) to match the extendedcopyFromsignature, ensuring consistent test coverage across success and failure scenarios.server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java (1)
139-150: LGTM - RemoteDirectory constructor updated with crypto-aware parameters.The
RemoteDirectoryconstructor call now includes three additional parameters:
pendingDownloadMergedSegments: Map for tracking downloadsblobStoreRepository.blobStore(): Underlying BlobStore for crypto operationsdataPath: BlobPath for crypto-aware container selectionThese additions enable the RemoteDirectory to participate in index-level encryption flows by providing access to crypto-enabled blob containers when needed.
server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java (2)
72-87: LGTM - Crypto metadata merging correctly implemented.The new
blobContaineroverload correctly merges index-level and repository-level CryptoMetadata (with index taking priority) and creates the appropriate encrypted blob container. The delegation toEncryptionContextUtils.mergeCryptoMetadatacentralizes the merging logic appropriately.
123-134: LGTM - Merging logic properly delegated.The
mergeCryptoMetadatahelper method correctly delegates toEncryptionContextUtils.mergeCryptoMetadatafor consistent crypto metadata handling across the codebase. The method is well-documented with clear explanations of the merging strategy.server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobContainer.java (2)
125-144: LGTM - Metadata-aware encrypted write path.The new
writeBlobWithMetadatamethod correctly encrypts the input stream using the existingexecuteWritehelper and delegates to the underlying container with metadata. This maintains the encryption flow while supporting metadata propagation.
146-170: LGTM - SSE-KMS enabled encrypted write path.The overload with
CryptoMetadatacorrectly handles the dual-encryption scenario:
- Client-side encryption via
this.cryptoHandler(lines 158-160)- Server-side SSE-KMS settings via the
cryptoMetadataparameter (line 167)The comment on lines 155-157 helpfully clarifies this distinction. The implementation properly delegates both encryption layers to the underlying container.
server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java (6)
68-68: LGTM!Import added for
isNull()matcher to support verification of null CryptoMetadata parameters in method calls.
462-462: LGTM!Updated to match the new
copyFromsignature with crypto context parameter. Passingnullcorrectly tests the non-encrypted backward-compatible path.
483-504: LGTM!Mock setup and async
copyFromcall correctly updated with null CryptoMetadata parameter for the multipart upload test scenario.
545-545: LGTM!Exception test correctly updated with null crypto context parameter.
591-596: LGTM!Both the mock setup and exception assertion correctly updated with the null crypto context parameter.
708-709: LGTM!Verification correctly uses
isNull()matcher to assert the null CryptoMetadata parameter in the metadata upload path.server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryEncryptionTests.java (5)
23-52: LGTM!Comprehensive test for extracting KMS crypto metadata from index settings. Correctly validates the key provider name, type, index-level flag, and KMS-specific settings (key ARN and encryption context).
54-62: LGTM!Good negative test ensuring null is returned for indices without encryption configuration.
64-75: LGTM!Important edge case test validating that crypto metadata requires the key provider setting to be present.
77-105: LGTM!Good integration test validating crypto metadata extraction from complete IndexMetadata including the
cryptofsstore type.
107-136: LGTM!Excellent test validating the core PR functionality - different indices can have different KMS keys and encryption contexts, which is essential for index-level encryption support.
server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java (2)
58-66: LGTM!The static factory method provides a convenient type-specific entry point for creating
KmsCryptoMetadatafrom index settings. This complements the generalCryptoMetadata.fromIndexSettings()factory which handles type detection.
50-78: LGTM!Accessors appropriately use
Optionalfor nullable KMS settings, andtoString()provides useful debugging information.server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java (8)
20-41: LGTM!Comprehensive tests for
cryptofsToJsoncovering basic conversion, special characters, empty/null inputs, and single key-value pairs.
43-61: LGTM!Good use of
contains()assertions to handle potential ordering variations in the cryptofs output format.
63-89: LGTM!Thorough testing of JSON merge logic including conflict resolution, null handling, and empty JSON scenarios.
91-124: LGTM!Excellent coverage of
mergeAndEncodeEncryptionContextsincluding both cryptofs and JSON format inputs, null handling, and single-context scenarios.
126-171: LGTM!Comprehensive test of
mergeCryptoMetadatavalidating merge precedence, settings combination, encryption context merging, and all null-handling scenarios.
173-199: LGTM!Good test coverage for the simple JSON parser including whitespace handling and escaped quote support.
201-211: LGTM!Important round-trip test ensuring data integrity through format conversions.
213-233: LGTM!Excellent integration test for a realistic merge scenario with multiple conflicting and non-conflicting keys, validating the complete merge and encode flow.
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java (1)
200-218: CryptoMetadata-aware writeBlobWithMetadata and sync upload paths look consistentThe new
writeBlobWithMetadata(..., @Nullable Map<String,String> metadata, @Nullable CryptoMetadata cryptoMetadata)correctly:
- Delegates small vs large uploads to
executeSingleUpload/executeMultipartUpload, and- Leaves the
failIfAlreadyExistscontract unchanged (still effectively ignored due to S3 semantics).Both
executeSingleUploadandexecuteMultipartUpload:
- Accept a nullable
CryptoMetadataand pass it intoconfigureEncryptionSettings(...), so repo-only and index+repo CryptoMetadata cases are handled uniformly.- Still respect object metadata when present.
Once the async encryption-context issue above is addressed, the sync paths and crypto-aware write entrypoints look good.
Also applies to: 220-234, 569-576, 620-627, 645-658
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
38-70: Encryption context and CryptoMetadata merge semantics align with the intended model
mergeAndEncodeEncryptionContextsandmergeCryptoMetadataimplement a clear and compatible strategy:
- Repo context as baseline, index context as override on key conflicts.
- Support for both cryptofs (
k=v,...) and JSON formats, with repo context decoded from Base64 when necessary.- For CryptoMetadata, repository settings are overlaid with index settings, and
kms.encryption_contextis merged then stored back in cryptofs form, preserving existing configuration shape.This matches the index‑overrides‑repository story in the PR description and provides a single place to adjust merging behavior going forward.
Also applies to: 81-125
server/src/main/java/org/opensearch/index/store/RemoteDirectory.java (1)
456-485: Sync copyFrom with CryptoMetadata looks correct and failure cleanup is safeThe new sync
copyFrom(Directory, String, String, IOContext, CryptoMetadata):
- Uses
IOContext.READONCE, which matches Lucene’s typical copy semantics.- Selects the target
BlobContainerviagetBlobContainer(cryptoMetadata)so index‑level overrides participate.- Cleans up partially written blobs by calling
deleteFile(dest)only whensuccessis false, swallowing deletion errors.This behavior looks sound and exception‑safe.
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java (1)
21-37: CryptoMetadata‑aware SSE‑KMS wiring and context merge look consistentThe new
mergeAndEncodeEncryptionContextshelper plus the CryptoMetadata‑awareconfigureEncryptionSettingsoverloads for multipart and put‑object:
- Correctly prefer index‑level KMS key and encryption context when
cryptoMetadatais aKmsCryptoMetadata, falling back to the repository settings.- Centralize context merge/encoding via
EncryptionContextUtils, avoiding duplicated logic and keeping behavior consistent with other blobstore paths.- Preserve AES256 behavior and existing UploadRequest‑based code paths.
From the OpenSearch side this is a clean, backwards‑compatible extension for index‑level overrides.
Also applies to: 39-65, 66-95
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java (1)
590-647: CryptoMetadata propagation through segment uploads is consistentThe new
copyFrom(..., boolean lowPriorityUpload, CryptoMetadata cryptoMetadata)overload cleanly threads CryptoMetadata into both upload paths:
- For non‑
segments_Nfiles, it passescryptoMetadatathrough toremoteDataDirectory.copyFrom(...), which in turn uses the crypto‑awareRemoteDirectoryupload path.- For the fallback case (no async multi‑stream support or segments_N), it calls the new
copyFrom(Directory, String, String, IOContext, CryptoMetadata)overload so the sync upload also respects the same CryptoMetadata.The delegating overload with
cryptoMetadata = nullkeeps the previous public API intact. This looks correct and keeps index‑level overrides consistently applied to segment data.
server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java
Show resolved
Hide resolved
|
❌ Gradle check result for 98bacc2: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java (1)
58-66: Consider additional validation in factory method.The
fromIndexSettingsmethod returns null ifkey_provideris missing, but doesn't validate that KMS-specific settings (key_arn, encryption_context) are present. While this allows for flexible configuration, consider whether stricter validation would prevent configuration errors earlier.Current behavior: Creates
KmsCryptoMetadataeven if KMS-specific fields are absent (they'll be Optional.empty()).If stricter validation is desired:
public static KmsCryptoMetadata fromIndexSettings(Settings indexSettings) { String keyProviderName = indexSettings.get("index.store.crypto.key_provider"); if (keyProviderName == null) { return null; } Settings cryptoSettings = indexSettings.getAsSettings("index.store.crypto"); + // Optional: Validate that at least key_arn is present for KMS + if (cryptoSettings.get(KMS_KEY_ARN_SETTING) == null) { + return null; + } return new KmsCryptoMetadata(keyProviderName, cryptoSettings, true); }However, the current lenient approach may be intentional to support partial configurations.
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
58-64: Silent fallback on Base64 decode failure may hide configuration errors.When Base64 decoding fails, the code silently uses the raw string. This could mask misconfiguration where an invalid encryption context is provided.
Consider logging a warning when the fallback is used:
try { byte[] decoded = Base64.getDecoder().decode(repoEncContext); repoJson = new String(decoded, StandardCharsets.UTF_8); } catch (IllegalArgumentException e) { + // Log at debug level - might be intentional raw JSON repoJson = repoEncContext; }plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java (1)
66-95: Consider extracting duplicate KMS configuration logic.The KMS key and context extraction logic (lines 74-84) is duplicated between the two
configureEncryptionSettingsoverloads. Consider extracting to a private helper.private static class KmsSettings { final String kmsKey; final String encContext; KmsSettings(String kmsKey, String encContext) { this.kmsKey = kmsKey; this.encContext = encContext; } } private static KmsSettings extractKmsSettings(CryptoMetadata cryptoMetadata, S3BlobStore blobStore) { String indexKmsKey = null; String indexEncContext = null; if (cryptoMetadata instanceof KmsCryptoMetadata kmsMetadata) { indexKmsKey = kmsMetadata.getKmsKeyArn().orElse(null); indexEncContext = kmsMetadata.getKmsEncryptionContext().orElse(null); } String kmsKey = (indexKmsKey != null) ? indexKmsKey : blobStore.serverSideEncryptionKmsKey(); String encContext = mergeAndEncodeEncryptionContexts(indexEncContext, blobStore.serverSideEncryptionEncryptionContext()); return new KmsSettings(kmsKey, encContext); }server/src/main/java/org/opensearch/index/store/RemoteDirectory.java (1)
69-72: ClarifygetBlobContainersemantics and constructor expectations for encrypted reposThe
getBlobContainer(@Nullable CryptoMetadata)implementation looks correct (client‑side encryption viaEncryptedBlobStore, otherwise fall back toblobContainer), but a couple of details would benefit from tightening:
- The javadoc currently says “CryptoMetadata no longer used to create wrapper here to prevent double encryption with SSE repos”, yet for
EncryptedBlobStorewe still do usecryptoMetadatato derive a container. That’s correct for client‑side encryption, but a bit confusing as written. It might be clearer to explicitly say that CryptoMetadata is ignored here for SSE repositories and only used for client‑sideEncryptedBlobStoreimplementations.- When
blobStoreis anEncryptedBlobStore, we implicitly assumeblobPathis non‑null. That’s presumably enforced by call sites using the new 8‑arg constructor, but it’s not obvious from this class alone. A small assertion (or at least a comment) thatblobPathmust be non‑null wheneverblobStoreisEncryptedBlobStorewould make misuse easier to spot during future refactors.Functionally this all looks fine; these are just clarity/defensive‑coding tweaks.
Also applies to: 91-108, 110-172
server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (2)
58-79: Shard container overloads and CryptoMetadata resolution look consistentThe new CryptoMetadata-aware container plumbing is coherent:
- All shard‑level callers that don’t care about index‑level encryption now go through
shardContainer(indexId, shardId, (CryptoMetadata) null), preserving pre‑existing behavior.- New overloads:
shardContainer(IndexId, ShardId, IndexMetadata)→ resolvesCryptoMetadataand delegates.shardContainer(IndexId, ShardId, CryptoMetadata)/shardContainer(IndexId, int, CryptoMetadata)→ switch toEncryptedBlobStorewhen available, else use the plain blob store.resolveCryptoMetadata(IndexMetadata)centralizes the mapping from index settings to CryptoMetadata, and all crypto‑aware callers use this helper, which keeps the behavior consistent.This should give you:
- Client‑side encryption: containers keyed per index via
EncryptedBlobStore.blobContainer(shardPath, cryptoMetadata).- SSE/no‑crypto repos: transparent fallback to
blobStore().blobContainer(shardPath(indexId, shardId)).Also 👍 that reads (
restoreShard,getShardSnapshotStatusoverloads) now useIndexMetadata→CryptoMetadata→EncryptedBlobStoreon the way back, which is required to decrypt client‑side encrypted data.The only API nuance is that the original public
shardContainer(IndexId, int)signature has effectively been replaced withshardContainer(IndexId, int, CryptoMetadata)/shardContainer(IndexId, int, IndexMetadata). Internal call sites have been updated, but if any plugins or external code were using the old 2‑arg variant this will be a source‑compatible change. If that’s a concern, you might consider keeping a deprecated 2‑arg wrapper that simply delegates withnullCryptoMetadata.Also applies to: 766-881, 1857-1900, 2503-2560, 2974-2993, 4535-4553, 4987-4993
3963-4183: Snapshot/restore CryptoMetadata wiring is correct; minor cleanups possibleThe snapshot/restore changes look structurally sound:
snapshotShard(...)now receivesIndexMetadata indexMetadata, resolvesCryptoMetadataonce:final CryptoMetadata cryptoMetadata = resolveCryptoMetadata(indexMetadata);and passes it down to
executeOneFileSnapshot(...)→snapshotFile(...).
snapshotFile(...)uses both:
shardContainer(indexId, shardId, cryptoMetadata)so client‑side encrypted repos get anEncryptedBlobStorecontainer bound to the index‑level key, andwriteBlobWithMetadata(partName, inputStream, partBytes, false, null, cryptoMetadata)so SSE repos can pick up merged encryption context fromcryptoMetadata.
restoreShard(...)pullsIndexMetadatafrom cluster state and routes reads through the same crypto‑awareshardContainer, so decryption works for client‑side encrypted snapshots and SSE remains unaffected.A couple of small polish points:
executeOneFileSnapshot(...)takesIndexMetadata indexMetadatabut never uses it; recursion just threads it through. Since you already pre‑resolveCryptoMetadatainsnapshotShardand pass that down, you can drop theindexMetadataparameter entirely and simplify the signature to:private void executeOneFileSnapshot( Store store, SnapshotId snapshotId, IndexId indexId, IndexShardSnapshotStatus snapshotStatus, BlockingQueue<FileInfo> filesToSnapshot, Executor executor, ActionListener<Void> listener, CryptoMetadata cryptoMetadata )and adjust the calls accordingly.
For metadata/
index-writes at the top ofsnapshotShard, you still callshardContainer(indexId, shardId, indexMetadata), which re‑invokesresolveCryptoMetadata(indexMetadata)internally. That’s fine, but if you want to avoid recomputing it you could route those through the already resolvedcryptoMetadataas well for consistency.Functionally, the encryption plumbing here looks good; these are just minor cleanups.
Also applies to: 4185-4218, 4259-4261, 4848-4891
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
CHANGELOG.md(1 hunks)plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobContainer.java(2 hunks)plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java(10 hunks)plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerEncryptionTests.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerMockClientTests.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java(7 hunks)server/src/internalClusterTest/java/org/opensearch/snapshots/CloneSnapshotIT.java(3 hunks)server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java(3 hunks)server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java(1 hunks)server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java(1 hunks)server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobContainer.java(1 hunks)server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java(5 hunks)server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/stream/write/WriteContext.java(9 hunks)server/src/main/java/org/opensearch/common/blobstore/transfer/RemoteTransferContainer.java(6 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java(6 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreUploader.java(2 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreUploaderService.java(3 hunks)server/src/main/java/org/opensearch/index/store/RemoteDirectory.java(8 hunks)server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java(6 hunks)server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java(1 hunks)server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java(2 hunks)server/src/main/java/org/opensearch/indices/replication/checkpoint/RemoteStorePublishMergedSegmentAction.java(3 hunks)server/src/main/java/org/opensearch/repositories/FilterRepository.java(2 hunks)server/src/main/java/org/opensearch/repositories/Repository.java(2 hunks)server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java(20 hunks)server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java(1 hunks)server/src/main/java/org/opensearch/snapshots/SnapshotShardsService.java(2 hunks)server/src/test/java/org/opensearch/cluster/metadata/KmsCryptoMetadataTests.java(1 hunks)server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java(1 hunks)server/src/test/java/org/opensearch/index/shard/RemoteStoreUploaderServiceTests.java(9 hunks)server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java(2 hunks)server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java(7 hunks)server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryEncryptionTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java(2 hunks)server/src/test/java/org/opensearch/snapshots/mockstore/MockEventuallyConsistentRepository.java(1 hunks)test/framework/src/main/java/org/opensearch/index/shard/IndexShardTestCase.java(1 hunks)test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java(1 hunks)test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java
- test/framework/src/main/java/org/opensearch/index/shard/IndexShardTestCase.java
- server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java
- test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java
- server/src/test/java/org/opensearch/cluster/metadata/KmsCryptoMetadataTests.java
- server/src/test/java/org/opensearch/index/shard/RemoteStoreUploaderServiceTests.java
- server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java
- server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java
- server/src/main/java/org/opensearch/repositories/Repository.java
- plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerEncryptionTests.java
- server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java
- server/src/internalClusterTest/java/org/opensearch/snapshots/CloneSnapshotIT.java
- plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerMockClientTests.java
- server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (2)
server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java (1)
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
EncryptionContextUtils(25-235)
server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java (2)
server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java (1)
ExperimentalApi(30-185)server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java (1)
ExperimentalApi(23-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: gradle-check
- GitHub Check: Analyze (java)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: detect-breaking-change
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: Mend Security Check
🔇 Additional comments (55)
server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java (1)
93-107: LGTM! The factory method is well-structured.The
fromIndexSettingsmethod correctly extracts crypto metadata from index settings and delegates toKmsCryptoMetadatafor AWS KMS provider types. The null-check forkeyProviderNameensures graceful handling when crypto is not configured.plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobContainer.java (1)
140-150: Placeholder implementation for future Azure crypto support.The method currently delegates to
writeBlobwithout usingmetadataorcryptoMetadata. This establishes the API surface for future encryption support in Azure repositories while maintaining backward compatibility.server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java (1)
150-166: LGTM! Parameter propagation is correct.The test correctly extends the
snapshotShardsignature to includeIndexMetadataand properly forwards it to the superclass, maintaining existing test assertions.server/src/main/java/org/opensearch/index/shard/RemoteStoreUploaderService.java (1)
49-95: LGTM! CryptoMetadata properly propagated through upload path.The method signature extension and parameter propagation to
remoteDirectory.copyFromcorrectly integrates crypto metadata into the remote segment upload flow.server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java (1)
178-211: LGTM! Well-designed API extension.The new
writeBlobWithMetadataoverload cleanly extends the API to support optional index-level encryption metadata while maintaining backward compatibility through default delegation. The@ExperimentalApiannotation appropriately marks this as a new feature.server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java (1)
209-220: LGTM! Appropriate delegation for file system storage.The comment correctly notes that
FsBlobContainerdoesn't handle metadata or encryption at the blob level. Delegating towriteBlobis appropriate since file system encryption would be handled at the OS/filesystem layer.server/src/test/java/org/opensearch/snapshots/mockstore/MockEventuallyConsistentRepository.java (1)
417-427: LGTM! Test mock correctly updated.The mock implementation appropriately delegates to
writeBlobto match the new API surface while maintaining existing test behavior.server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java (1)
139-150: Based on my investigation, I've encountered limitations in accessing the OpenSearch repository directly and retrieving the complete RemoteDirectory constructor signature through web search. However, I can provide analysis based on the context available:Verification Analysis:
The code snippet clearly shows two distinct
blobStoreinstances being passed:
- Line 140:
blobStoreRepository.blobStore(isServerSideEncryptionEnabled)— returns a blobStore configured with SSE awareness- Line 148:
blobStoreRepository.blobStore()— returns a base blobStore without SSE parameterThe intentional difference is evident from the method signatures themselves. The first instance is parameterized with the SSE flag, while the second is not. This suggests they serve different purposes within the RemoteDirectory implementation.
However, without direct access to the RemoteDirectory constructor definition, I cannot definitively verify:
- What each parameter is specifically used for
- Whether both parameters are actually utilized
- If there's any redundancy or if this is an intentional dual-purpose design
What needs clarification:
The RemoteDirectory constructor signature (parameter names and their usage) should be examined to confirm eachblobStoreinstance's distinct role.server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java (1)
123-134: LGTM!The
mergeCryptoMetadatahelper correctly delegates to the centralized utility class, maintaining separation of concerns and enabling reuse of the merging logic.server/src/main/java/org/opensearch/indices/replication/checkpoint/RemoteStorePublishMergedSegmentAction.java (1)
166-202: LGTM!The crypto metadata extraction logic correctly handles null safety at each level (indexSettings → indexMetadata → CryptoMetadata) and properly passes the extracted metadata to the
uploadSegmentscall. This aligns well with the index-level encryption pattern across the PR.server/src/main/java/org/opensearch/snapshots/SnapshotShardsService.java (1)
473-488: LGTM!The
IndexMetadatais correctly retrieved from the cluster state and passed torepository.snapshotShard(), enabling index-level crypto metadata to be available during snapshot operations. This integrates well with the broader encryption context propagation.server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java (3)
228-232: LGTM!The crypto metadata is extracted once at the start of
syncSegments()and correctly reused throughout the upload flow, avoiding redundant resolutions.
333-346: LGTM!The
resolveCryptoMetadatahelper is clean with proper null-safety. The Javadoc clearly documents the 2-layer priority and fallback behavior.
354-374: LGTM!The
uploadNewSegmentsmethod correctly accepts and propagatesCryptoMetadatato the underlying uploader service, completing the crypto metadata propagation chain.server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java (2)
68-68: LGTM!The
isNull()import is correctly added to support Mockito verifications with the new nullable parameter.
462-462: LGTM!All test call sites are correctly updated to pass
nullfor the newtrailingContext/CryptoMetadataparameter, maintaining backward compatibility in tests while accommodating the expanded API signatures. TheisNull()matcher is appropriately used in verification.Also applies to: 483-483, 500-500, 545-545, 591-596, 708-709
server/src/main/java/org/opensearch/repositories/FilterRepository.java (1)
210-236: LGTM: Clean parameter addition for crypto metadata propagation.The addition of
IndexMetadataparameter tosnapshotShardenables index-level crypto context to flow through the repository layer. The implementation correctly forwards all parameters to the underlying repository.server/src/main/java/org/opensearch/common/blobstore/stream/write/WriteContext.java (4)
11-11: LGTM: Proper import addition.
35-86: LGTM: Well-structured CryptoMetadata integration.The
CryptoMetadatafield is properly encapsulated with:
- Private final field for immutability
@Nullableannotation indicating optional nature- Proper initialization in both constructors
- Copy constructor correctly handles the new field
152-157: LGTM: Clean getter implementation.The getter method provides appropriate access to the crypto metadata with clear javadoc.
174-239: LGTM: Builder pattern properly extended.The Builder class correctly:
- Adds the
cryptoMetadatafield- Provides a fluent setter method
- Passes the field to the
WriteContextconstructor inbuild()server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobContainer.java (2)
125-144: LGTM: Clean implementation of metadata-aware write.This overload properly encrypts the input stream via
executeWriteand forwards metadata to the underlying blob container.
146-170: LGTM: Excellent clarification of dual encryption support.The comment on lines 155-157 clearly distinguishes between:
- Client-side encryption (handled by
this.cryptoHandler)- SSE-KMS settings (passed via
cryptoMetadataparameter)This separation of concerns is well-implemented, allowing both encryption layers to coexist.
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java (3)
130-140: LGTM: Compilation error fixed.The call to
executeSingleUploadnow correctly passes 6 arguments, resolving the compilation error from the previous review.
611-698: LGTM: Test properly exercises metadata parameter.The test correctly passes metadata Map and null for cryptoMetadata, verifying the metadata is properly forwarded to the S3 request.
700-857: LGTM: All multipart upload test calls corrected.All
executeMultipartUploadcalls now pass exactly 6 arguments, fixing the compilation errors from previous reviews. The tests properly exercise the extended method signatures.server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryEncryptionTests.java (3)
23-52: LGTM: Comprehensive test of KMS metadata extraction.The test properly verifies:
CryptoMetadata.fromIndexSettingsreturns aKmsCryptoMetadatainstance- Provider name and type are correct
isIndexLevel()returns true- Optional KMS-specific fields (keyArn, encryptionContext) are present and correct
54-75: LGTM: Good edge case coverage.These tests verify that
CryptoMetadata.fromIndexSettingsreturns null when:
- No encryption configuration is present
- Key provider is missing despite other crypto fields
77-136: LGTM: Excellent multi-index scenario testing.The tests verify:
- Extraction from complete
IndexMetadataobjects- Different indices can have different KMS keys and encryption contexts
- All fields are properly extracted and distinct
server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java (4)
20-89: LGTM: Comprehensive format conversion and merging tests.The tests thoroughly cover:
- Cryptofs ↔ JSON conversions with special characters and edge cases
- JSON merging with proper conflict resolution (index overrides repo)
- Null and empty input handling
91-124: LGTM: Base64 encoding tests are thorough.The test verifies:
- Merging cryptofs and base64-encoded JSON contexts
- Proper base64 encoding of results
- Correct precedence (index context keys present in output)
- Null handling for both partial and complete absence of inputs
126-171: LGTM: CryptoMetadata merging logic well-tested.The test validates:
- Key provider precedence (index wins)
- Settings merging (both index and repo settings preserved)
- Encryption context merging with proper precedence
- All null input combinations
173-233: LGTM: Edge cases and complex scenarios covered.These tests verify:
- JSON parsing with whitespace and special characters
- Round-trip conversion integrity
- Complex multi-key merges with conflict resolution
server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java (4)
24-48: LGTM: Proper field encapsulation and serialization.The
isIndexLevelfield is correctly:
- Declared as private final
- Initialized in constructors
- Serialized/deserialized via StreamInput/StreamOutput
- Exposed via public getter
50-56: LGTM: Appropriate use of Optional for nullable KMS fields.The getters correctly return
Optional<String>for KMS-specific settings that may not be present.
68-83: LGTM: equals and hashCode properly overridden.The previous review concern about missing
equals()andhashCode()overrides has been addressed. Both methods correctly include theisIndexLevelfield in equality comparison and hash computation.
85-95: LGTM: Informative toString implementation.The
toString()method provides useful debugging information including the key provider name, KMS key ARN, and index-level flag.server/src/main/java/org/opensearch/index/shard/RemoteStoreUploader.java (1)
11-31: Manual verification required — unable to access repository to audit all implementations.Unfortunately, I'm unable to clone the OpenSearch repository to search for all implementations of the
RemoteStoreUploaderinterface. The review comment's concern is valid: the addition of two new parameters (boolean isLowPriorityUploadandCryptoMetadata cryptoMetadata) to theuploadSegmentsmethod is a breaking interface change that requires all implementing classes to be updated.To complete this verification, you will need to manually:
- Search for all classes that implement
RemoteStoreUploader- Verify each implementation's
uploadSegmentsmethod includes the two new parameters in the correct positions- Confirm any call sites pass these parameters correctly
This is critical to prevent runtime errors or compilation failures.
server/src/main/java/org/opensearch/common/blobstore/transfer/RemoteTransferContainer.java (4)
77-99: LGTM - Constructor delegation with backward compatibility.The constructor properly delegates to the new overload with
nullvalues for bothmetadataandcryptoMetadata, maintaining backward compatibility for existing callers.
114-137: LGTM - Proper constructor overload for metadata.The constructor correctly delegates to the primary constructor with
nullforcryptoMetadata.
153-175: LGTM - Primary constructor with CryptoMetadata support.The primary constructor properly handles all parameters including the new
cryptoMetadatafield.
180-192: LGTM - WriteContext properly includes CryptoMetadata.The
createWriteContext()method correctly chains thecryptoMetadatainto theWriteContext.Builder.server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
81-125: LGTM - CryptoMetadata merge logic is correct.The merge logic properly prioritizes index-level settings over repository-level settings while preserving backward compatibility when only one source is available.
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java (3)
602-647: LGTM - Crypto-aware copyFrom with proper delegation.The existing
copyFrommethod correctly delegates to the new crypto-aware overload withnullfor backward compatibility. The new overload properly propagatescryptoMetadatathrough both the multi-stream upload path and the fallback path.
739-749: LGTM - Crypto-aware copyFrom overload with proper delegation.The new overload correctly propagates
cryptoMetadatato the underlyingremoteDataDirectory, and the@Overridemethod properly delegates withnull.
823-823: Verify: Metadata files intentionally use repository-level encryption only.The explicit
nullforcryptoMetadatameans metadata files won't use index-level encryption. This appears intentional (metadata should be readable regardless of index-level key), but please confirm this design decision.plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java (6)
202-218: LGTM - Crypto-aware writeBlobWithMetadata implementation.The method correctly propagates
cryptoMetadatato both single upload and multipart upload paths based on blob size threshold.
220-234: LGTM - Backward-compatible overload.The overload correctly delegates to the crypto-aware version with
nullforCryptoMetadata.
237-268: LGTM - Crypto metadata extraction and context merging.The implementation correctly extracts KMS-specific settings using
instanceof KmsCryptoMetadata, merges encryption contexts, and prioritizes index-level key over repository-level key. This addresses the past review feedback about usingKmsCryptoMetadatato avoid generic checks.
283-304: LGTM - Fallback path correctly propagates crypto metadata.When falling back to synchronous multipart upload for large files, the
cryptoMetadatais correctly passed through.
570-616: LGTM - Single upload with crypto metadata support.The method correctly passes
cryptoMetadatatoconfigureEncryptionSettingsfor proper encryption configuration.
621-659: LGTM - Multipart upload with crypto metadata support.The method correctly configures encryption settings on the
CreateMultipartUploadRequest. Note that individualUploadPartRequestcalls don't need encryption configuration as AWS S3 handles this automatically for parts of an encrypted multipart upload.plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java (2)
22-37: LGTM - Delegation to centralized utility.The method correctly delegates to
EncryptionContextUtils.mergeAndEncodeEncryptionContexts, ensuring consistent behavior across the codebase.
39-64: LGTM - Crypto-aware encryption settings for multipart upload.The method correctly extracts KMS settings from
KmsCryptoMetadatawhen available and falls back to repository-level settings. The encryption context merging ensures both index and repository contexts are included.server/src/main/java/org/opensearch/index/store/RemoteDirectory.java (1)
435-455: Given that all verification attempts have been exhausted—the repository clone failed and web searches cannot locate the specific implementation—I must conclude that the review comment cannot be verified with the available capabilities.The review comment raises substantive architectural concerns about:
- CryptoMetadata propagation in sync vs. async code paths
- SSE/KMS context handling in repository-level vs. index-level encryption
- Unused context parameter in the sync copyFrom overload
These are legitimate design-level questions that require inspection of the actual implementation to verify whether:
- The concerns accurately reflect the code behavior
- The issues have already been addressed
- The suggested fixes are necessary and appropriate
Verify CryptoMetadata handling differences between sync and async
copyFrompaths.The review raises valid architectural concerns about whether the sync
copyFromoverload properly propagates index-level SSE/KMS context through theRemoteIndexOutputwrite path, distinct from the asyncuploadBlobpath which explicitly plumbscryptoMetadatathroughgetBlobContainer()andRemoteTransferContainer(). Additionally, the unusedIOContext contextparameter warrants clarification in javadoc or implementation. Verification of these concerns requires inspection ofRemoteIndexOutput,getBlobContainer(), and the complete sync write path to confirm design intent and implementation consistency.
| // overloadded method to get blob container with the new crypto metadata | ||
| public BlobContainer blobContainer(BlobPath path, CryptoMetadata cryptoMetadata) { | ||
|
|
||
| // Merge index metadata with repository metadata for context merging | ||
| CryptoMetadata merged = (cryptoMetadata != null) ? mergeCryptoMetadata(cryptoMetadata) : this.repositoryCryptoMetadata; | ||
|
|
||
| CryptoHandlerRegistry cryptoHandlerRegistry = CryptoHandlerRegistry.getInstance(); | ||
| assert cryptoHandlerRegistry != null : "CryptoManagerRegistry is not initialized"; | ||
| CryptoHandler IndexCryptoHandler = cryptoHandlerRegistry.fetchCryptoHandler(merged); | ||
|
|
||
| BlobContainer blobContainer = blobStore.blobContainer(path); | ||
| if (blobContainer instanceof AsyncMultiStreamBlobContainer) { | ||
| return new AsyncMultiStreamEncryptedBlobContainer<>((AsyncMultiStreamBlobContainer) blobContainer, IndexCryptoHandler); | ||
| } | ||
| return new EncryptedBlobContainer<>(blobContainer, IndexCryptoHandler); | ||
| } |
There was a problem hiding this comment.
Fix variable naming and add null check for CryptoHandler.
Several issues in this method:
- Line 72: Typo "overloadded" → "overloaded"
- Line 80:
IndexCryptoHandlerviolates Java camelCase naming convention - Line 79: Assert message says "CryptoManagerRegistry" but the class is
CryptoHandlerRegistry - Missing null check for
IndexCryptoHandler— iffetchCryptoHandlerreturns null, Lines 84/86 will throw NPE
- // overloadded method to get blob container with the new crypto metadata
+ // Overloaded method to get blob container with the new crypto metadata
public BlobContainer blobContainer(BlobPath path, CryptoMetadata cryptoMetadata) {
// Merge index metadata with repository metadata for context merging
CryptoMetadata merged = (cryptoMetadata != null) ? mergeCryptoMetadata(cryptoMetadata) : this.repositoryCryptoMetadata;
CryptoHandlerRegistry cryptoHandlerRegistry = CryptoHandlerRegistry.getInstance();
- assert cryptoHandlerRegistry != null : "CryptoManagerRegistry is not initialized";
- CryptoHandler IndexCryptoHandler = cryptoHandlerRegistry.fetchCryptoHandler(merged);
+ assert cryptoHandlerRegistry != null : "CryptoHandlerRegistry is not initialized";
+ CryptoHandler<?, ?> indexCryptoHandler = cryptoHandlerRegistry.fetchCryptoHandler(merged);
+ if (indexCryptoHandler == null) {
+ throw new CryptoRegistryException(
+ merged.keyProviderName(),
+ merged.keyProviderType(),
+ "Crypto manager not found during encrypted blob container creation."
+ );
+ }
BlobContainer blobContainer = blobStore.blobContainer(path);
if (blobContainer instanceof AsyncMultiStreamBlobContainer) {
- return new AsyncMultiStreamEncryptedBlobContainer<>((AsyncMultiStreamBlobContainer) blobContainer, IndexCryptoHandler);
+ return new AsyncMultiStreamEncryptedBlobContainer<>((AsyncMultiStreamBlobContainer) blobContainer, indexCryptoHandler);
}
- return new EncryptedBlobContainer<>(blobContainer, IndexCryptoHandler);
+ return new EncryptedBlobContainer<>(blobContainer, indexCryptoHandler);
}🤖 Prompt for AI Agents
In server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java
around lines 72 to 87, fix the comment typo and variable naming and add a null
check: correct the comment "overloadded" → "overloaded"; rename the local
variable IndexCryptoHandler to indexCryptoHandler to follow Java camelCase;
correct the assert message to reference CryptoHandlerRegistry (not
CryptoManagerRegistry); after fetching the handler check if indexCryptoHandler
is null and if so throw an IllegalStateException (or handle appropriately) with
a clear message including the merged CryptoMetadata so callers don’t hit an NPE
when creating EncryptedBlobContainer instances.
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java
Show resolved
Hide resolved
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java
Show resolved
Hide resolved
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java
Outdated
Show resolved
Hide resolved
| private static int findClosingQuote(String s, int start) { | ||
| for (int i = start; i < s.length(); i++) { | ||
| if (s.charAt(i) == '"' && (i == 0 || s.charAt(i - 1) != '\\')) { | ||
| return i; | ||
| } | ||
| } | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Escaped backslash handling is incorrect in findClosingQuote.
The logic s.charAt(i - 1) != '\\' fails when the backslash itself is escaped. For input "test\\", the quote at the end is NOT escaped (the backslash is), but this code would incorrectly skip it.
Example: {"key":"value\\"} - the closing quote should be found, but the logic sees \\ before " and skips it.
For a robust fix, count consecutive backslashes:
private static int findClosingQuote(String s, int start) {
for (int i = start; i < s.length(); i++) {
- if (s.charAt(i) == '"' && (i == 0 || s.charAt(i - 1) != '\\')) {
+ if (s.charAt(i) == '"') {
+ // Count preceding backslashes
+ int backslashes = 0;
+ for (int j = i - 1; j >= start && s.charAt(j) == '\\'; j--) {
+ backslashes++;
+ }
+ // Quote is escaped only if preceded by odd number of backslashes
+ if (backslashes % 2 == 0) {
+ return i;
+ }
+ }
- return i;
- }
}
return -1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static int findClosingQuote(String s, int start) { | |
| for (int i = start; i < s.length(); i++) { | |
| if (s.charAt(i) == '"' && (i == 0 || s.charAt(i - 1) != '\\')) { | |
| return i; | |
| } | |
| } | |
| return -1; | |
| } | |
| private static int findClosingQuote(String s, int start) { | |
| for (int i = start; i < s.length(); i++) { | |
| if (s.charAt(i) == '"') { | |
| // Count preceding backslashes | |
| int backslashes = 0; | |
| for (int j = i - 1; j >= start && s.charAt(j) == '\\'; j--) { | |
| backslashes++; | |
| } | |
| // Quote is escaped only if preceded by odd number of backslashes | |
| if (backslashes % 2 == 0) { | |
| return i; | |
| } | |
| } | |
| } | |
| return -1; | |
| } |
🤖 Prompt for AI Agents
In
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java
around lines 227 to 234, the current check (s.charAt(i - 1) != '\\') incorrectly
treats quotes preceded by an escaped backslash as escaped; instead, when a quote
is found, walk backward counting consecutive '\\' characters immediately before
the quote and consider the quote escaped only if that count is odd; if the count
is even (including zero) return the quote index. Implement this by, on
encountering '"', iterating j = i - 1 downwards while s.charAt(j) == '\\' to
count backslashes, then if (count % 2 == 0) return i; otherwise continue the
loop.
|
❌ Gradle check result for 41de135: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java (1)
130-147:snapshotShardcall sites correctly updated for new parameterThe added trailing
nullargument matches the extendedsnapshotShard(...)signature and keeps this test focused on Fs repository behavior. If you later want to exercise index-level metadata here, you could pass a realIndexMetadatainstead ofnull, but it’s not required for this test’s purpose.Also applies to: 171-190
server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java (1)
93-107: Index-settings factory for CryptoMetadata matches intended behavior
fromIndexSettingscorrectly:
- Treats missing
index.store.crypto.key_provideras “no index-level crypto” (returnsnull).- Derives
key_provider_typewith a sensible default of"aws-kms".- Delegates to
KmsCryptoMetadatafor the aws-kms case and falls back to genericCryptoMetadataotherwise.If you expect
key_provider_typeto be configured with mixed case, consider normalizing (e.g.,toLowerCase(Locale.ROOT)) before comparing to"aws-kms"to make this more forgiving, but it’s not strictly required if configs are standardized.server/src/main/java/org/opensearch/indices/replication/checkpoint/RemoteStorePublishMergedSegmentAction.java (1)
166-174: Consider simplifying the null-check chain.The
indexShard.indexSettings()should never be null for an active shard with a validIndexMetadata. However, the defensive null checks are acceptable for safety.A minor simplification could be applied:
- // Extract crypto metadata for merged segment upload - CryptoMetadata cryptoMetadata = null; - if (indexShard.indexSettings() != null) { - IndexMetadata indexMetadata = indexShard.indexSettings().getIndexMetadata(); - if (indexMetadata != null) { - cryptoMetadata = CryptoMetadata.fromIndexSettings(indexMetadata.getSettings()); - } - } + // Extract crypto metadata for merged segment upload + CryptoMetadata cryptoMetadata = indexShard.indexSettings() != null + && indexShard.indexSettings().getIndexMetadata() != null + ? CryptoMetadata.fromIndexSettings(indexShard.indexSettings().getIndexMetadata().getSettings()) + : null;server/src/main/java/org/opensearch/index/shard/RemoteStoreUploader.java (1)
24-31: LGTM!The interface method signature is cleanly extended with the
CryptoMetadataparameter, enabling implementations to support index-level encryption during segment uploads.Consider adding a Javadoc comment for the
cryptoMetadataparameter to document its purpose (e.g., "the encryption metadata for the index, or null if no encryption is required").server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java (1)
229-232: ClarifyresolveCryptoMetadataJavadoc vs actual behaviorThe new wiring (compute
CryptoMetadataonce insyncSegmentsand pass it intouploadNewSegments/remoteStoreUploader.uploadSegments) looks correct and keeps the upload path crypto-aware.However, the Javadoc on
resolveCryptoMetadataclaims a two-layer priority including repository-level fallback, while the implementation only reads index settings viaCryptoMetadata.fromIndexSettings(indexMetadata.getSettings()). Repository-level fallback/merging appears to be handled elsewhere (e.g., inCryptoMetadata.fromIndexSettingsand downstream helpers), not in this method.Consider updating the comment to describe only the index-level resolution done here, to avoid implying repository logic lives in this class.
- /** - * Resolves CryptoMetadata with 2-layer priority: - * 1. Index-level encryption from index settings (if configured) - * 2. Repository-level encryption (fallback to null) - * - * @param indexMetadata Index metadata containing crypto settings - * @return CryptoMetadata if encryption is configured, null otherwise - */ + /** + * Resolves index-level {@link CryptoMetadata} from {@link IndexMetadata} settings. + * <p> + * Repository-level crypto metadata (if any) is expected to be merged further + * down the stack (e.g. in repository/blob store layers). + * + * @param indexMetadata Index metadata containing crypto settings + * @return CryptoMetadata if index-level encryption is configured, null otherwise + */Also applies to: 333-347, 354-374
server/src/main/java/org/opensearch/common/blobstore/stream/write/WriteContext.java (1)
35-36: PlumbingCryptoMetadatathroughWriteContextlooks good; consider marking getter as nullableThreading
CryptoMetadatavia the constructor, copy constructor, andBuilderis consistent and keeps the type immutable.Since the constructor and builder explicitly allow
nullforcryptoMetadata, consider annotating the getter with@Nullableas well to make this contract clear to callers and static analysis:- public CryptoMetadata getCryptoMetadata() { + @Nullable + public CryptoMetadata getCryptoMetadata() { return cryptoMetadata; }Also applies to: 48-59, 69-70, 152-157, 174-175, 221-224, 226-238
server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java (1)
20-41: Avoid over‑reliance on exact JSON string formatting in testsThese tests exercise important behavior, but assertions like:
assertEquals("{\"key1\":\"value1\",\"key2\":\"value2\"}", json);and the exact JSON expected when only index context is present make the tests sensitive to key ordering or minor formatting changes in
EncryptionContextUtils, even if the semantic content is unchanged.To make the tests more robust, consider parsing the JSON into a
Map<String,String>(or using a minimal JSON helper) and asserting on the map contents, or usingcontains-style checks as you already do in several places.Also applies to: 91-124, 201-233
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (2)
253-281: Add validation for nested JSON structures.The
parseJsonToMapmethod usesparser.text()on line 274 without checking the token type. If the input JSON contains nested objects or arrays (e.g.,{"key": {"nested": "value"}}), this will throw an unexpected exception rather than a clear error message.Consider adding token type validation:
if (token == XContentParser.Token.FIELD_NAME) { String key = parser.currentName(); - parser.nextToken(); // Move to value - String value = parser.text(); + XContentParser.Token valueToken = parser.nextToken(); // Move to value + if (valueToken != XContentParser.Token.VALUE_STRING + && valueToken != XContentParser.Token.VALUE_NUMBER + && valueToken != XContentParser.Token.VALUE_BOOLEAN) { + throw new IllegalArgumentException( + "Encryption context only supports simple string values, got: " + valueToken + " for key: " + key); + } + String value = parser.text(); map.put(key, value); }
66-72: Silent fallback on Base64 decode failure may mask configuration errors.When Base64 decoding fails (line 69-71), the code silently treats the input as plain text. This could mask misconfiguration where the repository context is expected to be Base64-encoded but isn't, leading to unexpected behavior.
Consider logging a warning when the fallback occurs:
try { byte[] decoded = Base64.getDecoder().decode(repoEncContext); repoJson = new String(decoded, StandardCharsets.UTF_8); } catch (IllegalArgumentException e) { + // Log at debug level - could indicate misconfiguration or plain text input repoJson = repoEncContext; }server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (2)
783-783: Crypto-awareshardContaineroverloads and resolution look sound; consider a helper to avoid explicit null castsThe new
shardContainer(IndexId, int, CryptoMetadata)plusresolveCryptoMetadata(IndexMetadata)correctly:
- Use
EncryptedBlobStoreonly when both a crypto-capable store and non-nullCryptoMetadataare present.- Fall back to the plain
blobStore().blobContainer(shardPath(...))in all other cases (SSE/no-crypto), so behavior for non-encrypted repos is preserved.Call sites passing
(CryptoMetadata) null(e.g., clone, delete, remote-store shallow copy) clearly opt out of index-level overrides and continue to use repository-level behavior.For readability and to avoid the repeated
(CryptoMetadata) nullcasts, consider adding a small convenience overload:+ public BlobContainer shardContainer(IndexId indexId, int shardId) { + return shardContainer(indexId, shardId, (CryptoMetadata) null); + }and then using it at those call sites.
Also applies to: 880-880, 1857-1857, 2508-2508, 2559-2559, 2974-2993, 4987-4993
3963-3986: End-to-end snapshot path now correctly threadsCryptoMetadata; one small duplication you might trimThe changes to
snapshotShard,executeOneFileSnapshot, andsnapshotFilecorrectly:
- Resolve
CryptoMetadataonce fromIndexMetadataand pass it down to each file upload.- Use a crypto-aware shard container only when appropriate (
EncryptedBlobStore), while keeping the non-encrypted/SSE flows unchanged.- Switch to
writeBlobWithMetadata(..., null, cryptoMetadata), which enables downstream S3/SSE-KMS or client-side crypto to see the index-level metadata.Minor polish you could consider:
snapshotShardcurrently resolvesCryptoMetadatatwice (once indirectly when buildingshardContainer(indexId, shardId, indexMetadata), and once explicitly for file uploads). If you care to micro-simplify, you could resolve once and reuse the sameCryptoMetadataboth for the shard container andsnapshotFileinvocations.Functionally this looks correct and backwards compatible.
Also applies to: 4160-4178, 4185-4215, 4843-4891
server/src/main/java/org/opensearch/index/store/RemoteDirectory.java (2)
456-485: SynccopyFromcorrectly honorsCryptoMetadata; consider deleting via the same target containerThe new synchronous
copyFrom(Directory, String, String, IOContext, CryptoMetadata):
- Opens a crypto-aware
BlobContainerviagetBlobContainer(cryptoMetadata).- Uses
RemoteIndexOutputover that container, so index-level overrides apply where supported.- Attempts cleanup on failure by calling
deleteFile(dest).If in the future a crypto-aware container ever routes to a different physical path than the base
blobContainer, the cleanup viadeleteFile(dest)(which always uses the base container) might leave a stray blob behind. To be maximally robust you could instead delete using the same container you wrote to, e.g.:- if (!success) { - try { - deleteFile(dest); - } catch (Exception e) { - // Ignore deletion errors in cleanup - } - } + if (!success) { + try { + targetContainer.deleteBlobsIgnoringIfNotExists(Collections.singletonList(dest)); + } catch (Exception e) { + // Ignore deletion errors in cleanup + } + }Not a blocker, but it would make the behavior independent of how crypto-aware containers are implemented.
435-454: Async upload path propagatesCryptoMetadatacorrectly; small guard could future-proof the type castThe async
copyFromanduploadBlobchanges correctly:
- Obtain
BlobContainer targetContainer = getBlobContainer(cryptoMetadata)so index-level overrides only affect client-side encrypted repos.- Pass
cryptoMetadatadown intoRemoteTransferContainer(and thusWriteContext), enabling remote-store implementations to use it (e.g., S3 SSE-KMS encryption contexts).- Preserve the existing
lowPriorityUploadlogic andremoteIntegrityEnabledflag.One robustness improvement you might make:
copyFromcurrently gates onblobContainer instanceof AsyncMultiStreamBlobContainer, butuploadBloblater caststargetContainertoAsyncMultiStreamBlobContainer:if (blobContainer instanceof AsyncMultiStreamBlobContainer) { uploadBlob(..., cryptoMetadata); ... } // ... ((AsyncMultiStreamBlobContainer) targetContainer).asyncBlobUpload(writeContext, completionListener);If a future crypto-aware container differed in type from the base
blobContainer, this could become aClassCastException. To be safer, consider basing both the guard and cast ontargetContainerinstead ofblobContainer, e.g.:BlobContainer targetContainer = getBlobContainer(cryptoMetadata); if (targetContainer instanceof AsyncMultiStreamBlobContainer) { // use targetContainer throughout, including the cast } else { return false; }Today this likely behaves correctly, but this small change would make the code more future-proof.
Also applies to: 487-531, 573-574
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
CHANGELOG.md(1 hunks)plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobContainer.java(2 hunks)plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java(10 hunks)plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerEncryptionTests.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerMockClientTests.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java(7 hunks)server/src/internalClusterTest/java/org/opensearch/snapshots/CloneSnapshotIT.java(3 hunks)server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java(3 hunks)server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java(1 hunks)server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java(1 hunks)server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobContainer.java(1 hunks)server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java(5 hunks)server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/stream/write/WriteContext.java(9 hunks)server/src/main/java/org/opensearch/common/blobstore/support/FilterBlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/transfer/RemoteTransferContainer.java(6 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java(6 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreUploader.java(2 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreUploaderService.java(3 hunks)server/src/main/java/org/opensearch/index/store/RemoteDirectory.java(8 hunks)server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java(6 hunks)server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java(1 hunks)server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java(2 hunks)server/src/main/java/org/opensearch/indices/replication/checkpoint/RemoteStorePublishMergedSegmentAction.java(3 hunks)server/src/main/java/org/opensearch/repositories/FilterRepository.java(2 hunks)server/src/main/java/org/opensearch/repositories/Repository.java(2 hunks)server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java(20 hunks)server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java(1 hunks)server/src/main/java/org/opensearch/snapshots/SnapshotShardsService.java(2 hunks)server/src/test/java/org/opensearch/cluster/metadata/KmsCryptoMetadataTests.java(1 hunks)server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java(1 hunks)server/src/test/java/org/opensearch/index/shard/RemoteStoreUploaderServiceTests.java(9 hunks)server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java(2 hunks)server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java(7 hunks)server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryEncryptionTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java(2 hunks)server/src/test/java/org/opensearch/snapshots/mockstore/MockEventuallyConsistentRepository.java(1 hunks)test/framework/src/main/java/org/opensearch/index/shard/IndexShardTestCase.java(1 hunks)test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java(1 hunks)test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java(2 hunks)test/framework/src/main/java/org/opensearch/snapshots/mockstore/MockRepository.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java
- server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java
- server/src/main/java/org/opensearch/snapshots/SnapshotShardsService.java
- test/framework/src/main/java/org/opensearch/index/shard/IndexShardTestCase.java
- server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java
- test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java
- server/src/test/java/org/opensearch/cluster/metadata/KmsCryptoMetadataTests.java
- server/src/test/java/org/opensearch/snapshots/mockstore/MockEventuallyConsistentRepository.java
- CHANGELOG.md
- server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java
- server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryEncryptionTests.java
- plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java
- server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java
🧰 Additional context used
🧬 Code graph analysis (3)
server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java (1)
server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java (1)
ExperimentalApi(30-185)
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerEncryptionTests.java (1)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java (1)
SseKmsUtil(22-116)
test/framework/src/main/java/org/opensearch/snapshots/mockstore/MockRepository.java (2)
server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (1)
BlobStoreRepository(217-4994)server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java (1)
FsBlobContainer(81-283)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: gradle-check
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: Analyze (java)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: Mend Security Check
🔇 Additional comments (43)
test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java (1)
42-42: Adaptation to CryptoMetadata-aware shardContainer looks correctImporting
CryptoMetadataand explicitly passing(CryptoMetadata) nulltorepository.shardContainercleanly targets the new overload while preserving existing non-encrypted behavior in this test utility.Also applies to: 192-204
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java (5)
130-161: Single-upload tests correctly adapted to CryptoMetadata-aware signatureBoth error-path tests now call
executeSingleUploadwith the new(metadata, cryptoMetadata)tail parameters as(null, null), which is sufficient for exercising size validation without touching crypto behavior.
611-698: Happy-path single-upload test aligned with new API
testExecuteSingleUploadnow passesmetadataplus a trailingnullCryptoMetadata, keeping existing assertions onPutObjectRequestintact while matching the extended method signature.
700-722: Multipart size-boundary tests updated consistentlyThe “too large” and “too small” multipart tests now invoke
executeMultipartUploadwith(null, null)for metadata andCryptoMetadata, matching the new signature and still isolating size validation only.
724-857: Multipart happy-path test correctly passesmetadataand null CryptoMetadata
testExecuteMultipartUploadnow providesmetadataand anullCryptoMetadataargument, which keeps the existing assertions (headers, ACL, encryption flags, etags) valid while matching the updated multipart upload signature.
859-943: Multipart abort-path test matches extended signature
testExecuteMultipartUploadAbortedpassesnullmetadata andnullCryptoMetadataintoexecuteMultipartUpload, which is appropriate for this failure/abort scenario and aligns with the new method arity.plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerMockClientTests.java (1)
755-762: Mockito verification updated to match new multipart signatureThe
verify(...).executeMultipartUpload(..., anyMap(), null)expectation now correctly reflects the additionalCryptoMetadataparameter and the fact that this test uses aWriteContextwithout crypto metadata (hencenullhere).server/src/main/java/org/opensearch/indices/replication/checkpoint/RemoteStorePublishMergedSegmentAction.java (1)
176-202: LGTM!The
cryptoMetadatais correctly extracted from the index settings and passed to the upload flow, enabling index-level encryption for merged segment uploads while maintaining backward compatibility whencryptoMetadatais null.server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java (1)
839-842: LGTM!The mock stubbing correctly accommodates the expanded
copyFromsignature with the additionalCryptoMetadataparameter. Usingany()matchers is appropriate here since these tests focus on upload timeout behavior rather than encryption specifics.server/src/main/java/org/opensearch/index/shard/RemoteStoreUploaderService.java (2)
48-56: LGTM!The
CryptoMetadataparameter is cleanly integrated into the method signature, enabling crypto-aware uploads while maintaining the existing upload flow logic.
87-94: LGTM!The
cryptoMetadatais correctly passed through toremoteDirectory.copyFrom(), completing the encryption metadata propagation chain from index settings to the remote storage layer.server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java (1)
139-166: LGTM!The
snapshotShardoverride correctly accepts and forwards the newIndexMetadataparameter to the superclass, maintaining compatibility with the updated repository API while preserving the test's metadata filtering assertions.server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java (1)
758-772: LGTM!The
TestRepository.snapshotShardmethod signature correctly updated to include the newIndexMetadataparameter, maintaining compatibility with the updatedRepositoryinterface contract.plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobContainer.java (2)
43-43: LGTM!Import for
CryptoMetadatais correctly added to support the new method signature.
140-150: Based on my investigation, I need to provide you with my findings. Unfortunately, I encountered a limitation accessing the actual OpenSearch repository code directly, but the web search results provide important context about how OpenSearch repository plugins handle metadata and encryption.Key Findings:
Encryption in OpenSearch Repository Plugins: OpenSearch uses two encryption models:
- Server-side encryption: Handled by the storage provider (e.g., S3 SSE-KMS, Azure Key Vault), configured at repository registration time
- Client-side encryption: Via community plugins like the Encrypted Repository Plugin
Azure Repository Design: The
repository-azureplugin does NOT expose encryption configuration like S3 does. Azure Blob Storage handles server-side encryption separately, and client-side encryption would be handled by a separate plugin.BlobContainer Interface: The
writeBlobWithMetadatamethod appears to be part of a standard interface contract wheremetadataandcryptoMetadataparameters are included for extensibility, but individual repository implementations may not use them.The Ignored Parameters: The fact that
metadataandcryptoMetadataare ignored in AzureBlobContainer appears intentional and consistent with how the plugin is designed (relying on Azure's server-side encryption rather than client-side encryption).However, I cannot definitively confirm the S3 implementation pattern without code access. For a complete verification, you would need to:
- Check if S3BlobContainer also ignores these parameters in its
writeBlobWithMetadata- Review any comments/documentation in the AzureBlobContainer explaining this design choice
- Check if there are any open issues or future encryption support TODOs
Verify that ignoring
metadataandcryptoMetadatais intentional and consistent with Azure repository design.The method accepts
metadataandcryptoMetadataparameters but delegates towriteBlobwithout using them. This appears intentional: the repository-azure plugin relies on Azure Blob Storage's server-side encryption (Microsoft-managed or Key Vault customer-managed keys) configured separately from the snapshot operations, not client-side encryption in the plugin code. The parameters are likely part of the BlobContainer interface contract for extensibility or compatibility with future encryption support. To confirm this is the intended design, verify whether the S3 repository implementation similarly ignores these parameters in itswriteBlobWithMetadatamethod.server/src/internalClusterTest/java/org/opensearch/snapshots/CloneSnapshotIT.java (3)
41-41: LGTM!Import correctly added to support the updated
shardContainermethod signature.
811-811: LGTM!The explicit cast
(CryptoMetadata) nullcorrectly adapts to the updatedshardContainerAPI. Passingnullis appropriate for tests not exercising encryption functionality.
827-827: LGTM!Consistent update to the
shardContainercall with the new signature.server/src/test/java/org/opensearch/index/shard/RemoteStoreUploaderServiceTests.java (7)
144-144: LGTM!Test correctly updated to pass
nullfor the newCryptoMetadataparameter, appropriate for tests not exercising encryption.
195-195: LGTM!Mock setup correctly extended to 8 arguments with
any(Boolean.class), any()for the trailing parameters, matching the updatedcopyFromsignature.
204-204: LGTM!Consistent update to
uploadSegmentscall with the additionalCryptoMetadataparameter.
258-267: LGTM!Low priority upload test correctly updated with the extended signatures for both the mock and the method call.
324-333: LGTM!Composite directory test appropriately updated with the new parameter.
393-400: LGTM!Corrupt index exception test correctly updated to exercise the extended API.
460-467: LGTM!Generic exception test properly adapted to the new method signatures.
server/src/main/java/org/opensearch/index/shard/RemoteStoreUploader.java (1)
11-11: LGTM!Import correctly added for the new parameter type.
server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java (2)
108-109: LGTM!Test correctly updated with the extended
copyFromsignature, passingfalsefor low priority upload andnullforCryptoMetadata.
147-148: LGTM!Exception test case consistently updated with the new parameters.
server/src/main/java/org/opensearch/repositories/FilterRepository.java (1)
209-235: ForwardingIndexMetadataintosnapshotShardlooks correctThe updated
snapshotShardsignature cleanly threadsIndexMetadatathrough to the delegate repository without altering behavior, keepingFilterRepositorya transparent wrapper.server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java (1)
68-69: Tests correctly adapted to newcopyFrom/getBlobContainercontext parameterThe updates consistently pass a
nulltrailing context intocopyFromandgetBlobContainer, and the verifications useisNull()on the new argument. This keeps existing behavior while matching the new APIs.Also applies to: 462-463, 483-484, 500-501, 545-546, 592-596, 704-710
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerEncryptionTests.java (1)
1-45: S3 encryption context andCryptoMetadatatests look appropriateThe tests validate both index/repository encryption-context merging and basic
CryptoMetadatasettings access, providing good coverage for the S3 SSE-KMS integration path.server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobContainer.java (1)
125-170:writeBlobWithMetadataoverloads correctly integrate encryption and CryptoMetadataBoth overloads reuse
executeWritefor client-side encryption and cleanly delegate to the underlyingBlobContainer, with the second overload passing through theCryptoMetadatafor SSE-KMS. This keeps the concerns separated and behavior consistent with the existingwriteBlob/writeBlobAtomicmethods.server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java (1)
24-66: I'm unable to access the OpenSearch repository to run verification scripts due to persistent cloning errors. Without access to the codebase, I cannot definitively verify the serialization concerns raised in the review comment.However, based on the code provided in the review comment itself, I can provide a rewritten version that acknowledges the verification limitation:
Verify stream serialization compatibility for
KmsCryptoMetadatawith broader codebase contextThe class overrides
writeTo()to append an extrabooleanfield (isIndexLevel), which changes the wire format compared to the baseCryptoMetadataclass. While the provided constructor fromStreamInputand serialization methods appear correct in isolation, verification requires examining:
- All deserialization sites across the codebase to confirm they instantiate
KmsCryptoMetadataobjects (not genericCryptoMetadata) when the type isaws-kms- Cluster state serialization/deserialization flows to ensure no generic code path treats all
CryptoMetadatainstances uniformly- Any polymorphic registry or factory patterns that might instantiate the wrong subclass
Stream misalignment can occur if older code tries to deserialize a
KmsCryptoMetadatainstance as a baseCryptoMetadata, or if mixed-version nodes interact without proper type routing.
server/src/main/java/org/opensearch/common/blobstore/support/FilterBlobContainer.java (1)
100-137: LGTM - Clean delegation implementation for metadata-aware blob operations.The new methods correctly follow the established delegation pattern in
FilterBlobContainer. All four overloads properly forward to the delegate without modifying the control flow, maintaining consistency with the existing implementation.server/src/main/java/org/opensearch/repositories/Repository.java (1)
378-440: LGTM - Backward-compatible API extension.The design is sound: converting the original method to a default that delegates to the new overload with
nullensures existing implementations continue to work. AddingIndexMetadataas the last parameter follows the convention for optional parameters.test/framework/src/main/java/org/opensearch/snapshots/mockstore/MockRepository.java (1)
593-670: LGTM - Consistent mock implementations for metadata-aware write paths.The new methods correctly replicate the blocking and failure simulation logic from their non-metadata counterparts (
writeBlobandwriteBlobAtomic), ensuring test coverage for metadata-aware write paths behaves identically to the existing paths.server/src/main/java/org/opensearch/common/blobstore/transfer/RemoteTransferContainer.java (1)
77-175: LGTM - Well-structured constructor overloading with CryptoMetadata support.The telescoping constructor pattern properly chains through all overloads, ensuring backward compatibility while adding CryptoMetadata support. The
createWriteContext()method correctly propagates the crypto metadata.server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java (2)
602-647: LGTM - Consistent crypto-aware copyFrom implementation.The delegation pattern maintains backward compatibility while properly threading
CryptoMetadatathrough both upload paths (multi-stream viaremoteDataDirectory.copyFromand fallback viacopyFrom(from, src, src, context, cryptoMetadata)).
739-749: LGTM - Clean overload delegation for destination-copy variant.The existing
copyFrom(Directory, String, String, IOContext)now properly delegates to the crypto-aware variant withnull, maintaining backward compatibility.server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
140-161: Good use of XContentBuilder for proper JSON escaping.Using
XContentBuilder.field()ensures proper escaping of special characters in keys and values, addressing the previous concerns about malformed JSON output.server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (1)
4259-4261: Restoration and status paths handle missingIndexMetadatasafely while enabling index-level cryptoUsing
clusterService.state().metadata().index(indexId.getName())to obtainIndexMetadataand then routing through the crypto-awareshardContaineris a reasonable way to honor index-level crypto at restore and status time.Because
resolveCryptoMetadatais null-safe, passingindexMetadatadirectly (or(CryptoMetadata) nullfor remote-store shallow copies) means:
- If the current cluster state has metadata for that index, you get the correct
CryptoMetadata.- If not (older snapshots or non-crypto indices), you naturally fall back to the repository-level container with no NPE risk.
No changes needed here.
Also applies to: 4535-4535, 4545-4552
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java (1)
15-18: AWS KMS index-level key/context override is wired correctly and preserves existing behaviorThe new
mergeAndEncodeEncryptionContextshelper and the CryptoMetadata-aware overloads ofconfigureEncryptionSettings:
- Keep AES256 behavior identical.
- For
AWS_KMS, preferentially use index-levelKmsCryptoMetadata(key ARN + encryption context) when present, otherwise fall back to the repository’s KMS key and context.- Delegate context merging/encoding to
EncryptionContextUtils, so repository defaults and index overrides are combined consistently.This should be backwards compatible for repos without index-level metadata (the repo context is effectively passed through) while enabling per-index overrides when
KmsCryptoMetadatais provided.Looks good as-is.
Also applies to: 23-35, 37-61, 64-88, 91-93
server/src/main/java/org/opensearch/index/store/RemoteDirectory.java (1)
69-72: Crypto-aware container selection inRemoteDirectoryis reasonable and backwards compatibleStoring
blobStore/blobPathand routinggetBlobContainer(@Nullable CryptoMetadata)as:
- Encrypted path only when
blobStore instanceof EncryptedBlobStoreandcryptoMetadata != null, and- Falling back to the original
blobContainerin all other cases,gives a clean separation between:
- Client-side encryption repos (via
EncryptedBlobStore) that need index-level overrides, and- SSE/no-crypto repos where CryptoMetadata should not change the container type.
Existing constructors delegate to the new one with
blobStore/blobPathasnull, so previous usages continue to behave the same. No changes needed here.Also applies to: 91-108, 110-121, 123-141, 155-168
|
❌ Gradle check result for 6765f53: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
udabhas
force-pushed
the
3.3_ile
branch
2 times, most recently
from
d7c8fb8 to
fcd6e29
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java (1)
72-87: Fix typo, naming, and add null‑check inblobContainer(BlobPath, CryptoMetadata)The new overload still has the issues previously flagged:
- Comment typo: "overloadded" → "Overloaded".
- Assert message refers to
"CryptoManagerRegistry"instead ofCryptoHandlerRegistry.- Local variable
IndexCryptoHandlerviolates camelCase and is raw‑typed.- No null check for the fetched handler; a misconfigured
CryptoHandlerRegistryentry will cause an NPE when constructing the encrypted containers, unlike the constructor which throwsCryptoRegistryException.Consider aligning this overload with the constructor’s behavior and cleaning up naming:
- // overloadded method to get blob container with the new crypto metadata - public BlobContainer blobContainer(BlobPath path, CryptoMetadata cryptoMetadata) { - - // Merge index metadata with repository metadata for context merging - CryptoMetadata merged = (cryptoMetadata != null) ? mergeCryptoMetadata(cryptoMetadata) : this.repositoryCryptoMetadata; - - CryptoHandlerRegistry cryptoHandlerRegistry = CryptoHandlerRegistry.getInstance(); - assert cryptoHandlerRegistry != null : "CryptoManagerRegistry is not initialized"; - CryptoHandler IndexCryptoHandler = cryptoHandlerRegistry.fetchCryptoHandler(merged); - - BlobContainer blobContainer = blobStore.blobContainer(path); - if (blobContainer instanceof AsyncMultiStreamBlobContainer) { - return new AsyncMultiStreamEncryptedBlobContainer<>((AsyncMultiStreamBlobContainer) blobContainer, IndexCryptoHandler); - } - return new EncryptedBlobContainer<>(blobContainer, IndexCryptoHandler); - } + // Overloaded method to get blob container with the new crypto metadata + public BlobContainer blobContainer(BlobPath path, CryptoMetadata cryptoMetadata) { + // Merge index metadata with repository metadata for context merging + final CryptoMetadata merged = cryptoMetadata != null ? mergeCryptoMetadata(cryptoMetadata) : this.repositoryCryptoMetadata; + + final CryptoHandlerRegistry cryptoHandlerRegistry = CryptoHandlerRegistry.getInstance(); + assert cryptoHandlerRegistry != null : "CryptoHandlerRegistry is not initialized"; + + final CryptoHandler<?, ?> indexCryptoHandler = cryptoHandlerRegistry.fetchCryptoHandler(merged); + if (indexCryptoHandler == null) { + throw new CryptoRegistryException( + merged.keyProviderName(), + merged.keyProviderType(), + "Crypto manager not found during encrypted blob container creation." + ); + } + + final BlobContainer delegate = blobStore.blobContainer(path); + if (delegate instanceof AsyncMultiStreamBlobContainer) { + return new AsyncMultiStreamEncryptedBlobContainer<>((AsyncMultiStreamBlobContainer) delegate, indexCryptoHandler); + } + return new EncryptedBlobContainer<>(delegate, indexCryptoHandler); + }This keeps behavior consistent with the constructor and avoids hard‑to‑debug NPEs when the registry is misconfigured.
Also applies to: 123-134
🧹 Nitpick comments (11)
server/src/main/java/org/opensearch/repositories/Repository.java (1)
364-403: Consider clarifying the Javadoc to indicate this is a delegating method.The Javadoc for this default method could explicitly mention that it delegates to the overload with
indexMetadatafor backward compatibility, helping implementers understand the relationship between the two methods.Apply this diff to enhance the Javadoc:
/** * Creates a snapshot of the shard based on the index commit point. + * <p> + * This is a backward-compatible delegating method that calls the overload with {@code indexMetadata} set to {@code null}. + * Implementations should override the overload that accepts {@link IndexMetadata} to support index-level encryption. * <p>test/framework/src/main/java/org/opensearch/index/shard/IndexShardTestCase.java (1)
1513-1536: Consider passing actual IndexMetadata instead ofnullinsnapshotShardhelperThe new
snapshotShardparameter is currently passed asnull, which is fine for non-crypto tests but means this widely used helper won't exercise index-metadata / encryption-aware snapshot paths.If the new parameter is indeed
IndexMetadata, consider wiring the shard's metadata through here:- repository.snapshotShard( + repository.snapshotShard( shard.store(), shard.mapperService(), snapshot.getSnapshotId(), indexId, wrappedIndexCommit.get(), null, snapshotStatus, Version.CURRENT, Collections.emptyMap(), - future, - null + future, + shard.indexSettings().getIndexMetadata() );This keeps tests closer to production behavior and allows future index-level encryption scenarios to be validated via this utility.
server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java (1)
35-37: FS container metadata/crypto no-op is reasonable for nowThe new
writeBlobWithMetadata(..., @Nullable Map<String,String> metadata, @Nullable CryptoMetadata cryptoMetadata)overload delegating towriteBlobkeeps FS behavior unchanged while satisfying the extended BlobContainer API. Given FS repositories aren't expected to honor object metadata or crypto context, this is a pragmatic choice.If we ever introduce FS-based testing for encryption-context propagation, we might want to assert on
metadatahere, but it's not necessary for current usage.Also applies to: 209-220
server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java (1)
20-22: CryptoMetadata propagation through remote segment upload looks correct; adjust helper Javadoc
syncSegments()now resolvesIndexMetadataand derives aCryptoMetadatainstance once per sync, then passes it intouploadNewSegments(...).uploadNewSegments(...)forwards thiscryptoMetadatatoremoteStoreUploader.uploadSegments(...)along with the low‑priority flag, which is the right place to attach index‑level encryption context to the remote upload.resolveCryptoMetadata(IndexMetadata)is null‑safe and delegates toCryptoMetadata.fromIndexSettings(indexMetadata.getSettings()), which is appropriate for deriving index‑level encryption config on each run.One minor nit: the Javadoc on
resolveCryptoMetadatamentions a “2‑layer priority” including repository‑level encryption fallback, but this method currently only inspects index settings. It would be clearer either to drop the repository‑level bullet from this comment or to point to where that merging actually occurs (e.g., in the repository/blobstore layer).Also applies to: 229-232, 333-346, 356-375
test/framework/src/main/java/org/opensearch/snapshots/mockstore/MockRepository.java (1)
41-45: Metadata/crypto-aware write/atomic write correctly mirror existing mock behaviorThe new methods on
MockBlobContainer:
writeBlobWithMetadata(..., Map<String,String> metadata)and theCryptoMetadataoverload both reusemaybeIOExceptionOrBlockand theblockOnWriteShardLevelMetagate, then delegate tosuper.writeBlobWithMetadata(...)and optionally inject a post‑write failure. This keeps shard‑level metadata/snapshot blob writes subject to the same failure and blocking conditions as plain writes.writeBlobAtomicWithMetadata(...)mirrorswriteBlobAtomic(...):
- Respects
failOnIndexLatestforindex.latest.- Applies
blockAndFailOnWriteIndexFile/blockOnWriteIndexFileforindex-Nblobs.- For
FsBlobContainerdelegates, simulates the temp‑file +moveBlobAtomicflow using metadata writes; otherwise falls back tosuper.writeBlobAtomicWithMetadata(...).This preserves the mock’s semantics for all of the new metadata/crypto‑aware APIs, ensuring snapshot tests still see the same injected failures and blocking behavior as before.
You could optionally factor the shared pre/post logic between the two
writeBlobWithMetadataoverloads into a small helper, but that’s not required.Also applies to: 593-670
server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java (1)
93-107: fromIndexSettings logic looks correct; consider reducing AWS‑KMS duplication.The factory correctly:
- Returns
nullif noindex.store.crypto.key_provideris set.- Defaults
key_provider_typeto"aws-kms"and returnsKmsCryptoMetadatain that case.- Falls back to a generic
CryptoMetadataotherwise.To keep things DRY and easier to evolve:
- You could delegate the
"aws-kms"branch toKmsCryptoMetadata.fromIndexSettings(indexSettings)so key names and behavior stay in one place.- If you expect different casings for
key_provider_type, normalizing (e.g.,toLowerCase(Locale.ROOT)) before the equality check would make configuration more forgiving.These are non-blocking API-shape tweaks; current behavior is sound.
server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java (1)
35-36: New CryptoMetadata‑aware writeBlobWithMetadata overload is backward‑compatible; consider parity with atomic writes.The added default method cleanly extends the API:
- Existing
BlobContainerimplementations that only override the 5‑argwriteBlobWithMetadatacontinue to work because the new 6‑arg default delegates to them.- Callers can start passing
CryptoMetadatawithout forcing all implementations to change immediately.Two follow‑ups to consider:
- For feature parity, if any snapshot/remote paths use
writeBlobAtomicWithMetadata, you may eventually want a similar CryptoMetadata‑accepting overload there; otherwise those paths can’t benefit from index‑level encryption overrides.- This introduces a dependency from
common.blobstoretocluster.metadataviaCryptoMetadata; if you care about keeping those layers loosely coupled, it might be worth revisiting whereCryptoMetadatalives longer‑term.Behavior as written is fine and non‑breaking.
Also applies to: 178-211
server/src/main/java/org/opensearch/indices/replication/checkpoint/RemoteStorePublishMergedSegmentAction.java (1)
17-19: Index‑level CryptoMetadata resolution for merged segment uploads looks good; null‑checks could be simplified.Deriving
CryptoMetadatafromIndexMetadata.getSettings()and passing it intouploadSegments(…, true, cryptoMetadata)correctly enables index‑level encryption for merged segment uploads while falling back to repo defaults when no crypto settings are present.Given typical
IndexShardcontracts,indexShard.indexSettings()and itsIndexMetadataare expected to be non‑null in this context, so the nested null checks are likely defensive rather than required. If that holds, you could simplify to something like:CryptoMetadata cryptoMetadata = CryptoMetadata.fromIndexSettings( indexShard.indexSettings().getIndexMetadata().getSettings() );or, if
IndexSettings.getSettings()is equivalent for your use case, directly from there. Optional cleanup only.Also applies to: 167-175, 202-202
server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (2)
3969-4183: Snapshot pipeline CryptoMetadata wiring looks good; clean up unused parameterThe end‑to‑end plumbing for index‑level CryptoMetadata in the snapshot path is coherent:
snapshotShard(...)now:
- Uses
shardContainer(indexId, shardId, indexMetadata)so shard‑level metadata IO goes through anEncryptedBlobStorecontainer that can merge index and repository crypto metadata.- Resolves
CryptoMetadata cryptoMetadata = resolveCryptoMetadata(indexMetadata);once and passes it intoexecuteOneFileSnapshot(...).executeOneFileSnapshot(...)threadscryptoMetadatathrough all recursive calls.snapshotFile(...)now:
- Re‑obtains the shard container via
shardContainer(indexId, shardId, cryptoMetadata).- Uses
writeBlobWithMetadata(partName, inputStream, partBytes, false, null, cryptoMetadata);so underlying blob stores (e.g., S3) can apply index‑level encryption context overrides.This should allow per‑index crypto configuration to override repository‑level defaults while keeping behavior unchanged when no index CryptoMetadata is configured.
One minor nit:
executeOneFileSnapshot(...)now takesIndexMetadata indexMetadatabut never uses it in the body; onlyCryptoMetadata cryptoMetadatais actually needed. To reduce API noise and avoid confusion:
- Either drop the
IndexMetadataparameter fromexecuteOneFileSnapshot(and its call sites), or- Move
resolveCryptoMetadata(indexMetadata)intoexecuteOneFileSnapshotand drop the explicitcryptoMetadataparameter, depending on which layering you prefer.Functionally this is fine as‑is; this is just a maintainability cleanup.
Also applies to: 4848-4891
4243-4277: Verify CryptoMetadata source for restore/status when index metadata changes or is absent
restoreShardand thegetShardSnapshotStatus(...)overloads now derive CryptoMetadata via:IndexMetadata indexMetadata = clusterService.state().metadata().index(indexId.getName()); BlobContainer container = shardContainer(indexId, shardId, indexMetadata);where:
private CryptoMetadata resolveCryptoMetadata(IndexMetadata indexMetadata) { if (indexMetadata == null) { return null; } Settings indexSettings = indexMetadata.getSettings(); return CryptoMetadata.fromIndexSettings(indexSettings); }This means:
- When the index still exists and its encryption‑related settings match those used when the snapshot was taken, you correctly get index‑level CryptoMetadata merged with repository‑level metadata via
EncryptedBlobStore.blobContainer(path, cryptoMetadata).- When the index no longer exists (or has been recreated with different crypto settings),
indexMetadatais null or divergent and you fall back to repository‑level CryptoMetadata (or potentially the wrong index CryptoMetadata).For operations like:
- Restoring a snapshot of an index that has since been deleted or had its encryption settings changed, or
- Querying shard snapshot status for such a snapshot,
this may break decryption of shard‑level metadata or cause it to use an incorrect crypto context, depending on how
CryptoHandlerusesCryptoMetadatafor decryption.Consider:
- For restore and status of completed snapshots, deriving CryptoMetadata from the snapshot’s stored index metadata (
getSnapshotIndexMetaData(repositoryData, snapshotId, indexId)) rather than from the current cluster‑state IndexMetadata, and- Keeping the current cluster‑state‑based resolution for in‑flight snapshots where index metadata is guaranteed to be in sync.
Please verify behavior for these scenarios with an index that has index‑level CryptoMetadata, including the case where the index is deleted between snapshot and restore/status.
Also applies to: 4535-4553, 4987-4993
plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobContainer.java (1)
140-150: AzurewriteBlobWithMetadatacurrently ignores metadata and CryptoMetadataThe new override:
public void writeBlobWithMetadata( String blobName, InputStream inputStream, long blobSize, boolean failIfAlreadyExists, @Nullable Map<String, String> metadata, @Nullable CryptoMetadata cryptoMetadata ) throws IOException { writeBlob(blobName, inputStream, blobSize, failIfAlreadyExists); }keeps Azure repository behavior unchanged, but it silently discards both
metadataandcryptoMetadata.If Azure repositories are not intended to participate in index‑level encryption/context handling yet, this is fine, but it would be good to:
- Add a brief comment/TODO here stating that Azure currently ignores CryptoMetadata, or
- When/if Azure gains crypto support, plumb
metadata/cryptoMetadatathrough to the underlying SDK call so it can participate in the same semantics as S3 and other backends.This is a behavior clarity / future‑proofing suggestion, not a blocker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
CHANGELOG.md(1 hunks)plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobContainer.java(2 hunks)plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java(10 hunks)plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerEncryptionTests.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerMockClientTests.java(2 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java(7 hunks)server/src/internalClusterTest/java/org/opensearch/snapshots/CloneSnapshotIT.java(3 hunks)server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java(3 hunks)server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java(1 hunks)server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java(1 hunks)server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobContainer.java(1 hunks)server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java(5 hunks)server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/stream/write/WriteContext.java(9 hunks)server/src/main/java/org/opensearch/common/blobstore/support/FilterBlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/transfer/RemoteTransferContainer.java(6 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java(6 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreUploader.java(2 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreUploaderService.java(3 hunks)server/src/main/java/org/opensearch/index/store/RemoteDirectory.java(8 hunks)server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java(6 hunks)server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java(1 hunks)server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java(2 hunks)server/src/main/java/org/opensearch/indices/replication/checkpoint/RemoteStorePublishMergedSegmentAction.java(3 hunks)server/src/main/java/org/opensearch/repositories/FilterRepository.java(2 hunks)server/src/main/java/org/opensearch/repositories/Repository.java(2 hunks)server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java(20 hunks)server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java(1 hunks)server/src/main/java/org/opensearch/snapshots/SnapshotShardsService.java(2 hunks)server/src/test/java/org/opensearch/cluster/metadata/KmsCryptoMetadataTests.java(1 hunks)server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java(1 hunks)server/src/test/java/org/opensearch/index/shard/RemoteStoreUploaderServiceTests.java(9 hunks)server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java(2 hunks)server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java(7 hunks)server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryEncryptionTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java(2 hunks)server/src/test/java/org/opensearch/snapshots/mockstore/MockEventuallyConsistentRepository.java(1 hunks)test/framework/src/main/java/org/opensearch/index/shard/IndexShardTestCase.java(1 hunks)test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java(1 hunks)test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java(2 hunks)test/framework/src/main/java/org/opensearch/snapshots/mockstore/MockRepository.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- server/src/main/java/org/opensearch/common/blobstore/stream/write/WriteContext.java
- server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java
- server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java
- server/src/main/java/org/opensearch/index/shard/RemoteStoreUploader.java
- server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java
- server/src/main/java/org/opensearch/common/blobstore/support/FilterBlobContainer.java
- server/src/test/java/org/opensearch/cluster/metadata/KmsCryptoMetadataTests.java
- server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java
- plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerEncryptionTests.java
- server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java
- server/src/test/java/org/opensearch/index/shard/RemoteStoreUploaderServiceTests.java
- server/src/main/java/org/opensearch/snapshots/SnapshotShardsService.java
- test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java
- plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerMockClientTests.java
- plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java
🧰 Additional context used
🧬 Code graph analysis (6)
server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java (2)
server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java (1)
ExperimentalApi(30-185)server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java (1)
ExperimentalApi(23-96)
server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java (1)
server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java (1)
ExperimentalApi(30-185)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java (3)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java (1)
SseKmsUtil(22-116)server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java (1)
ExperimentalApi(30-185)server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java (1)
ExperimentalApi(23-96)
server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (1)
server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java (1)
EncryptedBlobStore(27-148)
test/framework/src/main/java/org/opensearch/snapshots/mockstore/MockRepository.java (1)
server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java (1)
FsBlobContainer(81-283)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java (1)
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
EncryptionContextUtils(33-283)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: gradle-check
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: Analyze (java)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, windows-latest)
🔇 Additional comments (32)
server/src/main/java/org/opensearch/repositories/FilterRepository.java (1)
209-235: IndexMetadata passthrough insnapshotShardlooks correctThe updated
snapshotShardsignature and the corresponding delegate call toin.snapshotShard(...)correctly add and forwardIndexMetadatawithout altering existing behavior, keepingFilterRepositorya thin wrapper over the underlyingRepositoryimplementation.test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java (1)
42-46: Null CryptoMetadata ingetShardContainersis appropriate for this test utilityUsing the new
repository.shardContainer(indexId, i, (CryptoMetadata) null)overload keeps behavior identical for these consistency checks while binding to the updated API. Since this helper only validates blob layout and generations, not encryption behavior, passingnullCryptoMetadata is the right choice here.Also applies to: 192-204
server/src/test/java/org/opensearch/snapshots/mockstore/MockEventuallyConsistentRepository.java (1)
417-427: Metadata/crypto-aware write correctly reuses eventual-consistency behaviorThe new
writeBlobWithMetadata(..., Map<String,String> metadata, CryptoMetadata cryptoMetadata)override simply delegates towriteBlob, which means all the existing eventual-consistency and failure-injection logic still applies to these writes. Since this mock repository is focused on S3-like consistency characteristics rather than metadata/crypto semantics, this is an appropriate implementation.server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java (1)
62-69: Tests correctly adapted to newcopyFrom/getBlobContainercontext parameterAll the updated tests now:
- Pass an explicit trailing
nullcontext intoRemoteSegmentStoreDirectory.copyFrom(...)andremoteDataDirectory.getBlobContainer(null).- Verify the new parameter via
eq(IOContext.DEFAULT), isNull()on the mockedcopyFromcall.This keeps the test behavior equivalent while binding to the extended APIs that accept an extra context argument (e.g., for encryption metadata). The use of
isNull()in the verification is appropriate and type-safe.Also applies to: 462-463, 483-501, 545-546, 592-597, 704-710
server/src/main/java/org/opensearch/common/blobstore/transfer/RemoteTransferContainer.java (1)
16-17: RemoteTransferContainer now cleanly carries metadata and CryptoMetadata into WriteContextThe constructor refactor and new fields look solid:
- The original constructor now delegates to the new “full” ctor with
metadataandcryptoMetadataset tonull, preserving existing behavior.- The added overload with
Map<String,String> metadataand the new “full” overload withmetadata+CryptoMetadataallow callers to attach both object metadata and index-level crypto context.createWriteContext()now populates both.metadata(metadata)and.cryptoMetadata(cryptoMetadata), which is the right place to surface this information to downstream upload implementations.API evolution here is backward compatible and provides a clear path for crypto-aware remote transfers.
Also applies to: 61-62, 77-99, 114-137, 139-175, 181-191
CHANGELOG.md (1)
36-36: Changelog entry is clear and consistent.Description and PR link accurately summarize the new index-level encryption behavior for snapshots and remote store.
server/src/main/java/org/opensearch/index/shard/RemoteStoreUploaderService.java (1)
18-56: CryptoMetadata and priority threading through uploadSegments/copyFrom looks correct.The new
isLowPriorityUploadandCryptoMetadata cryptoMetadataparameters are cleanly propagated toremoteDirectory.copyFrom, and passingcryptoMetadataonce per call (same for all segments in the shard) is appropriate. WithcryptoMetadata == null, behavior remains identical to the previous implementation, so callers that don’t opt into index-level encryption keep existing semantics.Also applies to: 87-94
server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java (1)
14-15: Passing null CryptoMetadata into shardContainer correctly preserves existing searchable snapshot behavior.Updating
shardContainerto the new(IndexId, int, CryptoMetadata)signature and explicitly passing(CryptoMetadata) nullis the right way to keep this read‑only searchable snapshot path using repository‑level encryption settings only. No functional regression here.Also applies to: 103-107
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java (1)
137-150: Please verify blobStore passed into RemoteDirectory matches SSE / crypto configuration.
RemoteDirectoryis now constructed with:
- A container from
blobStoreRepository.blobStore(isServerSideEncryptionEnabled).blobContainer(dataPath), and- A separate
blobStoreRepository.blobStore()instance plusdataPath.If
blobStore()andblobStore(isServerSideEncryptionEnabled)can differ in how they configure server‑side encryption or other crypto behavior,RemoteDirectory.getBlobContainer(cryptoMetadata)might end up using a BlobStore whose configuration doesn’t match the container used elsewhere.It’s worth double‑checking that:
- Either both calls return equivalently configured BlobStores for this repository, or
- The intention is indeed to use the non‑SSE (or base) BlobStore for the crypto‑aware path, in which case a short comment here explaining that distinction would help avoid confusion in future changes.
server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java (1)
72-110: Tests correctly adapted to the new copyFrom signature.Both
testCopyFromandtestCopyFromWithExceptionnow passfalseforisLowPriorityUploadandnullforCryptoMetadata, which preserves the previous behavior while satisfying the extended API. The mockedAsyncMultiStreamBlobContainerinteractions and assertions remain valid.Also applies to: 116-149
server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobContainer.java (2)
125-144: LGTM!The metadata-only overload correctly delegates to
executeWritefor client-side encryption and passes metadata through to the underlying container.
146-170: Dual encryption handling looks correct.The comment at lines 155-157 clearly documents that:
- Client-side encryption is handled by
this.cryptoHandler(already configured in the constructor)- SSE-KMS settings from
cryptoMetadataare passed to the underlying container for server-side encryptionThis design allows repositories to layer client-side encryption (via EncryptedBlobContainer) on top of server-side encryption (via S3/Azure SSE-KMS), which is the intended behavior for this PR.
server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java (1)
24-96: LGTM! Past review feedback addressed.The
equals()andhashCode()overrides correctly include theisIndexLevelfield, as verified in the past review comments. The implementation properly handles KMS-specific settings and serialization.server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryEncryptionTests.java (1)
25-136: Comprehensive test coverage for CryptoMetadata extraction.The tests validate:
- KMS metadata extraction from index settings
- Null returns for non-encrypted indices
- Null returns when key provider is missing
- Complete IndexMetadata with encryption settings
- Multiple indices with different KMS keys
This provides good coverage of the extraction logic.
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java (5)
202-218: LGTM!The method correctly propagates
cryptoMetadatato bothexecuteSingleUploadandexecuteMultipartUploadbased on blob size threshold.
220-234: Clean backward compatibility.The delegating overload ensures existing callers continue to work while enabling new crypto-aware paths.
570-616: LGTM!The
executeSingleUploadmethod correctly passescryptoMetadatatoconfigureEncryptionSettings, enabling index-level encryption context and key override for single-part uploads.
621-731: LGTM!The
executeMultipartUploadmethod correctly passescryptoMetadatatoconfigureEncryptionSettings, ensuring consistent encryption configuration for multipart uploads.
237-267: Verify CryptoMetadata propagation through WriteContext.The code extracts
CryptoMetadatafromwriteContext.getCryptoMetadata()and uses it to configure KMS settings. This requires thatWriteContexthas been updated to carryCryptoMetadata.Verify that:
- The
WriteContextclass has thegetCryptoMetadata()method- All callers of
asyncBlobUploadproperly initialize theCryptoMetadatain theWriteContextbefore passing it to this methodserver/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java (3)
602-647: LGTM! Clean delegation pattern.The original
copyFromdelegates to the crypto-aware version withnullcryptoMetadata, ensuring backward compatibility while enabling encryption-aware uploads whencryptoMetadatais provided.
739-749: LGTM!The synchronous
copyFrommethod correctly propagatescryptoMetadatato the underlyingremoteDataDirectory.copyFromcall, and the delegating overload maintains backward compatibility.
823-823: Verify metadata encryption behavior.The
uploadMetadatamethod passesnullforcryptoMetadata, meaning metadata files are not encrypted with index-level keys. This may be intentional if metadata is considered system-level data, but it should be verified that this aligns with the security requirements.Should metadata files (segment_N, etc.) be encrypted using the index-level
CryptoMetadata, or is it acceptable to encrypt them only with repository-level settings? If index-level encryption should apply to metadata, this call should pass the appropriateCryptoMetadatainstead ofnull.Consider discussing with the security team to confirm the intended behavior.
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
140-281: LGTM! Past review feedback addressed.The JSON handling methods now use
XContentBuilderandXContentParserfor robust JSON serialization and deserialization, addressing the escaping concerns raised in previous reviews. The use of OpenSearch's XContent APIs ensures proper handling of special characters.Note the documented limitation in
jsonToCryptofs(lines 168-171) that keys and values must not contain commas or equals signs.plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java (3)
23-35: LGTM! Clean delegation.The
mergeAndEncodeEncryptionContextsmethod delegates to the centralizedEncryptionContextUtils, promoting code reuse and consistent behavior across the codebase.
37-62: LGTM! Correct priority and merging logic.The method correctly:
- Uses
instanceofto safely extract KMS-specific values fromCryptoMetadata- Prioritizes index-level KMS key over repository-level key
- Merges encryption contexts from both levels using the centralized utility
This ensures index-level encryption settings override repository defaults as intended.
64-93: LGTM! Consistent implementation.The
PutObjectRequestvariant mirrors theCreateMultipartUploadRequestlogic, ensuring consistent encryption configuration across single and multipart uploads. The delegating overload maintains backward compatibility.server/src/main/java/org/opensearch/index/store/RemoteDirectory.java (4)
91-108: Correct handling for different encryption types.The comment at lines 92-96 and the logic correctly distinguish between:
- Client-side encryption repos (EncryptedBlobStore): Use crypto-aware container when cryptoMetadata is provided
- Server-side encryption repos: Use default container and pass cryptoMetadata through upload methods
This prevents double-wrapping of encryption for repositories that already encrypt at the blob store level.
110-172: LGTM! Clean constructor delegation.The constructors properly delegate to the most complete variant, maintaining backward compatibility while adding support for
blobStoreandblobPathneeded for crypto-aware container creation.
435-454: LGTM!The
copyFrommethod correctly propagatescryptoMetadatatouploadBlobfor async multi-stream uploads.
467-579: LGTM! Consistent crypto-aware upload paths.Both the synchronous
copyFromanduploadBlobmethods correctly:
- Obtain a crypto-aware container via
getBlobContainer(cryptoMetadata)- Propagate
cryptoMetadatathroughRemoteTransferContainertoWriteContext- Use the appropriate container for blob uploads
This ensures encryption settings flow through the entire upload pipeline.
server/src/internalClusterTest/java/org/opensearch/snapshots/CloneSnapshotIT.java (1)
799-835: Test helper updates toshardContainerlook correctThe changes in
readShardGenerationandreadShardSnapshotto call:repository.shardContainer(repositoryShardId.index(), repositoryShardId.shardId(), (CryptoMetadata) null)correctly adapt to the new
BlobStoreRepository.shardContainerAPI while preserving existing behavior (tests remain crypto‑agnostic).This keeps the integration tests focused on clone semantics without coupling them to encryption details; future tests that exercise index‑level encryption can supply appropriate metadata if needed.
server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (1)
766-860: Verify CryptoMetadata handling in shard clone operations against encrypted indicesThe
cloneShardSnapshotandcloneRemoteStoreIndexShardSnapshotmethods obtain the shard container viashardContainer(index, shardNum, (CryptoMetadata) null), which may not honor index-level encryption settings for repositories usingEncryptedBlobStore.If index-level
CryptoMetadatais required to correctly decrypt existing shard snapshot metadata and encrypt cloned metadata (consistent withsnapshotShardandrestoreShardpatterns), clone operations for encrypted indices may fail or use incorrect encryption context.Verify shard clone behavior against indices with index-level crypto settings and confirm that decryption and re-encryption work as expected. If index metadata is available during clone, consider deriving and passing the proper
CryptoMetadatato ensure consistency with the snapshot/restore pipeline.
| Version repositoryMetaVersion, | ||
| Map<String, Object> userMetadata, | ||
| ActionListener<String> listener, | ||
| IndexMetadata indexMetadata |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Mark the indexMetadata parameter as @Nullable.
The default delegating method at line 401 passes null for this parameter, so implementations must handle null values. The parameter should be annotated with @Nullable to make this contract explicit and prevent potential NPE issues.
Apply this diff:
ActionListener<String> listener,
- IndexMetadata indexMetadata
+ @Nullable IndexMetadata indexMetadata
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IndexMetadata indexMetadata | |
| ActionListener<String> listener, | |
| @Nullable IndexMetadata indexMetadata | |
| ); |
🤖 Prompt for AI Agents
server/src/main/java/org/opensearch/repositories/Repository.java around line
439: the IndexMetadata parameter on this method is sometimes called with null
(via the delegating method at line 401) but is not annotated; annotate the
parameter with @Nullable to make the contract explicit and avoid NPEs in
implementations, and add the corresponding import (javax.annotation.Nullable or
org.jetbrains.annotations.Nullable depending on project conventions) if not
already present.
|
❌ Gradle check result for fcd6e29: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Uday Bhaskar <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java (1)
31-32: Harden crypto-aware blobContainer overload (null handler, naming, and messaging)The new
blobContainer(BlobPath, CryptoMetadata)correctly routes per-index metadata throughmergeCryptoMetadata, but there are a few issues to address:
CryptoHandler IndexCryptoHandler = cryptoHandlerRegistry.fetchCryptoHandler(merged);is not null-checked. If the registry cannot resolve a handler for the merged metadata, this will surface later as an NPE when constructing the encrypted container, instead of the explicitCryptoRegistryExceptionyou already use in the constructor.- Local variable
IndexCryptoHandlerviolates Java naming conventions (capitalized local name).- The assert message still references
"CryptoManagerRegistry"while the actual type isCryptoHandlerRegistry.- The comment has a typo (“overloadded”).
To keep behavior consistent with the constructor and fail fast on misconfiguration, consider something along these lines:
@@ - private final CryptoMetadata repositoryCryptoMetadata; // Store for merging with index metadata + private final CryptoMetadata repositoryCryptoMetadata; // Store for merging with index metadata @@ - // overloadded method to get blob container with the new crypto metadata + // Overloaded method to get blob container with per-index crypto metadata public BlobContainer blobContainer(BlobPath path, CryptoMetadata cryptoMetadata) { - - // Merge index metadata with repository metadata for context merging - CryptoMetadata merged = (cryptoMetadata != null) ? mergeCryptoMetadata(cryptoMetadata) : this.repositoryCryptoMetadata; - - CryptoHandlerRegistry cryptoHandlerRegistry = CryptoHandlerRegistry.getInstance(); - assert cryptoHandlerRegistry != null : "CryptoManagerRegistry is not initialized"; - CryptoHandler IndexCryptoHandler = cryptoHandlerRegistry.fetchCryptoHandler(merged); - + // Merge index metadata with repository metadata for context merging + CryptoMetadata merged = cryptoMetadata != null ? mergeCryptoMetadata(cryptoMetadata) : this.repositoryCryptoMetadata; + + CryptoHandlerRegistry cryptoHandlerRegistry = CryptoHandlerRegistry.getInstance(); + assert cryptoHandlerRegistry != null : "CryptoHandlerRegistry is not initialized"; + CryptoHandler<?, ?> indexCryptoHandler = cryptoHandlerRegistry.fetchCryptoHandler(merged); + if (indexCryptoHandler == null) { + throw new CryptoRegistryException( + merged.keyProviderName(), + merged.keyProviderType(), + "Crypto manager not found during encrypted blob container creation." + ); + } + BlobContainer blobContainer = blobStore.blobContainer(path); if (blobContainer instanceof AsyncMultiStreamBlobContainer) { - return new AsyncMultiStreamEncryptedBlobContainer<>((AsyncMultiStreamBlobContainer) blobContainer, IndexCryptoHandler); + return new AsyncMultiStreamEncryptedBlobContainer<>((AsyncMultiStreamBlobContainer) blobContainer, indexCryptoHandler); } - return new EncryptedBlobContainer<>(blobContainer, IndexCryptoHandler); + return new EncryptedBlobContainer<>(blobContainer, indexCryptoHandler); @@ - private CryptoMetadata mergeCryptoMetadata(CryptoMetadata indexMetadata) { - // Delegate to centralized utility class - return EncryptionContextUtils.mergeCryptoMetadata(indexMetadata, this.repositoryCryptoMetadata); - } + private CryptoMetadata mergeCryptoMetadata(CryptoMetadata indexMetadata) { + // Delegate to centralized utility class + return EncryptionContextUtils.mergeCryptoMetadata(indexMetadata, this.repositoryCryptoMetadata); + }This keeps error handling symmetric with the constructor, avoids surprising NPEs, and fixes the naming/typo issues.
Also applies to: 53-54, 72-87, 131-134
server/src/main/java/org/opensearch/repositories/Repository.java (1)
378-440: MarkindexMetadataas@Nullableto document the contract.The new default
snapshotShard(...)overload passesnullforindexMetadata, but the abstract method:void snapshotShard(..., ActionListener<String> listener, IndexMetadata indexMetadata);does not mark this parameter as nullable. Implementors may assume it is always non-null and dereference it, leading to NPEs when the default delegating overload is used.
Recommend:
- Annotate the parameter as
@Nullable, and- Optionally mention in the Javadoc that implementations must handle
indexMetadata == null(e.g., for older callers that don’t provide it).Example:
- ActionListener<String> listener, - IndexMetadata indexMetadata + ActionListener<String> listener, + @Nullable IndexMetadata indexMetadata
🧹 Nitpick comments (13)
server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java (1)
333-346: Clarify the comment about the method's scope.The comment claims a "2-layer priority" with repository-level encryption as a fallback, but this method only extracts index-level metadata. The repository-level fallback must be handled elsewhere (likely in the repository or blob container layer when null is returned).
Update the comment to clarify that this method is responsible only for extracting index-level CryptoMetadata, with the understanding that null returns will trigger repository-level fallback downstream:
/** - * Resolves CryptoMetadata with 2-layer priority: - * 1. Index-level encryption from index settings (if configured) - * 2. Repository-level encryption (fallback to null) + * Extracts index-level CryptoMetadata from settings. + * Returns null if no index-level encryption is configured, allowing + * repository-level encryption to be used as fallback at the blob store layer. * * @param indexMetadata Index metadata containing crypto settings - * @return CryptoMetadata if encryption is configured, null otherwise + * @return Index-level CryptoMetadata if configured, null otherwise */server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java (1)
103-107: Consider documenting why CryptoMetadata is null.The explicit cast
(CryptoMetadata) nullhelps with method overload resolution, but consider adding a brief comment explaining why crypto metadata is not available or applicable in this snapshot directory creation path.Apply this diff to add clarifying comment:
final BlobContainer blobContainer = blobStoreRepository.shardContainer( new IndexId("DUMMY", indexId, pathType.getCode()), shardId, + // CryptoMetadata not available in searchable snapshot restoration path (CryptoMetadata) null );server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java (1)
209-220: LGTM with minor suggestion: Forward-compatibility stub correctly implemented.The
writeBlobWithMetadatamethod properly delegates towriteBlobwhile ignoring metadata and encryption parameters. The comment acknowledges this limitation.Consider enhancing the comment to be more specific:
- // FsBlobContainer doesn't handle metadata or encryption + // FsBlobContainer does not support metadata or encryption - delegating to standard writeBlob + // TODO: Add support for storing metadata alongside blobs if needed writeBlob(blobName, inputStream, blobSize, failIfAlreadyExists);server/src/test/java/org/opensearch/snapshots/mockstore/MockEventuallyConsistentRepository.java (1)
417-427: LGTM: Mock updated for API compatibility.The
writeBlobWithMetadataoverride correctly delegates to the existingwriteBlobmethod, maintaining mock behavior while supporting the extended API.Consider adding a brief comment for clarity:
@Override public void writeBlobWithMetadata( String blobName, InputStream inputStream, long blobSize, boolean failIfAlreadyExists, @Nullable Map<String, String> metadata, @Nullable org.opensearch.cluster.metadata.CryptoMetadata cryptoMetadata ) throws IOException { + // Mock doesn't need to handle metadata/encryption - delegate to standard writeBlob writeBlob(blobName, inputStream, blobSize, failIfAlreadyExists); }plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobContainer.java (1)
140-150: LGTM: Forward-compatibility stub added.The
writeBlobWithMetadatamethod correctly delegates to the existingwriteBlobimplementation, maintaining current behavior while supporting the extended API.Consider adding a clarifying comment:
@Override public void writeBlobWithMetadata( String blobName, InputStream inputStream, long blobSize, boolean failIfAlreadyExists, @Nullable Map<String, String> metadata, @Nullable CryptoMetadata cryptoMetadata ) throws IOException { + // Azure blob container does not yet support metadata or encryption parameters writeBlob(blobName, inputStream, blobSize, failIfAlreadyExists); }plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java (1)
638-655: Encryption-context assertions align with SSE‑KMS behavior; consider de‑duplicating test setupThe new
kmsContextandexpectedEncodedContext = Base64(kmsContext)checks for both single and multipart uploads correctly validate that thessekmsEncryptionContextheader is the Base64‑encoded JSON context when SSE‑KMS is enabled, andnullwhen using AES256. The logic looks sound.You might consider extracting a small helper to build
(kmsContext, expectedEncodedContext)to avoid duplicating the setup in both tests, but that’s optional.Also applies to: 693-703, 751-768, 827-835
server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java (1)
90-111: RemoteDirectory.copyFrom tests correctly wired to new signatureUsing
false, nullfor the new tail parameters keeps the semantics of these tests (no extra flags, no crypto) unchanged while matching the extendedcopyFromAPI. You may eventually want a dedicated test that passes non‑nullCryptoMetadata, but it’s not required for this PR.Also applies to: 128-151
server/src/main/java/org/opensearch/snapshots/SnapshotShardsService.java (1)
473-488: EnsureIndexMetadatasource and null handling are intentionalPassing
IndexMetadataintorepository.snapshotShardis a good way to propagate index‑level crypto configuration. One minor edge case: resolving it viaclusterService.state().metadata().index(indexId.getName())could returnnullif the cluster state has already dropped the index whileindexServiceSafestill succeeded, which would push anullintosnapshotShard.You might consider instead using
indexService.getMetadata()(already available above) or adding a small null‑handling/fallback to avoid surprising NPEs in repository code when snapshots race with index deletion/closing.server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java (1)
1-234: Strong coverage for encryption-context utilities; consider adding malformed-input testsThis new test class does a nice job exercising the main behaviors of
EncryptionContextUtils: cryptofs⇆JSON conversions, JSON merge semantics, base64‑encoded context merging,CryptoMetadatamerging (including index‑over‑repo precedence), and round‑trip scenarios. If you want to harden it further, you could add a few negative tests (e.g., invalid JSON, invalid/non‑JSON Base64) to assert that failures are surfaced in a predictable way, but that’s optional given the current scope.server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (2)
58-79: Shard container crypto wiring looks correct; consider minor cleanup.The new
shardContaineroverloads andresolveCryptoMetadata(IndexMetadata)correctly:
- Derive
CryptoMetadatafrom index settings.- Route to
EncryptedBlobStore.blobContainer(path, cryptoMetadata)only when non-null and the underlying store is encrypted.- Fall back to the existing
blobStore().blobContainer(path)behavior otherwise, preserving backward compatibility for non-encrypted and repo-level-encrypted repos.One tiny readability/perf tweak: inside
shardContainer(IndexId, int, CryptoMetadata)you could cacheblobStore()in a local variable to avoid multiple calls, though the current form is functionally fine.Also applies to: 2974-2992, 4987-4993
3962-3999: Snapshot path crypto propagation is sound; drop unused parameter to reduce noise.The snapshot flow now:
- Uses
shardContainer(indexId, shardId, indexMetadata)so shard-level metadata IO goes through the crypto-aware container.- Resolves
CryptoMetadataonce per snapshot viaresolveCryptoMetadata(indexMetadata)and passes it through toexecuteOneFileSnapshotand ultimatelysnapshotFile, which in turn useswriteBlobWithMetadata(..., cryptoMetadata).This is the right place to inject index-level crypto, and the early return when
indexIncrementalFileCount == 0preserves the old fast-path.One minor cleanup:
executeOneFileSnapshot(...)takes anIndexMetadata indexMetadataparameter that is never used. You can safely remove it from the signature and call sites to keep the API tight.Also applies to: 4159-4178, 4185-4218, 4848-4891
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
270-276: Consider validating value token type before callingtext().The method assumes all values are strings/scalars. If the JSON contains nested objects or arrays,
parser.text()may throw or return unexpected results. Since this utility is for simple key-value encryption contexts, consider adding validation or a more informative error message.if (token == XContentParser.Token.FIELD_NAME) { String key = parser.currentName(); - parser.nextToken(); // Move to value - String value = parser.text(); + XContentParser.Token valueToken = parser.nextToken(); // Move to value + if (valueToken != XContentParser.Token.VALUE_STRING && valueToken != XContentParser.Token.VALUE_NUMBER) { + throw new IllegalArgumentException("Encryption context values must be strings, got: " + valueToken + " for key: " + key); + } + String value = parser.text(); map.put(key, value); }plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java (1)
64-89: Consider extracting common KMS configuration logic.The key extraction and context merging logic is duplicated between the
CreateMultipartUploadRequest.BuilderandPutObjectRequest.Builderversions. Consider extracting to a private helper method for maintainability.private static class KmsConfig { final String kmsKey; final String encContext; KmsConfig(String kmsKey, String encContext) { this.kmsKey = kmsKey; this.encContext = encContext; } } private static KmsConfig resolveKmsConfig(@Nullable CryptoMetadata cryptoMetadata, S3BlobStore blobStore) { String indexKmsKey = null; String indexEncContext = null; if (cryptoMetadata instanceof KmsCryptoMetadata) { KmsCryptoMetadata kmsMetadata = (KmsCryptoMetadata) cryptoMetadata; indexKmsKey = kmsMetadata.getKmsKeyArn().orElse(null); indexEncContext = kmsMetadata.getKmsEncryptionContext().orElse(null); } String kmsKey = (indexKmsKey != null) ? indexKmsKey : blobStore.serverSideEncryptionKmsKey(); String encContext = mergeAndEncodeEncryptionContexts(indexEncContext, blobStore.serverSideEncryptionEncryptionContext()); return new KmsConfig(kmsKey, encContext); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
CHANGELOG.md(1 hunks)plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobContainer.java(2 hunks)plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java(10 hunks)plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerEncryptionTests.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerMockClientTests.java(2 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java(11 hunks)server/src/internalClusterTest/java/org/opensearch/snapshots/CloneSnapshotIT.java(3 hunks)server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java(3 hunks)server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java(1 hunks)server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java(1 hunks)server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobContainer.java(1 hunks)server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java(5 hunks)server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/stream/write/WriteContext.java(9 hunks)server/src/main/java/org/opensearch/common/blobstore/support/FilterBlobContainer.java(2 hunks)server/src/main/java/org/opensearch/common/blobstore/transfer/RemoteTransferContainer.java(6 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java(6 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreUploader.java(2 hunks)server/src/main/java/org/opensearch/index/shard/RemoteStoreUploaderService.java(3 hunks)server/src/main/java/org/opensearch/index/store/RemoteDirectory.java(8 hunks)server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java(6 hunks)server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java(1 hunks)server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java(2 hunks)server/src/main/java/org/opensearch/indices/replication/checkpoint/RemoteStorePublishMergedSegmentAction.java(3 hunks)server/src/main/java/org/opensearch/repositories/FilterRepository.java(2 hunks)server/src/main/java/org/opensearch/repositories/Repository.java(2 hunks)server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java(20 hunks)server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java(1 hunks)server/src/main/java/org/opensearch/snapshots/SnapshotShardsService.java(2 hunks)server/src/test/java/org/opensearch/cluster/metadata/KmsCryptoMetadataTests.java(1 hunks)server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java(1 hunks)server/src/test/java/org/opensearch/index/shard/RemoteStoreUploaderServiceTests.java(9 hunks)server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java(2 hunks)server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java(7 hunks)server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryEncryptionTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java(1 hunks)server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java(2 hunks)server/src/test/java/org/opensearch/snapshots/mockstore/MockEventuallyConsistentRepository.java(1 hunks)test/framework/src/main/java/org/opensearch/index/shard/IndexShardTestCase.java(1 hunks)test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java(1 hunks)test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java(2 hunks)test/framework/src/main/java/org/opensearch/snapshots/mockstore/MockRepository.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java
- CHANGELOG.md
- server/src/test/java/org/opensearch/cluster/metadata/KmsCryptoMetadataTests.java
- server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobContainer.java
- plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerEncryptionTests.java
- server/src/main/java/org/opensearch/common/blobstore/support/FilterBlobContainer.java
- server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java
- server/src/main/java/org/opensearch/index/shard/RemoteStoreUploaderService.java
- server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java
- plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java
- server/src/main/java/org/opensearch/repositories/FilterRepository.java
- server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java
- server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java
- server/src/main/java/org/opensearch/index/shard/RemoteStoreUploader.java
🧰 Additional context used
🧬 Code graph analysis (5)
server/src/test/java/org/opensearch/repositories/blobstore/EncryptionContextUtilsTests.java (1)
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
EncryptionContextUtils(33-283)
server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java (1)
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
EncryptionContextUtils(33-283)
server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (1)
server/src/main/java/org/opensearch/common/blobstore/EncryptedBlobStore.java (1)
EncryptedBlobStore(27-148)
test/framework/src/main/java/org/opensearch/snapshots/mockstore/MockRepository.java (2)
server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (1)
BlobStoreRepository(217-4994)server/src/main/java/org/opensearch/common/blobstore/fs/FsBlobContainer.java (1)
FsBlobContainer(81-283)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java (2)
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (1)
EncryptionContextUtils(33-283)plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobStore.java (1)
S3BlobStore(72-348)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: gradle-check
- GitHub Check: detect-breaking-change
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: Analyze (java)
- GitHub Check: Mend Security Check
🔇 Additional comments (40)
server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java (1)
37-37: LGTM! Signature update correctly propagates index metadata.The addition of the
IndexMetadata indexMetadataparameter tosnapshotShardand its forwarding to the superclass method aligns with the PR's index-level encryption objectives. The test correctly passes through the new parameter without requiring additional assertions, as it focuses on user metadata filtering rather than encryption behavior.Also applies to: 140-167
test/framework/src/main/java/org/opensearch/snapshots/mockstore/MockRepository.java (4)
41-41: LGTM!The new imports are necessary for the metadata-aware write methods.
Also applies to: 44-44
593-613: LGTM!The implementation correctly mirrors the existing
writeBlobpattern, applying consistent blocking and error injection logic before and after the write operation to simulate network-based repository failures.
615-636: LGTM!The CryptoMetadata overload correctly extends the mock to support crypto-aware blob writes while maintaining the same blocking and error simulation behavior as the non-crypto variant.
638-670: LGTM!The atomic write implementation correctly mirrors the existing
writeBlobAtomicpattern, including proper handling of index file blocking scenarios and simulation of FsBlobContainer's temp-write-then-move behavior for metadata-bearing blobs.server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java (4)
20-21: LGTM!The new imports are necessary for extracting and propagating index-level encryption metadata.
228-232: LGTM!Extracting crypto metadata once at the start of sync ensures consistency throughout the upload operation.
354-375: LGTM!The signature update correctly propagates CryptoMetadata through the upload path with proper documentation.
277-277: LGTM!The updated comments clarify that metadata is uploaded in plaintext while segment files use encryption, which is an important distinction for understanding the security model.
Also applies to: 306-306
test/framework/src/main/java/org/opensearch/index/shard/IndexShardTestCase.java (1)
1534-1535: LGTM: Test infrastructure updated for new signature.The call to
repository.snapshotShardcorrectly passesnullfor the newIndexMetadataparameter, maintaining compatibility with the extended API while preserving existing test behavior.server/src/main/java/org/opensearch/common/blobstore/stream/write/WriteContext.java (1)
35-35: LGTM: Clean integration of CryptoMetadata.The
cryptoMetadatafield is properly threaded through the builder pattern with:
- Appropriate
@Nullableannotation- Correct propagation in both constructors (primary and copy)
- Standard getter method
- Builder setter following existing conventions
The implementation is complete and consistent with the rest of the class.
Also applies to: 58-59, 69-69, 85-85, 152-157, 174-174, 221-224, 236-237
test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java (1)
202-214: LGTM: Test stub updated for new interface signature.The
snapshotShardmethod signature correctly adds theIndexMetadataparameter to maintain compatibility with the updatedRepositoryinterface. The empty method body is expected for this restore-only test repository.server/src/test/java/org/opensearch/index/shard/RemoteStoreUploaderServiceTests.java (1)
144-144: LGTM: Test suite consistently updated for CryptoMetadata parameter.All test methods correctly pass
nullfor the newCryptoMetadataparameter touploadSegments, and mock setups have been updated to accommodate the extendedcopyFromsignature (8 parameters instead of 7). The changes maintain existing test coverage while supporting the new API.Also applies to: 195-195, 204-204, 258-258, 267-267, 324-324, 333-333, 393-393, 400-400, 460-460, 467-467
server/src/main/java/org/opensearch/cluster/metadata/CryptoMetadata.java (1)
93-107: Index-settings factory looks correct and preserves backward compatibilityThe
fromIndexSettingshelper cleanly handles the “no crypto configured” case and selectsKmsCryptoMetadataonly foraws-kms, with other providers falling back to genericCryptoMetadata. No functional issues spotted here.test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java (1)
42-42: Shard-container call correctly updated for crypto-aware APIPassing
(CryptoMetadata) nullkeeps existing behavior while binding to the new crypto-awareshardContaineroverload; no issues here.Also applies to: 200-201
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobStoreContainerTests.java (1)
137-158: Upload helper invocations match the extended signaturesAll direct calls to
executeSingleUploadandexecuteMultipartUploadhave been correctly updated to pass the trailingCryptoMetadataargument (asnullin non-crypto tests), keeping the previous behavioral intent (boundary checks and abort paths) intact.Also applies to: 711-714, 723-726, 817-820, 947-950
server/src/main/java/org/opensearch/indices/replication/checkpoint/RemoteStorePublishMergedSegmentAction.java (1)
17-19: Index-level CryptoMetadata is correctly threaded into merged-segment uploadsDeriving
CryptoMetadatafromIndexMetadataviaCryptoMetadata.fromIndexSettings(...)and passing it as the new final argument touploadSegmentscleanly enables index-level overrides while preserving the “no crypto configured” path (null metadata). The additional null checks aroundindexSettings/indexMetadataare defensive but not harmful.Also applies to: 167-175, 176-202
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerMockClientTests.java (1)
91-92: Mock verification updated correctly for crypto-aware multipart uploadsThe added
isNull()matcher for the trailing argument inexecuteMultipartUploadverification aligns with the new method signature and clearly asserts that noCryptoMetadatais supplied along this path.Also applies to: 756-763
server/src/internalClusterTest/java/org/opensearch/snapshots/CloneSnapshotIT.java (1)
41-41: Shard container usage correctly adapted to CryptoMetadata-aware APIThe test updates to call
repository.shardContainer(index, shard, (CryptoMetadata) null)keep the existing behavior while matching the new signature. This is appropriate here since these tests aren’t exercising encryption behavior; you can always add variants with non‑nullCryptoMetadatalater if you want explicit coverage of crypto‑aware shard containers.Also applies to: 799-818, 820-834
server/src/main/java/org/opensearch/common/blobstore/transfer/RemoteTransferContainer.java (1)
16-25: CryptoMetadata propagation throughRemoteTransferContainerlooks correctThe constructor chaining keeps existing usages working while introducing the metadata+
CryptoMetadatavariant, andcreateWriteContext()now cleanly passes bothmetadataandcryptoMetadataintoWriteContext. This is a straightforward and consistent way to thread index‑level encryption metadata through the transfer pipeline.Also applies to: 60-62, 77-175, 180-191
server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java (1)
68-68: Tests correctly updated for new context parameters oncopyFrom/getBlobContainerAll affected tests now pass an explicit
nulltrailing/context argument intocopyFromandgetBlobContainer, and the verification intestUploadMetadataNonEmptyasserts thatcopyFromis invoked withIOContext.DEFAULTplus a null context. This keeps existing non‑encrypted behaviors intact while exercising the new method signatures.Also applies to: 461-463, 482-501, 529-546, 587-596, 704-710
server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (2)
4259-4261: Confirm index-metadata lookup is correct in restore / status for renamed indices.
restoreShardand bothgetShardSnapshotStatusvariants now resolve:IndexMetadata indexMetadata = clusterService.state().metadata().index(indexId.getName()); BlobContainer container = shardContainer(indexId, shardId, indexMetadata);This is appropriate when
indexId.getName()matches the concrete index name in the current cluster state and index-level crypto settings are immutable.However, if restore-with-rename or other flows ever use an
IndexIdwhosenamediffers from the target index’s actual name,indexMetadatawill benulland these calls silently fall back to repo-level crypto. Please double-check thatindexId.getName()always corresponds to the restored index name in these paths, or explicitly handle the rename case if not.Also applies to: 4545-4546, 4551-4552
783-783: Explicit(CryptoMetadata) nullusage preserves existing behavior on non-data paths.In clone, delete/cleanup, and shallow-copy metadata paths you now call:
final BlobContainer shardContainer = shardContainer(indexId, shardId, (CryptoMetadata) null);Given
shardContainer(IndexId, int, CryptoMetadata)falls back toblobStore().blobContainer(...)when the metadata is null, this maintains the prior behavior for:
- Clone operations
- Shard-metadata updates and deletes
- Remote-store lock cleanup
- Shallow-copy metadata reads
This is appropriate since these flows generally do not need index-level overrides and often operate when index metadata may no longer be available.
Also applies to: 880-881, 1857-1858, 2508-2509, 2559-2560, 4535-4536
server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryEncryptionTests.java (1)
1-137: Encryption metadata tests are well targeted.These tests nicely exercise both
CryptoMetadata.fromIndexSettingsandKmsCryptoMetadata.fromIndexSettings, including:
- Happy-path KMS configs,
- Non-encrypted and partially-configured indices (returning null), and
- Distinct KMS key/contexts per index.
They should give good confidence that index settings are interpreted correctly for index-level crypto.
server/src/main/java/org/opensearch/cluster/metadata/KmsCryptoMetadata.java (1)
24-66: KMS crypto metadata modeling and serialization look solid.
KmsCryptoMetadatacleanly extendsCryptoMetadata:
- Stream-based constructor and
writeTomaintain a consistent serialization order and include theisIndexLevelflag.fromIndexSettingscorrectly derives KMS-specific settings fromindex.store.crypto.*, returning null when no key provider is configured.equals,hashCode, andtoStringtakeisIndexLevelinto account, so index-level and non-index-level instances are distinguished properly.No issues from a correctness or API-usage perspective.
Also applies to: 68-95
server/src/main/java/org/opensearch/repositories/blobstore/EncryptionContextUtils.java (5)
46-78: LGTM! Robust handling of encryption context merging.The method properly handles null/empty cases, detects JSON vs cryptofs format, and uses Base64 decoding with a fallback for invalid input. The merging strategy (repo baseline, index override) is clearly documented.
89-133: LGTM! Clear precedence and merging logic.The method correctly implements the documented priority (index > repository) for key provider fields and properly merges settings with repository as base and index as overlay. The encryption context merging handles format conversion appropriately.
140-161: LGTM! Proper JSON escaping with XContentBuilder.The use of
XContentBuilder.field()correctly handles JSON escaping for special characters in keys and values.
163-192: LGTM! Clear documentation of format limitations.The important limitation about comma and equals characters is well-documented in the Javadoc, which helps prevent misuse.
202-227: LGTM! Correct JSON merging with proper escaping.The use of
LinkedHashMappreserves insertion order for deterministic output, andXContentBuilderhandles JSON escaping correctly.server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java (3)
602-647: LGTM! Clean delegation pattern for crypto-aware uploads.The existing method now delegates to the new crypto-aware overload with
nullcryptoMetadata, maintaining backward compatibility. The new method correctly propagates theCryptoMetadatathrough toremoteDataDirectory.copyFrom().
739-749: LGTM! Proper delegation for synchronous copy.The crypto-aware overload correctly passes
CryptoMetadatato the underlyingremoteDataDirectory, and the original method delegates withnullfor backward compatibility.
823-823: Verify: Metadata files intentionally not encrypted with index-level encryption.The
nullcryptoMetadata for metadata file upload appears intentional. Please confirm this aligns with the design where only segment data (not metadata) uses index-level encryption.plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/utils/SseKmsUtil.java (2)
37-62: LGTM! Correct key precedence and context merging for multipart uploads.The logic correctly:
- Extracts KMS key and encryption context from
KmsCryptoMetadatawhen available- Uses index-level key over repository-level key (correct precedence)
- Merges encryption contexts with the centralized utility
91-93: LGTM! Backward-compatible convenience overload.The overload correctly delegates to the crypto-aware version with
nullcryptoMetadata, maintaining backward compatibility.server/src/main/java/org/opensearch/index/store/RemoteDirectory.java (5)
101-108: LGTM! Correct crypto-aware container resolution.The method properly checks for
EncryptedBlobStoreinstance and non-nullcryptoMetadatabefore creating a crypto-wrapped container, falling back to the default container otherwise.
155-172: LGTM! Extended constructor for crypto-aware operations.The new constructor properly stores
blobStoreandblobPathfor crypto-aware container creation, and the constructor chaining maintains backward compatibility.
435-454: LGTM! CryptoMetadata correctly propagated to upload path.The
CryptoMetadataparameter is correctly passed through touploadBlobfor crypto-aware uploads.
467-485: LGTM! Proper resource handling with failure cleanup.The method correctly:
- Resolves the crypto-aware container via
getBlobContainer(cryptoMetadata)- Uses try-with-resources for
IndexInputandIndexOutput- Attempts cleanup (delete) on failure with silent exception handling
503-573: LGTM! Comprehensive crypto-aware upload implementation.The upload path correctly:
- Resolves the crypto-aware container via
getBlobContainer(cryptoMetadata)- Passes
cryptoMetadatatoRemoteTransferContainer- Uses
targetContainerconsistently for integrity checks and the actual upload
|
❌ Gradle check result for c6c4608: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
| if (crypto instanceof KmsCryptoMetadata) { | ||
| KmsCryptoMetadata kmsMetadata = (KmsCryptoMetadata) crypto; | ||
| indexKmsKey = kmsMetadata.getKmsKeyArn().orElse(null); | ||
| indexEncContext = kmsMetadata.getKmsEncryptionContext().orElse(null); | ||
| } |
There was a problem hiding this comment.
KMS is just one key provider, we need to make this interface depend on a generic key provider interface similar to CryptoMetadata. At run-time we can bind it to KmsCryptoMetadata
There was a problem hiding this comment.
Key details and encryption contexts are quite generic . Even BlobStore uses them . So it should be okay for CryptoMetadata to have that implementation .
gbbafna
left a comment
gbbafna
left a comment
There was a problem hiding this comment.
For Remote Store, we are moving away from encrypting the data ourselves and using server side encryption of object store. Would it make sense for us to support ILE using just that ? That would minimize the changes here .
| indexKmsKey = kmsMetadata.getKmsKeyArn().orElse(null); | ||
| indexEncContext = kmsMetadata.getKmsEncryptionContext().orElse(null); | ||
| } | ||
| String mergeEncContext = SseKmsUtil.mergeAndEncodeEncryptionContexts( |
There was a problem hiding this comment.
why do we need to merge the encryption contexts ? Can't we just one of them based on priority and non empty check ?
| if (crypto instanceof KmsCryptoMetadata) { | ||
| KmsCryptoMetadata kmsMetadata = (KmsCryptoMetadata) crypto; | ||
| indexKmsKey = kmsMetadata.getKmsKeyArn().orElse(null); | ||
| indexEncContext = kmsMetadata.getKmsEncryptionContext().orElse(null); | ||
| } |
There was a problem hiding this comment.
Key details and encryption contexts are quite generic . Even BlobStore uses them . So it should be okay for CryptoMetadata to have that implementation .
| } | ||
|
|
||
| public static CryptoMetadata fromIndexSettings(Settings indexSettings) { | ||
| String keyProviderName = indexSettings.get("index.store.crypto.key_provider"); |
There was a problem hiding this comment.
Would this setting will be registered only if storage-encryption-plugin is installed ? If yes, we should declare it in the core and use it in the plugin . We should not use a setting which is defined in a plugin .
4 participants
Description
The changes are to provide index-level-encryption support for data stored in snapshots and remote store. Today we have encryption metadata configured at the Repository level. For Index Level Encryption, the expectation is that same key which is provided by user would also be used to encrypt data at rest. The changes are focused on passing Index-level encryption metadata and override it and use it for encryption. If an index does not contain any CryptoMetadata, the encryption would fallback to repository level cryptoMetadata for usage, i.e Backward compatible. The changes also merge encryption context from the Index and Repository level settings.
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
opensearch-project/opensearch-storage-encryption#17
opensearch-project/opensearch-storage-encryption#18
Testing
Plugins installed
snapshot testing
Snapshot repository register
Create Crypto index ,ingest doc and capture snapshot
DELETED index locally
in S3
Files are encrypted
Klled opensearch and restarted back. Restored Index from snapshot
Remote Store Client Side Enc test
Current repo state
Create index, ingested doc and checked S3 for current state
Data files are encrypted
Remote store ServerSide Enc testing
Repos state
new index with enc context
in s3
Cloudtrail event
For simple non crypto index
using default repo key
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Summary by CodeRabbit
New Features
Enhancements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.