Gate external-principal indexing behind an opt-in sync option - #1094
Gate external-principal indexing behind an opt-in sync option#1094johnallers wants to merge 4 commits into
Conversation
PR #1046 replaced the per-grant linear scan of external principals in processGrantsWithExternalPrincipals with an indexed lookup. The scan was O(grants x principals) and did not finish at all on a tenant with ~99k externally matched grants, so the index is the right fix -- but it is new code in security-relevant grant-matching logic that would otherwise become mandatory for every live tenant the moment its connector bumps this SDK. Make the indexed path opt-in via WithExternalPrincipalIndex(bool), defaulting to the pre-#1046 linear scan, so the fix can be rolled out per tenant behind a downstream feature flag and rolled back without an SDK downgrade. This is a kill switch, not a revert: the index and its tests are untouched, and the flag is expected to reach 100% and then be removed. Only the "match by key/val" branch is forked, into matchExternalPrincipalsLegacy and matchExternalPrincipalsIndexed. MatchAll, MatchByID, the delete/put loop, the ctx checks and #1046's progress logging are shared and unchanged, and the index is not built at all when the legacy matcher is selected. Equivalence between the two paths is the entire premise of the flag, so it is asserted directly: TestExternalPrincipalMatchLegacyIndexedParity runs one fixture -- users matched by profile key, by user-trait email, by both at once, a principal with an unreadable user trait, group profile matches with and without a remappable expandable entitlement, an unmatched key, an unconfigured trait, plus MatchAll/MatchByID carriers -- through both matchers on both storage engines and requires byte-identical grants. The option is threaded through tasks/local, connectorrunner and the CLI (--external-principal-index / BATON_EXTERNAL_PRINCIPAL_INDEX) so connector binaries built on the SDK get the same switch. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| local.WithTargetedSyncResources(resources), | ||
| local.WithSkipEntitlementsAndGrants(cfg.skipEntitlementsAndGrants), | ||
| local.WithSkipGrants(cfg.skipGrants), | ||
| local.WithExternalPrincipalIndex(cfg.externalPrincipalIndex), |
There was a problem hiding this comment.
🟠 Bug: cfg.externalPrincipalIndex is only consumed here, on the one-shot/local.NewSyncer branch. The service-mode branch below (c1api.NewC1TaskManager, runner.go:1150) never receives it, and pkg/tasks/c1api/full_sync.go never appends sdkSync.WithExternalPrincipalIndex, so --external-principal-index / BATON_EXTERNAL_PRINCIPAL_INDEX is a silent no-op for platform-run syncs — no error, no warning, and the indexed=false log line is the only clue.
That is the deployment mode the index was built for: the ~99k-grant × ~93k-principal tenant from #1046 that blew the sync deadline runs under the platform, not as a one-shot CLI invocation. As written this PR makes the O(grants × principals) scan the default everywhere and leaves the platform with no way to opt a tenant back onto the fast path, so the #1046 fix becomes unreachable where it mattered — which is the opposite of the PR's stated "let the platform enable the index per tenant" rollout plan.
Either thread the flag into NewC1TaskManager/full_sync.go (as keepPreviousSyncC1Z is threaded), or land the platform-side toggle first and say so in the PR.
| func parityPrincipals(t *testing.T) []*v2.Resource { | ||
| t.Helper() | ||
| return []*v2.Resource{ | ||
| // Matches "upn"/target@example.com, differing in case. |
There was a problem hiding this comment.
🟡 Suggestion: every value in this fixture is ASCII, so parity is only ever asserted over inputs where foldKey bucketing and strings.EqualFold trivially agree. The interesting divergences the index was designed around — final/medial sigma, long s, dotted capital I — are exactly where the two matchers could disagree, and they're never run through this comparison. external_match_folding_test.go used to cover them at the processGrantsWithExternalPrincipals seam, but it builds &syncer{store, state} without the flag, so post-PR it only exercises the legacy scan.
Adding a couple of the non-ASCII cases from externalMatchFoldCases() to parityPrincipals/parityCarrierGrants would make this test the actual proof it claims to be.
General PR Review: Gate external-principal indexing behind an opt-in sync optionBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. Both prior findings are addressed: the Risk triage (per
Security IssuesNone found. The restored legacy log path records only Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Three gaps found in review of the kill-switch change: 1. The option never reached service mode. runner.go threaded externalPrincipalIndex into local.NewSyncer only, so a daemon-mode connector (c1api.NewC1TaskManager) setting BATON_EXTERNAL_PRINCIPAL_INDEX kept silently running the linear scan -- the exact deployment mode the incident behind #1046 happened in, which made the flag useless where it matters most. Plumbed through NewC1TaskManager, c1ApiTaskManager, newFullSyncTaskHandler and fullSyncTaskHandler.sync, mirroring externalResourceTraits hop for hop. All four SyncOpt assembly sites in the tree now carry it. 2. external_match_folding_test.go built a bare &syncer{}, so defaulting the flag to false silently moved TestExternalResourceMatch*FoldingContract off the indexed matcher they were written to pin. The linear scan satisfies those cases for free by calling strings.EqualFold directly, so the index's bucketing -- where a fold can actually be gotten wrong -- was left unpinned. Both contracts and the one-grant-per-principal test now run in both modes. Verified by mutation: bucketing on strings.ToLower fails greek-final-sigma, long-s and dotted-capital-I in indexed=true only. 3. The parity suite could not see a lost dedup, because two grants for one principal share a generated grant id and PutGrants upserts them into one row before the digest is read. Added a test asserting on the grants the matchers RETURN. Verified by mutation: dropping the email-match continue in the legacy scan now fails. Also added a test pinning the option wiring itself: NewSyncer with no options is the linear scan, WithExternalPrincipalIndex(true) is the indexed path. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| @@ -207,6 +211,10 @@ func (c *fullSyncTaskHandler) sync(ctx context.Context, c1zPath string) error { | |||
| syncOpts = append(syncOpts, sdkSync.WithExternalResourceTraits(c.externalResourceTraits...)) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
🟡 Suggestion: this hop is the defect the commit fixes, and nothing asserts it. TestWithExternalPrincipalIndexOptionWiring pins only SyncOpt → syncer.externalPrincipalIndexEnabled, which was never the broken hop; the silent no-op lived in runner.go → NewC1TaskManager → newFullSyncTaskHandler → this syncOpts assembly, and deleting any of those four lines still leaves the whole suite green. Consider a test that builds a fullSyncTaskHandler{externalPrincipalIndex: true} and asserts the assembled syncOpts apply to a *syncer with the field set — the same shape would also cover externalResourceTraits, which has no threading test either.
| externalC1Z string, | ||
| externalResourceEntitlementIdFilter string, | ||
| externalResourceTraits []v2.ResourceType_Trait, | ||
| externalPrincipalIndex bool, |
There was a problem hiding this comment.
🟡 Suggestion: NewC1TaskManager is exported, so inserting a positional bool in the middle of its parameter list is a compile break for any out-of-repo caller, and it makes the 16-argument list one bool harder to call correctly (skipFullSync and keepPreviousSyncC1Z are already positional bools). The break is loud rather than silent, and it matches how externalResourceTraits was threaded, so this isn't blocking — but per the repo's SDK-compatibility criteria it's worth either moving this constructor to a variadic-option/config-struct shape or calling the signature change out alongside a pkg/sdk/version.go bump (still v0.24.1 on this branch) so downstreams get a version signal for it plus the default-behavior flip.
1. The service-mode fix had no regression test: deleting any of its four plumbing lines left the suite green, which is how the path got dropped in the first place. Split the two hand-offs that carry configuration through positional argument lists into methods -- c1ApiTaskManager.newFullSyncHandler and fullSyncTaskHandler.syncOpts -- and asserted them by running the assembled options through a real sync engine and reading the field they set. The options are opaque closures and the fields they set are unexported, so the assertion reads them reflectively; a SyncOpt cannot be identified or applied from outside pkg/sync, making this the only way to prove an option survived the plumbing rather than merely that some option was appended. (Comparing option code pointers does not work: reflect gives a closure instance address, so two calls of one constructor already differ.) Verified by mutation: dropping the append in syncOpts, hardcoding the handler field to false, and passing false from the manager each fail. Covers three of the four hops; connectorrunner -> NewC1TaskManager still needs a live service client to reach, and is noted on the PR. 2. The verification-tagged crash-cut / resume / same-checkpoint-twice cell built bare syncers, so post-default-flip it only replayed the legacy scan -- the indexed path, the one being rolled out, was never resumed by any test. Parameterized over both matchers with the existing helper. Both modes agree on every batch size the cell pins. pkg/sdk/version.go is deliberately untouched: bumps in this repo are made by release automation in standalone commits. The release this lands in needs a minor bump rather than a patch -- exported signature break plus a default behavior change -- which is called out in the PR description instead. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| // externalResourceTraits reaches the engine through the same chain and had the | ||
| // same untested gap; it is cheap to cover here alongside. | ||
| // The manager -> handler hop is the other half of the chain, and the one that | ||
| // silently dropped the value the first time. It is asserted here rather than | ||
| // through Process(), which needs a live task queue and service client. | ||
| func TestC1TaskManagerThreadsExternalPrincipalIndexToHandler(t *testing.T) { |
There was a problem hiding this comment.
🟡 Suggestion: the first two lines of this comment describe externalResourceTraits, but they sit on TestC1TaskManagerThreadsExternalPrincipalIndexToHandler, which never touches traits. They belong on TestFullSyncTaskHandlerThreadsExternalResourceTraits (line 159), which is currently undocumented. Confidence: high (cosmetic).
| // externalResourceTraits reaches the engine through the same chain and had the | |
| // same untested gap; it is cheap to cover here alongside. | |
| // The manager -> handler hop is the other half of the chain, and the one that | |
| // silently dropped the value the first time. It is asserted here rather than | |
| // through Process(), which needs a live task queue and service client. | |
| func TestC1TaskManagerThreadsExternalPrincipalIndexToHandler(t *testing.T) { | |
| // The manager -> handler hop is the other half of the chain, and the one that | |
| // silently dropped the value the first time. It is asserted here rather than | |
| // through Process(), which needs a live task queue and service client. | |
| func TestC1TaskManagerThreadsExternalPrincipalIndexToHandler(t *testing.T) { |
| local.WithTargetedSyncResources(resources), | ||
| local.WithSkipEntitlementsAndGrants(cfg.skipEntitlementsAndGrants), | ||
| local.WithSkipGrants(cfg.skipGrants), | ||
| local.WithExternalPrincipalIndex(cfg.externalPrincipalIndex), |
There was a problem hiding this comment.
🟡 Suggestion: the new commit closes the service-mode plumbing gap (c1ApiTaskManager → handler → syncOpts → engine), but the local path that the new --external-principal-index CLI flag actually drives is still untested end to end: cli → connectorrunner.WithExternalPrincipalIndex → cfg.externalPrincipalIndex → this local.WithExternalPrincipalIndex → sdkSync.WithExternalPrincipalIndex. pkg/tasks/local has no syncer test and runner_test.go has no option-wiring test, so a value dropped on this branch would silently sync with the linear scan while the flag reads as on — the same silent-misconfiguration failure the c1api tests were added for. A small test asserting localSyncer.externalPrincipalIndex reaches the engine would cover it. Confidence: high (test gap, not a defect).
The local task path carries configuration to the sync engine through the same shape service mode does -- CLI flag, runner Option, task field, SyncOpt -- and had none of the coverage service mode just gained. Split localSyncer.syncOpts out of Process the way fullSyncTaskHandler.syncOpts was split, and asserted the chain by running the assembled options through a real engine and reading the field they set. Added the runner half too: WithExternalPrincipalIndex must reach runnerConfig. Verified by mutation: dropping the option from localSyncer.syncOpts, dropping the value in local.WithExternalPrincipalIndex, and dropping it in connectorrunner.WithExternalPrincipalIndex each fail. The one hop still not covered either way is NewConnectorRunner handing cfg to a task manager, which needs a live connector wrapper to construct. Also moved a doc comment in full_sync_opts_test.go that described the externalResourceTraits test but sat above the manager hand-off test. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| // matchTraits. The guard in the TRAIT_USER case above is not | ||
| // the same situation -- that case fires on the trait named by | ||
| // the grant, which need not be a configured match trait. | ||
| idx := indexByTrait[trait] |
There was a problem hiding this comment.
🟡 Suggestion: The TRAIT_USER arm above nil-checks idx, but this arm does not, and the invariant that justifies it (indexByTrait is populated for every key of matchTraits) now lives in a different function than the deref. Before this PR both maps were locals in one function body; now they are independent parameters, so any future caller that builds them asymmetrically panics here rather than matching nothing. The new TestExternalPrincipalMatchDedupesAtTheMatcher already constructs exactly that pair (matchTraits has USER+GROUP, indexByTrait has only USER) — it just never passes a GROUP match. A three-line if idx == nil { break } would make this arm self-contained the way the one above it is.
| WithC1ZPath(internalC1zpath), | ||
| WithTmpDir(tempDir), | ||
| WithExternalResourceC1ZPath(externalC1zpath), | ||
| WithExternalPrincipalIndex(true), |
There was a problem hiding this comment.
🟡 Suggestion: Nothing in this test is actually indexed-specific — it is a TRAIT_USER profile-key match on userPrincipalName, which the legacy arm handles through the same GetProfileStringValue + strings.EqualFold comparison, so it should pass in both modes. Pinning it to WithExternalPrincipalIndex(true) therefore moves the only end-to-end Sync() coverage of a user profile-key match (through a real external-c1z reader, under runWithSyncModes) off the path this PR ships as the default. The parity fixture asserts the same shape, but at the processGrantsWithExternalPrincipals level rather than end-to-end. Consider running it in both modes instead, the way external_match_folding_test.go and the verification cell were parameterized in this same PR.
1 participant
What
Makes the indexed external-principal matcher added in #1046 opt-in, and restores the pre-#1046 linear scan as the default.
New sync option:
Threaded through the layers standalone connector binaries go through, so they get the same switch for free:
local.WithExternalPrincipalIndex(bool) Option(pkg/tasks/local) — one-shot / on-demandc1api.NewC1TaskManager→newFullSyncTaskHandler→fullSyncTaskHandler.sync— service / daemon modeconnectorrunner.WithExternalPrincipalIndex(bool) Option— feeds both of the above--external-principal-index/BATON_EXTERNAL_PRINCIPAL_INDEX(field.ExternalPrincipalIndexField, default false)All four places in the tree that assemble
sdkSync.SyncOpts carry the flag.Why
#1046 is a real fix: the old matcher was O(grants × principals) proto unmarshals and a tenant with ~99k externally matched grants never finished the step inside the sync deadline. Nothing here questions that, and nothing in
pkg/sync/external_principal_index.gois modified.The concern is blast radius, not correctness. As merged, the new grant-matching path becomes mandatory for every tenant the moment its connector bumps its baton-sdk dependency —
baton-sharepoint,baton-aks,baton-eksandbaton-google-cloud-platformall use this feature today. A behavior change in grant matching is an access-correctness change, so it wants a per-tenant rollout and a rollback that doesn't require an SDK downgrade and a connector re-release.So: default to the behavior every tenant is running today, let the platform enable the index per tenant, and remove the flag once it's at 100%. This is a kill switch, not a revert.
What changed
Only the "match by key/val" branch of
processGrantsWithExternalPrincipalsis forked, into two methods:matchExternalPrincipalsLegacy— the pre-[CXP-498] Index external principals for grant matching #1046 loop (including thematchProfileAndExpandLegacyvariant that re-checks the profile inline, and the restoreduserTraitContainsEmailhelper).matchExternalPrincipalsIndexed— [CXP-498] Index external principals for grant matching #1046's logic, lifted out of the loop body unchanged.Everything else is shared and untouched: setup,
ExternalResourceMatchAll,ExternalResourceMatchID, the expandable-entitlement map, the delete/put loop. #1046's observability additions (progress log interval, the info/debug lines) are orthogonal and stay active in both modes, with an addedindexedfield so logs say which matcher ran.The per-trait index is only built when the indexed matcher is selected — indexing every principal is exactly the work the legacy path exists to avoid.
One deliberate deviation from a verbatim restore: the legacy path's "error getting user trait" log records the principal's ids rather than the whole resource. Logging the resource serializes its profile, which is where a directory keeps emails and employee numbers; #1046 fixed that and re-introducing it as part of a rollback switch would be a privacy regression. It has no effect on which grants are produced.
Tested
TestExternalPrincipalMatchLegacyIndexedParity(new) — the actual proof the flag is safe to flip. One fixture throughprocessGrantsWithExternalPrincipalstwice, once per mode, requiring byte-identical store contents (deterministic proto marshal of every resulting grant). Fixture covers: a user matched on a non-emailprofile key (case-insensitively), a user matched via user-trait email, a user matched both ways at once (must yield one grant, not two), a user whose user trait can't be unmarshalled (must be skipped outright, including for the profile key it would otherwise match), group principals matched on a profile key — one with a resolvable expandable entitlement and one hitting the not-found arm — a match key no principal carries, a match against an unconfigured trait, andMatchAll/MatchByIDcarriers that the flag does not fork. Run against both the SQLite and Pebble engines, and with both the default trait set and aTRAIT_GROUP-only set (which exercises the "TRAIT_USER match arrives but USER isn't a configured match trait" arm on both sides).TestExternalPrincipalMatchParityFixtureExpectations(new) — pins what the two matchers agree on, so parity can't be satisfied by two identically-wrong matchers.TestExternalPrincipalMatchParitySkipsUnreadableUserTrait(new) — the skip is an absence, which a digest comparison alone wouldn't catch.TestExternalResourceUserProfileMatch— addedWithExternalPrincipalIndex(true); it was written for the indexed path, which is no longer the default.TestExternalPrincipalMatchDedupesAtTheMatcher(new) — asserts on the grants the matchers return, not on store contents: two grants for one principal share a generated grant id and get upserted into a single row, so a lost dedup is invisible to any post-PutGrantsassertion.TestExternalResourceMatch*FoldingContract/TestExternalResourceMatchEmitsOneGrantPerPrincipal— now run in both modes (see review fixes below).TestWithExternalPrincipalIndexOptionWiring(new) — pins the default and the opt-in throughNewSyncer.external_principal_index_test.goand theverification-tagged suite are unchanged and still pass.Breaking change + release note
c1api.NewC1TaskManageris exported and this PR inserts a positionalbool(externalPrincipalIndex) mid-argument-list, so any out-of-repo caller will fail to compile. Together with the default behavior flip (indexed → linear scan for existing callers), the release this lands in should be a minor bump (v0.25.0), not a patch.pkg/sdk/version.gois deliberately not touched here: version bumps in this repo are made by release automation in standalone commits (c15e57f,d246787, …), so hand-editing it in a feature PR would fight that process. Flagging it for whoever cuts the release instead. Happy to include the bump in this PR if that's the preference.Reshaping
NewC1TaskManagerinto a variadic-option or config-struct constructor is the right long-term fix for this argument list (14 positional parameters, five of them now sync configuration). That is deliberately deferred — it is a much larger, riskier change than a kill-switch PR should carry, and it would break the same callers a second time.Review fixes (second commit)
An adversarial review found the equivalence itself clean under mutation testing, plus three gaps, all fixed in
e68cee0:runner.gothreaded it intolocal.NewSynceronly, so a daemon-mode connector (c1api.NewC1TaskManager) settingBATON_EXTERNAL_PRINCIPAL_INDEX=truesilently kept using the linear scan — the exact deployment mode [CXP-498] Index external principals for grant matching #1046's motivating incident happened in, which made the switch useless where it matters most. Now plumbed hop-for-hop alongsideexternalResourceTraits.external_match_folding_test.gosilently lost indexed coverage. It builds a bare&syncer{}, so defaulting the flag to false moved the folding contracts (added in [CXP-498] Pin external-resource match folding as a behavioral contract #1060) off the matcher they were written to pin. The linear scan satisfies them for free by callingstrings.EqualFolddirectly, leaving the index's bucketing — where a fold can actually be gotten wrong — unpinned. All three tests now run in both modes. Verified by mutation: bucketing onstrings.ToLowerfails greek-final-sigma, long-s and dotted-capital-İ inindexed=trueonly.PutGrantsbefore the comparison). Closed with the matcher-level assertion above. Verified by mutation: deleting the email-matchcontinuein the legacy scan now fails the suite.Review fixes (third commit)
Automated review found two further gaps, fixed in
af9a848:The service-mode fix had no regression test — deleting any of its four plumbing lines left the suite green, which is precisely how the path came to be dropped in the first place. The two hand-offs that carry configuration through positional argument lists are now methods (
c1ApiTaskManager.newFullSyncHandler,fullSyncTaskHandler.syncOpts, both pure extractions), andpkg/tasks/c1api/full_sync_opts_test.goasserts them by running the assembled options through a real sync engine and reading the field they set.externalResourceTraitsis covered the same way, since it had the identical gap. Verified by mutation: dropping the append insyncOpts, hardcoding the handler field, and passingfalsefrom the manager each fail the suite.Note on technique: the assertion reads unexported
syncerfields reflectively. ASyncOptis an opaque closure that cannot be identified or applied from outsidepkg/sync, so letting the options act on a real engine is the only way to prove one survived the plumbing rather than merely that some option was appended. (Comparing option code pointers does not work —reflectyields a closure instance address, so two calls of the same constructor already differ.) Three of the four hops are now mutation-covered;connectorrunner→NewC1TaskManagerstill needs a live service client to reach and remains uncovered.The
verification-tagged crash-cut / resume / same-checkpoint-twice cell built bare syncers, so after the default flip it only replayed the legacy scan — the indexed path was never resumed or replayed by any test. Now parameterized over both matchers with the same helper used for the folding contracts. Both modes agree on every batch size the cell pins ({9},{33},{33, 27}), which is itself a small extra equivalence signal across the checkpoint seam.One inline bot comment about the service-mode branch not receiving the flag was posted against the pre-fix commit
79ecb38and is stale; the reviewer's own summary ate68cee0confirms it.Review fixes (fourth commit)
Two more from automated review, fixed in
1faf55b:The local / CLI chain had no coverage — round 3 proved out the service-mode chain and left its twin untested, even though
--external-principal-index→connectorrunner.WithExternalPrincipalIndex→cfg→local.WithExternalPrincipalIndex→sdkSync.WithExternalPrincipalIndex→ engine has exactly the same drop-a-value-silently shape.localSyncer.syncOptsis now split out ofProcess(same pure extraction asfullSyncTaskHandler.syncOpts) and asserted the same way, plus a runner-level test that theOptionreachesrunnerConfig. Mutation-verified: dropping the option fromlocalSyncer.syncOpts, dropping the value inlocal.WithExternalPrincipalIndex, and dropping it inconnectorrunner.WithExternalPrincipalIndexeach fail.Misplaced doc comment in
full_sync_opts_test.go— theexternalResourceTraitscomment sat above the manager hand-off test. Moved.Wiring coverage after all rounds
SyncOpt→syncer.externalPrincipalIndexEnabledpkg/sync—TestWithExternalPrincipalIndexOptionWiringconnectorrunner.Option→runnerConfigpkg/connectorrunner—TestWithExternalPrincipalIndexOptionlocalSyncer→ enginepkg/tasks/local—TestLocalSyncerThreadsExternalPrincipalIndexc1ApiTaskManager→ handlerpkg/tasks/c1api—TestC1TaskManagerThreadsExternalPrincipalIndexToHandlerpkg/tasks/c1api—TestFullSyncTaskHandlerThreadsExternalPrincipalIndexNewConnectorRunner→ task managerEvery row except the last is mutation-verified.
Coordination note
ben.su/ce-975/external-resource-traits(referenced from #1046's description) touches the same function. This PR restructures that region intomatchExternalPrincipalsLegacy/matchExternalPrincipalsIndexed, so whichever lands second will need a rebase. Not resolved here — flagging for whoever sequences the two. The new trait plumbing should slot into both matchers identically (both already takematchTraits), but that wants a look from someone with that branch's context.Open question
The CLI flag is registered visible rather than
WithHidden(true)(which is what--skip-grantsdoes). It's an internal engine toggle, but it's also the only escape hatch a standalone connector operator has on a large tenant. Happy to hide it if the preference is to keep the flag surface minimal.