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

cli: fix Azure SEV-SNP latest version logic #2343

Merged
merged 20 commits into from
Sep 25, 2023

Conversation

elchead
Copy link
Contributor

@elchead elchead commented Sep 18, 2023

Context

The attestationconfigapi was broken today due to an updated version number, which was not globally rolled out by Azure on their SNP machines. Since Azure doesn't publicly post the version numbers, we read the versions from VM instances and adopt new versions after 2 weeks.
The problem with the current approach is that a single run with a newer version might not be globally rolled out after 2 weeks. Instead, we should consider all runs within a given time, find the minimal version value and use this as reference.
This more conservative approach should work more reliably.

Currently :

  • the fetcher fetches the latest version value older than 2 weeks

Proposed change(s)

  • use the configapi as source of truth: only update valid version values to the endpoint
    • the version fetcher simply fetches the latest value without any filter logic
    • delegate the version logic to the uploader (reporter)
    • the reporter (as part of E2E verify) caches observed version numbers in a separate directory and updates the API latest version value with this logic. Find the minimal version within the last 3 weeks and upload this version if it is newer than the current value
    • this change allows to easily patch the version manually without having to consider the timestamp and date selection logic.
    • the window size for the latest version decision is determined by a size of latest versions (default 15) instead of a time frame

NEW

  • fix the E2E for the attestationconfigapi
    • fix verify broken bazel target
    • use the test CDN for fetching latest in the CLI instead of fetching from prod during the test

Related issue

Additional info

IMPORTANT: for backward compatibility any manual version updates need to have a timestamp older than 2 weeks but newer than the current latest until v2.12 is released

DISCUSS:

  • the minimal length of cached version numbers is set to 3 which means that no updates will be considered in the next 3 weeks. Is this okay?
  • should we cache in a separate bucket so that it is not part of the CDN?
  • we invalidate the cache with every cache upload, do we care about this inefficiency or do we want to implement a separate S3 client without CDN cache?

Checklist

  • Update docs
  • Add labels (e.g., for changelog category)
  • Is PR title adequate for changelog?
  • Link to Milestone

@netlify
Copy link

netlify bot commented Sep 18, 2023

Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit 99b25e1
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/651154503487100008043a8c

@elchead elchead force-pushed the fix/attestationconfigapi/latest-version-fetch branch from e4e0707 to 040fac6 Compare September 18, 2023 14:33
@elchead elchead added this to the v2.12.0 milestone Sep 18, 2023
@elchead elchead added the no changelog Change won't be listed in release changelog label Sep 18, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 18, 2023

Coverage report

Package Old New Trend
cli/internal/cmd 56.00% 56.00% ↔️
internal/api/attestationconfigapi 56.90% 31.00% ↘️
internal/api/attestationconfigapi/cli 14.50% 6.80% ↘️
internal/api/fetcher [no test files] [no test files] 🚧
internal/api/versionsapi 69.80% 69.80% ↔️
internal/config 80.10% 80.10% ↔️

@elchead elchead force-pushed the fix/attestationconfigapi/latest-version-fetch branch 10 times, most recently from c302658 to 99e05e1 Compare September 18, 2023 18:45
@elchead elchead marked this pull request as ready for review September 19, 2023 07:57
@elchead elchead requested a review from derpsteb as a code owner September 19, 2023 07:57
@elchead elchead force-pushed the fix/attestationconfigapi/latest-version-fetch branch from ef447f0 to ba6d315 Compare September 19, 2023 08:04
@thomasten
Copy link
Member

I only skimmed the PR. This seems to implement the minimum selection. But there's another required change, which is to get the LaunchTCB from the SNP report instead of using the (presumably) CurrentTCB from the MAA token. If you think this will considerably change this PR, you should add it to this PR. Otherwise, you can either do it in this PR or another one as you prefer.

@elchead elchead force-pushed the fix/attestationconfigapi/latest-version-fetch branch 4 times, most recently from d9a3d92 to 9f797d8 Compare September 20, 2023 14:52
@daniel-weisse
Copy link
Member

Changing the timing logic to the uploader seems like a good idea 👍
Didn't look too deep into the code, just noticed the one thing in the added docs.

@elchead elchead force-pushed the fix/attestationconfigapi/latest-version-fetch branch from 0cef3de to a9dc12a Compare September 22, 2023 13:11
@elchead elchead force-pushed the fix/attestationconfigapi/latest-version-fetch branch from 330da3a to de15070 Compare September 22, 2023 13:48
@elchead elchead force-pushed the fix/attestationconfigapi/latest-version-fetch branch 4 times, most recently from db3a25c to 5918aa1 Compare September 22, 2023 15:29
@elchead elchead force-pushed the fix/attestationconfigapi/latest-version-fetch branch from 5918aa1 to 289cad4 Compare September 23, 2023 05:29
@elchead elchead force-pushed the fix/attestationconfigapi/latest-version-fetch branch from db2d535 to 99b25e1 Compare September 25, 2023 09:35
@elchead elchead merged commit 118f789 into main Sep 25, 2023
@elchead elchead deleted the fix/attestationconfigapi/latest-version-fetch branch September 25, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Change won't be listed in release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants