Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

serverccl: TestVerifyPassword/some_admin_with_empty_password_should_fail fails with secondary tenants #106903

Closed
knz opened this issue Jul 17, 2023 · 5 comments · Fixed by #107086
Closed

serverccl: TestVerifyPassword/some_admin_with_empty_password_should_fail fails with secondary tenants #106903

knz opened this issue Jul 17, 2023 · 5 comments · Fixed by #107086
Assignees
@yuzefovich
Labels
A-security C-test-failure Broken test (automatically or manually discovered). db-cy-23 T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@knz
Copy link
Contributor

knz commented Jul 17, 2023 •
edited
Loading

Epic: CRDB-26687
Emerged from #106905.
Informs #76378.

Describe the problem

The sub-test some_admin_with_empty_password_should_fail in TestVerifyPassword fails when pointed to a secondary tenant.

This is surprising (and possibly indicative of a security bug) given the other sub-tests pass successfully.

To Reproduce

On top of the #106905 branch, modify the test to replace TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet by TestTenantAlwaysEnabled.

Expected behavior

The test should work the same whether or not a secondary tenant is used.
In an ideal world the parameter DefaultTestTenant is omitted and the default value Probabilistic can be used.

Jira issue: CRDB-29757
@knz knz added the C-test-failure Broken test (automatically or manually discovered). label Jul 17, 2023
knz added a commit to knz/cockroach that referenced this issue Jul 17, 2023
@knz
serverccl: mark TestVerifyPassword as problematic
4578544 
Followup issue:  cockroachdb#106903

Release note: None
@knz knz mentioned this issue Jul 17, 2023
Merged
@knz knz added A-security T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Jul 17, 2023
@knz knz mentioned this issue Jul 17, 2023
Merged
knz added a commit to knz/cockroach that referenced this issue Jul 17, 2023
@knz
serverccl: remove remaining uses of TODOTestTenantDisabled
c701779 
This also simplifies some tests so they don't need to start an entire
cluster.

The remaining mentions of `TODOTestTenantDisabled` are handled in
issues  cockroachdb#106897, cockroachdb#106903 and cockroachdb#106901.

Release note: None
craig bot pushed a commit that referenced this issue Jul 17, 2023
@knz
Merge #106905
0099578 
106905: serverccl: mark `TestVerifyPassword` as problematic r=yuzefovich a=knz

Informs #76378.
Epic: CRDB-18499
Followup issue:  #106903



Co-authored-by: Raphael 'kena' Poss <[email protected]>
@yuzefovich yuzefovich self-assigned this Jul 18, 2023
@yuzefovich yuzefovich mentioned this issue Jul 18, 2023
Closed
knz added a commit to knz/cockroach that referenced this issue Jul 18, 2023
@knz
serverccl: remove remaining uses of TODOTestTenantDisabled
e5a7c25 
This also simplifies some tests so they don't need to start an entire
cluster.

The remaining mentions of `TODOTestTenantDisabled` are handled in
issues  cockroachdb#106897, cockroachdb#106903 and cockroachdb#106901.

Release note: None
craig bot pushed a commit that referenced this issue Jul 18, 2023
@knz
Merge #106910
1cee475 
106910: serverccl: remove remaining uses of TODOTestTenantDisabled r=yuzefovich a=knz

Epic: CRDB-18499
Informs #76378.

This also simplifies some tests so they don't need to start an entire
cluster.

The remaining mentions of `TODOTestTenantDisabled` are handled in
issues  #106897, #106903 and #106901.



Co-authored-by: Raphael 'kena' Poss <[email protected]>
@yuzefovich yuzefovich mentioned this issue Jul 18, 2023
Merged
@craig craig bot closed this as completed in #107086 Jul 18, 2023
@craig craig bot closed this as completed in 6f42039 Jul 18, 2023
@DrewKimball
Copy link
Collaborator

Just saw this flake: https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_Ci_TestsGcpLinuxX8664BigVm_CclUnitTests/11000472?hideProblemsFromDependencies=false&hideTestsFromDependencies=false&expandBuildChangesSection=true&expandBuildProblemsSection=true&expandBuildTestsSection=true

@yuzefovich yuzefovich reopened this Jul 20, 2023
@DrewKimball
Copy link
Collaborator

Another here: https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_Ci_TestsGcpLinuxX8664BigVm_CclUnitTests/11000840?hideProblemsFromDependencies=false&hideTestsFromDependencies=false&expandBuildChangesSection=true&expandBuildProblemsSection=true&expandBuildTestsSection=true

@yuzefovich
Copy link
Member

Taking a look since I recently touched it.

@yuzefovich
Copy link
Member

I can't repro on master. It looks likely it's caused by #107235 that was in the bors batch.

@DrewKimball
Copy link
Collaborator

I can't repro on master. It looks likely it's caused by #107235 that was in the bors batch.

Didn't see that, good catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@yuzefovich yuzefovich

Labels
A-security C-test-failure Broken test (automatically or manually discovered). db-cy-23 T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
3 participants
@knz @yuzefovich @DrewKimball