[Connector APIs] Update existing endpoints #2675

jedrazb · 2024-07-04T13:30:56Z

Hey 👋

As discussed, I reviewed the existing Connector API endpoints and added patches to update them to the 8.15 state.

Changes

index_name and error (including last_sync_error and last_access_control_sync_error) are the only properties that use semantic null. index_name can be set to null to detach the connector, error can be reset to null to indicate healthy state again. We are using optional for everything else.
Add sync_cursor field in the Connector definition (we missed it before) and add this as an arg to _last_sync endpoint (new payload field added in 8.15)
Updated the ConnectorFeatures representation as it included long-deprecated feature names (filtering_advanced_config and filtering_rules), add missing native_connector_api_keys

Note

We have this open issue with _last_sync endpoint (confusion between semantic null and optional when updating last_sync_*_error ). We are looking into fixing this.

Also, this PR should address existing endpoints, there are couple of endpoints that are still missing. Let's work on this patch first. :)

jedrazb · 2024-07-04T16:59:52Z

Hey @l-trotta 👋 I see you assigned yourself to the issue. I tried addressing the concerns we discussed in the last call. Happy to answer any questions and address any other concerns!

EDIT: I see there are some failing tests, let me address it first

pquentin · 2024-07-05T06:51:46Z

Hey @jedrazb, this is good timing because we just managed to add Connectors YAML tests to the flight recorder, so I reran validation and you can see the results in the above comment.

I can report that compared to the current state, this pull requests:

fixes last_sync, post and update_name requests
fixes sync_job_get responses

The rest is unchanged. I can open an issue listing the failures to not block this pull request which definitely improves things.

jedrazb · 2024-07-05T08:16:52Z

Hey @pquentin ! The changes that fix remaining issues should be minimal, I'm having issues running make validate locally due to vault permission issues. I will ask more in Slack!

jedrazb · 2024-07-05T10:31:35Z

Now it looks better! Two issues:

The issues around connector.get/list endpoint responses - we are adressing this for 8.16 in [Connector API] Don't index literal nulls to connector doc elasticsearch#110543:
- Upon connector creation some fields get preset to null (to match the status quo from before). When handling the response, response definition is unhappy about null fields which are actually optional. If this doesn't break client we can probably ignore this until we change this in ES (?). null properties have been written to connector index with direct index access since 8.8 so there are tons of connectors with fields set to null
The issue with connector.update_configuration
- In ES implementation we have a yaml test case where a crawler-specifc setting is updated via this endpoint. We should not be doing this - since we are not planning to manage crawler via this API.
- Another test failure is with literal null tooltip being passed in the body (we could make it just optional in server implementation)

I think we can address those issues as a followup.

l-trotta · 2024-07-12T09:51:32Z

does this field exist only in the connector response? https://github.com/elastic/elasticsearch/blob/e1a091f1a2a7444873a1887c686186af580649e3/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/Connector.java#L211

l-trotta · 2024-07-12T09:53:54Z

I'm starting to consider mapping the connector request and response in two different classes, as you can see from the validator:
connector.get | 🔴 | 59/59 | 1/59 |
this is because in the response many types are not optional, for example
api_key_id?: string in the request
is actually
api_key_id: string | null in the response

jedrazb · 2024-07-23T13:43:27Z

Hey @l-trotta 👋 As we discussed in the call, I'm updating the spec to address main issues:

I'm starting to consider mapping the connector request and response in two different classes
++ Actually Connector type was only used to describe responses, so I updated the type accordingly.

I had to put tooltip property as string | null for the automated tests to pass. We always return tooltip field from the ES (even if it's null).

I marked last_sync_error and last_access_control_sync_error as just optional, until we address elastic/elasticsearch#110543 in ES.

There are just 2 failing test cases related to crawler state (in connector doc!). Due to historical reasons, some parts of the crawler state were, and still are, stored in the connector index. However, it was agreed that the connector API should not interact with the crawler, so those tests should be removed from ES. I don’t think these failures should block us in this PR, as this is a quirky edge case. Can explain more if needed :)

l-trotta · 2024-07-24T12:47:51Z

Hey @jedrazb, got it we'll wait for the yaml tests to be fixed!
~~One thing I found: in the connector POST request there should be the connector id path parameter, to allow defining the connector custom name.~~

Also: sync_cursor?: Dictionary<string, UserDefinedValue> -> the server value for this is Object, and the return value for this when there's no data is null instead of what the server usually sends as an empty dictionary which is {}. This is problematic for some static clients that have no way of dealing with null dictionaries, so I'd suggest changing this to sync_cursor?: UserDefinedValue, which simplifies it.

l-trotta · 2024-07-24T13:13:30Z

~~Another thing: there are some properties in ConnectorConfigProperties that should be nullable. (for example default_value, depends_on...)~~

jedrazb · 2024-07-25T08:08:37Z

got it we'll wait for the yaml tests to be fixed!

