Skip to content

GODRIVER-3513 Fix OIDC Evergreen task failures. #1995

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

Merged
merged 2 commits into from
Mar 25, 2025
Merged

GODRIVER-3513 Fix OIDC Evergreen task failures. #1995

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
70f4c49
GODRIVER-3513 Fix OIDC Evergreen task failures.
matthewdale Mar 21, 2025
b450dc6
Use --no-run-if-empty for xargs in evg-gather-test-suites task.
matthewdale Mar 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .evergreen/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ functions:
handle-test-artifacts:
- command: gotest.parse_files
params:
optional_output: "true"
files:
- "src/go.mongodb.org/mongo-driver/*.suite"
- command: ec2.assume_role
Expand Down Expand Up @@ -1932,7 +1933,10 @@ task_groups:
- name: testoidc_task_group
setup_group_can_fail_task: true
setup_group_timeout_secs: 1800
teardown_task_can_fail_task: true
# TODO(DRIVERS-3141): Uncomment the following line once the teardown bug is
# fixed. See DRIVERS-3141 for more context.
#
# teardown_task_can_fail_task: true
Copy link
Member

@prestonvasquez prestonvasquez Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does just commenting out this line resolve the issues described in GODRIVER-3513? All three seem specific to the teardown behavior.

Can we just remove this line? With the changes in this PR it's unclear to me why would ever add it back: these tests no longer fail on teardown.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't fix the teardown step error, it just prevents the error from failing the whole task. The error is caused by a bug in drivers-evergreen-tools and must be fixed there (tracked by DRIVERS-3141). Since it's not something we can fix in the Go Driver repo, the best option seemed to be reverting to the previous behavior, which ignores the teardown errors.

We could technically resolve the task failures for both testoidc_task_group and testgcpoidc_task_group by uncommenting teardown_task_can_fail_task: true (the default is false), but I prefer to keep that enabled because it exposes teardown bugs.

teardown_group_timeout_secs: 180 # 3 minutes (max allowed time)
setup_group:
- func: setup-system
Expand Down
2 changes: 1 addition & 1 deletion Taskfile.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ tasks:

evg-test-deployed-lambda-aws: bash ${DRIVERS_TOOLS}/.evergreen/aws_lambda/run-deployed-lambda-aws-tests.sh

evg-gather-test-suites: find . -name \*.suite | xargs tar czf test_suite.tgz
evg-gather-test-suites: find . -name \*.suite | xargs tar czf test_suite.tgz || true
Copy link
Member

@prestonvasquez prestonvasquez Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would / should there not be *.suite files? Also, the current solution doesn't fail when I run it locally. Does it fail in a spawn host?

❯ task evg-gather-test-suites
task: [evg-gather-test-suites] find . -name \*.suite | xargs tar czf test_suite.tgz

If this is a legitimate use case, then suggest we prevent the tar command from running when no files are found rather than appending || true to mask errors. Can use --no-run-if-empty

find . -name \*.suite  | xargs --no-run-if-empty tar czf test_suite.tgz

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests, like the OIDC prose tests), aren't implemented as Go tests and don't write "*.suite" files. We still want to run handle-test-artifacts to collect logs, tho.

The more correct solution would be to re-implement the OIDC prose tests as Go tests, and possibly separate handle-test-artifacts into two functions: one that collects Go test artifacts, and another that collects all other logs. However, that's a much larger change, so I did the quick version here. I created GODRIVER-3517 for re-implementing the OIDC prose tests as Go tests.

The command does fail on spawn hosts with an error like this (see here):

task: [evg-gather-test-suites] find . -name \*.suite | xargs tar czf test_suite.tgz
tar: Cowardly refusing to create an empty archive
Try 'tar --help' or 'tar --usage' for more information.
task: Failed to run task "evg-gather-test-suites": exit status 123

I'm not sure why it doesn't fail on macOS. I'd guess it's a difference in the tar version (GNU tar vs bsdtar).

The --no-run-if-empty fix is much more elegant, I'll try that!


build-kms-test: go build ${BUILD_TAGS} ./internal/cmd/testkms

Expand Down
13 changes: 8 additions & 5 deletions internal/cmd/testoidcauth/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package main

import (
"context"
"errors"
"fmt"
"log"
"os"
Expand Down Expand Up @@ -1781,14 +1782,16 @@ func machine51azureWithNoUsername() error {

func machine52azureWithBadUsername() error {
opts := options.Client().ApplyURI(uriSingle)
cred := options.Credential{
AuthMechanism: "MONGODB-OIDC",
Username: "bad",
}
opts.SetAuth(cred)

if opts == nil {
return fmt.Errorf("machine_5_2: failed parsing uri: %q", uriSingle)
}
if opts.Auth == nil || opts.Auth.AuthMechanism != "MONGODB-OIDC" {
return errors.New("machine_5_2: expected URI to contain MONGODB-OIDC auth information")
}

opts.Auth.Username = "bad"

client, err := mongo.Connect(opts)
if err != nil {
return fmt.Errorf("machine_5_2: failed connecting client: %v", err)
Expand Down
Loading