Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

snapshot/downloader: fix fetching snapshot URI #1791

Merged
merged 2 commits into from
Dec 5, 2022

Conversation

belimawr
Copy link
Contributor

@belimawr belimawr commented Nov 24, 2022

What does this PR do?

When parsing the response from the artifacts-api to get the latest snapshot (https://artifacts-api.elastic.co/v1/search/%s-SNAPSHOT/elastic-agent) it could happen that an entry that is not from the Elastic-Agent (like the Elastic-Agent-Shipper) would be evaluated and it would cause the snapshotURI function return an error.

This PR fixes it by iterating the whole map before returning an error.

Why is it important?

It fixes a bug

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.

## Author's Checklist

How to test this PR locally

Manual testing

  1. Build the 8.6 branch (it will build the version 8.6.0)
  2. Build the main branch with SNAPSHOT=true (it will build the version 8.7.0-SNAPSHOT)
  3. Get a Elastic-Stack up and running with Fleet server
  4. Install the Elastic-Agent v8.6.0
  5. Ensure it is healthy
  6. Stop the Elastic-Agent
  7. Edit it's elastic-agent.yml and add the dropPath settings:
    agent.download:
      dropPath: /home/vagrant/dropPath
  8. Copy the built artefacts to the drop path
  9. Restart the Elastic-Agent
  10. Run the upgrade command as root (just to be on the safe side, also export the dropPath env var):
    AGENT_DROP_PATH=/home/vagrant/dropPath/ ./elastic-agent upgrade 8.7.0-SNAPSHOT -v -e -d "*"
    

The caveat here is that because the bug depends on the order the API response is iterated on, sometimes it just works.

Automated version

  1. Grab my unittest file: https://github.com/belimawr/elastic-agent/blob/4e7a3b7ee3a011e4f8ac43c66984bc789448c251/internal/pkg/agent/application/upgrade/upgrade_integration_test.go
  2. Run it.

The same problem as testing manually, it depends on the order of iteration on the map, so it was an intermittent bug.

## Related issues
## Use cases
## Screenshots
## Logs

When parsing the response from the artifacts-api to get the latest
snapshot (https://artifacts-api.elastic.co/v1/search/%s-SNAPSHOT/elastic-agent)
it could happen that an entry that is not from the Elastic-Agent (like
the Elastic-Agent-Shipper) would be evaluated and it would cause the
`snapshotURI` function return an error.

This commit fixes it by iterating the whole map before returning an
error.
@belimawr belimawr added Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team backport-v8.6.0 Automated backport with mergify labels Nov 24, 2022
@belimawr belimawr requested a review from a team as a code owner November 24, 2022 18:44
@belimawr belimawr requested review from AndersonQ and michalpristas and removed request for a team November 24, 2022 18:44
@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 24, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-11-30T08:49:51.191+0000

  • Duration: 16 min 7 sec

Test stats 🧪

Test Results
Failed 0
Passed 4617
Skipped 13
Total 4630

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the 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
Copy link
Contributor

elasticmachine commented Nov 24, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 98.333% (59/60) 👍
Files 69.082% (143/207) 👍
Classes 69.133% (271/392) 👍
Methods 53.8% (814/1513) 👍
Lines 38.976% (8791/22555) 👎 -0.013
Conditionals 100.0% (0/0) 💚

@belimawr belimawr requested a review from cmacknz November 30, 2022 08:50
@cmacknz
Copy link
Member

cmacknz commented Dec 1, 2022

@belimawr let's get this merged so it can be included in the next 8.6 build candidate

@belimawr belimawr merged commit 8c034dd into elastic:main Dec 5, 2022
@belimawr belimawr deleted the fix-snapshot-downloader branch December 5, 2022 14:26
mergify bot pushed a commit that referenced this pull request Dec 5, 2022
When parsing the response from the artifacts-api to get the latest
snapshot (https://artifacts-api.elastic.co/v1/search/%s-SNAPSHOT/elastic-agent)
it could happen that an entry that is not from the Elastic-Agent (like
the Elastic-Agent-Shipper) would be evaluated and it would cause the
`snapshotURI` function return an error.

This commit fixes it by iterating the whole map before returning an
error.

(cherry picked from commit 8c034dd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.6.0 Automated backport with mergify Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants