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

googleca: close certificate authority client when done #930

Merged
merged 3 commits into from
Dec 19, 2022

Conversation

hectorj2f
Copy link
Contributor

Signed-off-by: Hector Fernandez hector@chainguard.dev

Summary

We realized that NewCertAuthorityService was creating a certificate authority client but the returned client was not closed when it was done being used. This is needed to clean up its underlying connections. I added a new interface function so any third party (consumer) could close the connection when they are done.

Release Note

Close certificate authority client when done.

Documentation

Signed-off-by: Hector Fernandez <hector@chainguard.dev>
cmd/app/serve.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2022

Codecov Report

Merging #930 (179acd2) into main (7016448) will decrease coverage by 0.13%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #930      +/-   ##
==========================================
- Coverage   53.83%   53.69%   -0.14%     
==========================================
  Files          37       37              
  Lines        2320     2326       +6     
==========================================
  Hits         1249     1249              
- Misses        980      986       +6     
  Partials       91       91              
Impacted Files Coverage Δ
cmd/app/serve.go 18.85% <0.00%> (-0.17%) ⬇️
pkg/ca/baseca/baseca.go 54.76% <0.00%> (-1.34%) ⬇️
pkg/ca/googleca/v1/googleca.go 37.05% <0.00%> (-0.45%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Hector Fernandez <hector@chainguard.dev>
@haydentherapper
Copy link
Contributor

haydentherapper commented Dec 19, 2022

Can we implement Close() in https://github.com/sigstore/fulcio/blob/main/pkg/ca/baseca/baseca.go, and then only reimplement it in googleca? That's the same way that TrustBundle is done, implemented once in baseca and overridden in googleca.

@hectorj2f
Copy link
Contributor Author

@haydentherapper @priyawadhwa I changed the implementation to follow the same approach we followed with TrustBundle.

Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

This is looking good, just a couple more comments!

@@ -188,19 +188,24 @@ func main() {

client, err := privateca.NewCertificateAuthorityClient(context.Background())
if err != nil {
client.Close()
log.Fatal(err)
}
parsedCerts, err := fetchCACertificate(context.Background(), *gcpCaParent, *kmsKey, *tinkKeysetPath, *tinkKmsKey, client)
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 we can probably just put the defer client.Close() call here after the error check once and then remove all the other calls to client.Close()

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC the problem is that log.Fatal ends execution immediately so the deferral doesn't happen. I think the linter complains about the lack of closure in that case. I ran into this with closing files in other tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is due to the nature of log.Fatal. It won't run the defer.

pkg/ca/fileca/fileca.go Outdated Show resolved Hide resolved
Signed-off-by: Hector Fernandez <hector@chainguard.dev>
@haydentherapper haydentherapper merged commit 67608e6 into sigstore:main Dec 19, 2022
@hectorj2f hectorj2f deleted the close_open_connection branch December 19, 2022 22:24
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.

4 participants