maint(pam/integration-tests): Run all the tests with a shared authd in race mode #593

3v1n0 · 2024-10-17T14:05:05Z

Run by default all the tests using an unique instance of authd daemon running so that we can test better that all the codepaths work well with lots of concurrency.

~~Leaving this as a draft for now because it has dependencies on some commits of #583~~

UDENG-5377

codecov-commenter · 2024-10-17T14:15:44Z

Codecov Report

Attention: Patch coverage is 85.18519% with 4 lines in your changes missing coverage. Please review.

Project coverage is 83.13%. Comparing base (36511cd) to head (b8e50b5).
Report is 242 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/brokers/manager.go	33.33%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #593      +/-   ##
==========================================
- Coverage   83.43%   83.13%   -0.31%     
==========================================
  Files          83       96      +13     
  Lines        8689     9630     +941     
  Branches       74       74              
==========================================
+ Hits         7250     8006     +756     
- Misses       1111     1241     +130     
- Partials      328      383      +55

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

denisonbarbosa

I'm not going to lie, I'm a bit worried... I like the idea of using a single instance as much as possible, but it will be handling an insane amount of requests (which is not a real use case [how many users do we expect to authenticate in the "same" moment?]).

This feels like it would be a pain to debug and fix if it starts going wrong, no? I'm just wondering if we might not be shooting ourselves in the foot here...

@adombeck, @didrocks any opinions regarding the above?

Despite the rambling: changes look good to me but a comment about naming, easy to fix.

denisonbarbosa · 2024-11-27T11:17:53Z

pam/integration-tests/helpers_test.go

+	gPasswd   string
+	groups    string


These are a bit vague... Do you mind updating them to indicate better what they mean or add a comment for them?

Changed the naming, let me know if it's better.

3v1n0 · 2024-11-27T11:36:12Z

Indeed it's not meant to respect a real scenario, but experience taught me that the more we stress a system, the more potential issues can arise.

So, in case we can also make this opt-in and just happen in the race job though.

didrocks · 2024-11-27T12:19:49Z

I agree that as we don’t have a context ID, this could make things difficult.

How hard would it be to only run in this mode if we are in race mode?

3v1n0 · 2024-11-29T04:11:49Z

I agree that as we don’t have a context ID, this could make things difficult.

How hard would it be to only run in this mode if we are in race mode?

Not much, it's just about inverting the logic, so it should be doable.

3v1n0 · 2024-12-11T05:31:44Z

Blocked by #661 for now, let's wait to reconsider

3v1n0 · 2024-12-20T03:00:57Z

How hard would it be to only run in this mode if we are in race mode?

This is now handled via efe85f9

Use const variables to build expected example broker user names so that we can be more reliable when testing them

…er names This is valid for the standard case, while on specific cases we still rely on manual names

This includes very long user names or prompt values

So we can run a shared authd for everything

… tests So that we won't clash with other test in the suite

…l tests So we can run a shared authd for everything

We removed the temporary dir on test cleanup but we may want to keep the lifecycle of the daemon bound to the daemon itself instead

The user name now has only to contain the pre-check value prefix while we don't care about its location to allow more combination of cases (such as pre-check MFA users).

… daemon The daemon is ref-counted in order to decide when killing it, so that multiple tests can share the same daemon if they want to

…nces It can be useful for debugging purposes

In case of issues it allows easier debugging

…by default In case, the env variable AUTHD_TESTS_USE_SHARED_AUTHD_INSTANCES can be set to force it even in non-race test runs

adombeck · 2025-01-23T10:57:51Z

I rebased on main again. I skipped ed3bc22 (from this branch) during the rebase which had conflicts with 6c1bf10 (from main). @3v1n0 please take a look if that was the correct decision.

3v1n0 · 2025-01-23T18:45:59Z

I rebased on main again. I skipped ed3bc22 (from this branch) during the rebase which had conflicts with 6c1bf10 (from main). @3v1n0 please take a look if that was the correct decision.

Sure thing, I had done it last night too (see), since that was the initial version of the fix that then I decided to improve in 6c1bf10

adombeck · 2025-02-03T11:57:52Z

examplebroker/broker.go

@@ -512,7 +512,7 @@ func qrcodeData(sessionInfo *sessionInfo) (content string, code string) {
 		"https://www.ubuntu-it.org/",
 	}

