Refactor grpc-server package #598

micahhausler · 2022-03-17T22:36:10Z

Description

Untangle certificate acquisition
Make gRPC server accept different registrable APIs
Move postgres setup logic out of tink-server's main.go to an internal package
Move gRPC server implementation to the new /server package
Remove unused global workflowData

Why is this needed

This will facilitate swapping out the current gRPC API implementation with an alternative in a successive PR.

Fixes: #

How Has This Been Tested?

I will have a secondary PR adding end-to-end tests

How are existing users impacted? What migration steps/scripts do we need?

Anyone importing or instantiating the grpcserver.SetupGRPC with grpserver.ConfigGRPCServer will need to use the new signature and pass in a grpcserver.Registrar such as server.DBServer.

Previously this looked like:

grpcConfig := &grpcServer.ConfigGRPCServer{
        Facility:      config.Facility,
        TLSCert:       "insecure",
        GRPCAuthority: listenAddr,
        DB:            tinkDB,
}
if config.TLS {
        grpcConfig.TLSCert = config.TLSCert
}
cert, modT := grpcServer.SetupGRPC(ctx, logger, grpcConfig, errCh)

And now, this looks like:

cert, certPEM, certModTime, err := grpcserver.GetCerts(certsDir)
grpcOpts := []grpc.ServerOption{grpc.Creds(credentials.NewServerTLSFromCert(cert))}
tinkAPI, err := server.NewDBServer(
        logger,
        tinkDB,
        server.WithCerts(*certModTime, certPEM),
)
_, err := grpcserver.SetupGRPC(
        ctx,
        tinkAPI,
        listenAddr,
        grpcOpts,
        errCh)

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

codecov · 2022-03-17T22:41:10Z

Codecov Report

Merging #598 (07886bc) into main (dbe434f) will increase coverage by 0.28%.
The diff coverage is 57.94%.

❗ Current head 07886bc differs from pull request most recent head dbf5c4b. Consider uploading reports for the commit dbf5c4b to get more accurate results

@@            Coverage Diff             @@
##             main     #598      +/-   ##
==========================================
+ Coverage   45.68%   45.96%   +0.28%     
==========================================
  Files          55       56       +1     
  Lines        3301     3283      -18     
==========================================
+ Hits         1508     1509       +1     
+ Misses       1707     1684      -23     
- Partials       86       90       +4

Impacted Files	Coverage Δ
server/dbserver.go	`0.00% <0.00%> (ø)`
server/dbserver_hardware.go	`2.40% <0.00%> (ø)`
server/dbserver_template.go	`37.27% <40.00%> (ø)`
server/dbserver_workflow.go	`41.74% <50.00%> (ø)`
grpc-server/grpc_server.go	`75.60% <73.52%> (+39.12%)`	⬆️
server/dbserver_worker_workflow.go	`91.24% <96.96%> (ø)`

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 dbe434f...dbf5c4b. Read the comment docs.

sonatype-lift · 2022-03-17T22:57:12Z

grpc-server/grpc_server.go

