fix(hubs): make ADF trigger startTime Z-suffix conditional on UTC fallback - #2291
fix(hubs): make ADF trigger startTime Z-suffix conditional on UTC fallback#2291Michael Flanakin (flanakin) wants to merge 8 commits into
Conversation
…nifest (#2260) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…storage in private mode (#2259) Private-mode deployments failed at the first deployment script with DeploymentScriptStorageAccountWithServiceEndpointEnabled, because the script storage account was reached through a Storage service endpoint while private subnets set defaultOutboundAccess to false. Azure Policy 7bca8353-aa3b-429b-904a-9229c4385837 ("Subnets should be private") requires defaultOutboundAccess to be present and false, so that setting has to stay. The service endpoint is the optional half of the conflict and is replaced here with the documented Azure Files private endpoint path: - Private subnets keep defaultOutboundAccess: false; the NAT Gateway continues to provide egress for the script and Data Explorer subnets. - The Storage service endpoint is removed from the script subnet. - The script storage account moves to publicNetworkAccess: Disabled with no virtual network rules, reached through a file private endpoint and a privatelink.file.core.windows.net zone. - HubsPrivateNetworking.Tests.ps1 pins all of the above in the compiled ARM. - Stale defaultOutboundAccess wording removed from six user-facing strings. Validated live in both affected modes (storage-only, westus): deployment succeeds, all 7 deployment scripts execute, all three subnets carry defaultOutboundAccess: false in private mode, the script storage has no virtual network rules and no public access, and the file private DNS zone is present. Fixes #2258
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: MSBrett <MSBrett@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ates (#2261) Co-authored-by: Roland Krummenacher <roland.krummenacher@alescent.com> Co-authored-by: MSBrett <MSBrett@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
… type (#2251) Co-authored-by: Roland Krummenacher <roland.krummenacher@alescent.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: MSBrett <MSBrett@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…lback FinOps hub deployments to regions not covered by timeZones.bicep's region-to-Windows-timezone map (e.g. Sweden Central) fail with InvalidWorkflowTriggerRecurrence. Data Factory requires startTime to end in 'Z' when timeZone resolves to 'UTC', but the three schedule triggers (config_DailySchedule, config_MonthlySchedule, queries_DailySchedule) hardcoded startTime without a trailing 'Z', which is only valid when timeZone is non-UTC. Emit the 'Z' suffix only when timeZones.outputs.Timezone resolves to the 'UTC' fallback, so mapped, non-UTC regions keep scheduling on local wall-clock time as before. Fixes #2157 Note: the reporter also saw a failure in Norway East, which IS in the timezone map (non-UTC), so this fix does not explain or address that - it needs separate diagnostic info and is treated as a distinct, unverified issue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Roland Krummenacher (RolandKrummenacher)
left a comment
There was a problem hiding this comment.
The Bicep change is correct and I would approve it as written — the reasoning in the PR description holds up under checking:
timeZones.bicep:63falls back to'UTC'viatimezoneobject[?loc] ?? 'UTC', andswedencentralis genuinely absent from the map, so the reported region does land on the fallback.- The ternary keeps the no-
Zform for all 36 mapped, non-UTC regions, so no currently-working region changes scheduling behavior. ARMequals()is case-sensitive and no map value is'UTC'in any casing, so the condition can only match the fallback. - All three
startTimeoccurrences undersrc/templates/are covered — there are no others. - Declining to unconditionally append
Z, and declining to grow the region map, are both the right calls for this PR.
I did not independently run bicep build, but the change is a plain ternary over two string literals in a property that already references timeZones.outputs.Timezone, so I'm not worried about the compiled output.
What blocks: the shipped deploy templates still reproduce #2157.
docs/deploy/*.json are regenerated only at release time, so on the flanakin/v15-prep base as it stands today:
| artifact | startTime |
swedencentral |
fallback |
|---|---|---|---|
finops-hub-latest.json |
no Z (3x) |
absent | 'UTC' |
finops-hub-14.0.json |
no Z (3x) |
absent | 'UTC' |
finops-hub-preview.json |
no Z (2x) |
absent | 'Universal Coordinated Time' |
Both latest and 14.0 already emit the 'UTC' fallback and have no swedencentral entry, and their startTime values have no Z — which is exactly the failing combination. So after this merges, anyone hitting the Deploy to Azure button in Sweden Central still gets InvalidWorkflowTriggerRecurrence, and the issue this PR closes remains reproducible through the primary deployment path until v15 ships.
That is the same situation as 91ce5a5e, which patched these artifacts surgically for this defect class in these same files, for the same triggers, on the grounds that they lag the src fixes. I would rather not close #2157 with the button still broken — either patch the three literals in latest and 14.0 here, or leave #2157 open with a note that the shipped-artifact half is outstanding.
finops-hub-preview.json has a second problem, pre-existing on the base and not caused by this PR: its fallback is still the display name 'Universal Coordinated Time', which ADF rejects outright — 91ce5a5e corrected the mapping values in that file but not the fallback. The Fallback context in HubsAdfTriggerTimeZones.Tests.ps1:139 only asserts against $bicepContent, so this passes CI. Since v15-prep users deploy from the preview artifact, and it is the same fallback path this PR is fixing, it seems in scope to fix here — your call.
Non-blocking:
- No changelog entry under
Unreleased > FinOps hubs > Fixed. The recent fix commits ondeveach add one. - No regression test pins the startTime/
Zinvariant — there are zerostartTimehits acrosssrc/powershell/Testsandsrc/scripts, so a template regeneration or refactor can silently drop this with CI green.91ce5a5eextendedHubsAdfTriggerTimeZones.Tests.ps1for exactly this reason; a case in itsFallbackcontext would cover it. See the inline comment.
On Norway East: your out-of-scope reasoning is right. norwayeast maps to 'W. Europe Standard Time' in the src Bicep, in finops-hub-latest.json, and in finops-hub-14.0.json, so this root cause does not explain the reporter's Norway East error, which named time zone 'UTC'. Agreed that needs separate diagnostics.
| // fallback for regions not in timeZones.bicep's map); a missing 'Z' fails trigger activation | ||
| // with InvalidWorkflowTriggerRecurrence. Mapped, non-UTC regions must keep the no-'Z' format | ||
| // so they continue scheduling on local wall-clock time. | ||
| startTime: timeZones.outputs.Timezone == 'UTC' ? '2023-01-01T01:01:00Z' : '2023-01-01T01:01:00' |
There was a problem hiding this comment.
The conditional and the comment are both right, and I like that the second occurrence points back here rather than repeating the rationale.
Non-blocking suggestion: pin this in HubsAdfTriggerTimeZones.Tests.ps1. Its Fallback context already asserts the ?? 'UTC' fallback against the Bicep source, so the natural companion is an assertion that every startTime in these two files carries the Z on the UTC branch and omits it on the other — something to the effect of every startTime: line matching Timezone == 'UTC' ? '<ts>Z' : '<ts>'.
Without it, the invariant lives only in a comment, and 91ce5a5e is the precedent for extending this specific test file so the fix cannot silently regress. If you extend it to the docs/deploy artifacts the way that commit did for the mappings, it also catches the shipped-template drift I raised in the review body.
|
@Michael Flanakin (@flanakin): you have some new feedback! Please review and resolve all comments and I'll let reviewers know by removing the |
Summary
Fixes #2157 — FinOps hub deployments fail in regions not covered by
timeZones.bicep's region→Windows-timezone map (e.g. Sweden Central) withInvalidWorkflowTriggerRecurrence.Root cause: Three ADF schedule triggers hardcode
startTimewithout a trailingZ(UTC zone designator):config_DailyScheduleandqueries_DailySchedule:'2023-01-01T01:01:00'config_MonthlySchedule:'2023-01-05T01:11:00'These triggers set
timeZonefromtimeZones.bicep's region lookup, which falls back to the literal'UTC'for any region not in its explicit map (confirmed: Sweden Central is one such region; likely also Qatar Central, Poland Central, Italy North, Israel Central, though the map should be checked directly rather than assuming this list is exhaustive). Azure Data Factory requiresstartTimeto end inZ(formatyyyy-MM-ddTHH:mm:ssZ) whenevertimeZoneresolves to'UTC'— otherwise trigger activation fails withInvalidWorkflowTriggerRecurrence, which surfaces during deployment asInit-DataFactory.ps1'sStart-AzDataFactoryV2Triggerretrying up toMAX_RETRIES = 20and then failing the whole deployment.Fix: Make the
Zsuffix conditional on the resolvedtimeZoneactually being'UTC':Applied to all three triggers:
src/templates/finops-hub/modules/Microsoft.CostManagement/ManagedExports/app.bicep—config_DailyScheduleandconfig_MonthlySchedulesrc/templates/finops-hub/modules/Microsoft.FinOpsHubs/IngestionQueries/app.bicep—queries_DailyScheduleThis preserves the existing local-wall-clock scheduling behavior for every region that is mapped (no
Z, non-UTCtimeZone), and only addsZfor the fallback case, fixing activation there.Explicitly not done, and why:
Zto all three literals — that would silently switch every mapped region from local-time scheduling to UTC wall-clock scheduling, a user-facing behavior change for currently-working regions.Out of scope: The issue reporter also mentioned a failure in Norway East. Norway East is present in
timeZones.bicep's map ('W. Europe Standard Time', non-UTC), so the root cause above does not explain a Norway East failure. This looks like a second, distinct issue that needs more diagnostic detail from the reporter — it is not addressed by this PR.Test plan
bicep buildon both changed files completes cleanly (only the expected@validate()experimental-feature warning, unrelated to this change)if(equals(...Timezone..., 'UTC'), '...Z', '...')expression for all threestartTimepropertiespwsh -Command "./src/scripts/Test-PowerShell.ps1 -Lint"— 3585/3585 tests passedInvalidWorkflowTriggerRecurrence🤖 Generated with Claude Code