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.

Sign up for GitHub

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

Jump to bottom

fix garage-push #67

Merged
merged 2 commits into from
Jul 6, 2023
Merged

fix garage-push #67

merged 2 commits into from
Jul 6, 2023

Conversation

cajun-rat
Copy link
Collaborator

Disable sending the manifest, because the torizon backend doesn't support the format, and add explicit 'uses network' annotations.

@cajun-rat cajun-rat mentioned this pull request Apr 17, 2023
Closed
@sergioprado
Copy link

sergioprado commented Apr 17, 2023 •
edited
Loading

I would recommend to:

  1. Break down into two commits, one for the network flag, and the other for the manifest.
  2. Since there are other meta-updater users, I would suggest defining a variable to disable sending the manifest, so we keep the current behavior.

@cajun-rat
Copy link
Collaborator Author

No problem to split the commits. My guess is that the upload to treehub feature has been broken since the kirkstone release a year ago, when the network access was disabled by default. Are there any active servers that accept an manifest via the url /treehub/api/v3/manifests/? I'm happy to put the work in to support them, but if no-one uses it then I'd suggest to nuke the --repo-manifest option.

@pattivacek
Copy link
Collaborator

I don't remember the use case for those manifests anymore. AFAIK Foundries and Toradex are the two major implementations, so if neither of y'all are using it, then I agree, we should just get rid of it.

Who might know on the Foundries side? @doanac or @mike-sul?

@mike-sul
Copy link
Collaborator

I don't remember the use case for those manifests anymore. AFAIK Foundries and Toradex are the two major implementations, so if neither of y'all are using it, then I agree, we should just get rid of it.

Who might know on the Foundries side? @doanac or @mike-sul?

I think we/Foundries should be fine with this change.

ricardosalveti pushed a commit to ricardosalveti/meta-updater-1 that referenced this pull request Apr 26, 2023
@shr-project
aktualizr: bump SRCREV
e64e1b0 
* brings 37 new commits:
git log --oneline 637d1f19d14d3a99fad71d110f5018505b85d2c7..bf0494df6382dd26e3daa89d57a393c0ff77f7d8
bf0494df6 Speed up compiles with Include What You Use
2a00664ea Merge pull request uptane#72 from cajun-rat/build-fix2
dcbe347da Fix build breakage caused by git CVE-2022-24765 fixes
4345deec2 Merge pull request uptane#68 from uptane/hsm-test-failure-fix
5e6e4aef3 tests: Suppress a memory leak in libcrypto
81ab71daa crypto: Make the p11 engine singleton within a test suite
7ac53f631 Merge pull request uptane#67 from uptane/fix-codecov-badge
7700175cd Update codecov URLs.
cd373e6e1 secondary: Init structure to make clang-tidy happy
caf558b24 docker: Create a home directory of the test user
3cb7e2a2a scripts: Fix the script to run tests in docker
5c9ba5d40 Revert "Disable broken p11 tests until someone can sort them out."
a7983c56e Merge pull request uptane#63 from uptane/disable-broken-p11-tests
e8f7ce9de Disable broken p11 tests until someone can sort them out.
b6d60658f Merge pull request uptane#64 from cajun-rat/fix-aktualizr-kill
305e3219e Merge pull request uptane#62 from uptane/shellcheck-fixes
e84be427b Fix flakey aktualizr_kill test
987b52dc4 More shellcheck fixes, this time with v0.8.0.
17e71e0f8 Add Github Actions job to run shellcheck in CI.
c8e3ac84f More shellcheck fixes.
67408853d Fix easy shellcheck problems.
df868c6ff Merge pull request uptane#61 from cajun-rat/extra-keys-root-json
d2ab5e51e Add test for extra keys in root.json
9e14d6a74 Merge pull request uptane#60 from uptane/follow-redirects
1b6e9ae2d http: Remove redundant curl options.
068ea3573 [fio extras]: httpclient: follow redirects by default
6f75976e1 Merge pull request uptane#55 from cajun-rat/api-queue-refactor
6daaa0939 Rewrite ApiQueue as an explicit implementation
91daac983 Tidy up: std::move tasks in Aktualizr
813c1a68b Add test for ApiQueue
4249e9f53 Merge pull request uptane#59 from cajun-rat/import-root-keys
3b8fd67b5 Import initial root.json from the filesystem
718bd87e2 Better handling of existing paths in uptane-generator
9a298772a Remove STORAGE_TYPE option from CMakeLists and tests
d64fcb81b Remove dead code
f73c68e95 Merge pull request uptane#58 from uptane/aktualizr-info-keys
06c5a631b aktualizr-info: Add options to print Secondary ECU keys and all key IDs.

