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

Elastic connector integration #10898

Merged
merged 9 commits into from
Sep 16, 2024

Conversation

jedrazb
Copy link
Member

@jedrazb jedrazb commented Aug 27, 2024

Proposed commit message

Initial implementation of the package for the connectors service, running exclusively in agentless mode. The package supports ~ 30 connector types (e.g., Google Drive, SharePoint, MongoDB), each syncing data to an ES index. We have a policy_template per connector type, so that we have a unique tiles displayed the integrations UI. This PR just adds support for Google Drive and Github Connectors.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
    • This component doesn't produce any datastreams (the logs are collected via "System" integration) do we need to document it here?
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

  • Verified policy locally with WIP connectors running in agent (custom docker image)

How to test this PR locally

  • Reach out to @jedrazb (@jedr.blaszyk on Slack) I have doc with all the steps to get this running locally, not linking here since it's public repo

Related issues

Screenshots

Screenshot 2024-09-04 at 14 32 33 Screenshot 2024-09-04 at 14 31 57

@jedrazb jedrazb changed the title [WIP] Elastic connector integration Elastic connector integration Sep 4, 2024
@jedrazb jedrazb marked this pull request as ready for review September 4, 2024 12:33
Copy link
Member

@seanstory seanstory left a comment

Choose a reason for hiding this comment

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

LGTM from the Search side. Two small comments.

packages/elastic_connectors/docs/github.md Outdated Show resolved Hide resolved
kibana:
version: "^9.0.0"
elastic:
subscription: "basic"
Copy link
Member

Choose a reason for hiding this comment

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

CC @danajuratoni - I expect we actually want this platinum+, but since it'll only be accessible through Agentless (cloud and serverless) maybe this doesn't matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

@seanstory This should be standard (I confirmed with Dana). Looking at integration manifest only 4 options are supported in enum: basic, gold, platinum and enterprise. So I think in such a case: basic == standard?

Choose a reason for hiding this comment

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

I expect we actually want this platinum+, but since it'll only be accessible through Agentless (cloud and serverless) maybe this doesn't matter?

This is a great place to get our wires crossed, so I'd like to double check:

  • licensing for Elastic managed connectors (i.e. Agentless connectors) should remain unchanged. This means Standard on Elastic Cloud hosted.
  • licensing for connectors on Agent (i.e. self-managed connectors via Agent) is not part of 9.0, but should be equivalent to self-managed connector clients. This would mean likely Platinum+ for self-managed connectors via Agent, final decision to be made in the future / if and when we'll add support.

Copy link
Member Author

Choose a reason for hiding this comment

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

The approach we took makes it hard if not impossible for users to run this on agent so I wouldn't worry about the self-managed agent case. But I'm following up on this thread.

Copy link
Member

Choose a reason for hiding this comment

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

"standard" == "platinum" IIRC. Standard is our lowest tier in cloud, but it's still a paid tier, to it's higher than basic.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤯 Good point, Sean. We need to verify that the cloud standard license is included if we enforce platinum in the integration definition.

@jlind23 jlind23 requested a review from a team September 10, 2024 05:51
@jedrazb
Copy link
Member Author

jedrazb commented Sep 10, 2024

Setting the Kibana version restriction to 9+ is causing a Buildkite error. I’m thinking we need to point it to an existing version instead

Error response from daemon: manifest for docker.elastic.co/kibana/kibana:9.0.0 not found: manifest unknown: manifest unknown
--
  | Error: booting up the stack failed: running docker-compose failed: running command failed: running Docker Compose up command failed: exit status 18: manifest for docker.elastic.co/kibana/kibana:9.0.0 not found: manifest

enabled: false
agentless:
enabled: true
multiple: false
Copy link
Contributor

Choose a reason for hiding this comment

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

pay attention a user will be able to enable more than one input using the high-level tile.

Correct me if I'm wrong @jsoriano but there's no way to validate there's only one input in use, correct?

Copy link
Member

Choose a reason for hiding this comment

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

Is there actually a way for us to hide/disable this high-level tile? Since we really only want one input at a time, only the lower-level tiles really make sense for us.

Copy link
Contributor

Choose a reason for hiding this comment

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

not that I know of
with CSPM integration the top-level tier doesn't serve our use case either. When we thought of enabling such capability we noticed that in the fleet, the top-level tier is used in multiple flows, so it seemed like a big change at the time to address. We ended up having a custom UI component instead.

Copy link
Member

Choose a reason for hiding this comment

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

We ended up having a custom UI component instead.

Can you elaborate?

template_path: github.yml.hbs
- name: google_drive
title: Google Drive Connector
icons:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: going forward it is recommended to have screenshots per policy_template

deployment_modes:
default:
enabled: false
agentless:
Copy link
Contributor

@eyalkraft eyalkraft Sep 12, 2024

Choose a reason for hiding this comment

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

FYI we'll soon require some owner tags as part of this section.
It's not yet part of the spec but you can follow here to add it when it'll be added to the spec:

Copy link
Member

Choose a reason for hiding this comment

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

I assume this won't block us from merging, will just be something we need to add to a todo list?

@seanstory
Copy link
Member

buildkite test this

@elasticmachine
Copy link

💚 Build Succeeded

History

Copy link

Quality Gate failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube

@seanstory
Copy link
Member

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

Can I ignore this? Looks like the PR would let me merge as is, but I don't want to skip something I shouldn't

@seanstory seanstory merged commit 0cb33d6 into elastic:main Sep 16, 2024
4 of 5 checks passed
@seanstory
Copy link
Member

Forgiveness over permission 🤞

@elasticmachine
Copy link

Package elastic_connectors - 0.0.1 containing this change is available at https://epr.elastic.co/search?package=elastic_connectors

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.

9 participants