Make crypto store settings immutable#20123
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds a validator that prevents modifications to crypto-related index store settings (including switching to or from the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
RajatGupta02
force-pushed
the
ile_disallow_settings_update
branch
from
e9c8610 to
65126b1
Compare
server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java
Outdated
Show resolved
Hide resolved
|
❌ Gradle check result for 65126b1: 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? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java (3)
145-145: Consider moving crypto validation inside the execute() method for consistency.The validation uses
clusterService.state(), which gets the cluster state at submission time. However,validateSearchReplicaCountSettings(line 296) performs similar index-specific validation inside theexecute()method using the execution-timecurrentState. MovingvalidateCryptoStoreSettingsinsideexecute()would ensure validation against the authoritative state and maintain consistency with the pattern used for other index-specific validations.While the bidirectional nature of the cryptofs checks (blocking both TO and FROM) provides inherent safety against race conditions, aligning with the existing pattern would be clearer.
594-608: LGTM! Clear validation logic for crypto setting immutability.The validation correctly enforces that crypto settings cannot be modified after index creation. The straightforward check against
indexSettings.keySet()is appropriate and efficient.Optional improvements:
- Extract the restricted settings array as a class-level constant for better maintainability:
+ private static final String[] RESTRICTED_CRYPTO_SETTINGS = { + "index.store.crypto.key_provider", + "index.store.crypto.kms.key_arn", + "index.store.crypto.kms.encryption_context" + }; + /** - * Validates crypto store settings are immutable after index creation. + * Validates that crypto-related store settings are immutable after index creation. + * Prevents updates to crypto configuration settings and validates store type changes + * to/from cryptofs are not permitted. */ public static void validateCryptoStoreSettings(Settings indexSettings, Index[] indices, ClusterState clusterState) { - final String[] restrictedCryptoSettings = { - "index.store.crypto.key_provider", - "index.store.crypto.kms.key_arn", - "index.store.crypto.kms.encryption_context" }; - // Crypto settings are completely immutable - reject any attempt to modify them - for (String settingKey : restrictedCryptoSettings) { + for (String settingKey : RESTRICTED_CRYPTO_SETTINGS) {
- Enhanced JavaDoc provides clearer documentation of the method's purpose.
610-628: LGTM! Bidirectional cryptofs validation correctly implemented.The logic properly prevents both changing TO cryptofs and changing FROM cryptofs, addressing the concern raised in past review comments. The approach of checking store type changes separately from the other crypto settings is appropriate, as it allows this validation to be granular (only affecting cryptofs) rather than blocking all store type changes.
Minor enhancement: The error messages could be more consistent in explaining the rationale:
// Prevent changing TO cryptofs if ("cryptofs".equals(newStoreType) && !"cryptofs".equals(currentStoreType)) { - throw new IllegalArgumentException("Cannot change store type to 'cryptofs' for index [" + index.getName() + "]"); + throw new IllegalArgumentException( + "Cannot change store type to 'cryptofs' for index [" + index.getName() + "] - cryptofs store type is immutable" + ); }Based on past review comments, the decision to handle store type separately from
restrictedCryptoSettingsis correct, as including it there would prevent all store type changes for all indices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java(2 hunks)
⏰ 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). (20)
- GitHub Check: gradle-check
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- 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 (25, macos-15-intel)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: Analyze (java)
- GitHub Check: detect-breaking-change
🔇 Additional comments (1)
server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java (1)
594-629: Well-structured validation method that successfully enforces crypto setting immutability.The implementation effectively achieves the PR objective by:
- ✅ Preventing updates to crypto configuration settings (key_provider, kms.key_arn, kms.encryption_context)
- ✅ Preventing store type changes to cryptofs from non-cryptofs
- ✅ Preventing store type changes from cryptofs to non-cryptofs
- ✅ Addressing the bidirectional requirement mentioned in past review comments
The validation logic is sound and the error messages are informative.
|
cc @cwperks , could you please review? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20123 +/- ##
============================================
+ Coverage 73.23% 73.26% +0.03%
- Complexity 71953 71977 +24
============================================
Files 5795 5795
Lines 329248 329262 +14
Branches 47410 47415 +5
============================================
+ Hits 241122 241238 +116
+ Misses 68841 68729 -112
- Partials 19285 19295 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java(2 hunks)
⏰ 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). (19)
- GitHub Check: gradle-check
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: Analyze (java)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: detect-breaking-change
🔇 Additional comments (1)
server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java (1)
145-145: Wiring the crypto store validator intoupdateSettingslooks goodCalling
validateCryptoStoreSettingsalongside the other top-level validators (before building open/closed settings and submitting the cluster state update task) is consistent with the existing pattern and ensures invalid store-type transitions are rejected early.
server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java
Show resolved
Hide resolved
|
@RajatGupta02 would it make sense to add any tests here? Is The changes in the PR itself look fine to me. |
Yeah, its available only when ILE is installed. |
|
❌ Gradle check result for c9d2bb9: 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? |
|
This PR is stalled because it has been open for 30 days with no activity. |
RajatGupta02
force-pushed
the
ile_disallow_settings_update
branch
from
c9d2bb9 to
7e2bf8e
Compare
|
❌ Gradle check result for 7e2bf8e: 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? |
server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java
Show resolved
Hide resolved
Signed-off-by: Rajat Gupta <gptrajat@amazon.com>
RajatGupta02
force-pushed
the
ile_disallow_settings_update
branch
from
7a4f96a to
80bb047
Compare
Signed-off-by: Rajat Gupta <gptrajat@amazon.com>
|
❌ Gradle check result for c80fe0f: 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 c80fe0f: 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 c80fe0f: 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 c80fe0f: 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 c80fe0f: 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? |
* Make crypto store settings immutable Signed-off-by: Rajat Gupta <gptrajat@amazon.com> * Update CHANGELOG Signed-off-by: Rajat Gupta <gptrajat@amazon.com> --------- Signed-off-by: Rajat Gupta <gptrajat@amazon.com> Co-authored-by: Rajat Gupta <gptrajat@amazon.com>
* Make crypto store settings immutable Signed-off-by: Rajat Gupta <gptrajat@amazon.com> * Update CHANGELOG Signed-off-by: Rajat Gupta <gptrajat@amazon.com> --------- Signed-off-by: Rajat Gupta <gptrajat@amazon.com> Co-authored-by: Rajat Gupta <gptrajat@amazon.com>
4 participants
Description
Opensearch-storage-encryption plugin introduces some index settings which should be immutable after index creation. This PR adds the validation to not allow these crypto settings update.
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
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.