The last commit fixes the includes so that it builds OK with new glibc and new boost, replacing
uptane#38
quaresmajose
quaresmajose reviewed Apr 26, 2023
View reviewed changes
Copy link
Contributor

@quaresmajose quaresmajose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree in splitting out the network access to a new a commit.

classes/image_types_ostree.bbclass
local SEND_MANIFEST=""
# check if garage-push supports the --repo-manifest option before trying
if $(garage-push --help | grep -q '^\s*--repo-manifest') && [ -f ${IMAGE_ROOTFS}${sysconfdir}/manifest.xml ]; then
SEND_MANIFEST="--repo-manifest ${IMAGE_ROOTFS}${sysconfdir}/manifest.xml"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add a variable outside the function with the manifest location that can be changed/cleaned to not send the manifest

OSTREE_SEND_MANIFEST ?= "${IMAGE_ROOTFS}${sysconfdir}/manifest.xml"

IMAGE_CMD:ostreepush () {
    # send a copy of the repo manifest to backend if available
    local SEND_MANIFEST=""
    if [ -f "${OSTREE_SEND_MANIFEST}" ]; then
        SEND_MANIFEST="--repo-manifest ${OSTREE_SEND_MANIFEST}"
    fi

@tkfu
Copy link
Member

tkfu commented Jun 16, 2023

I think it should be removed entirely. The root cause of this issue is that repo (upstream) changed the schema of the manifest, such that treehub cannot parse it. See https://github.com/uptane/treehub/blob/master/src/main/scala/com/advancedtelematic/treehub/http/ManifestParser.scala for details of that implementation. So the only way that pushing the manifest will work, is if someone happens to be intentionally using an old version of repo on kirkstone. I can't imagine why we would care to support that particular use case.

But even if we did want to support uploading the manifest, AFAIK nobody ever actually used this. There is a database table in treehub that stores it, but no codepath in treehub actually reads from this table. The intention, I believe, was to track statistics on the version of meta-updater people were using in ATS Garage/HERE OTA Connect, but that tracking/monitoring was obviously never implemented.

Unless/until someone wants to step up with a pull request for an implementation of treehub that actually uses the manifest parsing/storage, I don't think uptane/meta-updater should be concerned with supporting a feature that uptane/treehub does not support.

@pattivacek
Copy link
Collaborator

I think it should be removed entirely.

If by "it" you mean the manifest, then I agree with your analysis.

Unless/until someone wants to step up with a pull request for an implementation of treehub that actually uses the manifest parsing/storage, I don't think uptane/meta-updater should be concerned with supporting a feature that uptane/treehub does not support.

This isn't gonna happen. :) Nobody is interested in this and it would be easy to restore if someone ever becomes interested.

We seem to have consensus on that part, but there was a request to split into two commits. @cajun-rat can you still do that?

@cajun-rat
Copy link
Collaborator Author

We seem to have consensus on that part, but there was a request to split into two commits. @cajun-rat can you still do that?

Yep, I'm on it. Things have been busy and I've managed to break my Yocto build in a strange way which I've been trying to unpick. Sorry for the delay!

cajun-rat added 2 commits July 5, 2023 21:34
@cajun-rat
Tell Yocto that garage push needs network access
20e9d45 
The Kirkstone release requires that steps that require network access
explicitly ask for it. Add these for garage push.
@cajun-rat
Remove option to send a manifest in garage-push
8dbc660 
There are no servers that exist at the moment[1] that support sending a
manifest. Don't attempt to send it any more. It was failing to parse and
breaking uploading builds using SOTA_PACKED_CREDENTIALS.
pattivacek
pattivacek approved these changes Jul 6, 2023
View reviewed changes
Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM! Let's merge it.

@pattivacek pattivacek merged commit 9f7fe77 into uptane:kirkstone Jul 6, 2023
@quaresmajose quaresmajose mentioned this pull request Jul 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quaresmajose quaresmajose quaresmajose left review comments

@pattivacek pattivacek pattivacek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cajun-rat @sergioprado @pattivacek @mike-sul @tkfu @quaresmajose