-
Notifications
You must be signed in to change notification settings - Fork 273
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.
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
Return network error when logging in and the network connection fails #3432
base: master
Are you sure you want to change the base?
Conversation
This looks like some duplicate groups are being listed @marktheunissen - needs a fix on the minio upstream side. |
ab4ea95
to
56f4c9e
Compare
@harshavardhana I've tracked this error down to a specific location, this integration test is adding groups and then when getting them back, the duplicates are being introduced. The group name And then the same group name The function was modified and no longer does I can keep working on it but I thought you might be able to find the issue faster than me since it's the first time I've looked at this. |
Made a simpler test that is easy to work with and reached out to a few people here: https://github.com/minio/minio/pull/20388/files |
edb2c35
to
39b70c3
Compare
@@ -19,7 +19,7 @@ jobs: | |||
- name: Set up Go | |||
uses: actions/setup-go@v5 | |||
with: | |||
go-version: 1.22.5 | |||
go-version: 1.22 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this to 1.22 instead of 1.22.7, as I think the rest of the build pipelines all use the latest go patch version... correct me if I'm wrong.
api/errors.go
Outdated
@@ -111,6 +112,11 @@ func ErrorWithContext(ctx context.Context, err ...interface{}) *CodedAPIError { | |||
errorCode = 401 | |||
errorMessage = ErrInvalidLogin.Error() | |||
} | |||
if errors.Is(err1, ErrNetworkError) { | |||
detailedMessage = "" | |||
errorCode = 401 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we report 503 Service Unavailable
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep will change it
39b70c3
to
54ffca7
Compare
Displays "unable to login due to network error" to the user when a network error is encountered.
@harshavardhana Up to you, I know MINIO_SERVER_URL is deprecated but it looks like people still use it and get into trouble with DNS.
Fixes: #3427