Change monitoring workflow configuration argument to optional #543

linus-sun · 2024-11-18T19:14:36Z

Summary

This PR changes the config argument in the GitHub Actions reusable monitoring workflow to no longer be required, and sets the identity monitor portion of the workflow to only run if a config containing a desired set of identities to monitor has been passed in.

Release Note

Workflow config argument has been changed to optional

Documentation

N/A

codecov · 2024-11-18T19:16:05Z

Codecov Report

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

Project coverage is 62.90%. Comparing base (d271ec7) to head (3769ea8).
Report is 165 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/rekor_monitor/main.go	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #543      +/-   ##
==========================================
- Coverage   64.02%   62.90%   -1.13%     
==========================================
  Files           4       17      +13     
  Lines         303     1267     +964     
==========================================
+ Hits          194      797     +603     
- Misses         78      399     +321     
- Partials       31       71      +40

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

haydentherapper · 2024-11-18T19:18:41Z

cmd/rekor_monitor/main.go

@@ -76,12 +69,16 @@ func main() {
 		if err := yaml.Unmarshal([]byte(configString), &config); err != nil {
 			log.Fatalf("error parsing identities: %v", err)
 		}
+
+		configProvided = true


Do we need this? An uninitalized config should be sufficient for the checks below like config.EndIndex == nil

I believe that in the event that an uninitialized config is currently passed in, the workflow will still attempt to set StartIndex and EndIndex using the latest checkpoint and log checkpoint file and then attempt to run the identity monitor using the uninitialized config's values resulting in an error- if we would prefer to have checking for the config's indices stop the identity monitor from running I can refactor that in another PR after this one

https://github.com/sigstore/rekor-monitor/blob/114befbd5a9ab2639b80d592f7c777791495c192/cmd/rekor_monitor/main.go#L148C3-L175C4 As of right now I believe this check is only looking for nil values to set the start and end indices, not checking if the identity monitor is runnable- an uninitialized config will have the start and end indices set and then the identity monitor will run with nil arguments

an uninitialized config will have the start and end indices set and then the identity monitor will run with nil arguments

I thought the defaults for start and end were nil. So if the config is not initialized, then the default values of nil will be read in the code you linked, and then the monitor will initialize them with defaults.

Yes - I guess what I'm thinking is that in the event that a config is not initialized, there are also no identities provided to monitor; is the identity monitor portion of the workflow then still necessary to run?

Ah I see your question. What about if we gated calling IdentitySearch on if MonitoredValues is set? That was like the original implementation, where it would only run if a user configured a value to monitor for.

cmd/rekor_monitor/main.go

haydentherapper · 2024-11-18T19:21:57Z

cmd/rekor_monitor/main.go

 	}

 	if *configYamlInput != "" {
 		if err := yaml.Unmarshal([]byte(*configYamlInput), &config); err != nil {
 			log.Fatalf("error parsing identities: %v", err)
 		}
+
+		configProvided = true
 	}

 	if config.OutputIdentitiesFile == "" {


Below for VerifyConsistencyCheckInputs, given we provide defaults, do we need this check anymore? Just need to update config.LogINfoFile based on the default if it's not provided.

updating logInfoFile and removing this part of the check

haydentherapper · 2024-11-18T19:58:40Z

cmd/rekor_monitor/main.go

@@ -88,7 +81,7 @@ func main() {
 		config.OutputIdentitiesFile = outputIdentitiesFileName
 	}

-	err := rekor.VerifyConsistencyCheckInputs(interval, &config.LogInfoFile, once)
+	err := rekor.VerifyConsistencyCheckInputs(interval, logInfoFile, once)


Can we delete this check now, since all these variables have default values so this check will always pass?

deleting this from main.go and removing the function in a follow-up PR

haydentherapper · 2024-11-18T20:06:44Z

cmd/rekor_monitor/main.go

@@ -163,7 +162,6 @@ func main() {
 		}


Can we remove the default of 0? Scrapping the entire log is hard to do without careful rate limiting on the client side. If no start index is provided and no checkpoint is provided, then just throw an error.

haydentherapper · 2024-11-18T20:07:39Z

cmd/rekor_monitor/main.go

@@ -145,10 +138,16 @@ func main() {
 			}
 		}

+		err = rekor.RunConsistencyCheck(rekorClient, verifier, *logInfoFile)


For a follow up - I noticed RunConsistencyCheck calls GetLogInfo, so we could reduce duplication by having RunConsistencyCheck return the log info struct.

will work on this in a follow-up PR now

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

haydentherapper

Thanks!

linus-sun marked this pull request as ready for review November 18, 2024 19:17

linus-sun requested a review from haydentherapper November 18, 2024 19:17

haydentherapper reviewed Nov 18, 2024

View reviewed changes

cmd/rekor_monitor/main.go Show resolved Hide resolved

haydentherapper reviewed Nov 18, 2024

View reviewed changes

linus-sun force-pushed the linussun/fix-config-required branch from 938437a to 50330a1 Compare November 18, 2024 19:34

linus-sun requested a review from haydentherapper November 18, 2024 19:40

linus-sun force-pushed the linussun/fix-config-required branch from 50330a1 to 6c2def6 Compare November 18, 2024 19:47

haydentherapper reviewed Nov 18, 2024

View reviewed changes

change config to not required

3769ea8

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

linus-sun force-pushed the linussun/fix-config-required branch from 6c2def6 to 3769ea8 Compare November 18, 2024 20:11

haydentherapper approved these changes Nov 18, 2024

View reviewed changes

haydentherapper merged commit e4644fe into sigstore:main Nov 18, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change monitoring workflow configuration argument to optional #543

Change monitoring workflow configuration argument to optional #543

linus-sun commented Nov 18, 2024

codecov bot commented Nov 18, 2024 •

edited

Loading

haydentherapper Nov 18, 2024

linus-sun Nov 18, 2024

linus-sun Nov 18, 2024

haydentherapper Nov 18, 2024

linus-sun Nov 18, 2024

haydentherapper Nov 18, 2024

haydentherapper Nov 18, 2024

linus-sun Nov 18, 2024

haydentherapper Nov 18, 2024

linus-sun Nov 18, 2024

haydentherapper Nov 18, 2024

haydentherapper Nov 18, 2024

linus-sun Nov 18, 2024

haydentherapper left a comment

Change monitoring workflow configuration argument to optional #543

Change monitoring workflow configuration argument to optional #543

Conversation

linus-sun commented Nov 18, 2024

Summary

Release Note

Documentation

codecov bot commented Nov 18, 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

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

codecov bot commented Nov 18, 2024 •

edited

Loading