Okok I see, let me get rid of those tests from the server first.

github-actions · 2024-07-26T08:08:13Z

Following you can find the validation results for the APIs you have changed.

API	Status	Request	Response
`connector.check_in`	🟢	3/3	3/3
`connector.delete`	🟢	9/9	9/9
`connector.get`	🔴	60/60	59/60
`connector.last_sync`	🟢	7/7	7/7
`connector.list`	🟢	19/19	19/19
`connector.post`	🟢	7/7	7/7
`connector.put`	🟢	13/13	13/13
`connector.secret_delete`	🟠	Missing type	Missing type
`connector.secret_get`	🟠	Missing type	Missing type
`connector.secret_post`	🟠	Missing type	Missing type
`connector.secret_put`	🟠	Missing type	Missing type
`connector.sync_job_cancel`	🟢	3/3	3/3
`connector.sync_job_check_in`	🟠	Missing type	Missing type
`connector.sync_job_claim`	🟠	Missing type	Missing type
`connector.sync_job_delete`	🟢	4/4	4/4
`connector.sync_job_error`	🟠	Missing type	Missing type
`connector.sync_job_get`	🟢	22/22	22/22
`connector.sync_job_list`	🟢	12/12	12/12
`connector.sync_job_post`	🟢	50/50	50/50
`connector.sync_job_update_stats`	🟠	Missing type	Missing type
`connector.update_active_filtering`	🟢	1/1	1/1
`connector.update_api_key_id`	🟢	4/4	4/4
`connector.update_configuration`	🔴	8/9	9/9
`connector.update_error`	🟢	4/4	4/4
`connector.update_features`	🟠	Missing type	Missing type
`connector.update_filtering_validation`	🟢	3/3	3/3
`connector.update_filtering`	🟢	12/12	12/12
`connector.update_index_name`	🟢	4/4	4/4
`connector.update_name`	🟢	4/4	4/4
`connector.update_native`	🟢	3/3	3/3
`connector.update_pipeline`	🟢	3/3	3/3
`connector.update_scheduling`	🟢	3/3	3/3
`connector.update_service_type`	🟢	2/2	2/2
`connector.update_status`	🟢	3/3	3/3

You can validate these APIs yourself by using the make validate target.

jedrazb · 2024-07-26T08:12:07Z

@l-trotta addressed your feedback around sync_cursor type :)

github-actions · 2024-07-26T09:29:40Z

Following you can find the validation results for the APIs you have changed.

API	Status	Request	Response
`connector.check_in`	🟢	3/3	3/3
`connector.delete`	🟢	9/9	9/9
`connector.get`	🔴	60/60	59/60
`connector.last_sync`	🟢	7/7	7/7
`connector.list`	🟢	19/19	19/19
`connector.post`	🟢	7/7	7/7
`connector.put`	🟢	13/13	13/13
`connector.secret_delete`	🟠	Missing type	Missing type
`connector.secret_get`	🟠	Missing type	Missing type
`connector.secret_post`	🟠	Missing type	Missing type
`connector.secret_put`	🟠	Missing type	Missing type
`connector.sync_job_cancel`	🟢	3/3	3/3
`connector.sync_job_check_in`	🟠	Missing type	Missing type
`connector.sync_job_claim`	🟠	Missing type	Missing type
`connector.sync_job_delete`	🟢	4/4	4/4
`connector.sync_job_error`	🟠	Missing type	Missing type
`connector.sync_job_get`	🟢	22/22	22/22
`connector.sync_job_list`	🟢	12/12	12/12
`connector.sync_job_post`	🟢	50/50	50/50
`connector.sync_job_update_stats`	🟠	Missing type	Missing type
`connector.update_active_filtering`	🟢	1/1	1/1
`connector.update_api_key_id`	🟢	4/4	4/4
`connector.update_configuration`	🔴	8/9	9/9
`connector.update_error`	🟢	4/4	4/4
`connector.update_features`	🟠	Missing type	Missing type
`connector.update_filtering_validation`	🟢	3/3	3/3
`connector.update_filtering`	🟢	12/12	12/12
`connector.update_index_name`	🟢	4/4	4/4
`connector.update_name`	🟢	4/4	4/4
`connector.update_native`	🟢	3/3	3/3
`connector.update_pipeline`	🟢	3/3	3/3
`connector.update_scheduling`	🟢	3/3	3/3
`connector.update_service_type`	🟢	2/2	2/2
`connector.update_status`	🟢	3/3	3/3

You can validate these APIs yourself by using the make validate target.

github-actions · 2024-07-26T12:05:18Z

The backport to 8.15 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-8.15 8.15
# Navigate to the new working tree
cd .worktrees/backport-8.15
# Create a new branch
git switch --create backport-2675-to-8.15
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4b67d9a824878e54a250d4c9c333474a9a06a141
# Push it to GitHub
git push --set-upstream origin backport-2675-to-8.15
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-8.15

