Pass the parsed version to all downloaders #3824

pchila · 2023-11-27T14:51:30Z

What does this PR do?

Avoid stripping build metadata off the version string for agent upgrade versions

Why is it important?

It should be possible to use build metadata to identify specific builds of a given version for any downloader (fs, http and snapshot)

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~~

Author's Checklist

[ ]

How to test this PR locally

Let's create 2 packages with versions containing build metadata from the elastic-agent code:

Note: the way we create the version containing +buildYYYYMMDD is a hack and it's not compatible with SNAPSHOT versions

Create the first package camouflaging the agent as an 8.11.1 build by running:

AGENT_PACKAGE_VERSION=8.11.1+build20231130 BEAT_VERSION=8.11.1  DEV=true EXTERNAL=true PLATFORMS="linux/amd64" PACKAGES=tar.gz mage package

Create an empty commit before creating the second package (to avoid collisions in directory names during upgrade):
```
git commit --allow-empty -m "Empty commit to change agent hash"
```

Create a second package:

AGENT_PACKAGE_VERSION=8.11.1+build20231201 BEAT_VERSION=8.11.1  DEV=true EXTERNAL=true PLATFORMS="linux/amd64" PACKAGES=tar.gz mage package

Install the first package (no need to enroll in fleet)
Test that upgrading using the fs downloader works (adapt the file:// url to the location where the second package is located on disk):
```
elastic-agent upgrade 8.11.1+build20231201 --skip-verify --source-uri file:///vagrant/build/distributions/
```

Verify that the upgrade proceeds and that at the end we get an elastic-agent version similar to this:

elastic-agent version --yaml

root@elastic-agent-dev:~# elastic-agent version --yaml
binary:
  version: 8.11.1+build20231201
  commit: 29def7ee77ea9f10de7cb0b24e85153f07372ec2
  build_time: 2023-12-01T09:59:54Z
  snapshot: false
daemon:
  version: 8.11.1+build20231201
  commit: 29def7ee77ea9f10de7cb0b24e85153f07372ec2
  build_time: 2023-12-01T09:59:54Z
  snapshot: false

Create a simple directory structure, link the last part of the path to where the packages are located and start a simple http server (in the example I am using python)
```
mkdir beats
ln -s /vagrant/build/distributions beats/elastic-agent
 python3 -m http.server 
```
when the http server starts, it should look like this
```
vagrant@elastic-agent-dev:~$  python3 -m http.server
Serving HTTP on 0.0.0.0 port 8000 (http://0.0.0.0:8000/) ...
```

Now we can test the http downloader by specifying a source-uri by going back to the previous version

  root@elastic-agent-dev:~# elastic-agent upgrade 8.11.1+build20231130 --skip-verify --source-uri http://localhost:8000
Upgrade triggered to version 8.11.1+build20231130, Elastic Agent is currently restarting

root@elastic-agent-dev:~# elastic-agent version --yaml                                                    
binary:
  version: 8.11.1+build20231130
  commit: 8439fe6a814319e3ad9575abd5dcc01012678d35
  build_time: 2023-12-01T13:50:11Z
  snapshot: false
daemon:
  version: 8.11.1+build20231130
  commit: 8439fe6a814319e3ad9575abd5dcc01012678d35
  build_time: 2023-12-01T13:50:11Z
  snapshot: false

Related issues

Closes Upgrades from 8.9 to present strip build metadata from valid semantic versions #3813

Use cases

Screenshots

Logs

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?
...

mergify · 2023-11-27T14:52:08Z

This pull request does not have a backport label. Could you fix it @pchila? 🙏
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 · 2023-12-01T14:11:35Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

cmacknz · 2023-12-01T19:37:00Z

internal/pkg/agent/application/upgrade/artifact/download/snapshot/downloader.go

-	return e.downloader.Download(ctx, a, version)
+func (e *Downloader) Download(ctx context.Context, a artifact.Artifact, version *agtversion.ParsedSemVer) (string, error) {
+	// remove build metadata to match filename of the package for the specific snapshot build
+	strippedVersion := agtversion.NewParsedSemVer(version.Major(), version.Minor(), version.Patch(), version.Prerelease(), "")


What happens if we don't do this? Why can't we have a version like 8.13.0-SNAPSHOT+buildYYYYMMDD where -SNAPSHOT is the prerelease component?

We have to define what the URL for such a case looks like: right now packages from SNAPSHOTS builds don't have the build metadata or snapshot in the name, what should be the convention for the 8.13.0-SNAPSHOT+buildYYYYMMDD ?

Right the problem is that we have decided that snapshot IDs are specified as build metadata, even though that is not actually the version in the artifacts API. This limits our ability to handle the case where we actually want build metadata in the version.

We could solve this conflict in the artifacts API by doing something like:

https://snapshots.elastic.co/8.11.2-buildYYYYMMDD/downloads/beats/elastic-agent/elastic-agent-8.11.2-SNAPSHOT-darwin-aarch64.tar.gz

That said this doesn't exist yet, and because this is only for snapshots we can fix it without worrying about the stack release cadence. So let's just leave it as is.

internal/pkg/agent/application/upgrade/artifact/download/http/verifier.go

internal/pkg/agent/application/upgrade/artifact/download/snapshot/verifier.go

cmacknz · 2023-12-01T19:40:09Z

internal/pkg/agent/application/upgrade/step_download.go

@@ -219,7 +219,7 @@ func (u *Upgrader) downloadOnce(
 	// All download artifacts expect a name that includes <major>.<minor.<patch>[-SNAPSHOT] so we have to
 	// make sure not to include build metadata we might have in the parsed version (for snapshots we already
 	// used that to configure the URL we download the files from)
-	path, err := downloader.Download(ctx, agentArtifact, version.VersionWithPrerelease())
+	path, err := downloader.Download(ctx, agentArtifact, version)


This is the actual fix, are we able to unit or integration test this?

The integration tests already test the http and fs downloaders in the upgrade tests, not sure what scenario you would have in mind.

For unit tests, given the current code structure we would have to mock an artifact api on a test HTTP server, not exactly a "unit" test

I believe a mock HTTP server which is something that is really easily to create with golang is the way to test this. I agree that we need to test this to ensure that the behavior is working as expected.

I would prefer to see the test at this level as well, so its testing the whole step_download section.

For unit tests, given the current code structure we would have to mock an artifact api on a test HTTP server, not exactly a "unit" test

It won't be the first not exactly a "unit" test, besides as Blake pointed out, it's easy in Go and if you want, you could make it an integration test.

I agree a test is important, for me it can be in any form you prefer as long as there is a test

blakerouse

Overall this looks good.

I am worried that it doesn't have an associated unit test with it. I think we need to get that added before this gets merged.

blakerouse · 2023-12-04T19:18:50Z

internal/pkg/agent/application/upgrade/step_download.go

@@ -219,7 +219,7 @@ func (u *Upgrader) downloadOnce(
 	// All download artifacts expect a name that includes <major>.<minor.<patch>[-SNAPSHOT] so we have to
 	// make sure not to include build metadata we might have in the parsed version (for snapshots we already
 	// used that to configure the URL we download the files from)
-	path, err := downloader.Download(ctx, agentArtifact, version.VersionWithPrerelease())
+	path, err := downloader.Download(ctx, agentArtifact, version)


I believe a mock HTTP server which is something that is really easily to create with golang is the way to test this. I agree that we need to test this to ensure that the behavior is working as expected.

I would prefer to see the test at this level as well, so its testing the whole step_download section.

AndersonQ

As Blake said, it needs a test, unit or integration, it's up to you as long as there is a test

cmacknz

The new tests are great! Thank you!

Did a quick sanity check running sudo elastic-agent upgrade 8.12.0+buildYYYYMMDD and the build metadata is no longer stripped as expected.

❯ sudo elastic-agent status
┌─ fleet
│  └─ status: (STOPPED) Not enrolled into Fleet
├─ elastic-agent
│  └─ status: (UPGRADING) Upgrading to version 8.12.0+buildYYYYMMDD
└─ upgrade_details
   ├─ target_version: 8.12.0+buildYYYYMMDD
   ├─ state: UPG_DOWNLOADING
   └─ metadata
      ├─ download_percent: 0.00%
      ├─ retry_until: 1h59m44.87304s
      └─ retry_error_msg: unable to download package: 3 errors occurred:
             * package '/Library/Elastic/Agent/data/elastic-agent-f29cd5/downloads/elastic-agent-8.12.0+buildYYYYMMDD-darwin-aarch64.tar.gz' not found: open /Library/Elastic/Agent/data/elastic-agent-f29cd5/downloads/elastic-agent-8.12.0+buildYYYYMMDD-darwin-aarch64.tar.gz: no such file or directory
             * call to 'https://snapshots.elastic.co/8.12.0-51bw2wko/downloads/beats/elastic-agent/elastic-agent-8.12.0-darwin-aarch64.tar.gz' returned unsuccessful status code: 404
             * call to 'https://artifacts.elastic.co/downloads/beats/elastic-agent/elastic-agent-8.12.0+buildYYYYMMDD-darwin-aarch64.tar.gz' returned unsuccessful status code: 404

blakerouse

Looks good.

I really appreciate you adding the tests. I know that part is not fun and can be really boring, but it really does provide confidence in this PR and more confidence in the Elastic Agent overall. Thank you!

elastic-sonarqube · 2023-12-05T21:31:10Z

Quality Gate passed

The SonarQube Quality Gate passed, but some issues were introduced.

6 New issues
0 Security Hotspots
45.7% 45.7% Coverage on New Code
0.0% 0.0% Duplication on New Code

See analysis details on SonarQube

mergify bot assigned pchila Nov 27, 2023

mergify bot added the backport-skip label Nov 27, 2023

pchila force-pushed the pass-the-whole-version-string branch from 1486421 to 8f0832d Compare November 27, 2023 14:52

pchila linked an issue Nov 27, 2023 that may be closed by this pull request

Upgrades from 8.9 to present strip build metadata from valid semantic versions #3813

Closed

pchila added the bug Something isn't working label Dec 1, 2023

pchila force-pushed the pass-the-whole-version-string branch from 8f0832d to 29def7e Compare December 1, 2023 14:10

pchila added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Dec 1, 2023

pchila marked this pull request as ready for review December 1, 2023 14:11

pchila requested a review from a team as a code owner December 1, 2023 14:11

pchila requested review from AndersonQ and michalpristas December 1, 2023 14:11

cmacknz reviewed Dec 1, 2023

View reviewed changes

pchila requested a review from cmacknz December 4, 2023 09:14

pchila force-pushed the pass-the-whole-version-string branch from 45acd04 to 819ce7f Compare December 4, 2023 16:03

blakerouse requested changes Dec 4, 2023

View reviewed changes

AndersonQ requested changes Dec 4, 2023

View reviewed changes

pchila force-pushed the pass-the-whole-version-string branch from f2ae45c to 6635487 Compare December 5, 2023 16:11

pchila requested review from AndersonQ and blakerouse December 5, 2023 18:04

AndersonQ approved these changes Dec 5, 2023

View reviewed changes

pchila added 8 commits December 5, 2023 20:28

Pass the parsed version to all downloaders

ba3fc60

add changelog

a95f322

Remove aVersion variable names

e707aa5

fixup! Remove aVersion variable names

64c5a37

Add fs downloader unit tests

3e4b6f2

add unit tests for http downloader

6ff3976

add tests for snapshot downloader

afb111e

fixup! add tests for snapshot downloader

f9b71ab

pchila added 3 commits December 5, 2023 20:28

fixup! fixup! add tests for snapshot downloader

a73d675

fixup! fixup! fixup! add tests for snapshot downloader

477a43a

fixup! fixup! fixup! fixup! add tests for snapshot downloader

1bd9d52

pchila force-pushed the pass-the-whole-version-string branch from f29cd5b to 1bd9d52 Compare December 5, 2023 19:29

cmacknz approved these changes Dec 5, 2023

View reviewed changes

Merge branch 'main' into pass-the-whole-version-string

cf3cf3c

blakerouse approved these changes Dec 5, 2023

View reviewed changes

cmacknz merged commit fdce58b into elastic:main Dec 5, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass the parsed version to all downloaders #3824

Pass the parsed version to all downloaders #3824

pchila commented Nov 27, 2023 •

edited

mergify bot commented Nov 27, 2023

elasticmachine commented Dec 1, 2023

cmacknz Dec 1, 2023

pchila Dec 4, 2023 •

edited

cmacknz Dec 4, 2023 •

edited

cmacknz Dec 1, 2023

pchila Dec 4, 2023

blakerouse Dec 4, 2023

AndersonQ Dec 4, 2023

blakerouse left a comment

blakerouse Dec 4, 2023

AndersonQ left a comment

cmacknz left a comment

blakerouse left a comment

elastic-sonarqube bot commented Dec 5, 2023

Pass the parsed version to all downloaders #3824

Pass the parsed version to all downloaders #3824

Conversation

pchila commented Nov 27, 2023 • edited

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Questions to ask yourself

mergify bot commented Nov 27, 2023

elasticmachine commented Dec 1, 2023

cmacknz Dec 1, 2023

Choose a reason for hiding this comment

pchila Dec 4, 2023 • edited

Choose a reason for hiding this comment

cmacknz Dec 4, 2023 • edited

Choose a reason for hiding this comment

cmacknz Dec 1, 2023

Choose a reason for hiding this comment

pchila Dec 4, 2023

Choose a reason for hiding this comment

blakerouse Dec 4, 2023

Choose a reason for hiding this comment

AndersonQ Dec 4, 2023

Choose a reason for hiding this comment

blakerouse left a comment

Choose a reason for hiding this comment

blakerouse Dec 4, 2023

Choose a reason for hiding this comment

AndersonQ left a comment

Choose a reason for hiding this comment

cmacknz left a comment

Choose a reason for hiding this comment

blakerouse left a comment

Choose a reason for hiding this comment

elastic-sonarqube bot commented Dec 5, 2023

Quality Gate passed

pchila commented Nov 27, 2023 •

edited

pchila Dec 4, 2023 •

edited

cmacknz Dec 4, 2023 •

edited