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

[exporter/loadbalancing] Add return_hostnames option to k8s resolver #35411

Conversation

snuggie12
Copy link
Contributor

Description: Adds an optional configuration option to the k8s resolver which allows for hostnames to be returned instead of IPs. This allows certain scenarios like using istio in sidecar mode. Requirements have been added to the documentation.

Link to tracking Issue: #18412

Testing: Added corresponding hostname-based tests for adding backends/endpoints as well as deleting them. There were also tests missing for the k8s handler and so some tests were added as well there. Specifically failing if you want hostnames, but endpoints are returned that do not have hostnames.

Aside from unit tests, also ran this in our lab cluster and deleted pods or performed rollouts to our statefulset.

Somewhat tangential to the PR itself, but istio now reports mTLS traffic with zero workarounds required which was the motivation for the issue.

Documentation: Added documentation explaining the new option and the requirements needed to use it. Also added an additional "important" note object specifically calling out that the k8s resolver needs RBAC to work.

Copy link
Contributor

github-actions bot commented Oct 9, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 9, 2024
@snuggie12
Copy link
Contributor Author

@mx-psi @jpkrohling , please let me know if I have not completed a task to get this reviewed. I don't want to see it closed due to staleness.

@mx-psi
Copy link
Member

mx-psi commented Oct 17, 2024

Hey, thanks for the PR. No worries about the PR being closed, we can reopen if needed. It may take some time for @jpkrohling to get back to this PR (as in, it may take a couple of additional weeks at least), in the mean time, you can try to make sure that CI passes by:

  1. Adding a changelog entry
  2. Making sure linting passes

I am happy to help with both of those if anything is unclear.

@mx-psi mx-psi removed the Stale label Oct 17, 2024
@snuggie12
Copy link
Contributor Author

@mx-psi Can do, just wanted to make sure it wasn't going to get lost. I'll work on those two tasks soon. Thanks for getting back to me so fast.

@snuggie12 snuggie12 force-pushed the lbexporter-add-return-hostnames-opt-to-k8s-resolver branch from 0b8513d to 49cd9e4 Compare October 21, 2024 19:18
@snuggie12 snuggie12 force-pushed the lbexporter-add-return-hostnames-opt-to-k8s-resolver branch 2 times, most recently from e0892f1 to db3b7d4 Compare October 31, 2024 00:32
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 14, 2024
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Nov 28, 2024
@jpkrohling jpkrohling reopened this Dec 9, 2024
@jpkrohling jpkrohling removed the Stale label Dec 9, 2024
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Sorry, took me a while to work through the notifications, but I finally got to this one. I think this is in a good shape to be merged, just need to resolve the conflicts.

@snuggie12 snuggie12 force-pushed the lbexporter-add-return-hostnames-opt-to-k8s-resolver branch from db3b7d4 to 991dee9 Compare December 10, 2024 02:50
@snuggie12
Copy link
Contributor Author

Unit tests still passing after the slice -> map change. I also updated the image in our test environment and restarted all of our tail samplers and everything still is functioning.

@jpkrohling
Copy link
Member

There are some linting failures here:

Error: loadbalancingexporter/resolver_k8s_handler_test.go:106:1: unnecessary trailing newline (whitespace)

^
Error: loadbalancingexporter/resolver_k8s_test.go:186:4: unnecessary trailing newline (whitespace)


@snuggie12 snuggie12 force-pushed the lbexporter-add-return-hostnames-opt-to-k8s-resolver branch from 991dee9 to ca265dc Compare December 10, 2024 17:41
@snuggie12 snuggie12 force-pushed the lbexporter-add-return-hostnames-opt-to-k8s-resolver branch from ca265dc to a5c48c0 Compare December 11, 2024 22:29
@mx-psi mx-psi merged commit 964a652 into open-telemetry:main Dec 12, 2024
160 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 12, 2024
@snuggie12 snuggie12 deleted the lbexporter-add-return-hostnames-opt-to-k8s-resolver branch December 12, 2024 15:50
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
…pen-telemetry#35411)

**Description:** Adds an optional configuration option to the k8s
resolver which allows for hostnames to be returned instead of IPs. This
allows certain scenarios like using istio in sidecar mode. Requirements
have been added to the documentation.

**Link to tracking Issue:** open-telemetry#18412

**Testing:** Added corresponding hostname-based tests for adding
backends/endpoints as well as deleting them. There were also tests
missing for the k8s handler and so some tests were added as well there.
Specifically failing if you want hostnames, but endpoints are returned
that do not have hostnames.

Aside from unit tests, also ran this in our lab cluster and deleted pods
or performed rollouts to our statefulset.

Somewhat tangential to the PR itself, but istio now reports mTLS traffic
with zero workarounds required which was the motivation for the issue.

**Documentation:** Added documentation explaining the new option and the
requirements needed to use it. Also added an additional "important" note
object specifically calling out that the k8s resolver needs RBAC to
work.
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.

3 participants