Skip to content

update error code if ES dial connection error #797

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+10 −4
Open

update error code if ES dial connection error #797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c2da00f
update error code is ES dial connection error
rubvs Oct 5, 2025
eb54af7
code cleanup
rubvs Oct 5, 2025
d09f84d
Merge branch 'main' into update-apikey-error-code
rubvs Oct 5, 2025
a4954a4
improve error handling
rubvs Oct 9, 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
12 changes: 9 additions & 3 deletions extension/apikeyauthextension/authenticator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,12 +281,18 @@ func (a *authenticator) Authenticate(ctx context.Context, headers map[string][]s

hasPrivileges, username, err := a.hasPrivileges(ctx, authHeaderValue)
if err != nil {
if elasticsearchErr, ok := err.(*types.ElasticsearchError); ok {
if elasticsearchErr.Status == http.StatusUnauthorized || elasticsearchErr.Status == http.StatusForbidden {
switch elasticsearchErr := err.(type) {
case *types.ElasticsearchError:
Copy link
Contributor

Choose a reason for hiding this comment

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

My ide is giving me this warning btw "Type switch on errors fails on wrapped errors". I do not see any error wrapping in func (r HasPrivileges) Do, so just providing a suggestion to be safe.

Suggested change
case *types.ElasticsearchError:
var elasticsearchErr *types.ElasticsearchError
switch {
case errors.As(err, &elasticsearchErr):

switch elasticsearchErr.Status {
case http.StatusUnauthorized, http.StatusForbidden:
return ctx, status.Error(codes.Unauthenticated, err.Error())
default:
return ctx, status.Errorf(codes.Internal, "error checking privileges for API Key %q: %v", id, err)
}
default:
// If no ES error type is found, it implies an error on the TCP connection level.
Copy link
Member

Choose a reason for hiding this comment

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

Not exactly accurate. Reading the implementation of func (r HasPrivileges) Do(providedCtx context.Context) (*Response, error) {, TCP is only a subset of the errors falling into this branch. I believe it is an ElasticsearchError when http response can be parsed as an Elasticsearch error.

For example, a broken http proxy can return a body that cannot be parsed by hasPrivileges. It is not necessarily a TCP error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming we are both looking at the same method. It looks like we can also receive json.unmarshal errors. I would say a more general comment should suffice:

Suggested change
// If no ES error type is found, it implies an error on the TCP connection level.
// Received unexpected error response, return retryable error

return ctx, errorWithDetails(codes.Unavailable, fmt.Sprintf("retryable server error %q: %v", id, err), nil)
Copy link
Member

Choose a reason for hiding this comment

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

nit: why are we using errorWithDetails here instead of status.Errorf?

Copy link
Author

@rubvs rubvs Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This comes from a recommendation from @vigneshshanmugam in https://github.com/elastic/hosted-otel-collector/pull/1529#discussion_r2412175374 for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if I understand. Then why are we not using errorWithDetails on line 228 and 290? And what's the point of adding the domain with nil metadata as error info to the error?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the metadata is not used in the collector func GetSanitizedGRPCStatus but we add it in other places providing the below. I assume this metadata will appear in our logs so I think it would be good to add. Or at least keep it consistent with other used of errorWithDetails

map[string]string{
				"component": "apikeyauthextension",
				"api_key":   id,
}

}
return ctx, status.Errorf(codes.Unauthenticated, "error checking privileges for API Key %q: %v", id, err)
}
if !hasPrivileges {
cacheEntry := &cacheEntry{
Expand Down
2 changes: 1 addition & 1 deletion extension/apikeyauthextension/authenticator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func TestAuthenticator(t *testing.T) {
},
Status: 400,
}),
expectedErr: `rpc error: code = Unauthenticated desc = error checking privileges for API Key "id": status: 400, failed: [a_type], reason: a_reason`,
expectedErr: `rpc error: code = Internal desc = error checking privileges for API Key "id": status: 400, failed: [a_type], reason: a_reason`,
},
"auth_error": {
handler: newCannedErrorHandler(types.ElasticsearchError{
Expand Down