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

Support api_key authentication for agent-elasticsearch association #7598

Merged
merged 9 commits into from
Mar 29, 2024

Conversation

pkoutsovasilis
Copy link
Contributor

@pkoutsovasilis pkoutsovasilis commented Mar 5, 2024

This PR introduces support for utilising an api-key instead of a username,password from the elasticsearchRefs.secretName to authenticate against unmanaged ElasticSearch clusters for the Agent type only. Since references, such as ElasticSearch, KibanaRef, etc., of all types are captured in the code as Associations, this PR alters AgentESAssociation to introduce auth apikey support. In my thinking parsing of secrets could also be migrated per Association and thus achieve more special handling per one. More than happy to discuss the best way to introduce this feature.

@pkoutsovasilis pkoutsovasilis added >enhancement Enhancement of existing functionality discuss We need to figure this out labels Mar 5, 2024
@pkoutsovasilis pkoutsovasilis self-assigned this Mar 5, 2024
@pkoutsovasilis pkoutsovasilis force-pushed the pkoutsovasilis/apikey_agent_es_association branch from 78baea0 to 6d8c951 Compare March 5, 2024 13:42
@pkoutsovasilis pkoutsovasilis marked this pull request as ready for review March 5, 2024 14:05
@pkoutsovasilis pkoutsovasilis requested a review from thbkrkr March 5, 2024 14:05
Copy link
Contributor

@thbkrkr thbkrkr left a comment

Choose a reason for hiding this comment

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

I left a few minor comments but it looks good!
I need a bit more time for testing.

@pkoutsovasilis
Copy link
Contributor Author

pkoutsovasilis commented Mar 5, 2024

I left a few minor comments but it looks good! I need a bit more time for testing.

thank you for the comments @thbkrkr , glad that this approach makes sense to you! Please take as much time as you need to do testing; I tested this with two agents (one Daemonset and one Deployment) referencing an external Elastic Search cluster starting with a secret containing api-key. Then I updated the secret to switch to username and password and I saw both agents being restarted and successfully connecting to the Elastic Search cluster. Then another update to the secret migrating to api-key and again all green. Last I did spawn two agents referencing an ECK-managed Elastic Search cluster and I could see data flowing properly.

@pkoutsovasilis
Copy link
Contributor Author

@thbkrkr would it make sense to try to include this feature in v2.12.0?

@pkoutsovasilis pkoutsovasilis force-pushed the pkoutsovasilis/apikey_agent_es_association branch from 68e2e4f to e92237d Compare March 11, 2024 20:09
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

I wonder if we should also support api_key in Beats. Logstash would be another candidate but we should probably coordinate with the Logstash team on that.

@pkoutsovasilis
Copy link
Contributor Author

I wonder if we should also support api_key in Beats. Logstash would be another candidate but we should probably coordinate with the Logstash team on that.

hey @pebrc, I can give it a try 🙂 would you like to couple this under this PR or first wait for this one to get merged and then open a subsequent one?

@pebrc
Copy link
Collaborator

pebrc commented Mar 14, 2024

hey @pebrc, I can give it a try 🙂 would you like to couple this under this PR or first wait for this one to get merged and then open a subsequent one?

We can split it in two PRs if that's easier. But it should be very similar to the agent code you wrote.

@pkoutsovasilis
Copy link
Contributor Author

pkoutsovasilis commented Mar 14, 2024

We can split it in two PRs if that's easier. But it should be very similar to the agent code you wrote.

yep you are right, I just pushed the relevant commit

@pkoutsovasilis pkoutsovasilis requested review from thbkrkr and pebrc March 26, 2024 12:30
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM I think we want some documentation around this. Are you planning to cover that as well (in a separate PR?)

@pkoutsovasilis
Copy link
Contributor Author

pkoutsovasilis commented Mar 27, 2024

LGTM I think we want some documentation around this. Are you planning to cover that as well (in a separate PR?)

sure I will update the documentation; from a quick look this and this seem like appropriate to add info about supporting api-key for beats and agent, respectively, right?

@pkoutsovasilis pkoutsovasilis added v2.13.0 and removed discuss We need to figure this out labels Mar 28, 2024
@pebrc
Copy link
Collaborator

pebrc commented Mar 28, 2024

buildkite test this -f p=gke,E2E_TAGS=agent

@pebrc
Copy link
Collaborator

pebrc commented Mar 29, 2024

buildkite test this -f p=gke,E2E_TAGS=kibana

@pebrc pebrc merged commit c87b521 into main Mar 29, 2024
5 checks passed
@pebrc pebrc deleted the pkoutsovasilis/apikey_agent_es_association branch March 29, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement Enhancement of existing functionality v2.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants