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

[Agent] Respect connector_id defined in agentless #2989

Merged

Conversation

jedrazb
Copy link
Member

@jedrazb jedrazb commented Nov 20, 2024

Closes https://github.com/elastic/search-team/issues/8669

Changes:

  • if connector_id is provided in fleet policy, create a connector with such id
  • create all agentless connectors with is_native flag
  • remove secret api key usage for native connectors
  • support discovering native connectors by id (the framework will no longer be running in "force native" mode, hence the modified query works for both managed and self-managed connectors)

Other changes:

  • optimize agent dockerfile and fix issue with yq installation (I think in 9.0.0 "correct" yq was removed in agent image and apt-get was fetching different tool, confirmed it was coming from different repo)

See example in action:

Screenshot 2024-11-20 at 11 21 53 Screenshot 2024-11-20 at 11 20 46

Checklists

Pre-Review Checklist

  • this PR does NOT contain credentials of any kind, such as API keys or username/passwords (double check config.yml.example)
  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • if there is no GH issue, please create it. Each PR should have a link to an issue
  • this PR has a thorough description
  • Covered the changes with automated tests
  • Tested the changes locally
  • Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)

Followup work

I am aware that there is some more work to do, this PR is just what we need now, followup tickets are filed:

@jedrazb jedrazb marked this pull request as ready for review November 20, 2024 11:28
@jedrazb jedrazb requested a review from a team as a code owner November 20, 2024 11:28
@@ -231,7 +237,6 @@ async def supported_connectors(self, native_service_types=None, connector_ids=No
custom_connectors_query = {
"bool": {
"filter": [
{"term": {"is_native": False}},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we treat all connectors the same (discover by ID), remove this filter

@@ -56,6 +56,7 @@ async def ensure_connector_records_exist(self, agent_config, connector_name=None
connector_id=connector_id,
service_type=service_type,
connector_name=connector_name,
is_native=True,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we've settled on is_native flag reusage in the end?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I understood from the "mega-thread" we've had last week, will DM you the link

@@ -384,7 +384,7 @@ class BaseDataSource:
advanced_rules_enabled = False
dls_enabled = False
incremental_sync_enabled = False
native_connector_api_keys_enabled = True
native_connector_api_keys_enabled = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is old setting to enable native connector secrets for native connectors. Since we no longer rely on secrets for elastic managed connectors I just set this flag to false without thinking worrying about proper cleanup, I've filed followup ticket:
#2990

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it'll break connectors running in native mode in traditional setup in 9.x, but we don't care since it's something nobody's supposed to do, right?

Copy link
Member Author

@jedrazb jedrazb Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The native connectors would be broken anyway since only service account that manages the ent-search node has access to read the secrets https://github.com/elastic/elasticsearch/blob/8c20ac5884158b88fdd598e422db632e1734aabb/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/service/ElasticServiceAccounts.java#L48

After migration to 9.0 those secrets are useless because noone has permissions to read them

@jedrazb jedrazb enabled auto-merge (squash) November 21, 2024 10:05
@jedrazb jedrazb merged commit 4d861cc into main Nov 21, 2024
2 checks passed
@jedrazb jedrazb deleted the jedrazb-agentless-respects-connector-id-from-fleet-policy branch November 21, 2024 10:11
Copy link

💔 Failed to create backport PR(s)

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
backport --pr 2989 --autoMerge --autoMergeMethod squash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants