Refactor consistency check workflow #513

linus-sun · 2024-10-29T20:10:49Z

Summary

This PR does the following:

Renames the verifier task to rekor_consistency to more accurately reflect its functionality
refactors RunConsistencyCheck to take the looping functionality out of the check itself and moves it into the workflow
refactors RunConsistencyCheck to move its identity monitor functionality into a separate function, writeIdentitiesBetweenCheckpoints

This PR has been tested remotely via a GitHub reusable workflow: https://github.com/linus-sun/rekor-log-monitor/actions/runs/11582060548/job/32244210000

Release Note

Workflow configuration inputs changed to take in either a yaml as a string or local file path

Documentation

Readme has been updated to reflect new configuration input

codecov · 2024-10-29T20:31:52Z

Codecov Report

Attention: Patch coverage is 0% with 45 lines in your changes missing coverage. Please review.

Project coverage is 62.12%. Comparing base (d271ec7) to head (cff5a77).
Report is 153 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/rekor/verifier.go	0.00%	27 Missing ⚠️
cmd/rekor_consistency/main.go	0.00%	18 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #513      +/-   ##
==========================================
- Coverage   64.02%   62.12%   -1.91%     
==========================================
  Files           4       18      +14     
  Lines         303     1299     +996     
==========================================
+ Hits          194      807     +613     
- Misses         78      413     +335     
- Partials       31       79      +48

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

haydentherapper · 2024-10-29T20:54:28Z

cmd/consistency/main.go

+	// 3. Write latest checkpoint to file
+
+	// To get an immediate first tick
+	for ; ; <-ticker.C {


Did you want to include a loop here? RunConsistencyCheck includes a loop.

I would suggest renaming the file with git mv and then making changes, which I believe will properly show the diff in github.

Removing loop from within RunConsistencyCheck tried renaming via git mv, but diff doesn't still seem to be tracked- might be because I already renamed without using git mv

cmd/consistency/main.go

haydentherapper · 2024-10-30T19:36:08Z

pkg/rekor/verifier.go

-			return fmt.Errorf("failed to get log info: %v", err)
-		}
-		checkpoint, err := verifyLatestCheckpointSignature(logInfo, verifier)
+func RunConsistencyCheck(rekorClient *client.Rekor, verifier signature.Verifier, logInfoFile string, mvs identity.MonitoredValues, outputIdentitiesFile string, once bool) error {


Can we remove once as a parameter?

haydentherapper · 2024-10-30T21:06:03Z

cmd/rekor_consistency/config.go

+	"github.com/sigstore/rekor-monitor/pkg/identity"
+)
+
+type ConsistencyCheckConfiguration struct {


What do you think about omitting this and sticking to CLI flags? I think that yaml is great for complex input or for server configuration, but since we expect this to be run as a tool, these can be flags.

Given we also have other flags already, I don't think we need a configuration too.

If we remove this, then we can have a monitored-values flag as a string and a monitored-value-file flag as a path to a file.

Do we want to address the last part of this:

If we remove this, then we can have a monitored-values flag as a string and a monitored-value-file flag as a path to a file.

Adding a flag for passing by file as well?

haydentherapper · 2024-10-30T21:17:28Z

cmd/rekor_consistency/main.go

+	configFilePath := flag.String("config-file", "", "Name of the file containing the consistency check workflow configuration settings")
+	configString := flag.String("config-string", "", "Consistency check workflow configuration settings input as a string")
+	once := flag.Bool("once", false, "Perform consistency check once and exit")
+	logInfoFile := flag.String("file", "", "path to log info file")


You can specify default values instead of an empty string, and if --file is set, it'll override the value.

haydentherapper · 2024-10-30T21:19:35Z

.github/workflows/reusable_monitoring.yml

@@ -76,7 +76,7 @@ jobs:
        run: cat ${{ env.LOG_FILE }}
        # Skip on first run
        continue-on-error: true
-      - run: go run ./cmd/verifier --file ${{ env.LOG_FILE }} --once --monitored-values "${{ inputs.identities }}" --user-agent "${{ format('{0}/{1}/{2}', needs.detect-workflow.outputs.repository, needs.detect-workflow.outputs.ref, github.run_id) }}"
+      - run: go run ./cmd/consistency --file ${{ env.LOG_FILE }} --once --config-string "${{ inputs.config }}" --user-agent "${{ format('{0}/{1}/{2}', needs.detect-workflow.outputs.repository, needs.detect-workflow.outputs.ref, github.run_id) }}"


rekor_consistency?

haydentherapper

Just two small comments, looks good otherwise!

haydentherapper · 2024-10-31T22:40:40Z

cmd/rekor_consistency/main.go

 	"runtime"
 	"strings"
 	"time"

 	"github.com/sigstore/rekor-monitor/pkg/identity"
 	"github.com/sigstore/rekor-monitor/pkg/rekor"
 	"github.com/sigstore/rekor/pkg/client"
-	"gopkg.in/yaml.v3"


Is this necessary? v2 is a few years older than v3.

haydentherapper · 2024-10-31T22:42:00Z

cmd/rekor_consistency/main.go

-	if err := yaml.Unmarshal([]byte(*monitoredValsInput), &monitoredVals); err != nil {
-		log.Fatalf("error parsing identities: %v", err)
-	}
-	for _, certID := range monitoredVals.CertificateIdentities {


Can we keep the output? I think it's helpful to verify that you're monitoring the correct values.

go.mod

cmd/rekor_consistency/main.go

haydentherapper · 2024-11-04T18:21:50Z

LGTM, can you rebase and run go mod tidy to resolve the conflict? Can you also update the PR description to reflect what's in this PR?

Signed-off-by: linus-sun <[email protected]>

Signed-off-by: Linus Sun <[email protected]>

linus-sun force-pushed the linussun/refactor-verifier branch 3 times, most recently from 8390178 to cfce3f4 Compare October 29, 2024 20:29

linus-sun force-pushed the linussun/refactor-verifier branch 2 times, most recently from b9e07db to 8599c52 Compare October 29, 2024 20:36

linus-sun marked this pull request as ready for review October 29, 2024 20:47

linus-sun requested review from haydentherapper and mihaimaruseac October 29, 2024 20:47

haydentherapper reviewed Oct 29, 2024

View reviewed changes

linus-sun force-pushed the linussun/refactor-verifier branch 6 times, most recently from de2807b to 9491b69 Compare October 30, 2024 17:27

linus-sun requested a review from haydentherapper October 30, 2024 17:33

haydentherapper reviewed Oct 30, 2024

View reviewed changes

linus-sun force-pushed the linussun/refactor-verifier branch 6 times, most recently from 3662a5b to c554e93 Compare October 31, 2024 19:32

haydentherapper reviewed Oct 31, 2024

View reviewed changes

linus-sun force-pushed the linussun/refactor-verifier branch from c554e93 to 15d9ce3 Compare November 1, 2024 17:25

haydentherapper reviewed Nov 1, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

cmd/rekor_consistency/main.go Outdated Show resolved Hide resolved

linus-sun force-pushed the linussun/refactor-verifier branch 3 times, most recently from 5305cb3 to 350346d Compare November 4, 2024 17:30

haydentherapper previously approved these changes Nov 4, 2024

View reviewed changes

refactor verifier workflow

642da2a

Signed-off-by: linus-sun <[email protected]>

linus-sun dismissed haydentherapper’s stale review via 642da2a November 4, 2024 18:55

linus-sun force-pushed the linussun/refactor-verifier branch from 350346d to 642da2a Compare November 4, 2024 18:55

Merge branch 'main' into linussun/refactor-verifier

cff5a77

Signed-off-by: Linus Sun <[email protected]>

haydentherapper approved these changes Nov 4, 2024

View reviewed changes

haydentherapper merged commit a0aed3d into sigstore:main Nov 4, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor consistency check workflow #513

Refactor consistency check workflow #513

linus-sun commented Oct 29, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading

haydentherapper Oct 29, 2024

linus-sun Oct 30, 2024

haydentherapper Oct 30, 2024

haydentherapper Oct 30, 2024

haydentherapper Oct 31, 2024

haydentherapper Oct 30, 2024

haydentherapper Oct 30, 2024

haydentherapper left a comment

haydentherapper Oct 31, 2024

haydentherapper Oct 31, 2024

haydentherapper commented Nov 4, 2024

Refactor consistency check workflow #513

Refactor consistency check workflow #513

Conversation

linus-sun commented Oct 29, 2024 • edited Loading

Summary

Release Note

Documentation

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper commented Nov 4, 2024

linus-sun commented Oct 29, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading