Fix (*artifact.Config).Unpack not preserving not overridden values #3930

AndersonQ · 2023-12-19T19:10:31Z

What does this PR do?

(*artifact.Config).Unpack was creating a new instance of Config and manually coping all values from the original to the new one. New values added to config, RetrySleepInitDuration in this case, were neither maintaining its default value nor being updated with the new value. Now it uses reflection to ensure all values are correctly copied.

Why is it important?

When applying a new agent.download config some of the default values were set to the zero value and even if they'd come on the new fleet config, they'd be ignored.

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/fragments using the changelog tool~~
~~[ ] I have added an integration test or an E2E test~~

How to test this PR locally

Try any of the following:

Copy the test TestConfig_Unpack to main and see it failing.
Set the retry_sleep_init_duration in the agent's policy, enrol it to fleet and check it's not set.
Reproduce the issue described in 5k+ download attempts within few seconds in agent logs while upgrading 8.11.3 agent to 8.12.0 with invalid agent binary. #3914

Related issues

Closes 5k+ download attempts within few seconds in agent logs while upgrading 8.11.3 agent to 8.12.0 with invalid agent binary. #3914

Questions to ask yourself

How are we going to support this in production?
How are we going to measure its adoption?
How are we going to debug this?
What are the metrics I should take care of?
...

(*artifact.Config).Unpack was creating a new instance of Config and manually coping all values from the original to the new one. New values added to config, RetrySleepInitDuration in this case, were neither maintaining its default value nor being updated with the new value. Now it uses reflection to ensure all values are correctly copied.

elasticmachine · 2023-12-19T19:10:34Z

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

cmacknz · 2023-12-19T19:22:32Z

*artifact.Config).Unpack was creating a new instance of Config and manually coping all values from the original to the new one.

Do we know why it was doing this? Are there any hints in the commit history? This seems intentional so I am wondering if this was because of a bug or something from the past.

cmacknz · 2023-12-19T19:24:01Z

Ah the answer is the comment, thanks!

blakerouse

This is just nasty. Really sucks that this needs to be done this way. I think the way you handled it is the best way to go currently.

Really glad you added a test to ensure those structures stay inline.

cmacknz · 2023-12-19T20:16:20Z

Since we need to special case the transport, which includes the proxy configuration, I wonder if we should implement #3820 as part of this PR to ensure we didn't break it.

It's already in the current sprint and assigned to Anderson anyway.

mergify · 2023-12-20T05:42:32Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b 3914-fix-artifact-config-unpack-main upstream/3914-fix-artifact-config-unpack-main
git merge upstream/main
git push upstream 3914-fix-artifact-config-unpack-main

…-artifact-config-unpack-main

AndersonQ · 2023-12-20T06:08:13Z

buildkite test this

pierrehilbert · 2023-12-20T07:41:49Z

invalid character 'E' looking for beginning of value

Could it be something coming from your change?

belimawr · 2023-12-20T07:45:15Z

internal/pkg/agent/application/upgrade/artifact/config.go

+		if tmpField.IsValid() && tmpField.CanSet() {
+			tmpField.Set(cValue.Field(i))
+		}


You cannot be certain that tmpType.Filed(i) and cValue.Field(i)return the equivalent fields, it is safer to usehttps://pkg.go.dev/reflect#Value.FieldByName`

Suggested change

if tmpField.IsValid() && tmpField.CanSet() {

tmpField.Set(cValue.Field(i))

}

if tmpField.IsValid() && tmpField.CanSet() {

tmpField.Set(cValue.FieldByName(name))

}

If you change the field ordering from configWithoutHTTPTransportSettings and Config, then run the test TestConfig_Unpack it will fail.

Thanks for catching this, this suggests there is a test case missing to catch this.

belimawr · 2023-12-20T07:47:22Z

internal/pkg/agent/application/upgrade/artifact/config.go

-		DropPath:              tmp.DropPath,
-		HTTPTransportSettings: transport,
+		if cField.IsValid() && cField.CanSet() {
+			cField.Set(tmpValue.Field(i))


Same thing here

Suggested change

cField.Set(tmpValue.Field(i))

cField.Set(tmpValue.FieldByName(name))

jlind23 · 2023-12-20T10:21:47Z

@AndersonQ will you also tackle #3820 as suggested by Craig abovE?

AndersonQ · 2023-12-20T13:11:52Z

@AndersonQ will you also tackle #3820 as suggested by Craig abovE?

Not in this PR

AndersonQ · 2023-12-29T10:16:35Z

buildkite test this

elastic-sonarqube · 2024-01-02T10:40:34Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
100.0% 100.0% Coverage on New Code
0.0% 0.0% Duplication on New Code

See analysis details on SonarQube

…3930) (*artifact.Config).Unpack was creating a new instance of Config and manually coping all values from the original to the new one. New values added to config, RetrySleepInitDuration in this case, were neither maintaining its default value nor being updated with the new value. Now it uses reflection to ensure all values are correctly copied. --------- Co-authored-by: Pierre HILBERT <pierre.hilbert@elastic.co> (cherry picked from commit 1ee9cc7) # Conflicts: # NOTICE.txt # go.mod

