Add gRPC calls for Fulcio on prober #1062
Conversation
javanlacerda
force-pushed
the
javan.grpc-prober
branch
2 times, most recently
from
09c2612 to
d6030e3
Compare
javanlacerda
changed the title
javanlacerda
force-pushed
the
javan.grpc-prober
branch
from
d6030e3 to
544349d
Compare
cmd/prober/prometheus.go
Outdated
| Help: "API endpoint latency distribution across Rekor and Fulcio (milliseconds)", | ||
| Buckets: []float64{0.0, 200.0, 400.0, 600.0, 800.0, 1000.0}, | ||
| }, | ||
| []string{hostLabel, methodLabel}) |
There was a problem hiding this comment.
Any other metrics I could add here?
There was a problem hiding this comment.
Can we reuse the existing metrics? The method name should be enough to distinguish, or you could also prepend grpc:// to the host.
javanlacerda
force-pushed
the
javan.grpc-prober
branch
2 times, most recently
from
e7e18a8 to
1023a0f
Compare
cmd/prober/prober.go
Outdated
| @@ -201,6 +221,15 @@ func runProbers(ctx context.Context, freq int, runOnce bool) { | |||
| Logger.Errorf("error running request %s: %v", r.Endpoint, err) | |||
| } | |||
| } | |||
|
|
|||
| // Performing requests against Fulcio gRPC API | |||
| for _, method := range fulciopb.CA_ServiceDesc.Methods { | |||
There was a problem hiding this comment.
Rather than iterate over all possible methods, can we just pick one representative endpoint? GetTrustBundle?
There was a problem hiding this comment.
Sure!! I tried to replicate the behavior for http requests, making requests for all available endpoints.
cmd/prober/prober.go
Outdated
| @@ -185,7 +194,18 @@ func main() { | |||
| Logger.Fatal(http.ListenAndServe(addr, nil)) | |||
| } | |||
|
|
|||
| func runProbers(ctx context.Context, freq int, runOnce bool) { | |||
| func NewFulcioClient() (fulciopb.CAClient, error) { | |||
There was a problem hiding this comment.
nit, I would mention "GRPC" in the name since this is only creating a gRPC client. Also you can make this method lowercase so it's not exported.
cmd/prober/prober.go
Outdated
| go runProbers(ctx, frequency, oneTime) | ||
|
|
||
| if fulcioClient, err := NewFulcioClient(); err != nil { | ||
| Logger.Errorf("error creating fulcio grpc client %v", err) |
There was a problem hiding this comment.
Should this be Fatalf since if the client isn't properly initialized, it will fail again later?
There was a problem hiding this comment.
It will not fail again. I agree that should be a Fatalf instead Error
cmd/prober/prometheus.go
Outdated
| Help: "API endpoint latency distribution across Rekor and Fulcio (milliseconds)", | ||
| Buckets: []float64{0.0, 200.0, 400.0, 600.0, 800.0, 1000.0}, | ||
| }, | ||
| []string{hostLabel, methodLabel}) |
There was a problem hiding this comment.
Can we reuse the existing metrics? The method name should be enough to distinguish, or you could also prepend grpc:// to the host.
Signed-off-by: Javan lacerda <[email protected]>
Signed-off-by: Javan lacerda <[email protected]>
javanlacerda
force-pushed
the
javan.grpc-prober
branch
from
1023a0f to
e2f3a9c
Compare
Signed-off-by: Javan lacerda <[email protected]>
cmd/prober/prober.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| exportGrpcDataToPrometheus(0, "grpc://"+fulcioGrpcURL, "GetTrustBundle", "GET", latency) |
There was a problem hiding this comment.
gRPC is different than HTTP in that response status codes are a part of error when it returns a 4xx/5xx error. Can we log the code using something like https://github.com/sigstore/fulcio/blob/main/pkg/server/grpc_server_test.go#L126-L128 instead of 0?
You'll need to also not return early when there is an error.
Signed-off-by: Javan lacerda <[email protected]>
Closes #700
Summary
It implements a client for make requests via gRPC for prober against Fulcio. Only for reading methods were implemented.
This PR doesn't update any already existing feature, just add gRPC calls to the set of calls.
Release Note
Documentation