[Connector API] Support soft deletes of connectors #118282

jedrazb · 2024-12-09T16:15:00Z

Soft deletes for connectors

Add support for soft-deletes of connectors. Why?

In order to improve UX on connectors on agentless we need to track what connectors were deleted to detect and propagate state change to fleet to add/remove corresponding policies

Changes

define new system index .connectors-deleted that stores deleted connectors
adapt delete, get and list operations logic to support this feature
this change is backward compatible, nothing will be broken for existing users
permission-wise it's also backward compatible, we still rely on index-level access to .elastic-connectors-* pattern
- Although this is likely to change soon once we actually enforce manage_connector and monitor_connector privileges
update docs
add yaml tests and unit tests

Why new system index and not new `is_deleted` flag in existing index

The existing connector index is not a system index. Instead, we rely on index templates with non-dynamic mappings. This means that even if we update the template and incorporate logic to use the is_deleted field in the current index, it won’t be backward compatible, since the new field wouldn’t be present in mappings for users who created their connector indexes in the past. The is_deleted field will be included in the source, but we wouldn’t be able to run any queries against it, making it useless.

By using a separate index, we maintain backward compatibility for users who depend on direct access to the connector index. Deleted connectors will actually disappear from the original connector index and be moved into the deleted connectors index.

Followup work:

update specification for 9.0 CLI

Other work we might need to do in the future:

support force deletes, e.g pass force=true to delete endpoint to completely remove the connector

Validation

tested locally
yaml e2e tests
unit tests

jedrazb · 2024-12-11T10:00:58Z

@elasticmachine merge upstream

elasticsearchmachine · 2024-12-11T10:13:07Z

Hi @jedrazb, I've created a changelog YAML for you.

elasticsearchmachine · 2024-12-11T10:45:53Z

Pinging @elastic/search-eng (Team:SearchOrg)

elasticsearchmachine · 2024-12-11T10:45:53Z

Pinging @elastic/search-extract-and-transform (Team:Search - Extract & Transform)

artem-shelkovnikov

I don't feel I can review Java code in the PR well, but I left a couple comments/questions about features introduced by the PR and other minor stuff

artem-shelkovnikov · 2024-12-11T13:40:30Z

muted-tests.yml

- class: org.elasticsearch.xpack.application.connector.ConnectorIndexServiceTests
-  issue: https://github.com/elastic/elasticsearch/issues/116087


What about unmuting the tests?

It got muted due couple of flakey runs recently ... I checked the logs and it seemed to be weird ES error. It has been running for almost a year now fine

artem-shelkovnikov · 2024-12-11T13:41:40Z

rest-api-spec/src/main/resources/rest-api-spec/api/connector.list.json

+      "deleted": {
+        "type": "boolean",
+        "default": false,
+        "description": "A flag indicating whether to list connectors that have been soft-deleted."


Should we say that it's listing "only" deleted connectors, not all together?

artem-shelkovnikov · 2024-12-11T13:42:39Z

x-pack/plugin/core/template-resources/src/main/resources/connectors-deleted.json

@@ -0,0 +1,307 @@
+{


Is it possible to have a test that checks that both deleted and non-deleted indices have the same schema?

Additionally, does it make sense to make a lot of properties here searchable? Or we wanna have filters on deleted connectors too?

artem-shelkovnikov · 2024-12-11T13:43:08Z

x-pack/plugin/ent-search/qa/rest/roles.yml

  cluster:
    - manage_search_application
    - manage_behavioral_analytics
+    - manage_connector


Can you add a comment on why this and below permissions are needed? What does it practically mean for users, if it does anything?

artem-shelkovnikov · 2024-12-11T13:44:08Z

...rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/connector/20_connector_list.yml

+  - do:
+      connector.list: {}
+
+  - match: { count: 2 }


Curious - why 2? It's some sort of initial setup?

artem-shelkovnikov · 2024-12-11T13:44:51Z

...rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/connector/20_connector_list.yml

+
+  - match: { count: 3 }
+
+  # Alphabetical order by index_name for results


Curious - is it possible to do same matching without relying on sorting being stable?

artem-shelkovnikov · 2024-12-11T13:47:12Z

...earch/src/main/java/org/elasticsearch/xpack/application/connector/ConnectorIndexService.java

     * @param listener    The action listener to invoke on response/failure.
     */
-    public void getConnector(String connectorId, ActionListener<ConnectorSearchResult> listener) {
+    public void getConnector(String connectorId, Boolean isDeleted, ActionListener<ConnectorSearchResult> listener) {


One interesting side effect of it is that it's possible to have a non-deleted and deleted connectors with same id that will actually point to really different connectors. That can cause some confusion, but probably nothing flow-breaking, other than customer restoring deleted connector manually breaking something, but that's on them? Should we document this more?

Also in same scenario, deleting a live connector would erase previous record of a deleted connector. I guess it's also fine given our limitations, but might be nice to document.

kderusso

I found this to be a little confusing. As a user I would assume that passing a deleted flag into a list call would have included all of the connectors including the soft deleted ones - ideally with metadata indicating which ones were deleted. Or, I would have expect a different API call to get only deletions. Why did we choose to combine these in an exclusive way?

Another option we could have taken is to add a deleted flag to the connectors index. Was this determined to be unmanageable?

kderusso · 2024-12-11T14:54:47Z

muted-tests.yml

- class: org.elasticsearch.xpack.application.connector.ConnectorIndexServiceTests
-  issue: https://github.com/elastic/elasticsearch/issues/116087


jedrazb · 2024-12-12T09:33:17Z

We’ve discussed this with Jim, and there might be a better way to implement logic to handle soft-deleted documents, with mappings update. I’m implementing his suggestion as a separate branch of this PR. Suggested approach involves updating the template and applying new mappings if needed using the onClusterChange listener.

jedrazb · 2024-12-12T15:22:39Z

@kderusso Thank you for looking into this!

Another option we could have taken is to add a deleted flag to the connectors index. Was this determined to be unmanageable?

Jim made me aware that there is a way to upgrade connector index template mappings in clusterChanged logic, so we can just add a new field to the existing index. I’m following this approach and will have something to share soon.

As a user I would assume that passing a deleted flag into a list call would have included all of the connectors including the soft deleted ones - ideally with metadata indicating which ones were deleted

I don’t have a strong opinion here. For me, passing deleted=true could be interpreted as either:

“Show me (filter)” only the deleted ones, or
Include the deleted ones in the response with a flag indicating they were deleted.

Do you know if ES endpoints conventions prefer either approach?

jedrazb · 2024-12-13T14:59:30Z

Closing in favour of #118669

[Connector API] Add interface for soft-deletes

93ac07c

elasticsearchmachine added the v9.0.0 label Dec 9, 2024

jedrazb added >non-issue v8.9.0 :SearchOrg/Extract&Transform Label for the Search E&T team Team:Search - Extract & Transform and removed v8.9.0 labels Dec 9, 2024

jedrazb added 4 commits December 10, 2024 10:21

Define connector deleted system index

12567b2

Got soft-delete logic working

4fdc2ca

Add unit tests

d9be216

Add yaml e2e test and attempt to update permissions

183ed7a

jedrazb changed the title ~~[Connector API] Add interface for soft-deletes~~ [Connector API] Support soft deletes of connectors Dec 10, 2024

jedrazb added 2 commits December 11, 2024 10:36

Fix permissions

397032b

Update docs

d8523e0

elasticmachine and others added 2 commits December 11, 2024 10:01

Merge branch 'main' into support-soft-deletes-to-connectors

2640383

Fix docs

6e1197c

jedrazb added >feature and removed >non-issue labels Dec 11, 2024

Update docs/changelog/118282.yaml

3d77bb2

jedrazb requested review from artem-shelkovnikov, navarone-feekery and seanstory December 11, 2024 10:45

jedrazb marked this pull request as ready for review December 11, 2024 10:45

elasticsearchmachine added the Team:SearchOrg Meta label for the Search Org (Enterprise Search) label Dec 11, 2024

artem-shelkovnikov reviewed Dec 11, 2024

View reviewed changes

kderusso reviewed Dec 11, 2024

View reviewed changes

jedrazb closed this Dec 13, 2024

		- class: org.elasticsearch.xpack.application.connector.ConnectorIndexServiceTests
		issue: https://github.com/elastic/elasticsearch/issues/116087


		- match: { count: 3 }

		# Alphabetical order by index_name for results

[Connector API] Support soft deletes of connectors #118282

[Connector API] Support soft deletes of connectors #118282

Uh oh!

Conversation

jedrazb commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Soft deletes for connectors

Add support for soft-deletes of connectors. Why?

Changes

Why new system index and not new is_deleted flag in existing index

Followup work:

Other work we might need to do in the future:

Validation

Uh oh!

jedrazb commented Dec 11, 2024

Uh oh!

elasticsearchmachine commented Dec 11, 2024

Uh oh!

elasticsearchmachine commented Dec 11, 2024

Uh oh!

elasticsearchmachine commented Dec 11, 2024

Uh oh!

artem-shelkovnikov 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!

jedrazb Dec 11, 2024 • 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!

kderusso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jedrazb commented Dec 12, 2024

Uh oh!

jedrazb commented Dec 12, 2024

Uh oh!

jedrazb commented Dec 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jedrazb commented Dec 9, 2024 •

edited

Loading

Why new system index and not new `is_deleted` flag in existing index

jedrazb Dec 11, 2024 •

edited

Loading