-
Notifications
You must be signed in to change notification settings - Fork 20
add a voucher_subscriber option #15
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
Conversation
this new option will allow one to use a GCP pub/sub subscription and automatically vouch INSERT messages that come in. this works best when used with a subscription that's subscribed to the default GCR pub/sub topic but will work with any pub/sub subscription that receives messages of the format: ```json { "action":"INSERT", "digest":"gcr.io/my-project/hello-world@sha256:6ec128e26cd5...", "tag":"gcr.io/my-project/hello-world:1.1" } ``` the implementation will: - re use the existing config file and flags - add support for pubsub.timeout, pubsub.project, and pubsub.subscription - push metrics every time a message is received and the total time it takes to vouch the message - some basic notion of retries; this can be extended fairly easily! - re use the logic for how voucher_server handles checks to actually perform the checks
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.
I have a few changes to request but this looks good to me so far!
```shell | ||
$ voucher_subscriber --project my-project --subscription my-subscription | ||
``` | ||
|
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.
Maybe useful to add a command on how to create a subscription, e.g.,
gcloud pubsub subscriptions create my-subscription --topic container-analysis-occurrences-v1
|
||
```json | ||
{ | ||
"action":"INSERT", |
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.
question: does this match the message format of Container Analysis topic?
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.
This matches the format used by GCR, sorry I forgot we were looking at containersnalysis pubsub!
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.
yeah sorry, I was under the impression we would be using the topic from the GCR topic! however the good thing here is that we can easily add options to listen to various types of default topics that expect different types of payloads; the end logic is the same for all of them as we want to take an image and start vouching it
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.
This looks good! I haven't reviewed the code, will do so soon, but left some high level comments on the usage for now.
subscriber/payload.go
Outdated
} | ||
|
||
if pl.Action != insertAction { | ||
return nil, errInvalidPayload |
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.
Customize this error method to specify that insert action is missing?
subscriber/payload.go
Outdated
// an image without a digest is only tagged; opt to skip this image | ||
// as the digest version has already been pushed and will be processed soon | ||
if pl.Digest == "" { | ||
return nil, errInvalidPayload |
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.
Customize this error method to specify that digest is missing?
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.
LGTM. Left some minor comments.
so it may be best to leave the refactoring to another PR since the refactor may not be as trivial as it seems; there are import loops that may happen and we'd probably want to split up that 1 edit: here's the issue |
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.
Looks good to me! !
this new option will allow one to use a GCP pub/sub subscription and automatically
vouch INSERT messages that come in. this works best when used with a subscription that's
subscribed to the default GCR pub/sub topic but will work with any pub/sub subscription that
receives messages of the format:
the implementation will:
to vouch the message
the checks
closes #9