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.

Sign up for GitHub

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

Jump to bottom

Error/warn if TLS flags are used when tls.enabled=false #2893

Closed
albertteoh opened this issue Mar 24, 2021 · 10 comments · Fixed by #3030
Closed

Error/warn if TLS flags are used when tls.enabled=false #2893

albertteoh opened this issue Mar 24, 2021 · 10 comments · Fixed by #3030
Assignees
@clock21am
Labels
good first issue Good for beginners

Comments

@albertteoh
Copy link
Contributor

Requirement - what kind of business use case are you trying to solve?

Motivated by this comment in Slack:

we tried the CLI flag --es.tls.skip-host-verify and the enviornment variable ES_TLS_SKIP_HOST_VERIFY
Neither one seems to have an effect
...
Found the issue, you also need to set --es.tls.enabled/ES_TLS_ENABLED 🤦

Problem - what in Jaeger blocks you from solving the requirement?

The user was, rightfully, confused when the --es.tls.skip-host-verify was set but wasn't working.

Proposal - what do you suggest to solve the problem or improve the existing situation?

Log a warning or error if any --*.tls.* flag is set when --*.tls.enabled == false, maybe somewhere here and here.

Any open questions to address

Should we log a warning or error (and prevent startup of service)? I would lean towards the latter to be more explicit, though I think it would be considered "breaking" behaviour. The former warning could be easily missed.
@albertteoh albertteoh added the good first issue Good for beginners label Mar 24, 2021
@yurishkuro
Copy link
Member

+1 to crash

@LostLaser
Copy link
Contributor

I would like to pick this one up. Is there a consensus that this incorrect configuration should be fatal?

@albertteoh
Copy link
Contributor Author

@LostLaser I think an incorrect configuration should be fatal rather than just being logged and my understanding is @yurishkuro is on the same page (but please correct me if I'm wrong).

@yurishkuro
Copy link
Member

Agreed

@jpkrohling
Copy link
Contributor

@LostLaser it's yours :-)

@clock21am
Copy link
Contributor

is there activity on this issue? if not can I take this over?

@albertteoh
Copy link
Contributor Author

@LostLaser are you okay with @clock21am taking over this task?

@LostLaser
Copy link
Contributor

Yes, that’s fine! Sorry for the delay.

@clock21am
Copy link
Contributor

it should fail for both clients and sever side if I am not right?

@clock21am clock21am mentioned this issue May 25, 2021
Merged
@albertteoh
Copy link
Contributor Author

it should fail for both clients and sever side if I am not right?

IIUC, the TLS flags should only apply to the server side, and so should fail for just the server side.

clock21am added a commit to clock21am/jaeger that referenced this issue Jun 20, 2021
@clock21am
Perform log.fatal if TLS flags are used when tls.enabled=false(jaeger…
24b9800 
…tracing#2893)

Signed-off-by: Rajdeep Kaur <rajdeep51994@gmail.com>
clock21am added a commit to clock21am/jaeger that referenced this issue Jun 21, 2021
@clock21am
Perform log.fatal if TLS flags are used when tls.enabled=false(jaeger…
796b84d 
…tracing#2893)

Signed-off-by: Rajdeep Kaur <rajdeep51994@gmail.com>
clock21am added a commit to clock21am/jaeger that referenced this issue Jun 21, 2021
@clock21am
Perform log.fatal if TLS flags are used when tls.enabled=false(jaeger…
68279e2 
…tracing#2893)

Signed-off-by: Rajdeep Kaur <rajdeep51994@gmail.com>
clock21am added a commit to clock21am/jaeger that referenced this issue Jun 21, 2021
@clock21am
Perform log.fatal if TLS flags are used when tls.enabled=false(jaeger…
8ba6555 
…tracing#2893)

Signed-off-by: Rajdeep Kaur <rajdeep51994@gmail.com>
clock21am added a commit to clock21am/jaeger that referenced this issue Jun 21, 2021
@clock21am
Perform log.fatal if TLS flags are used when tls.enabled=false(jaeger…
8c74894 
…tracing#2893)

Signed-off-by: Rajdeep Kaur <rajdeep51994@gmail.com>
clock21am added a commit to clock21am/jaeger that referenced this issue Jun 22, 2021
@clock21am
Add TLS flags test
ebbd75c 
This commit adds tls flag test in collector/app/builder_flag_test.go and query/app/flag_test.go
Also fixes InitFromViper check in builder_flag.go (jaegertracing#2893)

Signed-off-by: Rajdeep Kaur <rajdeep51994@gmail.com>
clock21am added a commit to clock21am/jaeger that referenced this issue Jul 4, 2021
@clock21am
Perform log.fatal if TLS flags are used when tls.enabled=false(jaeger…
f92ac6a 
…tracing#2893)

Signed-off-by: Rajdeep Kaur <rajdeep51994@gmail.com>
clock21am added a commit to clock21am/jaeger that referenced this issue Jul 4, 2021
@clock21am
Add TLS flags test
9e51d3e 
This commit adds tls flag test in collector/app/builder_flag_test.go and query/app/flag_test.go
Also fixes InitFromViper check in builder_flag.go (jaegertracing#2893)

Signed-off-by: Rajdeep Kaur <rajdeep51994@gmail.com>
clock21am added a commit to clock21am/jaeger that referenced this issue Jul 16, 2021
@clock21am
Add TLS flags test
f8f1cdf 
This commit adds tls flag test in collector/app/builder_flag_test.go and query/app/flag_test.go
Also fixes InitFromViper check in builder_flag.go (jaegertracing#2893)

Signed-off-by: Rajdeep Kaur <rajdeep51994@gmail.com>
yurishkuro pushed a commit that referenced this issue Apr 18, 2022
@clock21am
Perform log.fatal if TLS flags are used when tls.enabled=false #2893 (#…
52212b1 
…3030)
albertteoh pushed a commit to albertteoh/jaeger that referenced this issue Jul 13, 2022
@clock21am @albertteoh
Perform log.fatal if TLS flags are used when tls.enabled=false jaeger…
ca049f9 
…tracing#2893 (jaegertracing#3030)

Signed-off-by: Albert Teoh <see.kwang.teoh@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@clock21am clock21am

Labels
good first issue Good for beginners
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Perform log.fatal if TLS flags are used when tls.enabled=false #2893 clock21am/jaeger
5 participants
@jpkrohling @yurishkuro @clock21am @albertteoh @LostLaser