[ML] [Transforms] prefer secondary auth headers for transforms #86757
Conversation
|
Pinging @elastic/ml-core (Team:ML) |
|
Hi @benwtrent, I've created a changelog YAML for you. |
…:benwtrent/elasticsearch into feature/ml-transforms-prefer-sec-headers
There was a problem hiding this comment.
This commit also fixes a bug with
_starttransform that didn't utilize the stored headers to create the destination index.
Shouldn't there be a change to TransportStartTransformAction.java for this part?
Have you got a commit locally that made that part of the change but isn't pushed to GitHub?
| @@ -24,6 +24,12 @@ Requires the following privileges: | |||
| * source indices: `read`, `view_index_metadata` | |||
| * destination index: `read`, `create_index`, `index`. If a `retention_policy` is configured, the `delete` privilege is | |||
There was a problem hiding this comment.
You could also modify the start transform docs page to remove the index permission requirements if they're no longer needed.
The x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/persistence/TransformIndex.java contains the fix. Its about the creating destination index as part of |
Co-authored-by: David Roberts <[email protected]>
Ah, OK, thanks. It’s getting late for me today. I’ll have a proper look tomorrow. |
There was a problem hiding this comment.
LGTM on the secondary credentials.
I think there's another problem in the code with permissions for start vs create. That could be fixed in a separate PR as it's really nothing to do with secondary credentials, which is the subject of this PR.
Changing how permissions work for transforms is a more noticeable change for end users than introducing the secondary credentials option (which is more of an internal technical detail). So ideally we'd have a release note that emphasises this point.
Given that the PR is mixing two things, please can you do one of the following (whichever is less effort):
- Move the permissions fix for the destination index permissions into a separate PR and also fix the permissions required on the source index by the start API in that PR
- Open a new PR to fix the permissions required on the source index by the start API but release note it as fixing both the permissions problems
Either way that means we'll have something in the release notes that alerts users to the change. I don't see how anybody can complain because we're making the distinction in permissions required for different operations clearer and no operation will require more permissions than it did before.
When creating and updating transforms, it is possible for clients to provide secondary headers.
When PUT, _preview, _update is called with secondary authorization headers, those are then used or stored with the transform.
closes: #86731