Support reloading ES client's password from file #4342

haanhvu · 2023-03-26T20:53:08Z

Which problem is this PR solving?

Resolves the final 2nd and 3rd steps for #3924 as specified here

haanhvu · 2023-03-26T20:55:47Z

@yurishkuro Could you pls review if this implementation approach seems to make sense? If yes, I'll continue to fix current broken tests and add related tests.

plugin/storage/es/factory.go

haanhvu · 2023-04-08T04:23:31Z

I'm changing the approach from resetting the password to swapping the whole ES client. That might be easier to test and wouldn't introduce subtle errors...

haanhvu · 2023-04-15T18:52:51Z

@yurishkuro Could you help me a little here?

My approach is creating a channel of ES options and a channel of ES client. Everytime password from file changes, a new array of options is channeled to create a new client. That client is channeled to the factory.

Some tests are broken but basically the one problem here is the client channel can receive the client, but cannot assign client in the factory. The test result says assert.NotNil(t, <-f.primaryClientChan) passes but assert.NotNil(t, f.primaryClient) fails. When I debugged the problem seems to be in assigning client in factory.

Could you take a look to see what I did wrong here?

yurishkuro · 2023-04-15T20:49:13Z

I think channels are overkill for this. There is already a callback mechanism in fswatcher that can be used to propagate all the changes. Critically though, your changes so far don't actually affect how the storage works, because factory.CreateSpanReader() is called only once, which will instantiate the client only once:

jaeger/plugin/storage/es/factory.go

