*: parser support for MySQL dual passwords (RETAIN CURRENT PASSWORD / DISCARD OLD PASSWORD)#68028
Merged
ti-chi-bot[bot] merged 22 commits intoJun 3, 2026
Merged
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
do-not-merge/work-in-progress
|
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 MySQL-compatible dual-password support: parser recognizes ChangesDual-password feature
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Parser as Parser
participant Executor as Executor\n(pkg/executor/simple.go)
participant UserTable as mysql.user
participant Cache as Privilege Cache\n(pkg/privilege/privileges/cache.go)
Client->>Parser: ALTER USER 'u' IDENTIFIED BY 'new' RETAIN CURRENT PASSWORD
Parser->>Executor: AST (DualPasswordOption / RetainCurrentPassword)
Executor->>UserTable: SELECT authentication_string FOR UPDATE
UserTable-->>Executor: current_auth_string
Executor->>UserTable: UPDATE authentication_string=new_hash, user_attributes=JSON_MERGE/REMOVE($.additional_password)
UserTable-->>Executor: OK
Executor-->>Client: OK
sequenceDiagram
participant Client as Client
participant Auth as Authenticator\n(pkg/privilege/privileges/privileges.go)
participant Cache as Privilege Cache
participant Primary as Primary Hash Check
participant Secondary as Secondary Hash Check
Client->>Auth: CONNECT user / password
Auth->>Cache: Load UserRecord (AuthenticationString, AdditionalAuthenticationString)
Cache-->>Auth: UserRecord
Auth->>Primary: check(password, AuthenticationString)
Primary-->>Auth: FAIL (retryable)
Auth->>Secondary: check(password, AdditionalAuthenticationString)
Secondary-->>Auth: SUCCESS
Auth-->>Client: Authenticated (used retained password)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ti-chi-bot
Bot
added
contribution
|
Hi @takaidohigasi. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
|
Hi @takaidohigasi. 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. |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
![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
🧹 Nitpick comments (8)
pkg/executor/test/passwordtest/BUILD.bazel (1)
7-12: Confirm Bazel metadata was regenerated.Since a new Go test source file is added to
srcs, please ensuremake bazel_preparewas run and any resulting metadata changes are included in this PR. Theshard_countbump from 9 to 17 is a large jump for adding a single test file — if it was chosen empirically to keep wall-time reasonable that's fine, otherwise consider starting with a smaller bump (e.g. 12) to avoid unnecessary parallelism overhead.Based on learnings: "MUST run
make bazel_prepareand include resulting Bazel metadata changes in the PR when adding/moving/renaming/removing Go files, changing Bazel files, updating Bazel test targets".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/test/passwordtest/BUILD.bazel` around lines 7 - 12, Regenerate and commit Bazel metadata by running make bazel_prepare because a new Go test file ("dual_password_test.go") was added to the srcs array in BUILD.bazel; include the resulting changes in this PR and ensure the BUILD.bazel entry reflects those updates, and if the shard_count change (currently 17) was not chosen empirically, revert it to a smaller increment (e.g., 12) or add a brief justification comment in the PR explaining why 17 is required to keep wall-time reasonable.pkg/privilege/privileges/cache.go (1)
1065-1075: LGTM — follows the existing extraction pattern.Extraction mirrors the sibling
$.resource_groupblock (lines 1054-1064): parse path expr,Extract,Unquote,strings.Clone. This keeps the decoder style consistent.Optional nit (not blocking): the three
ParseJSONPathExprcalls ($.metadata.email,$.resource_group,$.additional_password) are re-parsed per row on every cache reload. If the hot path ever becomes a concern, these could be hoisted to package-levelvarinitializers. Out of scope for this PR — existing code already pays this cost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/cache.go` around lines 1065 - 1075, No change required for correctness—the added extraction of `$.additional_password` (in the block using types.ParseJSONPathExpr, bj.Extract, additionalBJ.Unquote and strings.Clone feeding value.AdditionalAuthenticationString) correctly mirrors the existing `$.resource_group` pattern; if you want to address the optional nit about repeated parsing on each cache reload, hoist the repeated calls to types.ParseJSONPathExpr for `$.metadata.email`, `$.resource_group`, and `$.additional_password` into package-level var initializers (e.g., pre-parse into vars like pkgEmailPathExpr, pkgResourceGroupPathExpr, pkgAdditionalPasswordPathExpr) and replace the per-row ParseJSONPathExpr calls in the decoder functions with those pre-parsed vars to avoid re-parsing on the hot path.pkg/parser/parser_test.go (1)
5260-5266: Add negative parser cases for unsupportedCREATE USER ... RETAIN/DISCARDcombinations.This block covers accepted syntax well, but it doesn’t pin the rejection behavior for
CREATE USERwith dual-password clauses. Adding explicitok=falserows here would guard the compatibility contract against parser regressions.Proposed test additions
+ {"CREATE USER 'u1'@'%' IDENTIFIED BY 'new' RETAIN CURRENT PASSWORD", false, ""}, + {"CREATE USER 'u1'@'%' DISCARD OLD PASSWORD", false, ""},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/parser_test.go` around lines 5260 - 5266, Add negative parser test rows next to the dual-password block in parser_test.go: add CREATE USER variants using RETAIN CURRENT PASSWORD and DISCARD OLD PASSWORD (e.g. "CREATE USER 'u1'@'%' IDENTIFIED BY 'x' RETAIN CURRENT PASSWORD", "CREATE USER 'u1'@'%' IDENTIFIED WITH 'mysql_native_password' BY 'x' DISCARD OLD PASSWORD", etc.) with ok=false so the parser is expected to reject these combinations; place them alongside the existing ALTER/SET examples and reference the clause text (CREATE USER, RETAIN CURRENT PASSWORD, DISCARD OLD PASSWORD) so future diffs can find and maintain these negative cases.pkg/privilege/privileges/privileges.go (1)
700-711: Consider emitting a log when the secondary password succeeds.During a credential rotation, operators typically want to know which accounts have already switched to the primary vs. are still using the retained secondary, so they can safely run
ALTER USER ... DISCARD OLD PASSWORDat the end. As implemented, a successful dual-password fallback is indistinguishable from a regular login in the logs. A single info-level log (or metric counter) on the fallback-success path would materially aid rotation workflows without affecting correctness.💡 Suggested hook
ok, retryable := checkHash(pwd) - if !ok && retryable && len(record.AdditionalAuthenticationString) > 0 { - // MySQL-compatible dual-password fallback: try the secondary password - // stored in user_attributes.$.additional_password with the same plugin. - ok, _ = checkHash(record.AdditionalAuthenticationString) - } + if !ok && retryable && len(record.AdditionalAuthenticationString) > 0 { + // MySQL-compatible dual-password fallback: try the secondary password + // stored in user_attributes.$.additional_password with the same plugin. + ok, _ = checkHash(record.AdditionalAuthenticationString) + if ok { + logutil.BgLogger().Info("authenticated using retained (secondary) password", + zap.String("authUser", authUser), zap.String("authHost", authHost), + zap.String("plugin", record.AuthPlugin)) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/privileges.go` around lines 700 - 711, When the dual-password fallback path in the switch for record.AuthPlugin (where checkHash(pwd) fails and checkHash(record.AdditionalAuthenticationString) returns ok) succeeds, emit an info-level log or increment a metric to indicate the fallback was used; specifically, after the successful checkHash(record.AdditionalAuthenticationString) result is true, record an informational message/metric including user.Username, user.Hostname and record.AuthPlugin (or otherwise tag by username/host/plugin) so operators can distinguish fallback logins from primary-password logins. Ensure the log/metric is lightweight and only executed on the fallback-success branch (i.e., immediately after ok, _ = checkHash(record.AdditionalAuthenticationString) and before returning success).pkg/executor/simple.go (3)
923-932: LGTM on the mutual-exclusion check.Correctly rejects
RETAIN CURRENT PASSWORDandDISCARD OLD PASSWORDspecified together. One nit: MySQL surfaces this asER_DUPLICATE_PASSWORD_SPECIFIED_KEYWORDS(3864). Using a specific error code would improve client compatibility, but this is a minor refinement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/simple.go` around lines 923 - 932, The mutual-exclusion check currently returns a generic error; replace the generic errors.Errorf call that checks info.retainCurrentPassword && info.discardOldPassword with the MySQL-specific error for duplicate password keywords (ER_DUPLICATE_PASSWORD_SPECIFIED_KEYWORDS / code 3864) so clients see the canonical error. Locate the clause that returns errors.Errorf("RETAIN CURRENT PASSWORD and DISCARD OLD PASSWORD can not be used together") and change it to construct and return the MySQL error (e.g., mysql.NewErrf(mysql.ER_DUPLICATE_PASSWORD_SPECIFIED_KEYWORDS, "RETAIN CURRENT PASSWORD and DISCARD OLD PASSWORD cannot be used together") or the equivalent project-specific errno helper) so the specific error code 3864 is emitted.
2015-2031: Recommended refactor: extract theadditional_passwordJSON builder.The logic to read the current
authentication_stringand JSON-encode it into anadditional_passwordattribute is duplicated betweenexecuteAlterUser(lines 2018–2031) andexecuteSetPwd(lines 2722–2737). Extracting a small helper keeps the two code paths aligned if either the encoding rule or the SQL-injection assumption changes.♻️ Proposed helper
// buildAdditionalPasswordJSON reads the user's current authentication_string and // returns a JSON object fragment `{"additional_password": "<hash>"}` suitable // for feeding into json_merge_patch on user_attributes. func buildAdditionalPasswordJSON(ctx context.Context, sqlExecutor sqlexec.SQLExecutor, user, host string) (string, error) { oldPwd, err := readAuthenticationString(ctx, sqlExecutor, user, host) if err != nil { return "", err } encoded, err := json.Marshal(oldPwd) if err != nil { return "", err } return fmt.Sprintf(`{"additional_password": %s}`, encoded), nil }Then use it from both call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/simple.go` around lines 2015 - 2031, Extract the duplicated logic that reads mysql.user.authentication_string and JSON-encodes it into a helper named buildAdditionalPasswordJSON(ctx, sqlExecutor, user, host) that returns the JSON fragment `{"additional_password": "<hash>"}` (string, error); replace the inline code in executeAlterUser and executeSetPwd with calls to this helper, returning errors unchanged on failure, so both call sites use the same encoding and error handling via readAuthenticationString and json.Marshal.
2533-2564: Minor:readAuthenticationStringsilently returns""on missing row.If the target
user@hostis not present, the function returns("", nil)rather than an error. Every production caller already verifies existence viauserExistsInternalbeforehand, so this is defensive — but it also means that any future caller that forgets the existence check will silently store an empty secondary password. Consider returning a sentinel error (or a distinctfound bool) to make the precondition explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/simple.go` around lines 2533 - 2564, readAuthenticationString currently returns ("", nil) when the user row is missing which can silently propagate an empty secondary password; change the function signature to readAuthenticationString(ctx context.Context, sqlExecutor sqlexec.SQLExecutor, name, host string) (string, bool, error) and return (authString, true, nil) when found and ("", false, nil) when not found; update all call sites (places that currently call readAuthenticationString, e.g., the RETAIN CURRENT PASSWORD handling logic and any callers that assume existence) to check the boolean before using the returned string and handle the not-found case (or convert it into an explicit error there) so a missing user no longer produces a silent empty password.pkg/executor/test/passwordtest/dual_password_test.go (1)
27-39: Recommended: avoid creating a secondMockStoreper authentication attempt.
rootTKcallstestkit.CreateMockStore(t)per test (fine), butauthAsthen creates a freshtestkit.TestKiton the same store viatk.Session().GetStore()each call. That's OK. However,rootTKreturns acleanup func() {}that is a no-op — callersdefer cleanup()it, which is dead code. Either wire the cleanup to something meaningful (e.g.,tk.Session().Close()or aDROP USER), or drop the return value and itsdefer cleanup()call-sites to reduce boilerplate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/test/passwordtest/dual_password_test.go` around lines 27 - 39, The rootTK helper currently returns a no-op cleanup which callers defer, creating dead code; update rootTK or its call-sites: either (A) make cleanup meaningful by closing the TestKit/session (call tk.Session().Close() or equivalent) and return that cleanup from rootTK so deferred cleanup actually releases resources, or (B) remove the cleanup return (change signature of rootTK to return only *testkit.TestKit) and remove the deferred cleanup calls where rootTK is used; ensure authAs continues to create sub TestKit via tk.Session().GetStore() and keep symbols rootTK, authAs, TestKit and session handling consistent.
🤖 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/executor/simple.go`:
- Around line 1817-1843: When handling RETAIN CURRENT PASSWORD in
executeAlterUser and executeSetPwd, ensure the primary password read via
readAuthenticationString is not empty before encoding/writing it as
additional_password; specifically, after capturing oldPwd in executeAlterUser
(near where readAuthenticationString is called) and after the
readAuthenticationString call in executeSetPwd, check if the returned string is
empty and, if so, return an error rejecting RETAIN CURRENT PASSWORD (e.g., an
errors.Errorf with a message like "Current password can not be retained for user
'%s'@'%s' because primary password is empty", using the same username/hostname
symbols used elsewhere); do not proceed to set additional_password when the
primary is empty.
In `@pkg/executor/test/passwordtest/dual_password_test.go`:
- Around line 156-163: Add an explicit negative assertion that DISCARD OLD
PASSWORD fails without APPLICATION_PASSWORD_ADMIN before granting the privilege:
in the test dual_password_test.go, call adminTK2.MustExec or use authAs to
attempt "ALTER USER dpvictim DISCARD OLD PASSWORD" (or attempt authAs after a
DISCARD attempt) and assert an error prior to granting the privilege, mirroring
the existing negative RETAIN case around lines 145–147; this ensures the
discardOldPassword guard in simple.go is actually enforced and regression-safe
before you grant the permission and exercise the success path with
adminTK2.MustExec("ALTER USER dpvictim DISCARD OLD PASSWORD") and subsequent
authAs checks.
- Around line 166-186: The test TestDualPasswordCachingSha2Password currently
claims to verify "decoded by the privilege cache" but only asserts persistence;
either update the test comment/docstring to state this is a persistence-only
check (no authentication attempted), or add an actual authentication check
implemented without using authAs() / sha1Password() (which only works for
mysql_native_password) — e.g., perform a real client login flow that uses
caching_sha2_password or call the server-side CheckHashingPassword test helpers
(see CheckHashingPassword in pkg/parser/auth/caching_sha2_test.go) to validate
the stored hashes; reference TestDualPasswordRetainAndDiscard and
TestDualPasswordSetPasswordRetain for expected auth patterns when choosing to
add authentication verification.
In `@pkg/privilege/privileges/privileges.go`:
- Around line 688-693: The error log is misleading because the block handling
mysql.AuthCachingSha2Password and mysql.AuthTiDBSM3Password always logs "Failed
to check caching_sha2_password"; update the log to include the actual plugin and
user context and avoid hardcoding the plugin name: when
auth.CheckHashingPassword returns an error, call logutil.BgLogger().Error with a
neutral message (e.g., "Failed to check hashed password") and add fields
zap.String("auth_plugin", record.AuthPlugin) and zap.String("auth_user",
record.User or authUser variable) plus zap.Error(err) so operators can see which
plugin (and user) failed; keep returning ok, true as before.
---
Nitpick comments:
In `@pkg/executor/simple.go`:
- Around line 923-932: The mutual-exclusion check currently returns a generic
error; replace the generic errors.Errorf call that checks
info.retainCurrentPassword && info.discardOldPassword with the MySQL-specific
error for duplicate password keywords (ER_DUPLICATE_PASSWORD_SPECIFIED_KEYWORDS
/ code 3864) so clients see the canonical error. Locate the clause that returns
errors.Errorf("RETAIN CURRENT PASSWORD and DISCARD OLD PASSWORD can not be used
together") and change it to construct and return the MySQL error (e.g.,
mysql.NewErrf(mysql.ER_DUPLICATE_PASSWORD_SPECIFIED_KEYWORDS, "RETAIN CURRENT
PASSWORD and DISCARD OLD PASSWORD cannot be used together") or the equivalent
project-specific errno helper) so the specific error code 3864 is emitted.
- Around line 2015-2031: Extract the duplicated logic that reads
mysql.user.authentication_string and JSON-encodes it into a helper named
buildAdditionalPasswordJSON(ctx, sqlExecutor, user, host) that returns the JSON
fragment `{"additional_password": "<hash>"}` (string, error); replace the inline
code in executeAlterUser and executeSetPwd with calls to this helper, returning
errors unchanged on failure, so both call sites use the same encoding and error
handling via readAuthenticationString and json.Marshal.
- Around line 2533-2564: readAuthenticationString currently returns ("", nil)
when the user row is missing which can silently propagate an empty secondary
password; change the function signature to readAuthenticationString(ctx
context.Context, sqlExecutor sqlexec.SQLExecutor, name, host string) (string,
bool, error) and return (authString, true, nil) when found and ("", false, nil)
when not found; update all call sites (places that currently call
readAuthenticationString, e.g., the RETAIN CURRENT PASSWORD handling logic and
any callers that assume existence) to check the boolean before using the
returned string and handle the not-found case (or convert it into an explicit
error there) so a missing user no longer produces a silent empty password.
In `@pkg/executor/test/passwordtest/BUILD.bazel`:
- Around line 7-12: Regenerate and commit Bazel metadata by running make
bazel_prepare because a new Go test file ("dual_password_test.go") was added to
the srcs array in BUILD.bazel; include the resulting changes in this PR and
ensure the BUILD.bazel entry reflects those updates, and if the shard_count
change (currently 17) was not chosen empirically, revert it to a smaller
increment (e.g., 12) or add a brief justification comment in the PR explaining
why 17 is required to keep wall-time reasonable.
In `@pkg/executor/test/passwordtest/dual_password_test.go`:
- Around line 27-39: The rootTK helper currently returns a no-op cleanup which
callers defer, creating dead code; update rootTK or its call-sites: either (A)
make cleanup meaningful by closing the TestKit/session (call
tk.Session().Close() or equivalent) and return that cleanup from rootTK so
deferred cleanup actually releases resources, or (B) remove the cleanup return
(change signature of rootTK to return only *testkit.TestKit) and remove the
deferred cleanup calls where rootTK is used; ensure authAs continues to create
sub TestKit via tk.Session().GetStore() and keep symbols rootTK, authAs, TestKit
and session handling consistent.
In `@pkg/parser/parser_test.go`:
- Around line 5260-5266: Add negative parser test rows next to the dual-password
block in parser_test.go: add CREATE USER variants using RETAIN CURRENT PASSWORD
and DISCARD OLD PASSWORD (e.g. "CREATE USER 'u1'@'%' IDENTIFIED BY 'x' RETAIN
CURRENT PASSWORD", "CREATE USER 'u1'@'%' IDENTIFIED WITH 'mysql_native_password'
BY 'x' DISCARD OLD PASSWORD", etc.) with ok=false so the parser is expected to
reject these combinations; place them alongside the existing ALTER/SET examples
and reference the clause text (CREATE USER, RETAIN CURRENT PASSWORD, DISCARD OLD
PASSWORD) so future diffs can find and maintain these negative cases.
In `@pkg/privilege/privileges/cache.go`:
- Around line 1065-1075: No change required for correctness—the added extraction
of `$.additional_password` (in the block using types.ParseJSONPathExpr,
bj.Extract, additionalBJ.Unquote and strings.Clone feeding
value.AdditionalAuthenticationString) correctly mirrors the existing
`$.resource_group` pattern; if you want to address the optional nit about
repeated parsing on each cache reload, hoist the repeated calls to
types.ParseJSONPathExpr for `$.metadata.email`, `$.resource_group`, and
`$.additional_password` into package-level var initializers (e.g., pre-parse
into vars like pkgEmailPathExpr, pkgResourceGroupPathExpr,
pkgAdditionalPasswordPathExpr) and replace the per-row ParseJSONPathExpr calls
in the decoder functions with those pre-parsed vars to avoid re-parsing on the
hot path.
In `@pkg/privilege/privileges/privileges.go`:
- Around line 700-711: When the dual-password fallback path in the switch for
record.AuthPlugin (where checkHash(pwd) fails and
checkHash(record.AdditionalAuthenticationString) returns ok) succeeds, emit an
info-level log or increment a metric to indicate the fallback was used;
specifically, after the successful
checkHash(record.AdditionalAuthenticationString) result is true, record an
informational message/metric including user.Username, user.Hostname and
record.AuthPlugin (or otherwise tag by username/host/plugin) so operators can
distinguish fallback logins from primary-password logins. Ensure the log/metric
is lightweight and only executed on the fallback-success branch (i.e.,
immediately after ok, _ = checkHash(record.AdditionalAuthenticationString) and
before returning success).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c6b4fa48-d776-478f-a021-d583bc8f49f8
📒 Files selected for processing (12)
pkg/executor/simple.gopkg/executor/test/passwordtest/BUILD.bazelpkg/executor/test/passwordtest/dual_password_test.gopkg/executor/windows/BUILD.bazelpkg/parser/ast/misc.gopkg/parser/keywords.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.ypkg/parser/parser_test.gopkg/privilege/privileges/cache.gopkg/privilege/privileges/privileges.go
💤 Files with no reviewable changes (1)
- pkg/executor/windows/BUILD.bazel
Contributor
Author
|
I will add minor fixes and add e2e tests |
takaidohigasi
force-pushed
the
feature/dual-password
branch
2 times, most recently
from
07be4ee to
7386798
Compare
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
Thanks @coderabbitai for the thorough review. Addressed in Actionable (4/4 fixed)
Nitpicks (5/8 applied, 3 justified)Applied:
Justified / skipped:
Verification
@coderabbitai review |
|
The three justified skips are all reasonable:
Triggering a re-review of the force-pushed changes now. 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/integrationtest/t/executor/dual_password.test (2)
41-60: Please cover the self-service privilege path as well.The PR contract says
APPLICATION_PASSWORD_ADMINis only required when changing other users. This test only exercises the cross-user branch, so a regression that accidentally gates self-serviceRETAIN/DISCARDwould slip through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integrationtest/t/executor/dual_password.test` around lines 41 - 60, Add a self-service test that ensures APPLICATION_PASSWORD_ADMIN is not required when a user alters their own password with RETAIN/DISCARD: use the existing dpvictim user and connect as dpvictim (e.g., connect (cvictim, 127.0.0.1, dpvictim, v1, test)), then run alter user dpvictim identified by 'v2' retain current password (and a DISCARD variant) and assert success (and check mysql.user.user_attributes like the cross-user test does); keep the cross-user tests for dpadmin unchanged so both paths (self-service via dpvictim and cross-user via dpadmin) are covered to catch regressions that might gate self-service operations.
7-39: Add an end-to-end login check for the retained-password window.These assertions prove JSON storage, but they do not validate the user-visible behavior added in
pkg/privilege/privileges/privileges.go:594-749: authenticating with the old password beforeDISCARD OLD PASSWORD, and rejecting it afterward. A regression inConnectionVerificationwould still pass this test.pkg/executor/simple.go (3)
2013-2024: Avoid string-surgery on JSON output ofbuildAdditionalPasswordJSON.Line 2023 peels the outer
{/}off the helper's return value so the inner pair can be concatenated with the othernewAttributesentries. This tight coupling between the helper's JSON-object shape and the caller's string-joining is fragile: any future change to the helper (e.g., adding a sibling key, pretty-printing, or different encoding) silently breaks the concatenation.Prefer having the helper return just the inner fragment (or the raw encoded hash) and let the caller compose the object. For example:
♻️ Alternative API
-func buildAdditionalPasswordJSON(ctx context.Context, sqlExecutor sqlexec.SQLExecutor, name, host string) (string, error) { +// buildAdditionalPasswordEntry returns `"additional_password": "<hash>"` suitable +// for inclusion in a user_attributes JSON object. +func buildAdditionalPasswordEntry(ctx context.Context, sqlExecutor sqlexec.SQLExecutor, name, host string) (string, error) { oldPwd, found, err := readAuthenticationString(ctx, sqlExecutor, name, host) ... - return fmt.Sprintf(`{"additional_password": %s}`, encoded), nil + return fmt.Sprintf(`"additional_password": %s`, encoded), nil }Then the caller becomes:
- attrObj, err := buildAdditionalPasswordJSON(ctx, sqlExecutor, spec.User.Username, spec.User.Hostname) + entry, err := buildAdditionalPasswordEntry(ctx, sqlExecutor, spec.User.Username, spec.User.Hostname) if err != nil { return err } - newAttributes = append(newAttributes, strings.TrimSuffix(strings.TrimPrefix(attrObj, "{"), "}")) + newAttributes = append(newAttributes, entry)Same applies to the
executeSetPwdcaller on line 2751 — it keeps the full-object form because it passes the JSON directly tojson_merge_patch, so either keep both forms (rename helpers accordingly) or construct the{ ... }wrapper at the single call site that needs it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/simple.go` around lines 2013 - 2024, The code performs fragile string-surgery on the JSON returned by buildAdditionalPasswordJSON (used when plOptions.retainCurrentPassword is true) by trimming outer braces and appending to newAttributes; change the API so buildAdditionalPasswordJSON returns either the inner fragment or the raw password/hash (or a structured map) and update callers accordingly: have buildAdditionalPasswordJSON return a well-defined fragment (or value) and then compose the surrounding JSON object at the call site that needs it (the block that appends to newAttributes and the executeSetPwd caller), removing any TrimPrefix/TrimSuffix operations and instead building the JSON explicitly from the returned fragment/value to avoid brittle string concatenation.
1103-1110: Use dedicated, code-carrying errors for the CREATE USER rejection.
errors.Errorfproduces a generic error without a MySQL error code, so clients see a plain string rather than the numeric code/SQLSTATE they'd get from MySQL. The rest of this PR already introduces named errors inexeerrorsfor the dual-password contract (empty-new, empty-primary, plugin-change). Consider adding analogous ones (or reusing an existing "clause not supported in this statement" error) here and at lines 1826 and 2682 for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/simple.go` around lines 1103 - 1110, Replace the generic errors.Errorf calls that reject plOptions.retainCurrentPassword and plOptions.discardOldPassword with dedicated executable errors from the exeerrors package (e.g., a new exeerrors.ErrUnsupportedClauseForCreateUser or reuse an existing "clause not supported in this statement" error) so the client receives proper MySQL error codes/SQLSTATE; update the two other analogous rejection sites (the other checks handling retainCurrentPassword/discardOldPassword elsewhere in this file) to use the same exeerrors symbol for consistency and ensure callers return that error value.
2013-2041: Twouser_attributes=assignments in one UPDATE — consider combining.When
DISCARD OLD PASSWORD(or an auth-plugin change) coincides with other attribute changes (metadata, resource_group, password_locking), the generated UPDATE ends up with two assignments touser_attributes: onejson_merge_patch(line 2030) and onejson_remove(line 2040). This only produces the intended "merge then remove" result because MySQL evaluates same-row assignments left-to-right so the second expression sees the value written by the first — a subtle contract to rely on, and fragile to future reordering/refactors.Prefer composing the two ops into a single assignment, e.g.:
♻️ Suggested consolidation
- if length := len(newAttributes); length > 0 { - if length > 1 || passwordLockingStr == "" { - passwordLockingInfo.containsNoOthers = false - } - newAttributesStr := fmt.Sprintf("{%s}", strings.Join(newAttributes, ",")) - fields = append(fields, alterField{"user_attributes=json_merge_patch(coalesce(user_attributes, '{}'), %?)", newAttributesStr}) - } - // DISCARD OLD PASSWORD removes the secondary password. - // MySQL also silently drops the secondary when the auth plugin is changed; - // detect that here and do the same. - dropSecondary := plOptions.discardOldPassword - if !dropSecondary && spec.AuthOpt != nil && spec.AuthOpt.AuthPlugin != "" && spec.AuthOpt.AuthPlugin != currentAuthPlugin { - dropSecondary = true - } - if dropSecondary && !plOptions.retainCurrentPassword { - fields = append(fields, alterField{"user_attributes=json_remove(coalesce(user_attributes, '{}'), '$.additional_password')", nil}) - } + dropSecondary := plOptions.discardOldPassword || + (spec.AuthOpt != nil && spec.AuthOpt.AuthPlugin != "" && spec.AuthOpt.AuthPlugin != currentAuthPlugin) + hasNewAttributes := len(newAttributes) > 0 + if hasNewAttributes { + if len(newAttributes) > 1 || passwordLockingStr == "" { + passwordLockingInfo.containsNoOthers = false + } + } + switch { + case hasNewAttributes && dropSecondary && !plOptions.retainCurrentPassword: + newAttributesStr := fmt.Sprintf("{%s}", strings.Join(newAttributes, ",")) + fields = append(fields, alterField{ + "user_attributes=json_remove(json_merge_patch(coalesce(user_attributes, '{}'), %?), '$.additional_password')", + newAttributesStr, + }) + case hasNewAttributes: + newAttributesStr := fmt.Sprintf("{%s}", strings.Join(newAttributes, ",")) + fields = append(fields, alterField{"user_attributes=json_merge_patch(coalesce(user_attributes, '{}'), %?)", newAttributesStr}) + case dropSecondary && !plOptions.retainCurrentPassword: + fields = append(fields, alterField{"user_attributes=json_remove(coalesce(user_attributes, '{}'), '$.additional_password')", nil}) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/simple.go` around lines 2013 - 2041, The UPDATE can end up with two separate user_attributes assignments; instead compose them into one so we perform merge then remove in a single expression. When newAttributes (built via buildAdditionalPasswordJSON and stored in newAttributesStr) exists and dropSecondary is true (and !plOptions.retainCurrentPassword), instead of appending both alterField entries ("user_attributes=json_merge_patch(..., %?)", newAttributesStr) and ("user_attributes=json_remove(..., '$.additional_password')", nil), create a single alterField that wraps the merge with json_remove (e.g. json_remove(json_merge_patch(coalesce(user_attributes, '{}'), %?), '$.additional_password') with the same newAttributesStr) and append only that; otherwise keep the existing single-operation branches. Ensure you still set passwordLockingInfo.containsNoOthers as currently done.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integrationtest/t/executor/dual_password.test`:
- Line 10: The test's mysql.user checks (e.g., the query "select
json_extract(user_attributes, '$.additional_password') is not null from
mysql.user where user = 'dpu1';") are not qualified by Host and can return
multiple/flaky rows; update each such query that filters by user ('dpu1' and
'dpvictim') to also filter by host (for this test use "and host = '%'" or the
exact host you create) so the assertions target the specific account, and apply
the same change to all other similar queries in this file that reference those
users.
---
Nitpick comments:
In `@pkg/executor/simple.go`:
- Around line 2013-2024: The code performs fragile string-surgery on the JSON
returned by buildAdditionalPasswordJSON (used when
plOptions.retainCurrentPassword is true) by trimming outer braces and appending
to newAttributes; change the API so buildAdditionalPasswordJSON returns either
the inner fragment or the raw password/hash (or a structured map) and update
callers accordingly: have buildAdditionalPasswordJSON return a well-defined
fragment (or value) and then compose the surrounding JSON object at the call
site that needs it (the block that appends to newAttributes and the
executeSetPwd caller), removing any TrimPrefix/TrimSuffix operations and instead
building the JSON explicitly from the returned fragment/value to avoid brittle
string concatenation.
- Around line 1103-1110: Replace the generic errors.Errorf calls that reject
plOptions.retainCurrentPassword and plOptions.discardOldPassword with dedicated
executable errors from the exeerrors package (e.g., a new
exeerrors.ErrUnsupportedClauseForCreateUser or reuse an existing "clause not
supported in this statement" error) so the client receives proper MySQL error
codes/SQLSTATE; update the two other analogous rejection sites (the other checks
handling retainCurrentPassword/discardOldPassword elsewhere in this file) to use
the same exeerrors symbol for consistency and ensure callers return that error
value.
- Around line 2013-2041: The UPDATE can end up with two separate user_attributes
assignments; instead compose them into one so we perform merge then remove in a
single expression. When newAttributes (built via buildAdditionalPasswordJSON and
stored in newAttributesStr) exists and dropSecondary is true (and
!plOptions.retainCurrentPassword), instead of appending both alterField entries
("user_attributes=json_merge_patch(..., %?)", newAttributesStr) and
("user_attributes=json_remove(..., '$.additional_password')", nil), create a
single alterField that wraps the merge with json_remove (e.g.
json_remove(json_merge_patch(coalesce(user_attributes, '{}'), %?),
'$.additional_password') with the same newAttributesStr) and append only that;
otherwise keep the existing single-operation branches. Ensure you still set
passwordLockingInfo.containsNoOthers as currently done.
In `@tests/integrationtest/t/executor/dual_password.test`:
- Around line 41-60: Add a self-service test that ensures
APPLICATION_PASSWORD_ADMIN is not required when a user alters their own password
with RETAIN/DISCARD: use the existing dpvictim user and connect as dpvictim
(e.g., connect (cvictim, 127.0.0.1, dpvictim, v1, test)), then run alter user
dpvictim identified by 'v2' retain current password (and a DISCARD variant) and
assert success (and check mysql.user.user_attributes like the cross-user test
does); keep the cross-user tests for dpadmin unchanged so both paths
(self-service via dpvictim and cross-user via dpadmin) are covered to catch
regressions that might gate self-service operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9c8ee980-d4fb-4123-abac-7ff348b7a7f6
📒 Files selected for processing (16)
pkg/errno/errcode.gopkg/errno/errname.gopkg/executor/simple.gopkg/executor/test/passwordtest/BUILD.bazelpkg/executor/test/passwordtest/dual_password_test.gopkg/parser/ast/misc.gopkg/parser/keywords.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.ypkg/parser/parser_test.gopkg/privilege/privileges/cache.gopkg/privilege/privileges/privileges.gopkg/util/dbterror/exeerrors/errors.gotests/integrationtest/r/executor/dual_password.resulttests/integrationtest/t/executor/dual_password.test
✅ Files skipped from review due to trivial changes (4)
- pkg/parser/keywords.go
- pkg/parser/misc.go
- pkg/parser/parser.y
- pkg/parser/parser_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/executor/test/passwordtest/BUILD.bazel
- pkg/executor/test/passwordtest/dual_password_test.go
Contributor
Author
|
improved 8.0 compatibility |
Contributor
Author
MySQL upstream test-coverage analysisCross-checked this PR's tests against MySQL's own dual-password suite so reviewers can see exactly which scenarios are covered and which are intentionally deferred. MySQL's test files (github.com/mysql/mysql-server)
Coverage matrix
Score: 8 of 14 scenarios covered. Additional TiDB-specific coverage not in the MySQL suite: Gaps I'd recommend closing before final review
Deliberately skipped
Happy to add #3–#7 as a follow-up commit if reviewers agree they're worth the marginal test cost. |
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
…D PASSWORD) Adds MySQL 8.0-compatible dual-password support for credential rotation without downtime: - Parser: new RETAIN CURRENT PASSWORD and DISCARD OLD PASSWORD clauses on ALTER USER and SET PASSWORD; RETAIN/DISCARD/OLD registered as non-reserved keywords. - Privilege cache: UserRecord.AdditionalAuthenticationString, decoded from mysql.user.user_attributes '$.additional_password'. - Auth: ConnectionVerification falls back to the secondary password for mysql_native_password / caching_sha2_password / tidb_sm3_password when the primary check fails. - Privilege: APPLICATION_PASSWORD_ADMIN dynamic privilege; required when RETAIN/DISCARD is applied to another user's account (self-service is always allowed, whether the statement uses CURRENT_USER() or names the caller explicitly). - Executor: executeCreateUser rejects RETAIN/DISCARD (per MySQL); executeAlterUser and executeSetPwd promote the current authentication_string to user_attributes.additional_password on RETAIN, JSON_REMOVE on DISCARD, and silently drop the secondary on a plugin change (matching MySQL behavior). - Error codes: ErrCurrentPasswordCannotBeRetainedPluginChange (4058), ErrCurrentPasswordCannotBeRetainedEmptyNew (4059) — MySQL-compatible. - Tests: 10 unit tests in pkg/executor/test/passwordtest/dual_password_test.go plus integration test in tests/integrationtest/t/executor/dual_password.test. Ref pingcap#60587
takaidohigasi
force-pushed
the
feature/dual-password
branch
from
7386798 to
1c659b0
Compare
Contributor
Author
Coverage update — MySQL scenarios #3–#7 added (commit
|
| # | MySQL scenario | New test |
|---|---|---|
| 3 | Chained RETAIN — secondary becomes previous primary each time |
TestDualPasswordChainedRetain |
| 4 | ALTER USER without RETAIN preserves existing secondary |
TestDualPasswordAlterWithoutRetainPreservesSecondary |
| 5 | RENAME USER preserves user_attributes.$.additional_password |
TestDualPasswordRenameUserPreservesSecondary |
| 6 | DROP USER removes secondary (row gone) |
TestDualPasswordDropUserRemovesSecondary |
| 7 | Multi-user ALTER USER with trailing RETAIN / DISCARD |
TestDualPasswordMultiUserAlter |
New coverage score: 13 of 14 MySQL scenarios (up from 8 of 14). Remaining deliberate gaps: non-built-in plugin (#12), password-expiry interaction (#13), binary log (#14) — each justified in the earlier analysis.
Caveat on scenario #7
MySQL allows per-spec RETAIN / DISCARD inside a multi-user ALTER USER:
ALTER USER u1 IDENTIFIED BY 'a' RETAIN CURRENT PASSWORD,
u2 IDENTIFIED BY 'b',
u3 DISCARD OLD PASSWORD;TiDB's grammar currently attaches PasswordOrLockOptions at the statement level (shared across all user specs), so the trailing clause applies to every spec. The test pins this behavior and the docstring flags the gap; tightening to MySQL's per-spec grammar would be a larger change worth a follow-up PR if reviewers prefer.
Verification
go test -tags=intest -count=1 -v -run '^TestDualPassword' ./pkg/executor/test/passwordtest/— 16 / 16 PASS (up from 11).gofmt -l— clean;make lint— clean.make bazel_prepare— no metadata drift beyond the expectedshard_countbump.bazel build --config=ci //... --//build:with_nogo_flag=true --//build:with_rbe_flag=true— 1406 targets, exit 0.
PR branch: feature/dual-password @ 1c659b0ac.
|
@takaidohigasi: The 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. |
Contributor
Author
|
note: issue #68607 "Flaky test: TestGCScanTasks in pkg/ttl/ttlworker" is open |
Contributor
Author
|
/test unit-test |
|
@takaidohigasi: 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. |
Contributor
Author
|
I have added 2 relatively small commit, and all the test passed 🙏 |
takaidohigasi
added a commit
to takaidohigasi/tidb
that referenced
this pull request
May 31, 2026
After merging pingcap#68028 (parser PR) into this branch, the AST shape for dual-password clauses changed from a wrapper struct to a value-typed enum. This commit was meant to land with the merge but was inadvertently dropped from the merge commit's index — restoring it here so the tree compiles. * pkg/executor/simple.go: - dualPasswordOption(): switch on spec.DualPasswordOption directly, use ast.DualPasswordRetainCurrent / ast.DualPasswordDiscardOld - Drop the loadOptions case branches for the now-removed PasswordOrLockOption.RetainCurrentPassword / DiscardOldPassword constants, plus the matching info.retainCurrentPassword / discardOldPassword fields and the rejected-pair validation - Drop the executeCreateUser checks against plOptions.retainCurrentPassword / discardOldPassword (now unreachable; per-spec dualPasswordOption check above still rejects RETAIN/DISCARD in CREATE USER) - Propagate s.CurrentDualPasswordOption into the synthetic UserSpec in executeAlterUser; trigger the synthesis path for the standalone USER() DISCARD OLD PASSWORD form as well - Drop the stmtDualPwdRequested OR-with-statement-level logic in executeAlterUser since dual-password no longer flows through PasswordOrLockOption * pkg/errno/{errcode,errname}.go, pkg/util/dbterror/exeerrors/errors.go: - Add ErrMaxKeysReadExceeded (8274), pulled in from master via the parser branch and required by pkg/store/copr * errors.toml: re-synced via `make errdoc` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
I mistakenly add the ir-relevant commit to the last, but it might not relate much. please let me know if we need to add revert commit for it. -- dropped it. |
# Conflicts: # pkg/parser/parser.go
Contributor
Author
|
resolved parser conflict |
Contributor
Author
|
/retest-required |
Contributor
Author
|
I confirmed all the test passed again. |
terry1purcell
approved these changes
Jun 2, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bb7133, D3Hunter, terry1purcell 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 |
Contributor
Author
|
/retest-required |
takaidohigasi
added a commit
to takaidohigasi/tidb
that referenced
this pull request
Jun 3, 2026
…D PASSWORD) This is the behavior PR (follow-up to pingcap#68028, which added parser-only support). Implements the full MySQL 8.0 dual-password feature on top of the AST shape that landed in pingcap#68028. * Privilege cache: UserRecord.AdditionalAuthenticationString, decoded from mysql.user.user_attributes '$.additional_password'. * Auth: ConnectionVerification falls back to the secondary password for mysql_native_password / caching_sha2_password / tidb_sm3_password when the primary check fails. LDAP / socket / token plugins are skipped. * Privilege: APPLICATION_PASSWORD_ADMIN dynamic privilege; required when RETAIN/DISCARD is applied to another user's account (self-service is always allowed, whether the statement uses CURRENT_USER() or names the same user explicitly). * Executor (ALTER USER): on RETAIN, move the current authentication_string into user_attributes.$.additional_password via JSON_MERGE_PATCH and install the new hash as primary; on DISCARD, JSON_REMOVE the secondary. Reject RETAIN when the new password is empty, when the plugin is being changed, or when the current plugin doesn't support secondary passwords. Silently discard any existing secondary when the plugin changes without RETAIN (matching MySQL). * Executor (CREATE USER): reject RETAIN / DISCARD per spec — MySQL does not allow dual passwords on user creation. * Executor (SET PASSWORD): full RETAIN support for both the named-user and current-user forms, with the same privilege gating as ALTER USER. * Executor (USER() form): propagate CurrentDualPasswordOption from the AlterUserStmt into the synthetic UserSpec so the per-spec loop only needs to inspect spec.DualPasswordOption. Covers both `ALTER USER USER() IDENTIFIED BY '...' RETAIN CURRENT PASSWORD` and the standalone `ALTER USER USER() DISCARD OLD PASSWORD`. * SecureText / SecurityString: surface RETAIN/DISCARD verbatim so the redacted output still indicates the statement targets the secondary slot. * Tests: dual_password_test.go covers self-service RETAIN/DISCARD, cross-user gating with APPLICATION_PASSWORD_ADMIN, login fallback to the secondary password, plugin-change handling, SHOW CREATE USER redaction, chained RETAIN, DROP USER cleanup, RENAME USER preservation, and the corruption fallback when the stored secondary hash is unreadable. Also adds tests/integrationtest/t/executor/dual_password.test for the SQL-level round trip. Closes pingcap#60587 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
thanks so much for your help! |
takaidohigasi
added a commit
to takaidohigasi/tidb
that referenced
this pull request
Jun 3, 2026
Codex GPT-5.5 review (P1) flagged that the privilege-check inversion
in 'executor: align dual-password privilege checks with MySQL' changed
behavior so that:
* cross-user RETAIN/DISCARD with APPLICATION_PASSWORD_ADMIN alone
succeeds (was: denied -> 1227)
* self-service SET PASSWORD ... RETAIN / standalone ALTER USER ...
DISCARD with no extra privilege succeeds (was: denied -> 1227)
... but tests/integrationtest/t/executor/dual_password.test and its
.result still asserted the old behavior. The integration CI run would
have failed.
Changes:
* executeAlterUser: extend the outer ALTER USER privilege bypass so
standalone DISCARD OLD PASSWORD on the current user (no AuthOpt,
hence alterPassword=false) also skips the CREATE USER check, matching
MySQL's "self-service needs no extra privilege" rule.
* dual_password.test: flip the 4 cases that used to expect 1227 to the
new MySQL-compatible behavior (succeed). Add a dpnopriv user with
neither CREATE USER nor APPLICATION_PASSWORD_ADMIN to keep the 1227
path covered for the truly-no-priv cross-user case. Update the CREATE
USER + RETAIN negative to expect 1064 (parser rejection) instead of
1105 (the old executor stub), matching the grammar landed in pingcap#68028.
* dual_password.result: regenerated via `./run-tests.sh -r executor/dual_password`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
I updated succeeding Pull Request, I am glad if I can get ok-to-test label on it. thanks. |
This was referenced Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #60587
Problem Summary:
This PR is the parser-only half of the MySQL 8.0 dual-password feature, split from the original PR per @D3Hunter's review request for easier review.
The behavior side (executor, privilege, error codes, dual-password tests, integration test) lives in a follow-up PR which stacks on top of this one: #68393.
What changed and how does it work?
Parser surface only:
pkg/parser/parser.yRETAIN CURRENT PASSWORDandDISCARD OLD PASSWORDperUserSpec(per-spec, matching MySQL);SET PASSWORD ... RETAIN CURRENT PASSWORD. New non-reserved keywordsRETAIN,OLD(DISCARD,CURRENT,PASSWORDalready existed).pkg/parser/parser.go,pkg/parser/keywords.goparser.yviagoyacc(large mechanical diff).pkg/parser/misc.goOLD,RETAINto keyword map.pkg/parser/ast/misc.goRetainCurrentPassword,DiscardOldPasswordin thePasswordOrLockOptioniota;UserSpec.DualPasswordOptionfield;SetPwdStmt.RetainCurrentPasswordfield;Restorewrites the clauses;SecurityStringandSecureTextsurface the (non-secret) clauses for redacted output.pkg/parser/parser_test.goIDENTIFIED WITH plugin).pkg/parser/keywords_test.goTestKeywordsLengthcount bumped 679 → 681 (two new non-reserved keywords; reserved count unchanged).The
parser.goregenerated diff is ~11k lines but is purely goyacc table output. Hand-written changes are ~70 lines acrossparser.y,ast/misc.go,misc.go, and the tests.Check List
Tests
pkg/parser/parser_test.go— parse + restore round-trip for all RETAIN/DISCARD forms (ALTER USER, SET PASSWORD).cd pkg/parser && go test -tags=intest ./...— all subpackages pass.go build ./pkg/executor/... ./pkg/privilege/... ./pkg/errno/... ./pkg/util/dbterror/...— clean compile (no other package depends on the new AST symbols on this branch).bazel build --config=ci //... --//build:with_nogo_flag=true --//build:with_rbe_flag=true— 1406 targets, exit 0.Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.