Repository navigation
feat(finops-hub): add Azure Data Explorer encryption options - #2338
Conversation
microsoft-github-policy-service
Bot
added
the
Needs: Review 👀
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Adds configurable Azure Data Explorer (ADX) disk and double encryption options to the FinOps Hub Bicep templates and wires them through module boundaries to the ADX (Kusto) cluster resource.
Changes:
- Introduced
enableAdxDiskEncryptionandenableAdxDoubleEncryptionparameters (defaultfalse) inmain.bicep,modules/hub.bicep, andAnalytics/app.bicep. - Applied both flags to the
Microsoft.Kusto/clustersresource properties. - Added a test deployment scenario enabling both encryption options and updated the toolkit changelog.
| File | Description |
|---|---|
| src/templates/finops-hub/test/main.test.bicep | Adds a second test module deployment enabling both ADX encryption flags. |
| src/templates/finops-hub/modules/hub.bicep | Adds params and forwards them into the Analytics module. |
| src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Analytics/app.bicep | Adds params and sets Kusto cluster encryption properties based on them. |
| src/templates/finops-hub/main.bicep | Adds top-level params and passes them to the hub module. |
| docs-mslearn/toolkit/changelog.md | Notes the new ADX encryption options in the changelog. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| enableStreamingIngest: true | ||
| enableDiskEncryption: enableAdxDiskEncryption | ||
| enableDoubleEncryption: enableAdxDoubleEncryption | ||
| enableAutoStop: false |
There was a problem hiding this comment.
Updated in f12bf19. The encryption properties are now conditionally added only when the corresponding option is enabled, so the default false values do not emit enableDiskEncryption or enableDoubleEncryption during redeployment. This follows the existing conditional property pattern used for storage infrastructure encryption.
|
@microsoft-github-policy-service agree |
| @description('Optional. Enable disk encryption on the Azure Data Explorer cluster. Can only be enabled during cluster creation. Default: false.') | ||
| param enableAdxDiskEncryption bool = false | ||
|
|
||
| @description('Optional. Enable double encryption on the Azure Data Explorer cluster. Can only be enabled during cluster creation. Default: false.') | ||
| param enableAdxDoubleEncryption bool = false |
There was a problem hiding this comment.
|
|
||
| - **Added** | ||
| - Added VNet and private network modes, including opt-in NAT Gateway support for private mode; NAT Gateway incurs additional cost when enabled ([#2163](https://github.com/microsoft/finops-toolkit/pull/2163)). | ||
| - Added Azure Data Explorer disk and double encryption options for FinOps hub deployments ([#2196](https://github.com/microsoft/finops-toolkit/issues/2196)). |
There was a problem hiding this comment.
Updated in f12bf19. Changed ms.date to 10/07/2026 to match the current changelog update.
Michael Flanakin (flanakin)
left a comment
Michael Flanakin (flanakin)
left a comment
There was a problem hiding this comment.
🤖 [AI][Claude] PR Review
Summary: Straightforward addition of two ADX encryption toggles, wired through all three Bicep layers with a test deployment and changelog entry. Existing Copilot review already caught the three biggest issues (immutable-property default risk, missing portal/createUiDefinition support, and the changelog ms.date not being bumped), so this review focuses on two naming/accuracy nits that weren't yet flagged.
⚠️ Should fix (1)
- New parameter names (
enableAdxDiskEncryption/enableAdxDoubleEncryption) break from the establisheddataExplorer*naming convention used by every other related parameter in these files, and the shared description text overstates disk encryption's immutability.
No blockers or suggestions beyond this from an independent pass — bicep build on main.bicep succeeds with no new warnings/errors introduced by this change.
| @description('Optional. Enable disk encryption on the Azure Data Explorer cluster. Can only be enabled during cluster creation. Default: false.') | ||
| param enableAdxDiskEncryption bool = false | ||
|
|
||
| @description('Optional. Enable double encryption on the Azure Data Explorer cluster. Can only be enabled during cluster creation. Default: false.') | ||
| param enableAdxDoubleEncryption bool = false |
There was a problem hiding this comment.
🤖 [AI][Claude]
Two small issues with these two new parameters (same pattern repeats in modules/hub.bicep and modules/Microsoft.FinOpsHubs/Analytics/app.bicep):
- Naming convention: every other Data Explorer parameter in this template uses the
dataExplorerprefix (dataExplorerName,dataExplorerSku,dataExplorerCapacity,dataExplorerRawRetentionInDays,dataExplorerFinalRetentionInMonths).enableAdxDiskEncryption/enableAdxDoubleEncryptionintroduce a newAdxabbreviation that doesn't appear anywhere else in the codebase. ConsiderenableDataExplorerDiskEncryption/enableDataExplorerDoubleEncryptionfor consistency. - Description accuracy: per the Azure Data Explorer disk encryption docs, disk encryption can be toggled after cluster creation (it causes up to 20 minutes of unavailability while it applies) — only double encryption is actually create-time-only and irreversible once enabled (double encryption docs). "Can only be enabled during cluster creation" is accurate for
enableAdxDoubleEncryptionbut misleading forenableAdxDiskEncryption.
There was a problem hiding this comment.
Updated in 96bb058. Renamed the parameters to enableDataExplorerDiskEncryption and enableDataExplorerDoubleEncryption across the template, modules, portal definition, and test to match the existing Data Explorer naming convention. I also removed the creation-only wording from the disk encryption description and tooltip while keeping it for double encryption.
Signed-off-by: Chaithanya Nallamothu <chaithanyan5177@gmail.com>
Signed-off-by: Chaithanya Nallamothu <chaithanyan5177@gmail.com>
|
Hi Michael Flanakin (@flanakin) Brett Wilson (@MSBrett) , I've addressed the earlier review feedback and merged the latest dev branch to resolve the changelog conflict. The PR diff remains focused on the Azure Data Explorer encryption changes. Please let me know if anything else needs adjustment. Thanks! |
Michael Flanakin (flanakin)
left a comment
Michael Flanakin (flanakin)
left a comment
There was a problem hiding this comment.
🤖 [AI][Claude] PR Review
Summary: Prior feedback is addressed (renamed params, emit-only-when-true avoids touching immutable properties, createUiDefinition wired). Remaining items:
⚠️ Should fix (2)
- Docs missing. The new parameters aren't in the hub template parameter table (
docs-mslearn/toolkit/hubs/template.md, neardataExplorerName), and the checklist marks docs as not needed. Public docs are required fordevPRs; add both parameters, noting double encryption is create-time only. - Merge conflict with
dev. The PR is currently CONFLICTING. Mergeorigin/dev(merge commit, no rebase), keep both changelog entries, and setms.dateindocs-mslearn/toolkit/changelog.mdto the merge date since the body changed.
# Conflicts: # docs-mslearn/toolkit/changelog.md # src/templates/finops-hub/createUiDefinition.json
|
Updated the FinOps hub template documentation with both ADX encryption parameters and merged the latest upstream Resolved the changelog and deployment UI conflicts, keeping the updated upstream retention logic intact. Also updated the documentation dates. |
🛠️ Description
Adds Azure Data Explorer disk and double encryption options to FinOps Hub.
enableDataExplorerDiskEncryptionandenableDataExplorerDoubleEncryption, both disabled by default.Fixes #2196
📷 Screenshots
Not applicable. This change only updates the FinOps Hub deployment templates.
📋 Checklist
🔬 How did you test this change?
Validated the updated Bicep templates locally and successfully ran
Build-Toolkit finops-hub.📦 Deploy to test?
🙋♀️ Do any of the following that apply?
📑 Did you update
docs/changelog.md?📖 Did you update documentation?