-	if strings.HasPrefix(sessionInfo.username, "user-integration-qrcode-static") {
+	if strings.HasPrefix(sessionInfo.username, UserIntegrationQrcodeStaticPrefix) {


-UserIntegrationQrcodeStaticPrefix +UserIntegrationQRcodeStaticPrefix

adombeck · 2025-02-03T12:07:59Z

examplebroker/broker.go

-	if info.sessionMode == auth.SessionModePasswd {
-		info.neededAuthSteps++
-		info.pwdChange = mustReset
-	}
-


examplebroker: Fix support for integration tests users in passwd mode

It would be nice if you could explain in the commit message what was broken and why this change fixes it.

adombeck · 2025-02-03T12:12:49Z

internal/brokers/broker.go

@@ -225,6 +225,7 @@ func (b Broker) cancelIsAuthenticated(ctx context.Context, sessionID string) {

 // UserPreCheck calls the broker corresponding method.
 func (b Broker) UserPreCheck(ctx context.Context, username string) (userinfo string, err error) {
+	log.Debugf(context.TODO(), "Pre-Checking user %q", username)


-Pre-Checking +Pre-checking

adombeck · 2025-02-03T12:22:48Z

pam/integration-tests/helpers_test.go

+	sharedAuthdInstance = authdInstance{}
+)
+
+func runAuthdForTesting(t *testing.T, gpasswdOutput, groupsFile string, currentUserAsRoot bool, args ...testutils.DaemonOption) (string, func(), func()) {


It's not clear from the function signature what the return values are for, so I would prefer to use named return values here.

adombeck · 2025-02-03T14:25:41Z

pam/integration-tests/helpers_test.go

+	sharedInstance := testutils.IsRace()
+	if s, err := strconv.ParseBool(os.Getenv("AUTHD_TESTS_USE_SHARED_AUTHD_INSTANCES")); err == nil {
+		sharedInstance = s
+	}
+
+	if !sharedInstance {


Suggested change

sharedInstance := testutils.IsRace()

if s, err := strconv.ParseBool(os.Getenv("AUTHD_TESTS_USE_SHARED_AUTHD_INSTANCES")); err == nil {

sharedInstance = s

}

if !sharedInstance {

useSharedInstance := testutils.IsRace()

if b, err := strconv.ParseBool(os.Getenv("AUTHD_TESTS_USE_SHARED_AUTHD_INSTANCES")); err == nil {

useSharedInstance = b

}

if !useSharedInstance {

3v1n0 force-pushed the pam-tests-shared-authd branch 3 times, most recently from 4bd8ec9 to 1493b08 Compare November 22, 2024 15:37

3v1n0 marked this pull request as ready for review November 22, 2024 15:48

3v1n0 requested a review from a team as a code owner November 22, 2024 15:48

denisonbarbosa reviewed Nov 27, 2024

View reviewed changes

3v1n0 marked this pull request as draft December 11, 2024 05:31

3v1n0 force-pushed the pam-tests-shared-authd branch 5 times, most recently from 1cc9930 to 5265441 Compare December 20, 2024 02:54

3v1n0 marked this pull request as ready for review December 20, 2024 02:59

3v1n0 force-pushed the pam-tests-shared-authd branch from 5265441 to efe85f9 Compare December 20, 2024 02:59

3v1n0 force-pushed the pam-tests-shared-authd branch from efe85f9 to e02ae0d Compare December 20, 2024 03:08

3v1n0 requested a review from denisonbarbosa December 20, 2024 03:08

3v1n0 mentioned this pull request Dec 21, 2024

Data race in pam/integration-tests #703

Closed

3v1n0 force-pushed the pam-tests-shared-authd branch from f176f2c to 3a62dcf Compare January 9, 2025 03:02

3v1n0 mentioned this pull request Jan 21, 2025

Replace spaces in test names with underscores #743

Merged

adombeck force-pushed the pam-tests-shared-authd branch from 3a62dcf to ed3bc22 Compare January 22, 2025 23:17

3v1n0 changed the title ~~maint(pam/integration-tests): Run all the tests with a shared authd when possible~~ maint(pam/integration-tests): Run all the tests with a shared authd in race mode Jan 22, 2025

3v1n0 force-pushed the pam-tests-shared-authd branch 2 times, most recently from 8c108d7 to dc00dfe Compare January 22, 2025 23:35

examplebroker: Add support for local groups integration users

64a7f77

3v1n0 added 18 commits January 23, 2025 11:55

examplebroker: Expose user integration prefixes and use them in tests

d1b394d

Use const variables to build expected example broker user names so that we can be more reliable when testing them

pam/integration-tests/vhs: Add function to define integration test us…

02f2fff

…er names This is valid for the standard case, while on specific cases we still rely on manual names

pam/integration-tests/vhs: Sanitize multi-line usernames in tape files

a845cc6

pam/integration-tests/vhs: Support waiting for multi-line values

02b5b2b

This includes very long user names or prompt values

pam/integration-tests/gdm: Use unique test user names

3c07eba

pam/integration-tests/native: Ensure user names are unique for all tests

f4a7eab

So we can run a shared authd for everything

examplebroker: Fix support for integration tests users in passwd mode

d24a623

pam/integration-tests/native-passwd: Use unique user names in all the…

741b6b0

… tests So that we won't clash with other test in the suite

pam/integration-tests/cli-passwd: Ensure user names are unique for al…

b89d4b1

…l tests So we can run a shared authd for everything

testutils/daemon: Remove temporary dir when process has been stopped

c199174

We removed the temporary dir on test cleanup but we may want to keep the lifecycle of the daemon bound to the daemon itself instead

examplebroker: Use simpler check for "pre-check" users

4ece8d9

The user name now has only to contain the pre-check value prefix while we don't care about its location to allow more combination of cases (such as pre-check MFA users).

pam/integration-tests/ssh: Use unique names for SSH tests

c59ea0a

pam/integration-tests/helpers: Add support for running a shared authd…

29c47c7

… daemon The daemon is ref-counted in order to decide when killing it, so that multiple tests can share the same daemon if they want to

pam/integration-tests: Run all the tests that we can with a shared authd

5ef7f57

pam/integration-tests: Allow to run tests with individual authd insta…

e4cd61f

…nces It can be useful for debugging purposes

pam/integration-tests: Allow single authd instances for each test

7b73927

In case of issues it allows easier debugging

brokers/manager: Run cleanup function, if any, on errors

4acb599

pam/integration-tests: Use a shared authd instance only in race mode …

b8e50b5

…by default In case, the env variable AUTHD_TESTS_USE_SHARED_AUTHD_INSTANCES can be set to force it even in non-race test runs

adombeck force-pushed the pam-tests-shared-authd branch from dc00dfe to b8e50b5 Compare January 23, 2025 10:58

3v1n0 requested review from adombeck and removed request for denisonbarbosa January 23, 2025 16:15

3v1n0 mentioned this pull request Jan 30, 2025

pam/native: Do not support device authentication mode in polkit #768

Open

adombeck reviewed Feb 3, 2025

View reviewed changes

adombeck approved these changes Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

maint(pam/integration-tests): Run all the tests with a shared authd in race mode #593

maint(pam/integration-tests): Run all the tests with a shared authd in race mode #593

3v1n0 commented Oct 17, 2024 •

edited

Loading

codecov-commenter commented Oct 17, 2024 •

edited

Loading

denisonbarbosa left a comment

denisonbarbosa Nov 27, 2024

3v1n0 Dec 20, 2024

3v1n0 commented Nov 27, 2024

didrocks commented Nov 27, 2024

3v1n0 commented Nov 29, 2024

3v1n0 commented Dec 11, 2024

3v1n0 commented Dec 20, 2024

adombeck commented Jan 23, 2025 •

edited

Loading

3v1n0 commented Jan 23, 2025

adombeck Feb 3, 2025

adombeck Feb 3, 2025

adombeck Feb 3, 2025

adombeck Feb 3, 2025

adombeck Feb 3, 2025

maint(pam/integration-tests): Run all the tests with a shared authd in race mode #593

Are you sure you want to change the base?

maint(pam/integration-tests): Run all the tests with a shared authd in race mode #593

Conversation

3v1n0 commented Oct 17, 2024 • edited Loading

codecov-commenter commented Oct 17, 2024 • edited Loading

Codecov Report

denisonbarbosa left a comment

Choose a reason for hiding this comment

denisonbarbosa Nov 27, 2024

Choose a reason for hiding this comment

3v1n0 Dec 20, 2024

Choose a reason for hiding this comment

3v1n0 commented Nov 27, 2024

didrocks commented Nov 27, 2024

3v1n0 commented Nov 29, 2024

3v1n0 commented Dec 11, 2024

3v1n0 commented Dec 20, 2024

adombeck commented Jan 23, 2025 • edited Loading

3v1n0 commented Jan 23, 2025

adombeck Feb 3, 2025

Choose a reason for hiding this comment

adombeck Feb 3, 2025

Choose a reason for hiding this comment

adombeck Feb 3, 2025

Choose a reason for hiding this comment

adombeck Feb 3, 2025

Choose a reason for hiding this comment

adombeck Feb 3, 2025

Choose a reason for hiding this comment

3v1n0 commented Oct 17, 2024 •

edited

Loading

codecov-commenter commented Oct 17, 2024 •

edited

Loading

adombeck commented Jan 23, 2025 •

edited

Loading