infoschema: add masking policy integration#66034
Conversation
ti-chi-bot
Bot
added
release-note-none
|
Hi @tiancaiamao. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds masking policy support: meta CRUD and bootstrapping, system table DDL and reserved ID, infoschema storage/accessors and builder wiring, ISSyncer fetch/integration, call-site and test updates to pass masking policies. Changes
Sequence Diagram(s)sequenceDiagram
participant ISSyncer as ISSyncer.Loader
participant Meta as meta.Reader
participant Builder as infoschema.Builder
participant InfoSchema as infoSchema
ISSyncer->>Meta: ListSchemas(), ListPolicies(), ListResourceGroups()
ISSyncer->>Meta: ListMaskingPolicies()
Meta-->>ISSyncer: schemas, policies, resourceGroups, maskingPolicies
ISSyncer->>Builder: InitWithDBInfos(schemas, policies, resourceGroups, maskingPolicies, version)
Builder->>InfoSchema: initMisc(policies, resourceGroups, maskingPolicies)
InfoSchema-->>Builder: maskingPolicy maps populated
Builder-->>ISSyncer: InitWithDBInfos returns
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/executor/slow_query_test.go (1)
825-825:⚠️ Potential issue | 🔴 CriticalMissing signature update will cause compilation failure.
This
InitWithDBInfoscall still uses the old 4-argument signature, but the function signature was extended to include themaskingPoliciesparameter. This will cause a compilation error.- err := newISBuilder.InitWithDBInfos(nil, nil, nil, 0) + err := newISBuilder.InitWithDBInfos(nil, nil, nil, nil, 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/slow_query_test.go` at line 825, The call to newISBuilder.InitWithDBInfos uses the old 4-argument signature; update the invocation to pass the new maskingPolicies argument (e.g., nil or an appropriate value) so it matches the extended InitWithDBInfos signature; locate the call to newISBuilder.InitWithDBInfos in slow_query_test.go and add the maskingPolicies parameter to the argument list.
🧹 Nitpick comments (3)
pkg/meta/model/masking_policy.go (1)
33-41: Consider a non-empty fallback for unknown masking status.Returning
""inMaskingPolicyStatus.String()makes invalid states harder to diagnose in logs/errors;UNKNOWNwould be clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/meta/model/masking_policy.go` around lines 33 - 41, MaskingPolicyStatus.String() currently returns an empty string for unknown values; change the default case to return a non-empty sentinel like "UNKNOWN" (or "UNKNOWN(<value>)" using fmt.Sprintf("%d", s) for extra context) so invalid/unknown MaskingPolicyStatus values are clearly visible in logs and errors; update the default branch in the MaskingPolicyStatus.String method accordingly.pkg/meta/meta_test.go (2)
167-182: Consider addingDropMaskingPolicytest coverage.The test covers Create, Get, Update, and List operations, but
DropMaskingPolicyis missing. Given thatActionDropMaskingPolicyis added to BDR actions (seepkg/meta/model/bdr.go), this operation likely exists and should be tested for completeness.Would you like me to help generate the
DropMaskingPolicytest coverage?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/meta/meta_test.go` around lines 167 - 182, Add a test case exercising DropMaskingPolicy: after creating and asserting the policy via meta.NewMutator(txn).CreateMaskingPolicy and verifying with GetMaskingPolicy/ListMaskingPolicies (the existing setup using model.MaskingPolicyInfo and m.GetMaskingPolicy), call m.DropMaskingPolicy(id) (ensure you use the same policy ID used earlier) and assert no error, then begin a new transaction and verify the policy is no longer returned by m.GetMaskingPolicy (expect not found/appropriate error) and not present in m.ListMaskingPolicies; also ensure the BDR action ActionDropMaskingPolicy path is covered if relevant by committing/starting transactions as in the surrounding test flow.
151-152: Consider verifying the specific error type for duplicate creation.The test checks that creating a duplicate masking policy returns an error, but unlike
TestPlacementPolicy(line 85) which verifiesmeta.ErrPolicyExists.Equal(err), this test only checks that the error is not nil. For consistency and better test coverage, consider verifying the specific error type.err = m.CreateMaskingPolicy(policy) require.NotNil(t, err) + require.True(t, meta.ErrMaskingPolicyExists.Equal(err))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/meta/meta_test.go` around lines 151 - 152, The test currently asserts only that m.CreateMaskingPolicy(policy) returned a non-nil error; update it to assert the specific duplicate-policy error like in TestPlacementPolicy by replacing the generic require.NotNil(t, err) with an assertion that the error equals meta.ErrPolicyExists (e.g., use meta.ErrPolicyExists.Equal(err) or an equivalent equality check) so the test verifies the exact duplicate-creation error for CreateMaskingPolicy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/infoschema/builder_misc.go`:
- Around line 74-80: The calls to b.infoSchema.setMaskingPolicy(...) need nil
guards: after calling m.GetMaskingPolicy(diff.SchemaID) in
applyCreateMaskingPolicy (and the other similar function in this file where
setMaskingPolicy is used), check whether the returned policy is nil and handle
it gracefully (e.g., return nil or skip setting) instead of calling
setMaskingPolicy with a nil value; update both places that call
b.infoSchema.setMaskingPolicy to perform this nil check and short-circuit before
invoking setMaskingPolicy.
In `@pkg/infoschema/infoschema.go`:
- Around line 293-300: MaskingPolicyByID currently iterates maskingPolicyMap
without locking, risking a data race with setMaskingPolicy/deleteMaskingPolicy
which use maskingPolicyMutex; modify MaskingPolicyByID to acquire a read lock on
maskingPolicyMutex (RLock) before iterating and release it (RUnlock) after
returning, mirroring the pattern used in MaskingPolicyByName to ensure
concurrent safe reads of maskingPolicyMap.
In `@pkg/meta/meta.go`:
- Around line 157-160: Replace the two plain errors ErrMaskingPolicyExists and
ErrMaskingPolicyNotExists with the project’s typed meta/db errors using the
dbterror/errno pattern (i.e., create error variables using the same
dbterror/errno factory used by other meta objects so they carry SQL error
semantics and codes), and update the masking policy existence check/get call
sites (the functions that return these errors) to wrap/annotate returned errors
with the standard stack/context helper (the same wrapper used elsewhere for meta
errors) so callers receive the typed error plus contextual stack information;
reference the symbols ErrMaskingPolicyExists and ErrMaskingPolicyNotExists and
the masking-policy check/get functions when applying the change.
In `@pkg/parser/keywords.go`:
- Line 153: The keywords list contains duplicate entries for "MASKING" with
conflicting reservedness; remove the duplicate unreserved/reserved entry so
"MASKING" has a single classification (keep the intended reserved entry added at
the top) by editing pkg/parser/keywords.go to delete the other "MASKING" entry
(refer to the two entries of the string "MASKING" in the keywords slice) and
then regenerate the keyword artifacts/bindings so the metadata is consistent.
---
Outside diff comments:
In `@pkg/executor/slow_query_test.go`:
- Line 825: The call to newISBuilder.InitWithDBInfos uses the old 4-argument
signature; update the invocation to pass the new maskingPolicies argument (e.g.,
nil or an appropriate value) so it matches the extended InitWithDBInfos
signature; locate the call to newISBuilder.InitWithDBInfos in slow_query_test.go
and add the maskingPolicies parameter to the argument list.
---
Nitpick comments:
In `@pkg/meta/meta_test.go`:
- Around line 167-182: Add a test case exercising DropMaskingPolicy: after
creating and asserting the policy via meta.NewMutator(txn).CreateMaskingPolicy
and verifying with GetMaskingPolicy/ListMaskingPolicies (the existing setup
using model.MaskingPolicyInfo and m.GetMaskingPolicy), call
m.DropMaskingPolicy(id) (ensure you use the same policy ID used earlier) and
assert no error, then begin a new transaction and verify the policy is no longer
returned by m.GetMaskingPolicy (expect not found/appropriate error) and not
present in m.ListMaskingPolicies; also ensure the BDR action
ActionDropMaskingPolicy path is covered if relevant by committing/starting
transactions as in the surrounding test flow.
- Around line 151-152: The test currently asserts only that
m.CreateMaskingPolicy(policy) returned a non-nil error; update it to assert the
specific duplicate-policy error like in TestPlacementPolicy by replacing the
generic require.NotNil(t, err) with an assertion that the error equals
meta.ErrPolicyExists (e.g., use meta.ErrPolicyExists.Equal(err) or an equivalent
equality check) so the test verifies the exact duplicate-creation error for
CreateMaskingPolicy.
In `@pkg/meta/model/masking_policy.go`:
- Around line 33-41: MaskingPolicyStatus.String() currently returns an empty
string for unknown values; change the default case to return a non-empty
sentinel like "UNKNOWN" (or "UNKNOWN(<value>)" using fmt.Sprintf("%d", s) for
extra context) so invalid/unknown MaskingPolicyStatus values are clearly visible
in logs and errors; update the default branch in the MaskingPolicyStatus.String
method accordingly.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
pkg/ddl/placement_policy_ddl_test.gopkg/executor/slow_query_test.gopkg/executor/stmtsummary_test.gopkg/infoschema/builder.gopkg/infoschema/builder_misc.gopkg/infoschema/builder_test.gopkg/infoschema/context/infoschema.gopkg/infoschema/infoschema.gopkg/infoschema/infoschema_test.gopkg/infoschema/infoschema_v2_test.gopkg/infoschema/issyncer/loader.gopkg/meta/meta.gopkg/meta/meta_test.gopkg/meta/metadef/system.gopkg/meta/metadef/system_tables_def.gopkg/meta/model/bdr.gopkg/meta/model/job.gopkg/meta/model/job_args.gopkg/meta/model/job_args_test.gopkg/meta/model/masking_policy.gopkg/meta/reader.gopkg/parser/keywords.gopkg/session/bootstrap.gopkg/session/upgrade_def.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #66034 +/- ##
================================================
- Coverage 77.7059% 76.3677% -1.3383%
================================================
Files 1991 2057 +66
Lines 552094 585480 +33386
================================================
+ Hits 429010 447118 +18108
- Misses 122164 135101 +12937
- Partials 920 3261 +2341
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
size/L
|
/retest |
|
@tiancaiamao: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
size/XL
|
/retest |
|
@tiancaiamao: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@tiancaiamao: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@tiancaiamao: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
/retest |
1 similar comment
|
/retest |
ti-chi-bot
Bot
added
component/statistics
sig/planner
|
/retest |
ti-chi-bot
Bot
added
size/L
The struct and its initializer had declared twice due to a bad merge. This caused a compile error: vet: conflict_resolution_test.go:120:2: tbl redeclared
|
/retest |
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
…hase5 - MaskingPolicyByName: return (nil, false) when multiple tables share the same policy name, instead of returning an arbitrary first match. This prevents ambiguous lookups that could mask the wrong column. - loadMaskingPoliciesWithTableIDs: add batch loading with maxBatchSize and normalizeMaskingPolicyTableIDs helper for dedup/filter/sort. - buildLoadMaskingPoliciesQuery: switch from OR chain to IN clause. - Add regression tests: TestMaskingPolicyByNameAmbiguous, TestNormalizeMaskingPolicyTableIDs. Addresses PR review finding: policy names are unique per table (uk_table_policy), not globally. The old code already used [TableID][ColumnID] as the primary index (maskingPolicyMap was removed in the base commit), but the ByName lookup and loader had not been hardened against same-name collisions.
ti-chi-bot
Bot
added
size/XXL
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bb7133, wjhuang2016 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
@tiancaiamao: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/test pull-integration-realcluster-test-next-gen |
|
@tiancaiamao: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
4 participants
What problem does this PR solve?
Issue Number: ref #65744
Problem Summary:
What changed and how does it work?
Stacked PRs
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Chores
Tests