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.

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

Update loginfo API endpoint to return information about inactive shards #738

Merged
merged 3 commits into from
Mar 21, 2022

Conversation

priyawadhwa
Copy link
Contributor

@priyawadhwa priyawadhwa commented Mar 17, 2022

I updated the LogInfo object in the openapi spec to also return information about inactive shards.

This PR also updates rekor-cli loginfo to verify inactive shards if they exist. loginfo will now first attempt to store state in state.json by TreeID instead of URL (that's because the URL for different shards is the same so this will prevent shards overwriting each other). If no TreeID is present then it will fall back to using the URL, so this change is backwards compatible.

This PR also updates the sharding integration test to run loginfo on inactive shards to ensure this works as expected.

cc @lkatalin @bobcallaway

fixes #713

Signed-off-by: Priya Wadhwa priya@chainguard.dev

Return loginfo data for inactive shards if they exist when calling `/api/v1/log`.

@priyawadhwa priyawadhwa requested a review from a team as a code owner March 17, 2022 12:45
@codecov-commenter
Copy link

Codecov Report

Merging #738 (94efbc5) into main (ebc1d7a) will decrease coverage by 0.26%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #738      +/-   ##
==========================================
- Coverage   47.48%   47.21%   -0.27%     
==========================================
  Files          66       66              
  Lines        5716     5742      +26     
==========================================
- Hits         2714     2711       -3     
- Misses       2713     2741      +28     
- Partials      289      290       +1     
Impacted Files Coverage Δ
cmd/rekor-cli/app/log_info.go 2.09% <0.00%> (-0.47%) ⬇️
pkg/types/helm/v0.0.1/entry.go 52.41% <0.00%> (-1.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebc1d7a...94efbc5. Read the comment docs.

This also updates `rekor-cli` to verify inactive shards if they exist. It also updates the sharding integration test to run loginfo and store state based on TreeID if available.

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>
Signed-off-by: Priya Wadhwa <priya@chainguard.dev>
Copy link
Member

@dlorenc dlorenc left a comment

Choose a reason for hiding this comment

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

One tiny not, otherwise LGTM!

pkg/api/tlog.go Outdated Show resolved Hide resolved
Signed-off-by: Priya Wadhwa <priya@chainguard.dev>
Copy link
Contributor

@lkatalin lkatalin left a comment

Choose a reason for hiding this comment

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

Awesome!
I had a couple of informational questions that would help me understand some of the code.

Comment on lines +182 to +185
// sign the log root ourselves to get the log root signature
if _, err := sth.Sign(viper.GetString("rekor_server.hostname"), api.signer, options.WithContext(ctx)); err != nil {
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this get the log root signature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this calls the Sign function, which stores the signature in the struct:

s.Signatures = append(s.Signatures, signature)

To access it you would call sth.Signatures. Sorry if that wasn't clear, let me know if that doesn't answer your question!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay, I think I get it, this enables us to get the signature later (not in this line of code) with sth.Signatures - the lack of clarity is just from me being less familiar here, I appreciate the explanation.

if err := state.Dump(treeID, &sth); err != nil {
log.CliLogger.Infof("Unable to store previous state: %v", err)
}
}
if err := state.Dump(serverURL, &sth); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no treeID this will just dump from the URL, but if there is both a treeID and a URL, what happens? Does the URL only cover the active tree and only in the case of an older client? Or does some state get stored twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some state will get stored twice! But when reading from the file the client will always prefer the treeID over the URL.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay cool, yeah I see that in oldState. Just trying to understand the mechanism here - the URL covers all trees, right? What does dumping by treeID (if available) add?

Copy link
Contributor Author

@priyawadhwa priyawadhwa Mar 18, 2022

Choose a reason for hiding this comment

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

So if we have two shards, they'll be accessed at the same URL (e.g. rekor.sigstore.dev). We can't store state for both of them, because the key by URL would be the same. The TreeID is unique for each shard, so we can store state for all shards at once if we use treeID as the key.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this makes sense, I see it is a hash map so we're giving it differentiated keys. I will try to get some output to see it for myself. Thanks for the explanation.

@dlorenc dlorenc merged commit caf126d into sigstore:main Mar 21, 2022
@github-actions github-actions bot added this to the v1.0.0 milestone Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need to update state.json for new shards
4 participants