Lines 195 to 196 in 6cbc11e

    
           writer := esSpanStore.NewSpanWriter(esSpanStore.SpanWriterParams{ 
        
           	Client:                 client,

If recreating the client is the only option (which may be fine), then this is how I would go about it:

don't pass a client to NewSpanReader, but pass a function that returns a client
the function will retrieve the client from f.primaryClient which should be turned into atomic.Value
pass a callback function to fswatcher such that when that call back is invoked the client is re-created and replaced in f.primaryClient

codecov · 2023-04-18T12:34:16Z

Codecov Report

Patch coverage is 91.25% of modified lines.

Files Changed	Coverage
plugin/storage/es/dependencystore/storage.go	`66.66%`
plugin/storage/es/factory.go	`90.47%`
plugin/storage/es/options.go	`100.00%`
plugin/storage/es/spanstore/reader.go	`100.00%`
plugin/storage/es/spanstore/service_operation.go	`100.00%`
plugin/storage/es/spanstore/writer.go	`100.00%`

📢 Thoughts on this report? Let us know!.

haanhvu · 2023-04-18T15:36:04Z

@yurishkuro I implemented your approach. Could you review the implementation, especially the lint failures in onPasswordChange()? From the failures, it seems like I didn't assign client successfully in onPasswordChange() (and probably in initializeClient() too)? Maybe I didn't use the pointer correctly? I'll add tests when the implementation is fine.

plugin/storage/es/spanstore/reader_test.go

plugin/storage/es/factory.go

pkg/es/config/config.go

plugin/storage/es/factory.go

pkg/es/config/config.go

plugin/storage/es/factory.go

yurishkuro · 2023-04-23T03:56:58Z

plugin/storage/es/factory.go

+}
+
+func (f *Factory) onPrimaryPasswordChange() {
+	primaryClient, err := f.newClientFn(f.primaryConfig, f.logger, f.metricsFactory)


You are using the old config here, not the new password. This needs a unit test

The current tests and lint all pass now so I'm adding new tests now.

But could you help explain why password is not updated here? I thought when we recall f.newClientFn(f.primaryConfig, ...) then we'd recall getConfigOptions and password file path would be reloaded?

This is why when you develop new functionality you often start with a unit test that verifies what you're trying to implement. In this case we want to observe that a new client is created once pwd file is changed - if you had that test you would've caught several bugs I already pointed out.

Yeah sorry I'll add that test now^^ I have exams until Thursday so maybe I'll push the test after that.

no need to apologize, your contributions are greatly appreciated.

Signed-off-by: haanhvu <[email protected]>

haanhvu

@yurishkuro I added a test for password change in primary config.

Also, in the implementation, there was a race detected so I changed config type to atomic.

Please help review! The test passed but there're probably some problems.

haanhvu · 2023-04-28T10:08:05Z

plugin/storage/es/factory_test.go

 )

-var _ storage.Factory = new(Factory)
+/*var _ storage.Factory = new(Factory)


Since I changed config type to atomic, I have to change it in the existed tests too to make them pass. But for now I'm commenting these tests to focus on the TestPasswordFromFile test.

haanhvu · 2023-04-28T10:10:49Z

plugin/storage/es/factory_test.go

+
+	_, err = passwordFile.WriteString("baz")
+	require.NoError(t, err)
+	f.onPrimaryPasswordChange()


I don't know why but I have to explicitly call f.onPrimaryPasswordChange() here to make the password change. Don't know if it's a problem of the watcher in the implementation or a problem in my test.

I think you need to either fsync or close the file after writing, otherwise the write may be still in the OS buffer

Also, because watcher is running in a separate goroutine, you cannot immediately validate if the client was updated, usually we use assert.Eventually (grep for examples in this repo)

yurishkuro · 2023-04-29T22:46:28Z

pkg/es/config/config.go

@@ -296,6 +297,10 @@ func (c *Configuration) TagKeysAsFields() ([]string, error) {

 // getConfigOptions wraps the configs to feed to the ElasticSearch client init
 func (c *Configuration) getConfigOptions(logger *zap.Logger) ([]elastic.ClientOptionFunc, error) {
+	if c.Password != "" && c.PasswordFilePath != "" {
+		return nil, fmt.Errorf("both Password and PasswordFilePath are set")
+	}


please move just before L319, to keep related logic together

yurishkuro · 2023-04-29T22:48:49Z

plugin/storage/es/factory.go

-		f.archiveClient, err = f.newClientFn(f.archiveConfig, logger, metricsFactory)
+	f.primaryClient.Store(&primaryClient)
+
+	primaryWatcher, err := fswatcher.New([]string{f.primaryConfig.Load().PasswordFilePath}, f.onPrimaryPasswordChange, f.logger)


creating a watcher should be conditional on PasswordFilePath != ""

yurishkuro · 2023-04-29T22:49:11Z

plugin/storage/es/factory.go

 		if err != nil {
 			return fmt.Errorf("failed to create archive Elasticsearch client: %w", err)
 		}
+		f.archiveClient.Store(&archiveClient)
+
+		archiveWatcher, err := fswatcher.New([]string{f.archiveConfig.Load().PasswordFilePath}, f.onArchivePasswordChange, f.logger)


also conditional

yurishkuro · 2023-04-29T22:52:09Z

plugin/storage/es/factory.go

+		f.logger.Error("failed to reload password for primary Elasticsearch client", zap.Error(err))
+	} else {
+		newPrimaryCfg.Password = newPrimaryPassword
+		f.primaryConfig.Store(&newPrimaryCfg)


there is no need to overwrite the primary config:

newConfig := primaryConfig // copy by value newConfig.Password = newPrimaryPassword f.newClientFn(newConfig, ...)

yurishkuro · 2023-04-29T22:54:36Z

plugin/storage/es/factory.go

 }

 func createSpanReader(
 	mFactory metrics.Factory,
 	logger *zap.Logger,
-	client es.Client,
+	client *atomic.Pointer[es.Client],


I recommend adding a function to the factory and passing it as argument to createSpanReader() and similar functions. This will reduce the coupling: createXyz functions would not need to know about atomic pointers.

func (f *Factory) getPrimaryClient() es.Client { return f.primaryClient.Load() }

yurishkuro · 2023-04-29T22:57:34Z

plugin/storage/es/factory_test.go

+
+	_, err = passwordFile.WriteString("baz")
+	require.NoError(t, err)
+	f.onPrimaryPasswordChange()


I think you need to either fsync or close the file after writing, otherwise the write may be still in the OS buffer

yurishkuro · 2023-04-29T23:04:06Z

plugin/storage/es/factory_test.go

+	}
+
+	f := NewFactory()
+	f.newClientFn = func(c *escfg.Configuration, logger *zap.Logger, metricsFactory metrics.Factory) (es.Client, error) {


why do you need this function? It looks like it's repeating the same code as in the factory.

You need to verify that the es.Client is updated once the password is modified. The easiest way is to do factory.getPrimaryClient and check its authentication settings, assuming they are accessible. If not accessible, the other option is to create a fake server and invoke a method on the client that would cause it to make an HTTP call. We don't care if the call succeeds or not, but the fake server will be able to inspect the auth headers in that call, which is the ultimate validation.

yurishkuro · 2023-04-29T23:05:55Z

plugin/storage/es/factory_test.go

+
+	_, err = passwordFile.WriteString("baz")
+	require.NoError(t, err)
+	f.onPrimaryPasswordChange()


Also, because watcher is running in a separate goroutine, you cannot immediately validate if the client was updated, usually we use assert.Eventually (grep for examples in this repo)

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro · 2023-09-08T16:20:41Z

pkg/es/config/config.go

@@ -412,7 +424,7 @@ func GetHTTPRoundTripper(c *Configuration, logger *zap.Logger) (http.RoundTrippe
 	return transport, nil
 }

-func loadToken(path string) (string, error) {
+func LoadFileContent(path string) (string, error) {


TODO this should probably remain being called LoadToken, since it's not a plain file read

Signed-off-by: Yuri Shkuro <[email protected]>

haanhvu requested a review from a team as a code owner March 26, 2023 20:53

haanhvu requested a review from albertteoh March 26, 2023 20:53

haanhvu mentioned this pull request Mar 26, 2023

Add option to read password from file in ES storage #4235

Closed

github-advanced-security bot found potential problems Mar 26, 2023

View reviewed changes

plugin/storage/es/factory.go Fixed Show fixed Hide fixed

yurishkuro reviewed Mar 26, 2023

View reviewed changes

plugin/storage/es/factory.go Outdated Show resolved Hide resolved

yurishkuro reviewed Mar 26, 2023

View reviewed changes

plugin/storage/es/factory.go Outdated Show resolved Hide resolved

yurishkuro reviewed Mar 26, 2023

View reviewed changes

plugin/storage/es/factory.go Outdated Show resolved Hide resolved

yurishkuro reviewed Mar 26, 2023

View reviewed changes

plugin/storage/es/factory.go Outdated Show resolved Hide resolved

haanhvu mentioned this pull request Apr 3, 2023

Jaeger's Google Summer of Code 2023 projects #4331

Closed

haanhvu marked this pull request as draft April 10, 2023 06:27

haanhvu force-pushed the issue3924-2-1 branch 2 times, most recently from 546c472 to d388310 Compare April 15, 2023 17:22

haanhvu marked this pull request as ready for review April 15, 2023 18:54

haanhvu force-pushed the issue3924-2-1 branch from d3b7485 to 07550e5 Compare April 18, 2023 12:27

yurishkuro reviewed Apr 19, 2023

View reviewed changes

haanhvu force-pushed the issue3924-2-1 branch from 6a9c6e9 to 6495012 Compare April 21, 2023 13:24

haanhvu marked this pull request as draft April 21, 2023 13:24

yurishkuro reviewed Apr 21, 2023

View reviewed changes

haanhvu force-pushed the issue3924-2-1 branch from 6495012 to 47a7cad Compare April 21, 2023 16:05

yurishkuro reviewed Apr 21, 2023

View reviewed changes

plugin/storage/es/factory.go Outdated Show resolved Hide resolved

yurishkuro reviewed Apr 23, 2023

View reviewed changes

haanhvu force-pushed the issue3924-2-1 branch from 93f19f7 to aac6ca9 Compare April 23, 2023 10:02

haanhvu added 2 commits April 28, 2023 16:31

Swap ES client when password changes

9ff837a

Signed-off-by: haanhvu <[email protected]>

Append watcher incrementally

3b5f514

Signed-off-by: haanhvu <[email protected]>

haanhvu added 3 commits April 28, 2023 16:31

Fix lint failures and append from nil slice

6dc3a2e

Signed-off-by: haanhvu <[email protected]>

Add options test

3641d67

Signed-off-by: haanhvu <[email protected]>

Make config atomic and add test on primary config

2bcfafc

Signed-off-by: haanhvu <[email protected]>

haanhvu force-pushed the issue3924-2-1 branch from b0a4821 to 2bcfafc Compare April 28, 2023 09:32

Add f.onPrimaryPasswordChange() in test

c9e8a2a

Signed-off-by: haanhvu <[email protected]>

haanhvu commented Apr 28, 2023

View reviewed changes

yurishkuro reviewed Apr 29, 2023

View reviewed changes

yurishkuro changed the title ~~Allow updating ES client's password from file~~ [WIP] Allow updating ES client's password from file Jun 5, 2023

yurishkuro mentioned this pull request Jun 27, 2023

[Feature]: Reloading of server certificates for server endpoints #4554

Closed

yurishkuro and others added 3 commits September 8, 2023 11:50

Merge branch 'main' into issue3924-2-1

447683c

Signed-off-by: Yuri Shkuro <[email protected]>

move pwd lines together

66ad336

Signed-off-by: Yuri Shkuro <[email protected]>

cleanup

1db1edd

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro reviewed Sep 8, 2023

View reviewed changes

yurishkuro added 3 commits September 8, 2023 12:38

restore main tests

8e64941

Signed-off-by: Yuri Shkuro <[email protected]>

Add test

dbac89c

Signed-off-by: Yuri Shkuro <[email protected]>

fix test

29e3cb6

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro changed the title ~~[WIP] Allow updating ES client's password from file~~ Support reloading ES client's password from file Sep 8, 2023

cleanup

4c719e7

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro marked this pull request as ready for review September 8, 2023 23:10

yurishkuro added 2 commits September 8, 2023 19:55

simplify, add tests

ee85e72

Signed-off-by: Yuri Shkuro <[email protected]>

add logging

842243c

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro merged commit 2ac6aac into jaegertracing:main Sep 9, 2023

yurishkuro mentioned this pull request Oct 18, 2024

[jaeger-v2] Refactor ElasticSearch/OpenSearch configurations to have more logical groupings #6090

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support reloading ES client's password from file #4342

Support reloading ES client's password from file #4342

haanhvu commented Mar 26, 2023

haanhvu commented Mar 26, 2023

haanhvu commented Apr 8, 2023

haanhvu commented Apr 15, 2023

yurishkuro commented Apr 15, 2023

codecov bot commented Apr 18, 2023 •

edited

Loading

haanhvu commented Apr 18, 2023 •

edited

Loading

yurishkuro Apr 23, 2023

haanhvu Apr 23, 2023 •

edited

Loading

yurishkuro Apr 23, 2023

haanhvu Apr 24, 2023

yurishkuro Apr 25, 2023

haanhvu left a comment •

edited

Loading

haanhvu Apr 28, 2023 •

edited

Loading

haanhvu Apr 28, 2023

yurishkuro Apr 29, 2023

yurishkuro Apr 29, 2023

yurishkuro Apr 29, 2023

yurishkuro Apr 29, 2023

yurishkuro Apr 29, 2023

yurishkuro Apr 29, 2023

yurishkuro Apr 29, 2023

yurishkuro Apr 29, 2023

yurishkuro Apr 29, 2023

yurishkuro Apr 29, 2023

yurishkuro Sep 8, 2023

Support reloading ES client's password from file #4342

Support reloading ES client's password from file #4342

Conversation

haanhvu commented Mar 26, 2023

Which problem is this PR solving?

haanhvu commented Mar 26, 2023

haanhvu commented Apr 8, 2023

haanhvu commented Apr 15, 2023

yurishkuro commented Apr 15, 2023

codecov bot commented Apr 18, 2023 • edited Loading

Codecov Report

haanhvu commented Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

haanhvu Apr 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haanhvu left a comment • edited Loading

Choose a reason for hiding this comment

haanhvu Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 18, 2023 •

edited

Loading

haanhvu commented Apr 18, 2023 •

edited

Loading

haanhvu Apr 23, 2023 •

edited

Loading

haanhvu left a comment •

edited

Loading

haanhvu Apr 28, 2023 •

edited

Loading