Skip to content
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

[TRANSFORM] Transform permissions enhancements to support Fleet install #93259

Closed
droberts195 opened this issue Jan 26, 2023 · 6 comments
Closed
Labels
>enhancement :ml/Transform Transform Team:ML Meta label for the ML team

Comments

@droberts195
Copy link
Contributor

elastic/kibana#137278 details a requirement for the Fleet installer to be able to install transforms where the kibana_system user does not have permissions to access the source and/or destination indices of the transform. In some cases the user installing the package may have access to the transform source and destination indices, while in other cases they may not, so this situation needs to be accounted for too.

The plan is to make the following changes to transforms in Elasticsearch to support this:

  • When defer_validation=true is set when the transform is created, we should still check whether the supplied credentials are adequate. However, if they are not and defer_validation=true is set, we should record that the credentials are invalid but still let the transform be created.
  • If a transform has invalid credentials this should be reported in the transform stats response. The health section of the response should show that the transform is red. In addition, it needs to be very easy for Kibana to determine whether each transform has invalid credentials from the stats response. Therefore, it may be best to add a dedicated field to reveal this in addition to the health information, which has a more complex structure.
  • If an unattended transform has invalid credentials then it should still be possible to start it, but it should not attempt any searches until it has been updated with valid credentials.
  • Starting a transform with invalid credentials that is not an unattended transform should return an error.

One problem here is that we'll be relying on _has_privileges to determine whether the credentials are valid, and that doesn't work for cross cluster search. This functionality will still be useful for transforms that don't search cross-cluster. But for transforms that do cross cluster search we'll end up documenting that the user who installs the Fleet package needs to have all the permissions required to run the transform, and relying on people to follow that documentation.

cc @qn895

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

@hendrikmuhs
Copy link

Sounds like a good plan, however I have to comment this wording:

The plan is to make the following changes to transforms in Elasticsearch to support this:

I think most of the requirements listed already work exactly as described, e.g. unattended in combination defer_validation=true lets you create and start a transform and health will report red.

There are some details which don't work as described, for example:

we should record that the credentials are invalid but still let the transform be created

Afaik it currently skips over the privilege check. To implement this we would need to still make the check but don't error. For some details I am not sure it is feasible, e.g. it might be complex to check if a destination index can potentially be created without creating it. We can check the rights to create for sure, however I don't think we can check every possible reason why this might fail later, especially as by design the system isn't completely setup yet. I suggest to relax this and record only that install runs in unattended mode and that not all parts are validated.

Note that health should report the credentials problem already, however maybe not as user friendly:

In addition, it needs to be very easy ...

Testing

I think the 1st step should be to create fully automated test cases for this. However best to my knowledge with the tooling that is available in elasticsearch it will be hard, #89759 only added very basic yaml tests that verify that you can create unattended transforms with insufficient privileges. It doesn't test health checks on them and neither does it check that they start working after all other parts are in place. I suggest to consult QA, maybe this can be implemented using the new qa framework or maybe a kibana integration test is better suited. I think we should be pragmatic.

@przemekwitek
Copy link
Contributor

It doesn't test health checks on them and neither does it check that they start working after all other parts are in place. I suggest to consult QA, maybe this can be implemented using the new qa framework or maybe a kibana integration test is better suited.

Thanks for this hint Hendrik! I'll start with expanding the scope of the yaml tests you created for unattended and if they are not enough, I'll consult QA.

@droberts195
Copy link
Contributor Author

it might be complex to check if a destination index can potentially be created without creating it. We can check the rights to create for sure, however I don't think we can check every possible reason why this might fail later, especially as by design the system isn't completely setup yet.

_has_privileges should be able to check permissions to create the destination index. I agree that creating the destination index could fail for some other reason that's not related to permissions. But if that is a persistent problem then it would be an unexpected scenario, whereas with Fleet package installs having insufficient permissions is an expected scenario. The UI needs to be able to easily find out if the expected problem is still a problem so that another user can be told to click somewhere to fix it.

This project is all about how we can make the overall experience of installing a Fleet package nicer. The outline plan is:

  1. Fleet package installer installs and starts the transforms even if the person doing the install does not have permissions on the transform source or destination indices.
  2. Later on, other users see banners (or some other warnings) elsewhere in Kibana telling them that the transforms need different credentials. They can then click somewhere to authorize the transform (which can be implemented as a no-op update to the transform, that just supplies different auth headers).

I'll start with expanding the scope of the yaml test

I am not sure yaml tests are the best way to go for testing this long sequence of events that will require multiple intermediate states to be waited for. Could it be simulated in the transform/qa/multi-node-tests instead?

@przemekwitek
Copy link
Contributor

I am not sure yaml tests are the best way to go for testing this long sequence of events that will require multiple intermediate states to be waited for. Could it be simulated in the transform/qa/multi-node-tests instead?

Sure. I'll use yaml only for the simple scenarios. What I meant by "expanding the scope" referred to adding more use-cases that possibly result from this issue.
For the more complex scenarios, the Java tests (multi-node tests) are the way to go, as usual.

@droberts195
Copy link
Contributor Author

Implemented by #94420 and #94724.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :ml/Transform Transform Team:ML Meta label for the ML team
Projects
None yet
Development

No branches or pull requests

4 participants