Skip to content

Conversation

alanshaw
Copy link
Member

This PR adds support to the client library for authorized retrievals.

Essentially it allows one or more delegations to be attached to the query, that are sent in a HTTP header X-Agent-Message.

The server currently does nothing with the delegation - that is the next PR!

@alanshaw alanshaw requested a review from a team September 29, 2025 13:53
@alanshaw alanshaw requested a review from volmedo as a code owner September 29, 2025 13:53
Copy link

codecov bot commented Sep 29, 2025

Codecov Report

❌ Patch coverage is 51.35135% with 18 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/client/client.go 51.35% 14 Missing and 4 partials ⚠️
Files with missing lines Coverage Δ
pkg/types/types.go 92.30% <ø> (ø)
pkg/client/client.go 42.24% <51.35%> (+5.77%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@volmedo volmedo left a comment

Choose a reason for hiding this comment

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

LGTM.

Just to confirm my understanding, the idea is the client will give the indexing service the delegations it needs to then retrieve (with auth) indexes from nodes. So these delegations will always be authorizations to space/content/retrieve on the space(s) the query is targeting.

Is that correct? If affirmative, should we enforce attached delegations are for space/content/retrieve? I understand the mechanism is generic and can be used for other stuff, but might be interesting to fail fast until it is used for something else. Just a quick thought.

"errors"
"fmt"
"io"
gohttp "net/http"
Copy link
Member

Choose a reason for hiding this comment

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

totally dismissible nitpitck (and I know this was already here before your changes): I'd rather this to be let as http and alias go-ucanto's http package instead.

c := Client{
servicePrincipal: servicePrincipal,
serviceURL: serviceURL,
httpClient: gohttp.DefaultClient,
Copy link
Member

Choose a reason for hiding this comment

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

(I know this was already this way before your PR) it'd be great if this wasn't the DefaultClient but a client with a timeout

Suggested change
httpClient: gohttp.DefaultClient,
httpClient: client := http.Client{
Timeout: 30 * time.Second,
},

Comment on lines +141 to +156
if len(query.Delegations) > 0 {
invs := make([]invocation.Invocation, 0, len(query.Delegations))
for _, d := range query.Delegations {
invs = append(invs, d)
}
msg, err := message.Build(invs, nil)
if err != nil {
return nil, fmt.Errorf("building agent message: %w", err)
}
headerValue, err := hcmsg.EncodeHeader(msg)
if err != nil {
return nil, fmt.Errorf("encoding %s header: %w", hcmsg.HeaderName, err)
}
req.Header.Set(hcmsg.HeaderName, headerValue)
}
Copy link
Member

Choose a reason for hiding this comment

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

Think it's worth asserting the headerValue is not > 4Kib, iirc there is an option for this: WithMaxSize - Or is the intention for this case to be handled by the checks on status code below?

Copy link
Member Author

Choose a reason for hiding this comment

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

hcmsg.EncodeHeader(...) does this and returns an error.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants