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

[fix] Enable Kafka TLS when TLS auth is specified #2107

Merged
merged 7 commits into from
Mar 5, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Remove deprecated TLS enabled flag
Signed-off-by: Pavol Loffay <[email protected]>
  • Loading branch information
pavolloffay committed Mar 3, 2020
commit 4c760ada63aaa80e6f5ffb363b95c8b627fa87ce
2 changes: 1 addition & 1 deletion cmd/agent/app/reporter/grpc/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const (

var tlsFlagsConfig = tlscfg.ClientFlagsConfig{
Prefix: gRPCPrefix,
Enabled: tlscfg.Show,
ShowEnabled: true,
ShowServerName: true,
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/collector/app/builder_flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ const (

var tlsFlagsConfig = tlscfg.ServerFlagsConfig{
Prefix: "collector.grpc",
ShowEnabled: tlscfg.Show,
ShowEnabled: true,
ShowClientCA: true,
}

Expand Down
4 changes: 1 addition & 3 deletions cmd/ingester/app/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"time"

"github.com/spf13/viper"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/pkg/kafka/auth"
kafkaConsumer "github.com/jaegertracing/jaeger/pkg/kafka/consumer"
Expand Down Expand Up @@ -115,7 +114,7 @@ func AddFlags(flagSet *flag.FlagSet) {
}

// InitFromViper initializes Builder with properties from viper
func (o *Options) InitFromViper(v *viper.Viper, logger *zap.Logger) {
func (o *Options) InitFromViper(v *viper.Viper) {
o.Brokers = strings.Split(stripWhiteSpace(v.GetString(KafkaConsumerConfigPrefix+SuffixBrokers)), ",")
o.Topic = v.GetString(KafkaConsumerConfigPrefix + SuffixTopic)
o.GroupID = v.GetString(KafkaConsumerConfigPrefix + SuffixGroupID)
Expand All @@ -127,7 +126,6 @@ func (o *Options) InitFromViper(v *viper.Viper, logger *zap.Logger) {
o.DeadlockInterval = v.GetDuration(ConfigPrefix + SuffixDeadlockInterval)
authenticationOptions := auth.AuthenticationConfig{}
authenticationOptions.InitFromViper(KafkaConsumerConfigPrefix, v)
authenticationOptions.Normalize(logger)
o.AuthenticationConfig = authenticationOptions
}

Expand Down
7 changes: 3 additions & 4 deletions cmd/ingester/app/flags_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/pkg/config"
"github.com/jaegertracing/jaeger/pkg/config/tlscfg"
Expand All @@ -42,7 +41,7 @@ func TestOptionsWithFlags(t *testing.T) {
"--ingester.parallelism=5",
"--ingester.deadlockInterval=2m",
})
o.InitFromViper(v, zap.NewNop())
o.InitFromViper(v)

assert.Equal(t, "topic1", o.Topic)
assert.Equal(t, []string{"127.0.0.1:9092", "0.0.0:1234"}, o.Brokers)
Expand Down Expand Up @@ -87,7 +86,7 @@ func TestTLSFlags(t *testing.T) {
v, command := config.Viperize(AddFlags)
err := command.ParseFlags(test.flags)
require.NoError(t, err)
o.InitFromViper(v, zap.NewNop())
o.InitFromViper(v)
assert.Equal(t, test.expected, o.AuthenticationConfig)
})
}
Expand All @@ -97,7 +96,7 @@ func TestFlagDefaults(t *testing.T) {
o := &Options{}
v, command := config.Viperize(AddFlags)
command.ParseFlags([]string{})
o.InitFromViper(v, zap.NewNop())
o.InitFromViper(v)

assert.Equal(t, DefaultTopic, o.Topic)
assert.Equal(t, []string{DefaultBroker}, o.Brokers)
Expand Down
2 changes: 1 addition & 1 deletion cmd/ingester/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func main() {
}

options := app.Options{}
options.InitFromViper(v, logger)
options.InitFromViper(v)
consumer, err := builder.CreateConsumer(logger, metricsFactory, spanWriter, options)
if err != nil {
logger.Fatal("Unable to create consumer", zap.Error(err))
Expand Down
36 changes: 8 additions & 28 deletions pkg/config/tlscfg/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,40 +33,24 @@ const (
tlsSkipHostVerify = tlsPrefix + ".skip-host-verify"
)

// Enabled configures TLS enabled flags
type Enabled int

const (
// Hide hides the enabled TLS flags
Hide Enabled = iota
// Show shows the enabled TLS flags
Show
// ShowDeprecated shows deprecation annotation for tls flags
ShowDeprecated
)

// ClientFlagsConfig describes which CLI flags for TLS client should be generated.
type ClientFlagsConfig struct {
Prefix string
Enabled Enabled
ShowEnabled bool
ShowServerName bool
}

// ServerFlagsConfig describes which CLI flags for TLS server should be generated.
type ServerFlagsConfig struct {
Prefix string
ShowEnabled Enabled
ShowEnabled bool
ShowClientCA bool
}

// AddFlags adds flags for TLS to the FlagSet.
func (c ClientFlagsConfig) AddFlags(flags *flag.FlagSet) {
if c.Enabled >= Show {
deprecated := ""
if c.Enabled == ShowDeprecated {
deprecated = "(deprecated) "
}
flags.Bool(c.Prefix+tlsEnabled, false, deprecated+"Enable TLS when talking to the remote server(s)")
if c.ShowEnabled {
flags.Bool(c.Prefix+tlsEnabled, false, "Enable TLS when talking to the remote server(s)")
flags.Bool(c.Prefix+tlsEnabledOld, false, "(deprecated) see --"+c.Prefix+tlsEnabled)
}
flags.String(c.Prefix+tlsCA, "", "Path to a TLS CA (Certification Authority) file used to verify the remote server(s) (by default will use the system truststore)")
Expand All @@ -80,12 +64,8 @@ func (c ClientFlagsConfig) AddFlags(flags *flag.FlagSet) {

// AddFlags adds flags for TLS to the FlagSet.
func (c ServerFlagsConfig) AddFlags(flags *flag.FlagSet) {
if c.ShowEnabled >= Show {
deprecated := ""
if c.ShowEnabled == ShowDeprecated {
deprecated = "(deprecated) "
}
flags.Bool(c.Prefix+tlsEnabled, false, deprecated+"Enable TLS on the server")
if c.ShowEnabled {
flags.Bool(c.Prefix+tlsEnabled, false, "Enable TLS on the server")
flags.Bool(c.Prefix+tlsEnabledOld, false, "(deprecated) see --"+c.Prefix+tlsEnabled)
}
flags.String(c.Prefix+tlsCert, "", "Path to a TLS Certificate file, used to identify this server to clients")
Expand All @@ -97,7 +77,7 @@ func (c ServerFlagsConfig) AddFlags(flags *flag.FlagSet) {
// InitFromViper creates tls.Config populated with values retrieved from Viper.
func (c ClientFlagsConfig) InitFromViper(v *viper.Viper) Options {
var p Options
if c.Enabled >= Show {
if c.ShowEnabled {
p.Enabled = v.GetBool(c.Prefix + tlsEnabled)

if !p.Enabled {
Expand All @@ -117,7 +97,7 @@ func (c ClientFlagsConfig) InitFromViper(v *viper.Viper) Options {
// InitFromViper creates tls.Config populated with values retrieved from Viper.
func (c ServerFlagsConfig) InitFromViper(v *viper.Viper) Options {
var p Options
if c.ShowEnabled >= Show {
if c.ShowEnabled {
p.Enabled = v.GetBool(c.Prefix + tlsEnabled)

if !p.Enabled {
Expand Down
4 changes: 2 additions & 2 deletions pkg/config/tlscfg/flags_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func TestClientFlags(t *testing.T) {
flagSet := &flag.FlagSet{}
flagCfg := ClientFlagsConfig{
Prefix: "prefix",
Enabled: Show,
ShowEnabled: true,
ShowServerName: true,
}
flagCfg.AddFlags(flagSet)
Expand Down Expand Up @@ -102,7 +102,7 @@ func TestServerFlags(t *testing.T) {
flagSet := &flag.FlagSet{}
flagCfg := ServerFlagsConfig{
Prefix: "prefix",
ShowEnabled: Show,
ShowEnabled: true,
ShowClientCA: true,
}
flagCfg.AddFlags(flagSet)
Expand Down
14 changes: 4 additions & 10 deletions pkg/kafka/auth/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (

"github.com/Shopify/sarama"
"github.com/spf13/viper"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/pkg/config/tlscfg"
)
Expand Down Expand Up @@ -81,23 +80,18 @@ func (config *AuthenticationConfig) InitFromViper(configPrefix string, v *viper.

var tlsClientConfig = tlscfg.ClientFlagsConfig{
Prefix: configPrefix,
Enabled: tlscfg.ShowDeprecated,
ShowEnabled: true,
ShowServerName: true,
}

config.TLS = tlsClientConfig.InitFromViper(v)

config.PlainText.UserName = v.GetString(configPrefix + plainTextPrefix + suffixPlainTextUserName)
config.PlainText.Password = v.GetString(configPrefix + plainTextPrefix + suffixPlainTextPassword)
}

// Normalize normalizes kafka options
func (config *AuthenticationConfig) Normalize(logger *zap.Logger) {
if config.TLS.Enabled {
logger.Warn("Flag .tls.enabled is deprecated use " + suffixAuthentication + " instead.")
config.Authentication = tls
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is correct. Other modes can be compatible with TLS according to @jpkrohling, so just having tls.enabled should not change the auth scheme.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have removed this and made change to pkg/kafka/auth/config.go to load TLS when tls.enabled=true

}
if config.Authentication == tls {
config.TLS.Enabled = true
}

config.PlainText.UserName = v.GetString(configPrefix + plainTextPrefix + suffixPlainTextUserName)
config.PlainText.Password = v.GetString(configPrefix + plainTextPrefix + suffixPlainTextPassword)
}
2 changes: 1 addition & 1 deletion pkg/kafka/auth/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func AddFlags(configPrefix string, flagSet *flag.FlagSet) {

tlsClientConfig := tlscfg.ClientFlagsConfig{
Prefix: configPrefix,
Enabled: tlscfg.ShowDeprecated,
ShowEnabled: true,
ShowServerName: true,
}
tlsClientConfig.AddFlags(flagSet)
Expand Down
2 changes: 1 addition & 1 deletion plugin/storage/cassandra/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ func (opt *Options) InitFromViper(v *viper.Viper) {
func tlsFlagsConfig(namespace string) tlscfg.ClientFlagsConfig {
return tlscfg.ClientFlagsConfig{
Prefix: namespace,
Enabled: tlscfg.Show,
ShowEnabled: true,
ShowServerName: true,
}
}
Expand Down
2 changes: 1 addition & 1 deletion plugin/storage/es/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func NewOptions(primaryNamespace string, otherNamespaces ...string) *Options {
func (config *namespaceConfig) getTLSFlagsConfig() tlscfg.ClientFlagsConfig {
return tlscfg.ClientFlagsConfig{
Prefix: config.namespace,
Enabled: tlscfg.Show,
ShowEnabled: true,
ShowServerName: true,
}
}
Expand Down
2 changes: 1 addition & 1 deletion plugin/storage/integration/kafka_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func (s *KafkaIntegrationTestSuite) initialize() error {
return err
}
options := app.Options{}
options.InitFromViper(v, s.logger)
options.InitFromViper(v)
traceStore := memory.NewStore()
spanConsumer, err := builder.CreateConsumer(s.logger, metrics.NullFactory, traceStore, options)
if err != nil {
Expand Down
1 change: 0 additions & 1 deletion plugin/storage/kafka/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger)
logger.Info("Kafka factory",
zap.Any("producer builder", f.Builder),
zap.Any("topic", f.options.topic))
f.options.config.Normalize(logger)
p, err := f.NewProducer()
if err != nil {
return err
Expand Down
2 changes: 0 additions & 2 deletions plugin/storage/kafka/options_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"github.com/Shopify/sarama"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/pkg/config"
"github.com/jaegertracing/jaeger/pkg/config/tlscfg"
Expand Down Expand Up @@ -203,7 +202,6 @@ func TestTLSFlags(t *testing.T) {
err := command.ParseFlags(test.flags)
require.NoError(t, err)
o.InitFromViper(v)
o.config.Normalize(zap.NewNop())
assert.Equal(t, test.expected, o.config.AuthenticationConfig)

})
Expand Down