-
Notifications
You must be signed in to change notification settings - Fork 33
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
Use Sigstore staging for test recordings #1577
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1577 +/- ##
=======================================
Coverage 80.79% 80.79%
=======================================
Files 65 65
Lines 4738 4738
=======================================
Hits 3828 3828
Misses 910 910
Flags with carried forward coverage won't be shown. Click here to find out more. |
Seems reasonable, but I'd suggest seeking a third & fourth opinions. |
@@ -385,8 +385,8 @@ func setupTUF(ctx context.Context, vars map[string]string, environment []string) | |||
} | |||
vars["TUF"] = tufURL | |||
|
|||
vars["CERT_IDENTITY"] = "https://kubernetes.io/namespaces/default/serviceaccounts/default" | |||
vars["CERT_ISSUER"] = "https://kubernetes.default.svc.cluster.local" | |||
vars["CERT_IDENTITY"] = "[email protected]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of tying this to a person's identity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. If it is executed as a GitHub Workflow, it would tie to that workflow's identity which I believe is much nicer.
FWIW, this doesn't prevent someone else from regenerating the data that expires (TUF root). In the case where a new image is needed, for whatever reason, and someone else runs the script to do so, then it's simply a matter of updating the identity values as well.
I want the process of renewing the TUF root data dead simple since it needs to happen every 6 months. Right now, I seem to be the bottleneck for performing that process. So in a way, these changes are less tied to me 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would tying this to the enterprisecontractcommunity[at]gmail[dot]com identity be better?
It wouldn't make it easier to re-generate the image.
I don't really see a problem with using this identity here. It's an immutable public resource and no access is granted to that identity.
The alternative is to introduce a workflow that runs the script and creates a PR against this repo. The identity used would be the one from the workflow, something like: https://github.com/enterprise-contract/ec-cli/.github/workflows/update-test-image.yaml@refs/heads/update-test-image
We probably want a workflow anyways as we can make it run automatically every so often. That does require a bit more of effort though.
Suggestions:
- Merge this PR as is and have a follow up to introduce the workflow as mentioned above.
- Close this PR and re-create it later while also introducing the workflow.
My preference, of course, is to go with option 1 because today that solves the immediate problem that it's really hard to refresh the expiring data and no one but myself seems to be able to do it. I'd rather not be the bottleneck here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #1622 to do this via a workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge & let's file an issue for the workflow followup
This commit modifies the acceptance tests which rely on recordings to be based on the staging instance of Sigstore. The main advantage of this approach is that to regenerate the data, we no longer need to spin up a local instance of Sigstore which can be quite resource intensive. Also, since the stagning Sigstore instance is not an ephemeral instance, we can be selective in only re-generating the data that expires. It would be great to eventually tie this into a GitHub Workflow so a PR can be created every so often. Signed-off-by: Luiz Carvalho <[email protected]>
20c6867
to
b3ac300
Compare
], | ||
"signed": { | ||
"_type": "timestamp", | ||
"expires": "2024-05-08T13:21:06Z", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh... it looks like the expiration is about 1 week 😭
Moving this to draft until I have some time to figure out how to make the TUF data last longer than a week. |
This commit modifies the acceptance tests which rely on recordings to be based on the staging instance of Sigstore.
The main advantage of this approach is that to regenerate the data, we no longer need to spin up a local instance of Sigstore which can be quite resource intensive. Also, since the stagning Sigstore instance is not an ephemeral instance, we can be selective in only re-generating the data that expires.
It would be great to eventually tie this into a GitHub Workflow so a PR can be created every so often.