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

Add *.kerberos.disable-fast-negotiation option to Kafka consumer #4520

Merged
merged 2 commits into from Jun 11, 2023
Merged

Add *.kerberos.disable-fast-negotiation option to Kafka consumer #4520

merged 2 commits into from Jun 11, 2023

Conversation

pmuls99
Copy link
Contributor

@pmuls99 pmuls99 commented Jun 9, 2023

Which problem is this PR solving?

Short description of the changes

  • Added a variable in KerborosConfig struct and configured it
  • Added flags and the configuration for the same

Signed-off-by: bugslayer-332 <ayashwanth9503@gmail.com>
flagSet.Bool(
configPrefix+kerberosPrefix+suffixKerberosDisablePAFXFAST,
defaultKerberosDisablePAFXFast,
"Disable FAST negotiation when not supported by KDC's like Active Directory")
Copy link
Member

Choose a reason for hiding this comment

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

Q: why are the variables in the code have additional "PA FX" prefix? Is there semantic difference between PF-FX-FAST and just FAST? (it would be useful to include a link to the spec in the help text)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the additional PA FX refers to the cookie used. I considered naming the variable like that because they were done the same way in the sarama library. There is not a logical difference between both so we can consider naming it FAST. Whats your say?

@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.04 ⚠️

Comparison is base (557bb1c) 97.08% compared to head (c43d1a1) 97.04%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4520      +/-   ##
==========================================
- Coverage   97.08%   97.04%   -0.04%     
==========================================
  Files         300      300              
  Lines       17813    17813              
==========================================
- Hits        17293    17287       -6     
- Misses        417      421       +4     
- Partials      103      105       +2     
Flag Coverage Δ
unittests 97.04% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro changed the title Added a configuration option for disabling FAST negotiation Add *.kerberos.disable-fast-negotiation option to Kafka consumer Jun 11, 2023
@yurishkuro yurishkuro enabled auto-merge (squash) June 11, 2023 19:18
@yurishkuro yurishkuro merged commit 71290f4 into jaegertracing:main Jun 11, 2023
31 checks passed
@pmuls99 pmuls99 deleted the disable_fast_negotiation branch June 12, 2023 02:10
kjschnei001 pushed a commit to kjschnei001/jaeger that referenced this pull request Jun 29, 2023
…gertracing#4520)

## Which problem is this PR solving?
- Solves jaegertracing#2744

## Short description of the changes
- Added a variable in KerberosConfig struct and configured it
- Added flags and the configuration for the same

---------

Signed-off-by: bugslayer-332 <ayashwanth9503@gmail.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: bugslayer-332 <ayashwanth9503@gmail.com>
Co-authored-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: KevinSchneider <kevin.schneider@target.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants