Skip to content

NAS-139511 / 27.0.0-BETA.1 / Replication form encryption validation improvements#13301

Open
AlexKarpov98 wants to merge 17 commits intomasterfrom
NAS-139511
Open

NAS-139511 / 27.0.0-BETA.1 / Replication form encryption validation improvements#13301
AlexKarpov98 wants to merge 17 commits intomasterfrom
NAS-139511

Conversation

@AlexKarpov98
Copy link
Copy Markdown
Contributor

Testing: see ticket.

Summary

  • Add real-time validation of target dataset encryption state for local targets (checks encryption mismatch, encryption root
    conflicts, readonly policy, and existing children)
  • Show info banners when source dataset is encrypted with "Preserve Dataset Properties" enabled, warning that encryption
    settings are inherited
  • Disable encryption checkbox when source preserves properties (encryption is inherited from source)
  • Validate readonly policy: error for local targets with readonly=off, warning for remote targets
  • Validate "Replication from scratch" when destination already has children
  • Replace plain-text eligible snapshots message with tn-banner component
  • Add spinner on Save while target dataset validation is in progress
  • Add same encryption validation logic to the Replication Wizard (what-and-where step)
  • Remove "Encrypted dataset" / "Unencrypted dataset" hint text from source and destination explorer fields

Test plan

  • Create replication task with encrypted source → unencrypted local destination (should show encryption mismatch error)
  • Create replication task with unencrypted source → encrypted destination that is its own encryption root (should show error)
  • Enable "Preserve Dataset Properties" with encrypted source → encryption checkbox should be disabled
  • Set readonly policy to REQUIRE with non-readonly local destination → should show error
  • Set readonly policy to REQUIRE with remote destination → should show warning (non-blocking)
  • Select local destination with children, leave "Replication from scratch" unchecked → should show error
  • Verify same validations work in the Replication Wizard

Quick Preview:

Screen.Recording.2026-03-05.at.12.20.21.mov

@bugclerk bugclerk changed the title NAS-139511: Replication form encryption validation improvements NAS-139511 / 27.0.0-BETA.1 / Replication form encryption validation improvements Mar 5, 2026
@bugclerk
Copy link
Copy Markdown
Contributor

bugclerk commented Mar 5, 2026

@claude
Copy link
Copy Markdown

claude Bot commented Mar 5, 2026

Claude finished @AlexKarpov98's task in 4m 18s —— View job


Code Review: NAS-139511 / Replication form encryption validation improvements

  • Read review guidelines and gather context
  • Analyze all changed files
  • Check which previous review issues have been addressed
  • Post comprehensive review feedback

Summary

This PR adds real-time validation of target dataset encryption state, readonly policy, and children/allow_from_scratch checks for replication tasks. It also adds equivalent validation to the replication wizard and shows info banners for encrypted sources with preserved properties. The code is well-structured, with a shared replication-encryption-validator.ts utility for consistent validation logic across both the form and wizard.

Previous Review Issues — Resolution Status

The author has addressed the majority of issues from previous rounds. Here's the status:

Issue Status
HEX key rotation on edit (getPayload() always regenerating keys) Fixed — Uses encryption_key_generate flag to preserve existing key on edit (target-section.component.ts:240-247)
Wizard getReplicationPayload missing !data.encryption_inherit guard Fixed — Now properly guards with if (!data.encryption_inherit) (replication-wizard.component.ts:304)
sourcePreservesProperties vs sourceEncryptedWithPreservedProperties mismatch Fixed — Now uses sourceEncryptedWithPreservedProperties throughout (replication-form.component.html:29, target-section.component.ts:132)
.some() vs .every() inconsistency for source encryption check Fixed — Form now uses .every() matching the wizard (replication-form.component.ts:422)
Missing Validators.required on encryption_key_passphrase Fixed — Added Validators.required (target-section.component.ts:75)
Next button not checking validatingEncryption Fixed — Template now has [disabled]="form.invalid || validatingEncryption" (replication-what-and-where.component.html:183)
Missing updateEncryptionFields() call at end of ngOnInit Fixed — Added at line 196
Encryption enable/disable running on every ngOnChanges Fixed — Guarded with 'sourceEncryptedWithPreservedProperties' in changes
distinctUntilChanged() blocking re-fetch on isLocalTarget transition Fixed — Removed distinctUntilChanged(), uses startWith() instead
Regex conflict error missing removable: false Fixed — Added removable: false (source-section.component.ts:248)
Missing initial validateRegexConflict() call Fixed — Added after subscription (source-section.component.ts:235)
Retention policy lifetime fields disabled in edit mode Fixed — Initial check added (target-section.component.ts:175-178)
Dead helptext exports Fixed — Removed from replication-wizard.ts and replication.ts
Dead encryption_key_generate/encryption_key_hex in ReplicationWizardData interface Fixed — Removed from interface
pushMissingSnapshotConfig/pullMissingNamingConfig not checking schema_or_regex mode Fixed — Now uses this.usesNameRegex to check mode
Unnecessary API calls for remote targets in listenForTargetDatasetValidation Fixed — Guard now checks !this.isLocalTarget() at line 294

Remaining Items

The code is in good shape overall. I found a few remaining items worth noting:

1. Minor: isLocalTarget transition with same target_dataset value

In target-section.component.ts:143-146, when isLocalTarget transitions from false to true, the code calls updateValueAndValidity() to trigger a re-fetch. Since distinctUntilChanged() was removed (good), this should now work correctly with the startWith() approach. However, startWith() only fires once when the subscription is created in ngOnInit. The updateValueAndValidity() will re-emit the current value through valueChanges, which should properly trigger the switchMap. This looks correct now.

2. Minor: encryption_key_hex form control still has exactLength(64) but no Validators.required (line 74)

While the wizard has [Validators.required, exactLength(64)] at its equivalent field, the form has only [exactLength(64)]. In practice this doesn't matter because getPayload() auto-generates the key for HEX format (or uses the existing one on edit), but the inconsistency is worth noting for completeness.

3. Test coverage

The test files are comprehensive and cover the key scenarios including:

  • Encryption mismatch validation (both encrypted→unencrypted and vice versa)
  • Encryption root validation
  • Readonly policy validation
  • Children/allow_from_scratch validation
  • Source encrypted with preserved properties
  • Edit mode encryption error detection

The wizard tests similarly cover encryption validation for local destinations.

Code Quality Notes

  • The shared replication-encryption-validator.ts utility is well-designed, keeping encryption validation logic DRY between the form and wizard.
  • The formValuesApplied guard in ngOnChanges is a clean solution to prevent re-applying values on subsequent change detection cycles.
  • The startWith() approach in listenForTargetDatasetValidation() is the correct Angular pattern for ensuring initial values are processed.
  • The updateEncryptionFields() call at the end of ngOnInit properly addresses the lifecycle timing issue where ngOnChanges fires before subscribers are set up.
  • Error messages are clear and actionable.

Verdict

The PR is in good shape. All previously identified bugs have been addressed. The remaining items are minor inconsistencies that don't affect functionality. The validation logic is comprehensive and well-tested.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 5, 2026

Codecov Report

❌ Patch coverage is 93.58289% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.69%. Comparing base (488facb) to head (1cb177e).
⚠️ Report is 44 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...replication-wizard/replication-wizard.component.ts 0.00% 10 Missing ⚠️
...ion/replication-form/replication-form.component.ts 94.11% 4 Missing ⚠️
...ections/target-section/target-section.component.ts 96.99% 4 Missing ⚠️
...ections/source-section/source-section.component.ts 92.50% 3 Missing ⚠️
...-and-where/replication-what-and-where.component.ts 96.80% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #13301      +/-   ##
==========================================
+ Coverage   86.64%   86.69%   +0.04%     
==========================================
  Files        1872     1873       +1     
  Lines       72026    72335     +309     
  Branches     8401     8479      +78     
==========================================
+ Hits        62406    62708     +302     
- Misses       9620     9627       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@AlexKarpov98 AlexKarpov98 marked this pull request as ready for review March 5, 2026 12:47
@AlexKarpov98 AlexKarpov98 requested a review from a team as a code owner March 5, 2026 12:47
@AlexKarpov98 AlexKarpov98 requested review from aervin, mattwyatt-ix and william-gr and removed request for a team March 5, 2026 12:47
@AlexKarpov98 AlexKarpov98 self-assigned this Mar 5, 2026
Comment thread src/app/helptext/data-protection/replication/replication-wizard.ts Outdated
@aervin aervin removed the request for review from mattwyatt-ix March 11, 2026 17:41
@william-gr william-gr removed their request for review March 25, 2026 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants