[Elastic-Agent] Added source uri reloading #756

pierrehilbert · 2022-07-21T11:40:14Z

What does this PR do?

Why is it important?

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

mergify · 2022-07-21T11:40:23Z

This pull request does not have a backport label. Could you fix it @pierrehilbert? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

elasticmachine · 2022-07-21T13:42:32Z

❕ Build Aborted

Either there was a build timeout or someone aborted the build.

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-07-21T11:40:30.165+0000
Duration: 122 min 57 sec

Test stats 🧪

Test	Results
Failed	0
Passed	4915
Skipped	21
Total	4936

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages.
run integration tests : Run the Elastic Agent Integration tests.
run end-to-end tests : Generate the packages and run the E2E Tests.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2022-07-21T13:42:39Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	97.297% (`72/74`)	👍 0.075
Files	69.027% (`156/226`)	👍 1.153
Classes	68.973% (`309/448`)	👍 0.946
Methods	53.129% (`866/1630`)	👍 1.214
Lines	39.142% (`9322/23816`)	👍 0.914
Conditionals	100.0% (`0/0`)	💚

criamico · 2022-07-21T14:24:55Z

I tested this functionality e2e together with my changes in Kibana. The reloading itself seems to work as expected:

Created In Kibana a new source download source object with a mock URI https://test-registry.co
Created a new agent policy and assigned the download source object to it
I ran elastic-agent inspect and the source_URI is present there:

elastic-agent@b907b836ccf4:~$ elastic-agent inspect
agent:
  download:
    source_uri: https://test-registry.co

I then changed back the agent policy to the default value and ran elastic-agent inspect again. The policy reloaded properly:

elastic-agent@b907b836ccf4:~$ elastic-agent inspect
agent:
  download:
    source_uri: https://artifacts.elastic.co/downloads/beats/elastic-agent

However I tried to force the upgrade of the agent and it doesn't read the source_uri from the policy as I would expect. It only gets the uri from the actions if it's passed through the endpoint, otherwise it uses some hardcoded values.

I can raise a ticket where I detail the use case and in the meantime this PR can be merged.

michalpristas · 2022-07-25T07:18:56Z

i will test it as well. i finally got my HW

michalpristas · 2022-07-25T12:55:18Z

i see where the problem is, kibana sets this as source_uri but we use sourceURI as mentioned in the original proposal/issue as well.
renaming from our end seems like breaking so i will handle both changes in reloader part but Kibana side should also stick to our naming
inspect returns whatever comes from fleet, it does not mean agent understands the value

michalpristas · 2022-07-25T12:57:58Z

actually looking at it it will make our code messy, @criamico please fix this on your end

michalpristas · 2022-07-25T13:52:10Z

nevermind, i added support for changed coming from fleet as well not exposing source_uri to standalone.
latest changes and fix from this one in my original PR #686

AndersonQ · 2022-07-25T16:30:34Z

internal/pkg/agent/application/pipeline/actions/handlers/handler_action_policy_change.go

@@ -151,7 +151,7 @@ func (h *PolicyChange) handleFleetServerHosts(ctx context.Context, c *config.Con
 			errors.TypeNetwork, errors.M("hosts", h.config.Fleet.Client.Hosts))
 	}
 	// discard body for proper cancellation and connection reuse
-	io.Copy(ioutil.Discard, resp.Body)
+	_, _ = io.Copy(ioutil.Discard, resp.Body)


AndersonQ · 2022-07-25T16:47:40Z

internal/pkg/artifact/config.go

+type Reloader struct {
+	log *logger.Logger
+	cfg *Config
+}
+
+func NewReloader(cfg *Config, log *logger.Logger) *Reloader {
+	return &Reloader{
+		log: log,
+		cfg: cfg,
+	}
+}
+
+func (r *Reloader) Reload(rawConfig *config.Config) error {
+	type c struct {
+		Config *Config `config:"agent.download" yaml:"agent.download" json:"agent.download"`
+	}
+
+	cfg := &c{
+		Config: DefaultConfig(),
+	}
+	if err := rawConfig.Unpack(&cfg); err != nil {
+		return err
+	}
+
+	r.log.Debugf("Source URI changed from %q to %q", r.cfg.SourceURI, cfg.Config.SourceURI)
+	if cfg.Config.SourceURI != "" {
+		r.cfg.SourceURI = cfg.Config.SourceURI
+	}
+
+	return nil
+}
+


[Blocker]
Sorry but I'm a bit puzzled here. How does it changes the source URI the agent uses? The changes made by the Reload method seem to be only local. It only affects r.cfg.SourceURI that isn't used anywhere else. Perhaps you forgot to change the SourceURI returned by DefaultConfig as elastic-agent/pull/686 did?

Also, it'd be good to add a test for that

r has a pointer to a config on a heap. this one is propagated down the stack to each component.
downloader has the pointer to the same thing so when it tries to resolve URI it will pick it up.
same logic applied for monitoring reloader.

pierrehilbert · 2022-07-25T17:05:49Z

@michalpristas is back now and added my changes to his PR.
I close this one to avoid confusion.

michalpristas and others added 9 commits July 7, 2022 10:08

added source uri reloading

f1d98a0

typo

319bd63

typo

78cd614

typo

19082fc

typo

635f75b

use reloadable

3b89b2b

use reloadable

48aa387

use reloadable

0649adf

Correcting nil pointer exception.

a74b7aa

pierrehilbert requested a review from a team as a code owner July 21, 2022 11:40

pierrehilbert requested review from AndersonQ and aleksmaus and removed request for a team July 21, 2022 11:40

mergify bot assigned pierrehilbert Jul 21, 2022

mergify bot added the backport-skip label Jul 21, 2022

pierrehilbert added enhancement New feature or request Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team v8.4.0 labels Jul 21, 2022

jen-huang mentioned this pull request Jul 21, 2022

[Elastic-Agent] Added source uri reloading #686

Merged

6 tasks

criamico mentioned this pull request Jul 22, 2022

[Elastic-Agent] Elastic Agent doesn't read sourceURI parameter from the policy. #766

Closed

AndersonQ requested changes Jul 25, 2022

View reviewed changes

pierrehilbert closed this Jul 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Elastic-Agent] Added source uri reloading #756

[Elastic-Agent] Added source uri reloading #756

pierrehilbert commented Jul 21, 2022

mergify bot commented Jul 21, 2022

elasticmachine commented Jul 21, 2022

Build stats

Test stats 🧪

elasticmachine commented Jul 21, 2022

criamico commented Jul 21, 2022 •

edited

Loading

michalpristas commented Jul 25, 2022

michalpristas commented Jul 25, 2022 •

edited

Loading

michalpristas commented Jul 25, 2022

michalpristas commented Jul 25, 2022

AndersonQ Jul 25, 2022

AndersonQ Jul 25, 2022

michalpristas Jul 25, 2022

pierrehilbert commented Jul 25, 2022

[Elastic-Agent] Added source uri reloading #756

[Elastic-Agent] Added source uri reloading #756

Conversation

pierrehilbert commented Jul 21, 2022

What does this PR do?

Why is it important?

Checklist

mergify bot commented Jul 21, 2022

elasticmachine commented Jul 21, 2022

❕ Build Aborted

Build stats

Test stats 🧪

🤖 GitHub comments

elasticmachine commented Jul 21, 2022

🌐 Coverage report

criamico commented Jul 21, 2022 • edited Loading

michalpristas commented Jul 25, 2022

michalpristas commented Jul 25, 2022 • edited Loading

michalpristas commented Jul 25, 2022

michalpristas commented Jul 25, 2022

AndersonQ Jul 25, 2022

Choose a reason for hiding this comment

AndersonQ Jul 25, 2022

Choose a reason for hiding this comment

michalpristas Jul 25, 2022

Choose a reason for hiding this comment

pierrehilbert commented Jul 25, 2022

criamico commented Jul 21, 2022 •

edited

Loading

michalpristas commented Jul 25, 2022 •

edited

Loading