-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[ML] Improve EIS auth call logs and fix revocation bug #132546
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
[ML] Improve EIS auth call logs and fix revocation bug #132546
Conversation
Hi @jonathan-buttner, I've created a changelog YAML for you. |
logger.debug(() -> Strings.format("Authorization entity limited to service task types, %s", authorizedTaskTypesAndModels)); | ||
|
||
// recalculate which default config ids and models are authorized now | ||
var authorizedDefaultModelIds = getAuthorizedDefaultModelIds(auth); |
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.
The bug is here and the line below where we reference auth
instead of authorizedTaskTypesAndModels
. In the fixed version we're using the auth.newLimitedToTaskTypes
response instead.
ElasticInferenceServiceAuthorizationModel.of( | ||
new ElasticInferenceServiceAuthorizationResponseEntity( | ||
List.of( | ||
new ElasticInferenceServiceAuthorizationResponseEntity.AuthorizedModel( |
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.
Previously the first response was merged with the second auth response. Now the latest successful auth response dictates the auth so we need to repeat the model again in the second response for this test.
listener.onResponse(ElasticInferenceServiceAuthorizationModel.newDisabledService()); | ||
|
||
logger.warn(errorMessage); | ||
listener.onFailure(new ElasticsearchException(errorMessage)); |
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.
We're now returning an error when a failure occurs. The caller will ignore the error and it will not affect the authorization. This way we can differentiate between a failure authorization and a successful one. Whenever onResponse
is called we'll use that response object as the source of truth for authorization.
Pinging @elastic/ml-core (Team:ML) |
logger.debug("Received authorization response"); | ||
var authorizedTaskTypesAndModels = authorizedContent.get().taskTypesAndModels.merge(auth) | ||
.newLimitedToTaskTypes(EnumSet.copyOf(implementedTaskTypes)); | ||
logger.debug(() -> Strings.format("Received authorization response, %s", auth)); |
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.
For future reference, loggers have a formatter, I believe, something like:
logger.debug("Received authorization response, {}", auth);
|
||
@Override | ||
public String toString() { | ||
return String.join(", ", authorizedModels.stream().map(AuthorizedModel::toString).toList()); |
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.
return String.join(", ", authorizedModels.stream().map(AuthorizedModel::toString).toList()); | |
return authorizedModels.stream().map(AuthorizedModel::toString).collect(Collectors.joining(", ")); |
…elasticsearch into inference-log-eis-auth
* Fixing revoking and adding logs * Fixing tests * Update docs/changelog/132546.yaml * [CI] Auto commit changes from spotless * Addressing feedback --------- Co-authored-by: elasticsearchmachine <[email protected]>
* Fixing revoking and adding logs * Fixing tests * Update docs/changelog/132546.yaml * [CI] Auto commit changes from spotless * Addressing feedback --------- Co-authored-by: elasticsearchmachine <[email protected]>
* Fixing revoking and adding logs * Fixing tests * Update docs/changelog/132546.yaml * [CI] Auto commit changes from spotless * Addressing feedback --------- Co-authored-by: elasticsearchmachine <[email protected]>
…2690) * Fixing revoking and adding logs * Fixing tests * Update docs/changelog/132546.yaml * [CI] Auto commit changes from spotless * Addressing feedback --------- Co-authored-by: elasticsearchmachine <[email protected]>
…2691) * Fixing revoking and adding logs * Fixing tests * Update docs/changelog/132546.yaml * [CI] Auto commit changes from spotless * Addressing feedback --------- Co-authored-by: elasticsearchmachine <[email protected]>
#132693) * [ML] Improve EIS auth call logs and fix revocation bug (#132546) * Fixing revoking and adding logs * Fixing tests * Update docs/changelog/132546.yaml * [CI] Auto commit changes from spotless * Addressing feedback --------- Co-authored-by: elasticsearchmachine <[email protected]> * Fixing mock registry --------- Co-authored-by: elasticsearchmachine <[email protected]>
… (#132692) * [ML] Improve EIS auth call logs and fix revocation bug (#132546) * Fixing revoking and adding logs * Fixing tests * Update docs/changelog/132546.yaml * [CI] Auto commit changes from spotless * Addressing feedback --------- Co-authored-by: elasticsearchmachine <[email protected]> * Fixing mock registry --------- Co-authored-by: elasticsearchmachine <[email protected]>
* upstream/8.19: (62 commits) Use consistent terminology for transport version resources/references (elastic#132882) (elastic#132898) Move inner records out of TransportVersionUtils (elastic#132872) (elastic#132886) Forward port release notes for v8.18.5 (elastic#132758) Add more transport version files validation (elastic#132373) (elastic#132777) Forward port release notes for v8.17.10 (elastic#132760) Refactor TransportVersion loading to support external consumers (elastic#132694) (elastic#132862) manual backporting| (elastic#132783) 9.1 docs backports for 8.19 features (elastic#132605) Migrate x-pack-deprecation REST tests (elastic#131444) (elastic#132802) Update 8.19.1.asciidoc (elastic#132755) Update wolfi (versioned) (elastic#132752) Update 8.19.0.asciidoc (elastic#132754) Prune changelogs after 8.19.2 release Bump versions after 8.19.2 release Finalize release notes for v8.19.2 Bump versions after 8.17.10 release Prune changelogs after 8.18.5 release Bump versions after 8.18.5 release Add release notes for v8.19.2 release (elastic#132696) [ML] Improve EIS auth call logs and fix revocation bug (elastic#132546) (elastic#132690) ...
This PR adds some logging to the EIS authorization call to help debugging authorization failures. While I was doing that I found a bug and made some improvements. The bug was that we didn't use the "merged" authorization object for the revoking but we use it for determining whether the service supports streaming and which task types.
After talking with the team we decided we don't need to merge the previous authorization object with the newly retrieved one. That way when we get a successful response it is always the source of truth. This also means we don't need a node reboot to perform revocation.
I'm backporting this to when we added the periodic auth call in this PR: #123639
New logs
Here's an example of what the logs will look like
Testing
Modify the acl that EIS returns in this file to change which models are authorized:
eis_dir/acl/acl.yaml
Execute EIS locally:
Run ES pointing to EIS
Change how often ES polls for authorization
Then you can stop EIS, modify the ACL file and restart it and ES will pickup the change.
Then use
GET _inference/_all
to retrieve the authorized default endpoints to ensure they match what EIS is returning.