Repository navigation
ISLE verifier: update GitHub cache from publish-artifacts workflow as well. - #15083
Conversation
|
(I've created the appropriate secrets in this repo pointing to the S3 bucket that I created for testing -- let's see if this works!) |
github-actions
Bot
added
cranelift
Subscribe to Label Actioncc @avanhatt, @cfallin, @fitzgen, @mmcloughlin DetailsThis issue or pull request has been labeled: "cranelift", "isle"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
| runs-on: ubuntu-latest | ||
| # Keep the two copies of this condition (here and in the verify step) in | ||
| # sync. | ||
| environment: ${{ ((github.event_name == 'merge_group' && github.event.merge_group.base_ref == 'refs/heads/main') || (github.event_name == 'workflow_dispatch' && github.ref == 'refs/heads/main')) && 'isle-veri-cache-writer' || '' }} |
There was a problem hiding this comment.
Could this unconditionally set the environment key? My assumption is that PRs already don't get access to secrets, even if this is set
There was a problem hiding this comment.
- You can restrict which workflows and branches can access an environment. Setting the environment here unconditionally means that wouldn't be possible as defense in depth.
- Each time a workflow runs, this causes an entry in https://github.com/bytecodealliance/wasmtime/deployments and in addition, an event is added to the PR that caused the environment saying that it deployed to the environment. So each time you add a new commit to a PR you did get a rather noisy event that is way bigger than a commit event and persists after force pushes.
(the PR I took this from has over 30 such events, taking up over half of the total scroll space)
There was a problem hiding this comment.
I don't understand much about GitHub environments beyond what I briefly read yesterday, but I did believe that this rule was necessary to ensure that secrets would not leak to PRs; and even if technically not with current scoping rules, I'd prefer to have defense-in-depth.
Reading the page in settings, I see: "They are not passed to workflows that are triggered by a pull request from a fork." That seems to imply (by omission) that secrets are present for PRs from branches within the main repo. I don't want to grant access to the secrets to anyone who has collaborator access on their own unreviewed PRs, so I do think we need an explicit condition here (or at least, that's my reading!).
What do you think?
There was a problem hiding this comment.
That seems to imply (by omission) that secrets are present for PRs from branches within the main repo.
I believe so yeah.
so I do think we need an explicit condition here (or at least, that's my reading!).
Yes and you will have to restrict the environment to the main branch (+ branch protection to prevent direct pushes) to prevent a contributor from just modifying the workflow to use the environment on their branch.
There was a problem hiding this comment.
PRs from other repos will never get secrets, and by default no new branches are allowed in the Wasmtime repo. In that sense there's no real chance of leakage AFAIK. We've had a PERSONAL_ACCESS_TOKEN secret for quite some time, not in an environment, and that's arguably much more important to protect than access to the s3 bucket.
If the environment were unconditionally configured here then my understanding is we'd go edit the environment settings to conditionally have access to the secret on some branches but not others. Why wouldn't that work to scope this? (or, alternatively, just remove environment here since it may not play well with our repo configuration)
| # (anonymous access) otherwise. | ||
| AWS_ACCESS_KEY_ID: ${{ secrets.ISLE_VERI_CACHE_AWS_ACCESS_KEY_ID }} | ||
| AWS_SECRET_ACCESS_KEY: ${{ secrets.ISLE_VERI_CACHE_AWS_SECRET_ACCESS_KEY }} | ||
| CACHE_MODE: ${{ ((github.event_name == 'merge_group' && github.event.merge_group.base_ref == 'refs/heads/main') || (github.event_name == 'workflow_dispatch' && github.ref == 'refs/heads/main')) && 'read-write' || 'read-only' }} |
There was a problem hiding this comment.
Could CACHE_MODE be inferred based on the presence/absence of the access key and secret access key?
There was a problem hiding this comment.
This file in general is pretty intense -- could this perhaps shell out to the AWS CLI which I believe is installed on github runners instead?
There was a problem hiding this comment.
Sure, I can take a look at this next week! My intent was to make this usable for developers running client-side too, for what it's worth, where I would not necessarily want to require them to install the AWS SDK. With the public-readonly ACL on the bucket, the idea is that folks could let the tool download the cache when the run locally and it should Just Work.
The other options, if not the external CLI tool, seemed to be either a little 400-line handwritten (err, robot-written) S3 client with just the methods we need, or the full AWS Rust SDK (and vet it + deps). I couldn't think of a secret fourth option.
What do you think?
There was a problem hiding this comment.
That's the part for me basically, I don't really want an unreviewed S3 client LLM-authored here, and I'd prefer to not have to review it myself. Could aws be used when available and maybe curl when it's not?
There was a problem hiding this comment.
So my thinking on this is: the S3 actions we need are more than just "get a single file with a known name", so curl is not going to lead to a simpler end-state: we will effectively need to write an AWS client still, except instead of using ureq to access the endpoint, we are shelling out to a subprocess with the right verb/URI/request headers/signature/etc and then parsing the result just the same.
Concretely for example: parts of s3.rs implement (i) XML parsing of the result of a list-object request; (ii) request signing, which requires an AWS-specific algorithm and inserts a custom header with the signature, for authorization; (iii) all the setup of other request details for each kind of action. Very little of the file is base-level HTTP stuff that curl would handle, and curl used to implement any cache usage or update (usage requires list-objects) would need all of the above. So we'd have "an S3 client but with more steps".
IMHO if we want to minimize custom logic that we carry ourselves, the right answer is probably to use a real S3 client crate. I haven't quantified exactly what the vetting burden of that is but that'd be the cost, vs. reviewing this code here (or equivalent code that does all the same things but generating curl commandlines). That's probably a better long-term direction if we hope to use S3 from other build infra though, now that I write it out...
Alternately, we redesign the cache so it really is still "a single file" and then decouple all uses of S3 to separate actions run in CI to download and upload; but then we lose the incremental-update benefit, and I think that's important here (otherwise we amplify our S3 PUT cost by ~20x compared to this compact-every-20-times approach, and PUTs are the expensive actions). I imagine that other kinds of caching we might want to do will probably want fine-grained hit/miss/update capabilities rather than "single giant blob we monolithically rebuild" so it seems better to me to push in the direction (i.e. figure out our S3 client approach) of enabling in-band S3 API access from whatever build tools, rather than making all our tooling use a local filesystem than snapshotting the whole thing at the end. At least that's the hypothesis of this PR.
What do you think?
|
I'll also note that ideally we'd leverage S3-based caching (or just something external) for much more than just ISLE, for example integrating |
…as well. We previously had a workflow where we published the updated cache only from CI runs in `main.yml`, but because of branch scoping combined with our use of the merge queue, runs on "`main`" actually ran on a merge-queue temporary branch and thus did not update the cache contents in a location available for other runs. The `main`-scoped content was updated only by our once-per-night full CI run on `main`. This commit updates `publish-artifacts.yml` to add a job to the flow that *does* run on `main`, *after* merge, on every merge: it fetches the artifact from the build we just did and publishes it to the GitHub cache. This way, cache updates from all changes that are merged will be immediately available to subsequent CI runs.
d21db3b to
8201057
Compare
|
I talked with @alexcrichton just now and we came upon a much simpler idea: we can solve the proximate issue (once-per-night frequency of updates to cached ISLE verification state) in a much simpler way by moving where the GitHub cache update happens into the on-every-merge We can still potentially use S3 for other caching actions, but it's probably more complexity than we want to accept for now. I just force-pushed away the S3-related changes here (and will update title/description) and made the yaml-only change; @alexcrichton let me know what you think! |
cfallin
changed the title
publish-artifacts workflow as well.
|
The full check job in the merge queue failed to restore the cache but the merge queue saved the cache with the key that failed lookup. I believe this is due to #14896 landing since last night when the cache was rebuilt so... I think it's all working? |
We previously had a workflow where we published the updated cache only from CI runs in
main.yml, but because of branch scoping combined with our use of the merge queue, runs on "main" actually ran on a merge-queue temporary branch and thus did not update the cache contents in a location available for other runs. Themain-scoped content was updated only by our once-per-night full CI run onmain.This commit updates
publish-artifacts.ymlto add a job to the flow that does run onmain, after merge, on every merge: it fetches the artifact from the build we just did and publishes it to the GitHub cache. This way, cache updates from all changes that are merged will be immediately available to subsequent CI runs.