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

Switch from localhost to 0.0.0.0 by default for all receivers #1006

Merged
merged 8 commits into from Jun 10, 2020

Conversation

flands
Copy link
Contributor

@flands flands commented May 21, 2020

Description: Switch from localhost to 0.0.0.0 by default

Link to tracking Issue: Addresses #592

Testing: Local

Documentation: Updated as needed

@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #1006 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1006   +/-   ##
=======================================
  Coverage   86.33%   86.33%           
=======================================
  Files         198      198           
  Lines       14169    14169           
=======================================
  Hits        12233    12233           
  Misses       1480     1480           
  Partials      456      456           
Impacted Files Coverage Δ
receiver/jaegerreceiver/factory.go 91.66% <ø> (ø)
receiver/zipkinreceiver/factory.go 100.00% <ø> (ø)
internal/data/testdata/metric.go 100.00% <100.00%> (ø)
receiver/opencensusreceiver/factory.go 94.59% <100.00%> (ø)
receiver/otlpreceiver/factory.go 94.59% <100.00%> (ø)
receiver/prometheusreceiver/factory.go 81.08% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 685777f...c579c96. Read the comment docs.

exporter/otlpexporter/otlp_test.go Outdated Show resolved Hide resolved
internal/collector/telemetry/telemetry.go Outdated Show resolved Hide resolved
@tigrannajaryan tigrannajaryan added this to In progress in Collector May 21, 2020
@tigrannajaryan
Copy link
Member

@flands please resolve the conflict.

@tigrannajaryan
Copy link
Member

And fix the build, please.

@tigrannajaryan tigrannajaryan self-assigned this May 21, 2020
@tigrannajaryan
Copy link
Member

@flands if you will not be able to finalize this @ccaraman volunteered to help :-)

@flands flands added this to the Beta 0.4 milestone Jun 5, 2020
@flands
Copy link
Contributor Author

flands commented Jun 5, 2020

@tigrannajaryan @bogdandrutu @jrcamp -- apologies for the delay, ready for another look.

@flands flands changed the title Switch from localhost to 0.0.0.0 by default Switch from localhost to 0.0.0.0 by default for all receivers Jun 5, 2020
extension/pprofextension/factory.go Outdated Show resolved Hide resolved
extension/zpagesextension/factory.go Outdated Show resolved Hide resolved
@owais
Copy link
Contributor

owais commented Jun 8, 2020

Can we have an env var like OPENTELEMERY_DEFAULT_LOCAL_ADDR. It can default to localhost but k8s deployment files and helm charts can change this default to 0.0.0.0. That way we'll default to localhost solving the security and macOS annoyances but still make k8s experience seamless and less confusing.

@tigrannajaryan
Copy link
Member

Can we have an env var like OPENTELEMERY_DEFAULT_LOCAL_ADDR. It can default to localhost but k8s deployment files and helm charts can change this default to 0.0.0.0. That way we'll default to localhost solving the security and macOS annoyances but still make k8s experience seamless and less confusing.

We'll be back to the original problem when people just don't know they need to do it.

@tigrannajaryan
Copy link
Member

@flands please resolve conflicts.

…ector into flands/0.0.0.0

* 'master' of github.com:open-telemetry/opentelemetry-collector:
  Add Grafana as an Adopter (open-telemetry#1095)
  Decentralize component documentation (open-telemetry#1089)
  Prevent the chance of a panic on shutdown of load scraper (open-telemetry#1091)
  Convert cpu/time to cpu/usage and correct unit / data type (open-telemetry#1092)
@owais
Copy link
Contributor

owais commented Jun 9, 2020

My point was almost everywhere outside k8s, people can use localhost and for k8s we provide example config (and in near future helm charts). Such config and helm charts could default to 0.0.0.0.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 10, 2020

CLA Check
The committers are authorized under a signed CLA.

@flands flands moved this from In progress to Review in progress in Collector Jun 10, 2020
docs/service-extensions.md Outdated Show resolved Hide resolved
docs/service-extensions.md Outdated Show resolved Hide resolved
extension/README.md Outdated Show resolved Hide resolved
extension/README.md Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

My point was almost everywhere outside k8s, people can use localhost

@owais is this true though? If I am running a standalone Collector on a physical machine or VM how does "localhost" work? I still need to listen to external interfaces to receive connections from other machines.

@owais
Copy link
Contributor

owais commented Jun 10, 2020

In my experience people generally put collector behind a reverse proxy like nginx or ambassador and don't expose the collector directly to the internet. But in any case, I was just throwing ideas into the mix. Not insisting we definitely do that.

Collector automation moved this from Review in progress to Reviewer approved Jun 10, 2020
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM

@tigrannajaryan
Copy link
Member

@flands the cla bot is not approving, not sure why. Can you retrigger the build maybe?

@flands flands closed this Jun 10, 2020
Collector automation moved this from Reviewer approved to Done Jun 10, 2020
@flands flands reopened this Jun 10, 2020
Collector automation moved this from Done to In progress Jun 10, 2020
@tigrannajaryan tigrannajaryan merged commit 3c93591 into open-telemetry:master Jun 10, 2020
Collector automation moved this from In progress to Done Jun 10, 2020
@flands flands deleted the flands/0.0.0.0 branch June 10, 2020 18:58
wyTrivail pushed a commit to mxiamxia/opentelemetry-collector that referenced this pull request Jul 13, 2020
…elemetry#1006)

Switch from localhost to 0.0.0.0 by default

Link to tracking Issue: Addresses open-telemetry#592
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Collector
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants