Split CAA checking out to its own service

[rolandshoemaker]

Which uses gRPC to communicate. Issuer domain, DNS resolver to talk to, DNS timeout, and address to serve the service are all configurable via a YAML config file (although since there are so few maybe just CLI flags could work).

Also a small test client to call the gRPC server. (Still need to copy the tests from the VA that check the CAA loop stuff etc)

[letsencryptbot]

cmd/caa-checker/proto/caa_checker.pb.go:15:1: don't use an underscore in package name cmd/caa-checker/proto/caa_checker.pb.go:35:6: exported type Domain should have comment or be unexported cmd/caa-checker/proto/caa_checker.pb.go:39:1: exported method Domain.Reset should have comment or be unexported cmd/caa-checker/proto/caa_checker.pb.go:41:1: exported method Domain.ProtoMessage should have comment or be unexported cmd/caa-checker/proto/caa_checker.pb.go:42:1: exported method Domain.Descriptor should have comment or be unexported cmd/caa-checker/proto/caa_checker.pb.go:44:6: exported type Valid should have comment or be unexported cmd/caa-checker/proto/caa_checker.pb.go:48:1: exported method Valid.Reset should have comment or be unexported cmd/caa-checker/proto/caa_checker.pb.go:50:1: exported method Valid.ProtoMessage should have comment or be unexported cmd/caa-checker/proto/caa_checker.pb.go:51:1: exported method Valid.Descriptor should have comment or be unexported cmd/caa-checker/proto/caa_checker.pb.go:68:6: exported type CAACheckerClient should have comment or be unexported cmd/caa-checker/proto/caa_checker.pb.go:76:1: exported function NewCAACheckerClient should have comment or be unexported cmd/caa-checker/proto/caa_checker.pb.go:91:6: exported type CAACheckerServer should have comment or be unexported cmd/caa-checker/proto/caa_checker.pb.go:95:1: exported function RegisterCAACheckerServer should have comment or be unexported cmd/caa-checker/proto/caa_checker.pb.go:99:6: don't use underscores in Go names; func _CAAChecker_ValidForIssuance_Handler should be _CAACheckerValidForIssuanceHandler cmd/caa-checker/proto/caa_checker.pb.go:111:5: don't use underscores in Go names; var _CAAChecker_serviceDesc should be _CAACheckerServiceDesc

[letsencryptbot]

godep: Package (google.golang.org/grpc) not found

[riking]

You didn't rewrite the imports in the protobuf file.

I think we should have a separate PR that just adds a single protobuf file and the godeps imports... #1651

[riking]

https://github.com/letsencrypt/boulder/blob/add-grpc/Makefile#L62-L66

[rolandshoemaker]

?

import proto "github.com/letsencrypt/boulder/Godeps/_workspace/src/github.com/golang/protobuf/proto"
import fmt "fmt"
import math "math"

import (
    context "github.com/letsencrypt/boulder/Godeps/_workspace/src/golang.org/x/net/context"
    grpc "github.com/letsencrypt/boulder/Godeps/_workspace/src/google.golang.org/grpc"
)

Also the end goal with splitting out this as it's own service is to create a new repo under the letsencrypt organization so other CAs can utilize our CAA checking logic, in that case it doesn't make sense to merge this protobuf definition with those for each Boulder service and have to split them back out again later.

[jsha]

First pass comments:

Instead of commenting out the lint test in test.sh, remove it.

Issuer domain should be an RPC parameter rather than a parameter configured into the CAA service at startup.

cAACheckerService is the wrong capitalization for Go, should be caaCheckerService.

Create a subdirectory, test/grpc-credentials for the certs and keys, since there will be many of them.

The stuff in CAAConfig in config.go should be an RPCConfig struct.

[jsha]

The stuff in CAAConfig in config.go should be an RPCConfig struct.

Also, the section of VA's main that initializes a credentials object based on that struct should be factored out somewhere common, probably a new grpc directory.

[rolandshoemaker]

Ready for re-review.

[riking]

message Result ?

[riking]

I see no unit tests with the grpc method?

[rolandshoemaker]

Ready for re-review.

[jsha]

Please add a .go file to cmd/caa-checker/proto that just contains a go generate command to generate the .go file from the .proto file.

Otherwise this looks great.

[riking]

didn't find any problems in the new changes, need to do another git diff check though

[rolandshoemaker]

Ready for re-review.

After offline conversation I've added a cmd.DebugServer to caa-checker so that it exposes it's memory stats and gRPC trace data as a best effort for metrics since there is no merged support for instrumentation hooks, grpc/grpc-go#240, and the various other options, e.g. code generation or Boulder-level metrics collection, are not really satisfactory.

[jsha]

The go:generate line doesn't need to be wrapped in sh -c. Otherwise LGTM.

[rolandshoemaker]

Fixed.

[jsha]

LGTM

[riking]

diff --git a/bdns/problem.go b/bdns/problem.go
index 5558340..1a3805a 100644
--- a/bdns/problem.go
+++ b/bdns/problem.go
@@ -14,7 +14,7 @@ import (
        "github.com/letsencrypt/boulder/probs"
 )

-type dnsError struct {
+type DNSError struct {
        recordType uint16

Missing comment on exported type.

[riking]

b/cmd/caa-checker/server_test.go

I think the tests should declare a var ctx = context.Background() and use that.

[riking]

The integration test starts the caa-checker server, but it doesn't run the test-client.

The test client should probably be in the test/ directory

[rolandshoemaker]

The integration test starts the caa-checker server, but it doesn't run the test-client.
The test client should probably be in the test/ directory

In the integration tests the VA is essentially the test client as it is using the -next config file which defines the gRPC configuration instead of AMQP for the Publisher.

test-client mainly just exists as an example of the API for when we split this out into its own package.

[riking]

grpc/bcodes.go

Error code 16 is already taken. Perhaps we should start at 1000?

CodeToProblem should probably define values for most of the predefined codes, like Internal and DataLoss.

[jsha]

@rolandshoemaker can you resolve the merge conflicts?

[rolandshoemaker]

Resolved.