Source URI Reloading backports #35327
Conversation
michalpristas
requested review from
AndersonQ,
michel-laterman,
rdner and
fearful-symmetry
and removed request for
a team
botelastic
bot
added
the
needs_team
|
This pull request doesn't have a |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
| @@ -113,12 +115,89 @@ func NewOperator( | |||
|
|
|||
| operator.initHandlerMap() | |||
|
|
|||
| os.MkdirAll(config.DownloadConfig.TargetDirectory, 0755) | |||
| os.MkdirAll(config.DownloadConfig.InstallPath, 0755) | |||
| if err := os.MkdirAll(config.DownloadConfig.TargetDirectory, 0755); err != nil { | |||
There was a problem hiding this comment.
Nitpick because I know that this is dead (or soon to be) code most likely: can't we check if it's already exist instead of trying to create it every time and log a warning when we get error (which could be any kind of error), something like
| if err := os.MkdirAll(config.DownloadConfig.TargetDirectory, 0755); err != nil { | |
| _, err := os.Stat(config.DownloadConfig.TargetDirectory | |
| if errors.Is(err, fs.ErrNotExist) { | |
| err := os.MkdirAll(config.DownloadConfig.InstallPath, 0755) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed creating %q: %v", config.DownloadConfig.TargetDirectory, err) | |
| } | |
| } | |
| if err != nil { | |
| return nil, fmt.Errorf("error accessing download target directory %q", config.DownloadConfig.TargetDirectory) | |
| } |
I realize it's a lot more verbose but the error checking should be more precise
Disclaimer: I wrote the code directly in this comment so it probably does not compile, please do not apply the suggestion as-is 😅
| @@ -166,7 +245,7 @@ func (o *Operator) HandleConfig(cfg configrequest.Request) (err error) { | |||
|
|
|||
| _, stateID, steps, ack, err := o.stateResolver.Resolve(cfg) | |||
| if err != nil { | |||
| if err == filterContextCancelled(err) { | |||
| if errors.Is(err, filterContextCancelled(err)) { | |||
| if tc.expectedTLS { | ||
| require.NotNil(t, cfg.TLS) | ||
| require.Equal(t, tc.expectedTLSEnabled, *cfg.TLS.Enabled) | ||
| //require.Equal(t, tc.expectedFingerprint, cfg.TLS.CATrustedFingerprint) |
There was a problem hiding this comment.
no it's backport, i'm copying code, this is just something not backported yet
| client, err := c.HTTPTransportSettings.Client( | ||
| httpcommon.WithAPMHTTPInstrumentation(), | ||
| ) | ||
| if err != nil { | ||
| return errors.New(err, "http.downloader: failed to generate client out of config") | ||
| } | ||
|
|
||
| client.Transport = download.WithHeaders(client.Transport, download.Headers) |
There was a problem hiding this comment.
Can't we do it like in x-pack/elastic-agent/pkg/artifact/download/http/verifier.go below ?
| client, err := c.HTTPTransportSettings.Client( | |
| httpcommon.WithAPMHTTPInstrumentation(), | |
| ) | |
| if err != nil { | |
| return errors.New(err, "http.downloader: failed to generate client out of config") | |
| } | |
| client.Transport = download.WithHeaders(client.Transport, download.Headers) | |
| client, err := c.HTTPTransportSettings.Client( | |
| httpcommon.WithAPMHTTPInstrumentation(), | |
| httpcommon.WithModRoundtripper(func(rt http.RoundTripper) http.RoundTripper { | |
| return download.WithHeaders(rt, download.Headers) | |
| }), | |
| ) | |
| if err != nil { | |
| return errors.New(err, "http.downloader: failed to generate client out of config") | |
| } |
|
@pchila i will ignore code style comments, as this is a code copy it would not be friendly for future backports. |
Backport of multiple PRs related to config/source uri reloading
elastic/elastic-agent#686
elastic/elastic-agent#1252
How to test:
Standalone:
needs to fail with proper message
Managed
Succ scenario:
change uri to
http://localhost:8080and start fileserver therepython3 -m http.serverdirectory needs to contain target asc, sha512 and archive files in location
beats/elastic-agent