+// The public key is expected to be named "bundle.pem" and the private key
+// "server.pem".
+func GetCerts(certsDir string) (*tls.Certificate, []byte, *time.Time, error) {
+	certFile, err := os.Open(filepath.Join(certsDir, "bundle.pem"))


G304: Potential file inclusion via variable
(at-me in a reply with help or ignore)

grpc-server/worker_workflow.go

displague

This looks good to me.

I left some comments that largely focused on implementing TinkServer as server/Server, server/NewServer (with options helper moving that way) since the grpc-server/ parentage seems unnecessary.

grpc-server/tinkerbell_server.go

grpc-server/grpc_server_test.go

grpc-server/grpc_server.go

chrisdoherty4 · 2022-03-24T16:59:08Z

cmd/tink-server/main.go

+			addr, err := grpcserver.SetupGRPC(
+				ctx,
+				tinkAPI,
+				config.GRPCAuthority,
+				grpcOpts,
+				errCh)
+			if err != nil {
+				return err
 			}
-			cert, modT := grpcServer.SetupGRPC(ctx, logger, grpcConfig, errCh)
+			logger.With("address", addr).Info("started listener")

-			httpConfig := &httpServer.Config{
+			httpConfig := &httpserver.Config{
 				HTTPAuthority: config.HTTPAuthority,
-				CertPEM:       cert,
-				ModTime:       modT,
+				CertPEM:       certPEM,
+				ModTime:       *certModTime,
 			}
-			httpServer.SetupHTTP(ctx, logger, httpConfig, errCh)
+			httpserver.SetupHTTP(ctx, logger, httpConfig, errCh)



This could really benefit from using https://github.com/oklog/run. Unnecessary for this PR, but if you get time separately it would help clean up the subsequent error channel code that does static draining of the channel.

* Untangle certificate acquisition * Make gRPC server accept different registrable APIs * Move postgres setup logic out of tink-server's main.go to an internal package * Move gRPC server implementation to the new /server package * Remove unused global workflowData Signed-off-by: Micah Hausler <[email protected]>

micahhausler requested review from nshalman and mmlb March 17, 2022 22:36

micahhausler force-pushed the refactor/grpcserver branch from 6945553 to e3588d5 Compare March 17, 2022 22:41

sonatype-lift bot reviewed Mar 17, 2022

View reviewed changes

micahhausler force-pushed the refactor/grpcserver branch 3 times, most recently from 669634c to dc23bb2 Compare March 18, 2022 16:15

micahhausler commented Mar 18, 2022

View reviewed changes

grpc-server/worker_workflow.go Outdated Show resolved Hide resolved

displague reviewed Mar 22, 2022

View reviewed changes

micahhausler force-pushed the refactor/grpcserver branch 3 times, most recently from c7afe78 to ae45cce Compare March 23, 2022 16:31

chrisdoherty4 reviewed Mar 24, 2022

View reviewed changes

micahhausler force-pushed the refactor/grpcserver branch 3 times, most recently from 7f702d0 to 4a9e988 Compare March 24, 2022 21:49

displague previously approved these changes Mar 28, 2022

View reviewed changes

thebsdbox previously approved these changes Mar 30, 2022

View reviewed changes

micahhausler dismissed stale reviews from thebsdbox and displague via dbf5c4b April 4, 2022 22:44

micahhausler force-pushed the refactor/grpcserver branch from 4a9e988 to dbf5c4b Compare April 4, 2022 22:44

displague approved these changes Apr 5, 2022

View reviewed changes

displague merged commit d3512cb into tinkerbell:main Apr 5, 2022

displague added the breaking-change Denotes a PR that introduces potentially breaking changes that require user action. label Apr 5, 2022

displague mentioned this pull request Apr 5, 2022

Stop serving and fetching the tls cert used for gRPC #584

Merged

displague added this to the 0.7.0 milestone Aug 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor grpc-server package #598

Refactor grpc-server package #598

micahhausler commented Mar 17, 2022 •

edited

Loading

codecov bot commented Mar 17, 2022 •

edited

Loading

sonatype-lift bot Mar 17, 2022

displague left a comment

chrisdoherty4 Mar 24, 2022 •

edited

Loading

Refactor grpc-server package #598

Refactor grpc-server package #598

Conversation

micahhausler commented Mar 17, 2022 • edited Loading

Description

Why is this needed

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

codecov bot commented Mar 17, 2022 • edited Loading

Codecov Report

sonatype-lift bot Mar 17, 2022

Choose a reason for hiding this comment

displague left a comment

Choose a reason for hiding this comment

chrisdoherty4 Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

micahhausler commented Mar 17, 2022 •

edited

Loading

codecov bot commented Mar 17, 2022 •

edited

Loading

chrisdoherty4 Mar 24, 2022 •

edited

Loading