Skip to content

Conversation

@khushijain21
Copy link
Contributor

@khushijain21 khushijain21 commented Oct 14, 2025

What does this PR do?

This PR moves kerberos package from beats to elastic-agent-libs so that we can import this in beatauthextension

Why is it important?

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works

Related issues

@khushijain21 khushijain21 changed the title kerberos Move Kerberos package from beats to elastic-agent-libs Oct 27, 2025
@khushijain21 khushijain21 reopened this Oct 27, 2025
@khushijain21 khushijain21 self-assigned this Oct 27, 2025
@khushijain21 khushijain21 marked this pull request as ready for review October 27, 2025 06:54
@khushijain21 khushijain21 requested a review from a team as a code owner October 27, 2025 06:54
@khushijain21 khushijain21 requested review from belimawr and mauri870 and removed request for a team October 27, 2025 06:55
@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Oct 27, 2025
AndersonQ
AndersonQ previously approved these changes Oct 27, 2025
Copy link
Member

@AndersonQ AndersonQ left a comment

Choose a reason for hiding this comment

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

It matches the beats package and the test failing seems unrelated.
Once all tests pass, it's good to go

@rdner
Copy link
Member

rdner commented Oct 27, 2025

@khushijain21 the Windows tests are failing:

---- FAIL: TestNewClient (1.55s)
FAIL
FAIL	github.com/elastic/elastic-agent-libs/transport/kerberos/kerberos	1.617s

mauri870
mauri870 previously approved these changes Oct 27, 2025
Copy link
Member

@mauri870 mauri870 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'm assuming it's an exact copy of the Beats codebase. Please look into the CI failures as Denis pointed out. Thank you.

@cmacknz
Copy link
Member

cmacknz commented Oct 27, 2025

I think that beatsauth extension should move into Beats instead of doing this:

  1. beatsauth extension is only ever going to be used with beats receivers
  2. this allows beatsauth and the kerberos client to use the beat integration testing framework(s) and be tested together.
  3. The kerberos client dependencies are not great (they are implementing crypto outside the standard library and it's unclear how well reviewed or maintainer they are), if we really need to keep them here it'd be better to put them in their own go.mod to isolate them. Putting them in here will have these become indirect dependencies of more things than is necessary.

@khushijain21
Copy link
Contributor Author

khushijain21 commented Oct 28, 2025

The only benefit of having beatsauthextension in opentelemetry-collector-components is that it is very easy to test changes with a real collector. Doing this in beats is a little more complicated with the translation logic and everything. I am in favor of keeping it there. We can still use the beats integration framework to write a full E2E test for beatsauth + Kerberos in beats repo.

The kerberos client dependencies are not great (they are implementing crypto outside the standard library and it's unclear how well reviewed or maintainer they are), if we really need to keep them here it'd be better to put them in their own go.mod to isolate them. Putting them in here will have these become indirect dependencies of more things than is necessary.

I agree, I can create a separate go.mod for it instead

@khushijain21 khushijain21 dismissed stale reviews from mauri870 and AndersonQ via c27baf1 October 28, 2025 06:09
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @khushijain21

@cmacknz
Copy link
Member

cmacknz commented Oct 28, 2025

The only benefit of having beatsauthextension in opentelemetry-collector-components is that it is very easy to test changes with a real collector.

We can and should be testing it this way in Beats as well, this is what https://github.com/elastic/ingest-dev/issues/6207 is describing.

We aren't in such a rush that we can't put the setup we need in place first. We could for example setup a test collector distro in Beats using the one in collector components as a starting place. There is no need to port over all existing tests immediately, and just having one example using this way of working is a benefit because it means people can stop copying the otelbeat tests as the reference.

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

Labels

Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants