Fail delete policy if pipeline exists #44438

hub-cap · 2019-07-16T15:32:32Z

If a pipeline that refrences the policy exists, we should not allow the
policy to be deleted. The user will need to remove the processor from
the pipeline before deleting the policy. This commit adds a check to
ensure that the policy cannot be deleted if it is referenced by any
pipeline in the system.

If a pipeline that refrences the policy exists, we should not allow the policy to be deleted. The user will need to remove the processor from the pipeline before deleting the policy. This commit adds a check to ensure that the policy cannot be deleted if it is referenced by any pipeline in the system.

elasticmachine · 2019-07-16T15:32:34Z

Pinging @elastic/es-core-features

hub-cap · 2019-07-17T14:53:02Z

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/bwc
@elasticmachine run elasticsearch-ci/default-distro

jbaiera

Looks great, though I think we have a minor issue with how Processors can be nested (and thus hidden) within other Processors.

jbaiera · 2019-07-17T15:24:41Z

...h/src/main/java/org/elasticsearch/xpack/enrich/action/TransportDeleteEnrichPolicyAction.java

+                    if (enrichProcessor.getPolicyName().equals(request.getName())) {
+                        listener.onFailure(
+                            new ElasticsearchStatusException("Could not delete policy [{}] because a pipeline is referencing it [{}]",
+                                RestStatus.INTERNAL_SERVER_ERROR, request.getName(), pipeline.getId()));


Should this be BAD_REQUEST instead?

I would think CONFLICT - 409

https://github.com/elastic/elasticsearch/pull/44438/files#diff-a37fe0a3791b5e0c8215af76056efd9eR85

jbaiera · 2019-07-17T15:32:44Z

...h/src/main/java/org/elasticsearch/xpack/enrich/action/TransportDeleteEnrichPolicyAction.java

+        for (PipelineConfiguration pc: pipelines) {
+            Pipeline pipeline = ingestService.getPipeline(pc.getId());
+
+            for (Processor processor: pipeline.getProcessors()) {


This might be a little bit of a conundrum in that the processors returned here could be complex processors that contain processors within them. For instance: The ForEachProcessor could contain an ExactMatchProcessor that is called on each value in field.

A quick fix to get more coverage would be to use

Suggested change

for (Processor processor: pipeline.getProcessors()) {

for (Processor processor: pipeline.flattenAllProcessors()) {

But that only flattens processors from CompoundProcessors which things like ForEachProcessor are not subclassed... Maybe we need to introduce an interface that allows for flattening all sub-processors when a processor contains any?

The ForEachProcessor and ConditionalProcessor are wrapping processor, meaning that they wrap other processor implementations. In both cases the .getProcessor() should return the processor that is wrapped. (I think those are the only 2 wrappers) ... so flattenAllProcessors should work for those.

However, if a wrapper processor wraps another wrapper e.g. if on a ForEachProcessor will result in ConditionalProcessor wrapping a ForEachProcessor which would miss the real processor. I think if add

if (processor instanceof ConditionalProcessor) { processor = ((ConditionalProcessor) processor).getProcessor()

in the loop it handle that edge case.

Other then that edge case, i think @jbaiera suggestion to use flattenAllProcessors() will work. Also, it may make sense to add ^^ to flattenAllProcessors() since a conditional processor wrapping the real processor is an implementation detail.

ug... I just took a look deeper look and getProcessor() is not called by the flattenAllProcessors() ... so your loop would need to also take into account ForEachProcessor

if (processor instanceof ConditionalProcessor) { processor = ((ConditionalProcessor) processor).getProcessor() if (processor instanceof ForEachProcessor) { processor = ((ForEachProcessor) processor).getProcessor()

We may want to consider adding in a interface like WrappedProcessor that exposes getProcessor() and possibly handle the unwrapping inside flattenAllProcessors(boolean unWrap)

all of this conversation was handled in 4ec68ec

jakelandis · 2019-07-17T19:16:24Z

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/AbstractEnrichProcessor.java

+
+public abstract class AbstractEnrichProcessor extends AbstractProcessor {
+
+    protected final String policyName;


nit: protected -> private

https://github.com/elastic/elasticsearch/pull/44438/files#diff-14bbb77a82061fbd39bca3e915d14c62R12

jakelandis · 2019-07-17T19:19:47Z

...h/src/main/java/org/elasticsearch/xpack/enrich/action/TransportDeleteEnrichPolicyAction.java

+            for (Processor processor: pipeline.getProcessors()) {
+                if (processor instanceof AbstractEnrichProcessor) {
+                    AbstractEnrichProcessor enrichProcessor = (AbstractEnrichProcessor) processor;
+                    if (enrichProcessor.getPolicyName().equals(request.getName())) {


nit: can we treat the policy names case-insensitive (e.g. equalsIgnoreCase)

we do not do this anywhere else best of my knowledge, so PUT /policyA and PUT /policya will create 2... its probably a good idea to change this tho

https://github.com/elastic/elasticsearch/pull/44438/files#diff-a37fe0a3791b5e0c8215af76056efd9eR76

The equals ignore case is here.

...h/src/main/java/org/elasticsearch/xpack/enrich/action/TransportDeleteEnrichPolicyAction.java

jakelandis · 2019-07-17T19:23:51Z

...h/src/main/java/org/elasticsearch/xpack/enrich/action/TransportDeleteEnrichPolicyAction.java

+                            new ElasticsearchStatusException("Could not delete policy [{}] because a pipeline is referencing it [{}]",
+                                RestStatus.INTERNAL_SERVER_ERROR, request.getName(), pipeline.getId()));
+                    }
+                }


should we collect all pipelines that may have a reference, so the user doesn't need to fix one, get the error, fix one, repeat. ?

[1] shows the list used to glean all of the pipelines that could be in use to display

[1] https://github.com/elastic/elasticsearch/pull/44438/files#diff-a37fe0a3791b5e0c8215af76056efd9eR70

jakelandis · 2019-07-17T19:43:16Z

...h/src/main/java/org/elasticsearch/xpack/enrich/action/TransportDeleteEnrichPolicyAction.java

+        for (PipelineConfiguration pc: pipelines) {
+            Pipeline pipeline = ingestService.getPipeline(pc.getId());
+
+            for (Processor processor: pipeline.getProcessors()) {


The ForEachProcessor and ConditionalProcessor are wrapping processor, meaning that they wrap other processor implementations. In both cases the .getProcessor() should return the processor that is wrapped. (I think those are the only 2 wrappers) ... so flattenAllProcessors should work for those.

However, if a wrapper processor wraps another wrapper e.g. if on a ForEachProcessor will result in ConditionalProcessor wrapping a ForEachProcessor which would miss the real processor. I think if add

if (processor instanceof ConditionalProcessor) { processor = ((ConditionalProcessor) processor).getProcessor()

in the loop it handle that edge case.

Other then that edge case, i think @jbaiera suggestion to use flattenAllProcessors() will work. Also, it may make sense to add ^^ to flattenAllProcessors() since a conditional processor wrapping the real processor is an implementation detail.

jakelandis · 2019-07-17T19:48:05Z

...h/src/main/java/org/elasticsearch/xpack/enrich/action/TransportDeleteEnrichPolicyAction.java

+                    if (enrichProcessor.getPolicyName().equals(request.getName())) {
+                        listener.onFailure(
+                            new ElasticsearchStatusException("Could not delete policy [{}] because a pipeline is referencing it [{}]",
+                                RestStatus.INTERNAL_SERVER_ERROR, request.getName(), pipeline.getId()));


I would think CONFLICT - 409

...n/enrich/qa/common/src/main/java/org/elasticsearch/test/enrich/CommonEnrichRestTestCase.java

…heck

jakelandis · 2019-08-13T20:21:31Z

...h/src/main/java/org/elasticsearch/xpack/enrich/action/TransportDeleteEnrichPolicyAction.java

+        if (pipelinesWithProcessors.isEmpty() == false) {
+            listener.onFailure(
+                new ElasticsearchStatusException("Could not delete policy [{}] because a pipeline is referencing it {}",
+                    RestStatus.CONFLICT, request.getName(), pipelinesWithProcessors));


I think you need to collect and join (comma seperated) these pipelines, and likely adjust the corresponding test.

Hey, this is already resolving it to a list (which is why there is no [] in the second arg in the string). I just for fun, added another processor to the test and got this error.

{"error":{"root_cause":[{"type":"status_exception","reason":"Could not delete policy [my_policy] because a pipeline is referencing it [my_pipeline, another_pipeline]"}],"type":"status _exception","reason":"Could not delete policy [my_policy] because a pipeline is referencing it [my_pipeline, another_pipeline]"},"status":409}

jakelandis

LGTM

…heck

jbaiera

LGTM!

If a pipeline that refrences the policy exists, we should not allow the policy to be deleted. The user will need to remove the processor from the pipeline before deleting the policy. This commit adds a check to ensure that the policy cannot be deleted if it is referenced by any pipeline in the system.

hub-cap added the :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP label Jul 16, 2019

jakelandis mentioned this pull request Jul 16, 2019

[ingest] Enrich documents prior to indexing #32789

Closed

55 tasks

jbaiera requested review from jakelandis and jbaiera July 16, 2019 15:34

jbaiera reviewed Jul 17, 2019

View reviewed changes

jakelandis requested changes Jul 17, 2019

View reviewed changes

hub-cap added 2 commits August 13, 2019 13:22

Merge remote-tracking branch 'upstream/enrich' into enrich_pipeline_c…

f8c5a4f

…heck

Fixes for the new ingest service method

ef0f605

jakelandis reviewed Aug 13, 2019

View reviewed changes

jakelandis approved these changes Aug 13, 2019

View reviewed changes

hub-cap added 3 commits August 13, 2019 16:44

address equals and test change

bd3fc09

Merge remote-tracking branch 'upstream/enrich' into enrich_pipeline_c…

f233529

…heck

Fix the docs

228caf0

jbaiera approved these changes Aug 14, 2019

View reviewed changes

hub-cap merged commit 9e22fd4 into elastic:enrich Aug 14, 2019

hub-cap deleted the enrich_pipeline_check branch August 14, 2019 18:43

hub-cap added the backport pending label Aug 14, 2019

hub-cap removed the backport pending label Aug 14, 2019

	for (Processor processor: pipeline.getProcessors()) {
	for (Processor processor: pipeline.flattenAllProcessors()) {


		public abstract class AbstractEnrichProcessor extends AbstractProcessor {

		protected final String policyName;

Fail delete policy if pipeline exists #44438

Fail delete policy if pipeline exists #44438

Uh oh!

Conversation

hub-cap commented Jul 16, 2019

Uh oh!

elasticmachine commented Jul 16, 2019

Uh oh!

hub-cap commented Jul 17, 2019

Uh oh!

jbaiera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelandis Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelandis Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

jbaiera left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jakelandis Jul 17, 2019 •

edited

Loading

jakelandis Jul 17, 2019 •

edited

Loading