…ot overridden values (#3979) (*artifact.Config).Unpack was creating a new instance of Config and manually coping all values from the original to the new one. New values added to config, RetrySleepInitDuration in this case, were neither maintaining its default value nor being updated with the new value. Now it uses reflection to ensure all values are correctly copied. --------- Co-authored-by: Pierre HILBERT <pierre.hilbert@elastic.co> (cherry picked from commit 1ee9cc7) --------- Co-authored-by: Anderson Queiroz <anderson.queiroz@elastic.co>

…lastic#3930) (*artifact.Config).Unpack was creating a new instance of Config and manually coping all values from the original to the new one. New values added to config, RetrySleepInitDuration in this case, were neither maintaining its default value nor being updated with the new value. Now it uses reflection to ensure all values are correctly copied. --------- Co-authored-by: Pierre HILBERT <pierre.hilbert@elastic.co>

…ot overridden values (#3979) (*artifact.Config).Unpack was creating a new instance of Config and manually coping all values from the original to the new one. New values added to config, RetrySleepInitDuration in this case, were neither maintaining its default value nor being updated with the new value. Now it uses reflection to ensure all values are correctly copied. --------- Co-authored-by: Pierre HILBERT <pierre.hilbert@elastic.co> (cherry picked from commit 1ee9cc7) --------- Co-authored-by: Anderson Queiroz <anderson.queiroz@elastic.co>

AndersonQ added Team:Elastic-Agent Label for the Agent team backport-v8.12.0 Automated backport with mergify labels Dec 19, 2023

AndersonQ self-assigned this Dec 19, 2023

AndersonQ requested a review from a team as a code owner December 19, 2023 19:10

AndersonQ requested review from michalpristas and blakerouse December 19, 2023 19:10

blakerouse approved these changes Dec 19, 2023

View reviewed changes

AndersonQ added 2 commits December 20, 2023 06:37

use golang.org/x/exp/slices instead of std lib

d71dd61

go mod tidy && mage notice

f4cb566

Merge branch 'main' of github.com:elastic/elastic-agent into 3914-fix…

d5219a1

…-artifact-config-unpack-main

This was referenced Dec 20, 2023

WIP: debugging 3914 #3921

Closed

5k+ download attempts within few seconds in agent logs while upgrading 8.11.3 agent to 8.12.0 with invalid agent binary. #3914

Closed

belimawr requested changes Dec 20, 2023

View reviewed changes

AndersonQ added 2 commits December 20, 2023 11:10

PR change

1afaa10

pr change

7d9610e

belimawr approved these changes Dec 20, 2023

View reviewed changes

AndersonQ added the skip-changelog label Dec 20, 2023

AndersonQ added 3 commits December 21, 2023 06:49

Merge branch 'main' into 3914-fix-artifact-config-unpack-main

59b1369

Merge branch 'main' into 3914-fix-artifact-config-unpack-main

794050e

Merge branch 'main' into 3914-fix-artifact-config-unpack-main

b49305e

Merge branch 'main' into 3914-fix-artifact-config-unpack-main

2572713

pierrehilbert and others added 3 commits January 1, 2024 13:07

Merge branch 'main' into 3914-fix-artifact-config-unpack-main

46409d1

Merge branch 'main' into 3914-fix-artifact-config-unpack-main

6ba169d

Merge branch 'main' into 3914-fix-artifact-config-unpack-main

e1923e5

AndersonQ merged commit 1ee9cc7 into elastic:main Jan 2, 2024

mergify bot mentioned this pull request Jan 2, 2024

[8.12](backport #3930) Fix (*artifact.Config).Unpack not preserving not overridden values #3979

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix (*artifact.Config).Unpack not preserving not overridden values #3930

Fix (*artifact.Config).Unpack not preserving not overridden values #3930

AndersonQ commented Dec 19, 2023

elasticmachine commented Dec 19, 2023

cmacknz commented Dec 19, 2023

cmacknz commented Dec 19, 2023

blakerouse left a comment

cmacknz commented Dec 19, 2023

mergify bot commented Dec 20, 2023

AndersonQ commented Dec 20, 2023

pierrehilbert commented Dec 20, 2023

belimawr Dec 20, 2023

cmacknz Dec 20, 2023

belimawr Dec 20, 2023

jlind23 commented Dec 20, 2023

AndersonQ commented Dec 20, 2023

AndersonQ commented Dec 29, 2023

elastic-sonarqube bot commented Jan 2, 2024

	cField.Set(tmpValue.Field(i))
	cField.Set(tmpValue.FieldByName(name))

Fix (*artifact.Config).Unpack not preserving not overridden values #3930

Fix (*artifact.Config).Unpack not preserving not overridden values #3930

Conversation

AndersonQ commented Dec 19, 2023

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

Questions to ask yourself

elasticmachine commented Dec 19, 2023

cmacknz commented Dec 19, 2023

cmacknz commented Dec 19, 2023

blakerouse left a comment

Choose a reason for hiding this comment

cmacknz commented Dec 19, 2023

mergify bot commented Dec 20, 2023

AndersonQ commented Dec 20, 2023

pierrehilbert commented Dec 20, 2023

belimawr Dec 20, 2023

Choose a reason for hiding this comment

cmacknz Dec 20, 2023

Choose a reason for hiding this comment

belimawr Dec 20, 2023

Choose a reason for hiding this comment

jlind23 commented Dec 20, 2023

AndersonQ commented Dec 20, 2023

AndersonQ commented Dec 29, 2023

elastic-sonarqube bot commented Jan 2, 2024

Quality Gate passed