Skip to content

Commit a20100e

Browse files
[refactor] Used OTEL Optional type for union Auth struct (#7316)
<!-- !! Please DELETE this comment before posting. We appreciate your contribution to the Jaeger project! πŸ‘‹πŸŽ‰ --> ## Which problem is this PR solving? part of - #7225 ## Description of the changes It is part of the planned work to add support for API authentication, as discussed in #7230 (comment) 1.Refactor `config.go` - Updated the `Authentication` struct to use `configoptional.Optional` for `BasicAuthentication` and `BearerTokenAuthentication`. - Modified tests to align with the new configoptional.Optional 2.Redact sensitive file paths from logs - Added a `redactPath` function to hash filenames in logs while keeping directory and extension. - Updated all log statements to use redacted paths instead of full file paths. - Expanded and updated tests to verify path redaction in log output. ## How was this change tested? - ## Checklist - [ ] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [ ] I have signed all commits - [ ] I have added unit tests for the new functionality - [ ] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: danish9039 <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>
1 parent 3cce66e commit a20100e

File tree

8 files changed

+608
-249
lines changed

8 files changed

+608
-249
lines changed

β€Žcmd/query/app/token_propagation_test.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,15 @@ import (
1717
"go.opentelemetry.io/collector/config/configgrpc"
1818
"go.opentelemetry.io/collector/config/confighttp"
1919
"go.opentelemetry.io/collector/config/confignet"
20+
"go.opentelemetry.io/collector/config/configoptional"
2021
"go.uber.org/zap/zaptest"
2122

2223
"github.com/jaegertracing/jaeger/cmd/internal/flags"
2324
"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
2425
v2querysvc "github.com/jaegertracing/jaeger/cmd/query/app/querysvc/v2/querysvc"
2526
"github.com/jaegertracing/jaeger/internal/auth/bearertoken"
2627
"github.com/jaegertracing/jaeger/internal/config"
28+
escfg "github.com/jaegertracing/jaeger/internal/storage/elasticsearch/config"
2729
es "github.com/jaegertracing/jaeger/internal/storage/v1/elasticsearch"
2830
"github.com/jaegertracing/jaeger/internal/storage/v2/v1adapter"
2931
"github.com/jaegertracing/jaeger/internal/telemetry"
@@ -80,9 +82,18 @@ func runQueryService(t *testing.T, esURL string) *Server {
8082
"--es.server-urls=" + esURL,
8183
"--es.create-index-templates=false",
8284
}))
85+
8386
f.InitFromViper(v, flagsSvc.Logger)
8487
// set AllowTokenFromContext manually because we don't register the respective CLI flag from query svc
85-
f.Options.Config.Authentication.BearerTokenAuthentication.AllowFromContext = true
88+
bearerAuth := escfg.BearerTokenAuthentication{
89+
AllowFromContext: true,
90+
}
91+
// set the authentication in the factory options
92+
f.Options.Config.Authentication = escfg.Authentication{
93+
BearerTokenAuthentication: configoptional.Some(bearerAuth),
94+
}
95+
96+
// Initialize the factory with metrics and logger
8697
require.NoError(t, f.Initialize(telset.Metrics, telset.Logger))
8798
defer f.Close()
8899