Then, create a pull request where the base branch is 8.15 and the compare/head branch is backport-2675-to-8.15.

Co-authored-by: Jedr Blaszyk <[email protected]>

[Connector APIs] Update existing endpoints

83f0014

jedrazb added the specification label Jul 4, 2024

This comment was marked as outdated.

Sign in to view

l-trotta self-assigned this Jul 4, 2024

jedrazb marked this pull request as ready for review July 4, 2024 16:58

This comment has been minimized.

Sign in to view

Attempt to fix validation targets

9e3f3dd

This comment was marked as outdated.

Sign in to view

jedrazb mentioned this pull request Jul 5, 2024

[Connector API] Don't index literal nulls to connector doc elastic/elasticsearch#110543

Merged

jedrazb mentioned this pull request Jul 18, 2024

The PUT connector should have connector_id as optional? #2715

Open

jedrazb added 2 commits July 23, 2024 14:05

Merge branch 'main' into connector-api-patch-nullable-usage

cee1265

Fix validation errors

4e6b246

This comment was marked as outdated.

Sign in to view

jedrazb requested a review from l-trotta July 23, 2024 13:43

making last_sync private

f56559d

This comment was marked as outdated.

Sign in to view

Change type of sync_cursor

f8bb2cc

changing remaining sync cursor

5750805

l-trotta approved these changes Jul 26, 2024

View reviewed changes

l-trotta added the backport 8.15 label Jul 26, 2024

jedrazb merged commit 4b67d9a into main Jul 26, 2024
7 checks passed

jedrazb deleted the connector-api-patch-nullable-usage branch July 26, 2024 12:04

l-trotta pushed a commit that referenced this pull request Jul 26, 2024

[Connector APIs] Update existing endpoints for 8.15 (#2675)

8ef6ecc

l-trotta mentioned this pull request Jul 26, 2024

[Connector APIs] Update existing endpoints for 8.15 (#2675) #2741

Merged

l-trotta added a commit that referenced this pull request Jul 26, 2024

[Connector APIs] Update existing endpoints for 8.15 (#2675) (#2741)

54eb485

Co-authored-by: Jedr Blaszyk <[email protected]>

l-trotta mentioned this pull request Jul 29, 2024

Non required field in connector config #2744

Merged

ElenaStoeva mentioned this pull request Oct 15, 2024

[Console] Update autocomplete definitions for 8.16 elastic/kibana#196284

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Connector APIs] Update existing endpoints #2675

[Connector APIs] Update existing endpoints #2675

jedrazb commented Jul 4, 2024 •

edited

Loading

This comment was marked as outdated.

jedrazb commented Jul 4, 2024 •

edited

Loading

This comment has been minimized.

pquentin commented Jul 5, 2024 •

edited

Loading

jedrazb commented Jul 5, 2024

This comment was marked as outdated.

jedrazb commented Jul 5, 2024 •

edited

Loading

l-trotta commented Jul 12, 2024

l-trotta commented Jul 12, 2024

This comment was marked as outdated.

jedrazb commented Jul 23, 2024 •

edited

Loading

l-trotta commented Jul 24, 2024 •

edited

Loading

l-trotta commented Jul 24, 2024 •

edited

Loading

jedrazb commented Jul 25, 2024

This comment was marked as outdated.

github-actions bot commented Jul 26, 2024

jedrazb commented Jul 26, 2024

github-actions bot commented Jul 26, 2024

github-actions bot commented Jul 26, 2024

[Connector APIs] Update existing endpoints #2675

[Connector APIs] Update existing endpoints #2675

Conversation

jedrazb commented Jul 4, 2024 • edited Loading

Changes

Note

This comment was marked as outdated.

jedrazb commented Jul 4, 2024 • edited Loading

This comment has been minimized.

pquentin commented Jul 5, 2024 • edited Loading

jedrazb commented Jul 5, 2024

This comment was marked as outdated.

jedrazb commented Jul 5, 2024 • edited Loading

l-trotta commented Jul 12, 2024

l-trotta commented Jul 12, 2024

This comment was marked as outdated.

jedrazb commented Jul 23, 2024 • edited Loading

l-trotta commented Jul 24, 2024 • edited Loading

l-trotta commented Jul 24, 2024 • edited Loading

jedrazb commented Jul 25, 2024

This comment was marked as outdated.

github-actions bot commented Jul 26, 2024

jedrazb commented Jul 26, 2024

github-actions bot commented Jul 26, 2024

github-actions bot commented Jul 26, 2024

jedrazb commented Jul 4, 2024 •

edited

Loading

jedrazb commented Jul 4, 2024 •

edited

Loading

pquentin commented Jul 5, 2024 •

edited

Loading

jedrazb commented Jul 5, 2024 •

edited

Loading

jedrazb commented Jul 23, 2024 •

edited

Loading

l-trotta commented Jul 24, 2024 •

edited

Loading

l-trotta commented Jul 24, 2024 •

edited

Loading