Add v1 Fulcio endpoint to prober #1160
Merged
+116
−5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cmurphy
force-pushed
the
probe-legacy-endpoint
branch
from
88cef8f
to
1bef61a
Compare
There is an SLO set up for the /api/v1/signingCert Fulcio endpoint[1], but it is currently reporting "No SLO status data" because the prober was never testing that endpoint. This lead to an outage that went undetected by the monitoring system. Cosign uses the legacy certificate request endpoint in its Fulcio client[2][3]. This means that the v1 endpoint is likely the most used and therefore an important health indicator. This change adds the v1 endpoint to the prober test, which should populate Prometheus with data which should activate the SLO. [1] https://github.com/sigstore/scaffolding/blob/8f7aa097e54eabcecbc671818f9eb5f0e723e54b/terraform/gcp/modules/monitoring/fulcio/slo.tf#L79-L83 [2] https://github.com/sigstore/cosign/blob/79db196e2d97e7dfc4d8201ef829d4ce906605a7/cmd/cosign/cli/fulcio/fulcio.go#L32 [3] https://github.com/sigstore/fulcio/blob/07b19da442b418ebcf072ac65a7abb25f0e3d5c8/pkg/api/client.go#L60 Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
cmurphy
force-pushed
the
probe-legacy-endpoint
branch
from
1bef61a
to
41a7bd0
Compare
| if certBlock == nil || chainPEM == nil { | ||
| Logger.Errorf("did not find expected certificates") | ||
| } | ||
| intermediateBlock, rootPEM := pem.Decode(chainPEM) |
There was a problem hiding this comment.
Just so this can support probing custom instances with any number of intermediates, we might want to continually decode until pem.Decode() parses the root and rest is empty, or use UnmarshalCertificatesFromPEM(chainPEM). We could add a configuration flag for how many intermediates are expected?
There was a problem hiding this comment.
I wrote it this way to be consistent with the v2 test which expects exactly 3:
scaffolding/cmd/prober/write.go
Lines 121 to 125 in f775933
I think making it configurable would be a separate scope.
There was a problem hiding this comment.
Ah fair point, then this LGTM!
haydentherapper
previously approved these changes
Jul 2, 2024
cmurphy
force-pushed
the
probe-legacy-endpoint
branch
from
dac9b11
to
41a7bd0
Compare
|
Accidentally pushed a commit to the wrong branch |
haydentherapper
approved these changes
Jul 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is an SLO set up for the /api/v1/signingCert Fulcio endpoint[1], but it is currently reporting "No SLO status data" because the prober was never testing that endpoint. This lead to an outage that went undetected by the monitoring system.
Cosign uses the legacy certificate request endpoint in its Fulcio client[2][3]. This means that the v1 endpoint is likely the most used and therefore an important health indicator. This change adds the v1 endpoint to the prober test, which should populate Prometheus with data which should activate the SLO.
[1]
scaffolding/terraform/gcp/modules/monitoring/fulcio/slo.tf
Lines 79 to 83 in 8f7aa09
[2] https://github.com/sigstore/cosign/blob/79db196e2d97e7dfc4d8201ef829d4ce906605a7/cmd/cosign/cli/fulcio/fulcio.go#L32
[3] https://github.com/sigstore/fulcio/blob/07b19da442b418ebcf072ac65a7abb25f0e3d5c8/pkg/api/client.go#L60
Summary
Release Note
Documentation