β€Žinternal/fswatcher/fswatcher_test.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,20 +80,16 @@ func TestFSWatcherWithMultipleFiles(t *testing.T) {
8080
testFile1, err := os.Create(tempDir + "test-file-1")
8181
require.NoError(t, err)
8282
defer testFile1.Close()
83-
8483
testFile2, err := os.Create(tempDir + "test-file-2")
8584
require.NoError(t, err)
8685
defer testFile2.Close()
87-
8886
_, err = testFile1.WriteString("test content 1")
8987
require.NoError(t, err)
90-
9188
_, err = testFile2.WriteString("test content 2")
9289
require.NoError(t, err)
9390

9491
zcore, logObserver := observer.New(zapcore.InfoLevel)
9592
logger := zap.New(zcore)
96-
9793
onChange := func() {
9894
logger.Info("Change happens")
9995
}
@@ -116,6 +112,7 @@ func TestFSWatcherWithMultipleFiles(t *testing.T) {
116112
return logObserver.FilterMessage("Change happens").Len() > 0
117113
},
118114
"Unable to locate 'Change happens' in log. All logs: %v", logObserver)
115+
119116
newHash1, err := hashFile(testFile1.Name())
120117
require.NoError(t, err)
121118
newHash2, err := hashFile(testFile2.Name())
@@ -133,6 +130,7 @@ func TestFSWatcherWithMultipleFiles(t *testing.T) {
133130
return logObserver.FilterMessage("Received event").Len() > 0
134131
},
135132
"Unable to locate 'Received event' in log. All logs: %v", logObserver)
133+
136134
assertLogs(t,
137135
func() bool {
138136
return logObserver.FilterMessage("Unable to read the file").Len() >= 2 // Check for multiple occurrences

β€Žinternal/storage/elasticsearch/config/config.go

Lines changed: 81 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/asaskevich/govalidator"
2222
esV8 "github.com/elastic/go-elasticsearch/v9"
2323
"github.com/olivere/elastic/v7"
24+
"go.opentelemetry.io/collector/config/configoptional"
2425
"go.opentelemetry.io/collector/config/configtls"
2526
"go.uber.org/zap"
2627
"go.uber.org/zap/zapcore"
@@ -193,8 +194,8 @@ type BulkProcessing struct {
193194
}
194195

195196
type Authentication struct {
196-
BasicAuthentication BasicAuthentication `mapstructure:"basic"`
197-
BearerTokenAuthentication BearerTokenAuthentication `mapstructure:"bearer_token"`
197+
BasicAuthentication configoptional.Optional[BasicAuthentication] `mapstructure:"basic"`
198+
BearerTokenAuthentication configoptional.Optional[BearerTokenAuthentication] `mapstructure:"bearer_token"`
198199
}
199200

200201
type BasicAuthentication struct {
@@ -340,8 +341,11 @@ func (bcb *bulkCallback) invoke(id int64, requests []elastic.BulkableRequest, re
340341
func newElasticsearchV8(ctx context.Context, c *Configuration, logger *zap.Logger) (*esV8.Client, error) {
341342
var options esV8.Config
342343
options.Addresses = c.Servers
343-
options.Username = c.Authentication.BasicAuthentication.Username
344-
options.Password = c.Authentication.BasicAuthentication.Password
344+
if c.Authentication.BasicAuthentication.HasValue() {
345+
basicAuth := c.Authentication.BasicAuthentication.Get()
346+
options.Username = basicAuth.Username
347+
options.Password = basicAuth.Password
348+
}
345349
options.DiscoverNodesOnStart = c.Sniffing.Enabled
346350
options.CompressRequestBody = c.HTTPCompression
347351
transport, err := GetHTTPRoundTripper(ctx, c, logger)
@@ -379,11 +383,33 @@ func (c *Configuration) ApplyDefaults(source *Configuration) {
379383
if len(c.RemoteReadClusters) == 0 {
380384
c.RemoteReadClusters = source.RemoteReadClusters
381385
}
382-
if c.Authentication.BasicAuthentication.Username == "" {
383-
c.Authentication.BasicAuthentication.Username = source.Authentication.BasicAuthentication.Username
384-
}
385-
if c.Authentication.BasicAuthentication.Password == "" {
386-
c.Authentication.BasicAuthentication.Password = source.Authentication.BasicAuthentication.Password
386+
// Handle BasicAuthentication defaults
387+
sourceHasBasicAuth := source.Authentication.BasicAuthentication.HasValue()
388+
targetHasBasicAuth := c.Authentication.BasicAuthentication.HasValue()
389+
if sourceHasBasicAuth {
390+
// If target doesn't have BasicAuth, copy it from source
391+
if !targetHasBasicAuth {
392+
c.Authentication.BasicAuthentication = source.Authentication.BasicAuthentication
393+
} else {
394+
// Target has BasicAuth, apply field-level defaults
395+
sourceBasicAuth := source.Authentication.BasicAuthentication.Get()
396+
// Make a copy of target BasicAuth
397+
basicAuth := *c.Authentication.BasicAuthentication.Get()
398+
399+
// Apply defaults for username if not set
400+
if basicAuth.Username == "" && sourceBasicAuth.Username != "" {
401+
basicAuth.Username = sourceBasicAuth.Username
402+
}
403+
// Apply defaults for password if not set
404+
if basicAuth.Password == "" && sourceBasicAuth.Password != "" {
405+
basicAuth.Password = sourceBasicAuth.Password
406+
}
407+
408+
// Only update BasicAuthentication if we have values to set
409+
if basicAuth.Username != "" || basicAuth.Password != "" {
410+
c.Authentication.BasicAuthentication = configoptional.Some(basicAuth)
411+
}
412+
}
387413
}
388414
if !c.Sniffing.Enabled {
389415
c.Sniffing.Enabled = source.Sniffing.Enabled
@@ -504,8 +530,14 @@ func (c *Configuration) getConfigOptions(ctx context.Context, logger *zap.Logger
504530
// 1. When health check is explicitly disabled
505531
// 2. When tokens are EXCLUSIVELY available from context (not from file)
506532
// because at startup we don't have a valid token to do the health check
507-
disableHealthCheck := c.DisableHealthCheck ||
508-
(c.Authentication.BearerTokenAuthentication.AllowFromContext && c.Authentication.BearerTokenAuthentication.FilePath == "")
533+
disableHealthCheck := c.DisableHealthCheck
534+
535+
// Check if we have bearer token or API key authentication that only allows from context
536+
if c.Authentication.BearerTokenAuthentication.HasValue() {
537+
bearerAuth := c.Authentication.BearerTokenAuthentication.Get()
538+
disableHealthCheck = disableHealthCheck || (bearerAuth.AllowFromContext && bearerAuth.FilePath == "")
539+
}
540+
509541
// Get base Elasticsearch options using the helper function
510542
options := c.getESOptions(disableHealthCheck)
511543
// Configure HTTP transport with TLS and authentication
@@ -521,19 +553,28 @@ func (c *Configuration) getConfigOptions(ctx context.Context, logger *zap.Logger
521553
}
522554

523555
options = append(options, elastic.SetHttpClient(httpClient))
556+
524557
// Basic authentication setup
525-
if c.Authentication.BasicAuthentication.Password != "" && c.Authentication.BasicAuthentication.PasswordFilePath != "" {
526-
return nil, errors.New("both Password and PasswordFilePath are set")
527-
}
528-
if c.Authentication.BasicAuthentication.PasswordFilePath != "" {
529-
passwordFromFile, err := loadTokenFromFile(c.Authentication.BasicAuthentication.PasswordFilePath)
530-
if err != nil {
531-
return nil, fmt.Errorf("failed to load password from file: %w", err)
558+
if c.Authentication.BasicAuthentication.HasValue() {
559+
basicAuth := c.Authentication.BasicAuthentication.Get()
560+
561+
password := basicAuth.Password
562+
passwordFilePath := basicAuth.PasswordFilePath
563+
564+
if password != "" && passwordFilePath != "" {
565+
return nil, errors.New("both Password and PasswordFilePath are set")
566+
}
567+
if passwordFilePath != "" {
568+
passwordFromFile, err := loadTokenFromFile(passwordFilePath)
569+
if err != nil {
570+
return nil, fmt.Errorf("failed to load password from file: %w", err)
571+
}
572+
password = passwordFromFile
532573
}
533-
c.Authentication.BasicAuthentication.Password = passwordFromFile
534-
}
535574

536-
options = append(options, elastic.SetBasicAuth(c.Authentication.BasicAuthentication.Username, c.Authentication.BasicAuthentication.Password))
575+
username := basicAuth.Username
576+
options = append(options, elastic.SetBasicAuth(username, password))
577+
}
537578

538579
// Add logging configuration
539580
options, err = addLoggerOptions(options, c.LogLevel, logger)
@@ -598,23 +639,27 @@ func GetHTTPRoundTripper(ctx context.Context, c *Configuration, logger *zap.Logg
598639

599640
// Wrap with authentication layer if configured.
600641
var roundTripper http.RoundTripper = transport
601-
if c.Authentication.BearerTokenAuthentication.AllowFromContext || c.Authentication.BearerTokenAuthentication.FilePath != "" {
602-
token := ""
603-
if c.Authentication.BearerTokenAuthentication.FilePath != "" {
604-
if c.Authentication.BearerTokenAuthentication.AllowFromContext {
605-
logger.Warn("Token file and token propagation are both enabled, token from file won't be used")
606-
}
607-
tokenFromFile, err := loadTokenFromFile(c.Authentication.BearerTokenAuthentication.FilePath)
608-
if err != nil {
609-
return nil, err
642+
643+
if c.Authentication.BearerTokenAuthentication.HasValue() {
644+
bearerAuth := c.Authentication.BearerTokenAuthentication.Get()
645+
if bearerAuth.AllowFromContext || bearerAuth.FilePath != "" {
646+
token := ""
647+
if bearerAuth.FilePath != "" {
648+
if bearerAuth.AllowFromContext {
649+
logger.Warn("Token file and token propagation are both enabled, token from file won't be used")
650+
}
651+
tokenFromFile, err := loadTokenFromFile(bearerAuth.FilePath)
652+
if err != nil {
653+
return nil, err
654+
}
655+
token = tokenFromFile
610656
}
611-
token = tokenFromFile
612-
}
613657

614-
roundTripper = &auth.RoundTripper{
615-
Transport: transport,
616-
OverrideFromCtx: c.Authentication.BearerTokenAuthentication.AllowFromContext,
617-
StaticToken: token,
658+
roundTripper = &auth.RoundTripper{
659+
Transport: transport,
660+
OverrideFromCtx: bearerAuth.AllowFromContext,
661+
StaticToken: token,
662+
}
618663
}
619664
}
620665

0 commit comments

Comments
Β (0)