Better compile-time type checking for gRPC server implementations (#7504

)

Replaced our embeds of foopb.UnimplementedFooServer with
foopb.UnsafeFooServer. Per the grpc-go docs this reduces the "forwards
compatibility" of our implementations, but that is only a concern for
codebases that are implementing gRPC interfaces maintained by third
parties, and which want to be able to update those third-party
dependencies without updating their own implementations in lockstep.
Because we update our protos and our implementations simultaneously, we
can remove this safety net to replace runtime type checking with
compile-time type checking.

However, that replacement is not enough, because we never pass our
implementation objects to a function which asserts that they match a
specific interface. So this PR also replaces our reflect-based unittests
with idiomatic interface assertions. I do not view this as a perfect
solution, as it relies on people implementing new gRPC servers to add
this line, but it is no worse than the status quo which relied on people
adding the "TestImplementation" test.

Fixes #7497

ca/ca.go

@@ -80,7 +80,7 @@ type certProfilesMaps struct {
 // certificateAuthorityImpl represents a CA that signs certificates.
 // It can sign OCSP responses as well, but only via delegation to an ocspImpl.
 type certificateAuthorityImpl struct {
-	capb.UnimplementedCertificateAuthorityServer
+	capb.UnsafeCertificateAuthorityServer
 	sa           sapb.StorageAuthorityCertificateClient
 	pa           core.PolicyAuthority
 	issuers      issuerMaps
@@ -101,6 +101,8 @@ type certificateAuthorityImpl struct {
 	lintErrorCount prometheus.Counter
 }
 
+var _ capb.CertificateAuthorityServer = (*certificateAuthorityImpl)(nil)
+
 // makeIssuerMaps processes a list of issuers into a set of maps for easy
 // lookup either by key algorithm (useful for picking an issuer for a precert)
 // or by unique ID (useful for final certs, OCSP, and CRLs). If two issuers with

ca/ca_test.go

@@ -41,11 +41,6 @@ import (
 	"github.com/letsencrypt/boulder/test"
 )
 
-func TestImplementation(t *testing.T) {
-	t.Parallel()
-	test.AssertImplementsGRPCServer(t, &certificateAuthorityImpl{}, capb.UnimplementedCertificateAuthorityServer{})
-}
-
 var (
 	// * Random public key
 	// * CN = not-example.com

ca/crl.go

@@ -19,13 +19,15 @@ import (
 )
 
 type crlImpl struct {
-	capb.UnimplementedCRLGeneratorServer
+	capb.UnsafeCRLGeneratorServer
 	issuers   map[issuance.NameID]*issuance.Issuer
 	profile   *issuance.CRLProfile
 	maxLogLen int
 	log       blog.Logger
 }
 
+var _ capb.CRLGeneratorServer = (*crlImpl)(nil)
+
 // NewCRLImpl returns a new object which fulfils the ca.proto CRLGenerator
 // interface. It uses the list of issuers to determine what issuers it can
 // issue CRLs from. lifetime sets the validity period (inclusive) of the

ca/crl_test.go

@@ -14,11 +14,6 @@ import (
 	"github.com/letsencrypt/boulder/test"
 )
 
-func TestImplementationCRL(t *testing.T) {
-	t.Parallel()
-	test.AssertImplementsGRPCServer(t, &crlImpl{}, capb.UnimplementedCRLGeneratorServer{})
-}
-
 type mockGenerateCRLBidiStream struct {
 	grpc.ServerStream
 	input  <-chan *capb.GenerateCRLRequest

ca/ocsp.go

@@ -22,7 +22,7 @@ import (
 
 // ocspImpl provides a backing implementation for the OCSP gRPC service.
 type ocspImpl struct {
-	capb.UnimplementedOCSPGeneratorServer
+	capb.UnsafeOCSPGeneratorServer
 	issuers        map[issuance.NameID]*issuance.Issuer
 	ocspLifetime   time.Duration
 	ocspLogQueue   *ocspLogQueue
@@ -32,6 +32,8 @@ type ocspImpl struct {
 	clk            clock.Clock
 }
 
+var _ capb.OCSPGeneratorServer = (*ocspImpl)(nil)
+
 func NewOCSPImpl(
 	issuers []*issuance.Issuer,
 	ocspLifetime time.Duration,

ca/ocsp_test.go

@@ -17,11 +17,6 @@ import (
 	"github.com/letsencrypt/boulder/test"
 )
 
-func TestImplementationOCSP(t *testing.T) {
-	t.Parallel()
-	test.AssertImplementsGRPCServer(t, &ocspImpl{}, capb.UnimplementedOCSPGeneratorServer{})
-}
-
 func serial(t *testing.T) []byte {
 	serial, err := hex.DecodeString("aabbccddeeffaabbccddeeff000102030405")
 	if err != nil {

cmd/akamai-purger/main.go

@@ -186,7 +186,7 @@ type cachePurgeClient interface {
 // to Akamai's Fast Purge API at regular intervals.
 type akamaiPurger struct {
 	sync.Mutex
-	akamaipb.UnimplementedAkamaiPurgerServer
+	akamaipb.UnsafeAkamaiPurgerServer
 
 	// toPurge functions as a stack where each entry contains the three OCSP
 	// response URLs associated with a given certificate.
@@ -197,6 +197,8 @@ type akamaiPurger struct {
 	log             blog.Logger
 }
 
+var _ akamaipb.AkamaiPurgerServer = (*akamaiPurger)(nil)
+
 func (ap *akamaiPurger) len() int {
 	ap.Lock()
 	defer ap.Unlock()

cmd/akamai-purger/main_test.go

@@ -13,10 +13,6 @@ import (
 	"github.com/letsencrypt/boulder/test"
 )
 
-func TestImplementation(t *testing.T) {
-	test.AssertImplementsGRPCServer(t, &akamaiPurger{}, akamaipb.UnimplementedAkamaiPurgerServer{})
-}
-
 func TestThroughput_optimizeAndValidate(t *testing.T) {
 	dur := func(in time.Duration) config.Duration { return config.Duration{Duration: in} }
 

crl/storer/storer.go

@@ -36,7 +36,7 @@ type simpleS3 interface {
 }
 
 type crlStorer struct {
-	cspb.UnimplementedCRLStorerServer
+	cspb.UnsafeCRLStorerServer
 	s3Client         simpleS3
 	s3Bucket         string
 	issuers          map[issuance.NameID]*issuance.Certificate
@@ -47,6 +47,8 @@ type crlStorer struct {
 	clk              clock.Clock
 }
 
+var _ cspb.CRLStorerServer = (*crlStorer)(nil)
+
 func New(
 	issuers []*issuance.Certificate,
 	s3Client simpleS3,

crl/storer/storer_test.go

@@ -29,10 +29,6 @@ import (
 	"github.com/letsencrypt/boulder/test"
 )
 
-func TestImplementation(t *testing.T) {
-	test.AssertImplementsGRPCServer(t, &crlStorer{}, cspb.UnimplementedCRLStorerServer{})
-}
-
 type fakeUploadCRLServerStream struct {
 	grpc.ServerStream
 	input <-chan *cspb.UploadCRLRequest

nonce/nonce.go

@@ -297,10 +297,12 @@ func NewServer(inner *NonceService) *Server {
 
 // Server implements the gRPC nonce service.
 type Server struct {
-	noncepb.UnimplementedNonceServiceServer
+	noncepb.UnsafeNonceServiceServer
 	inner *NonceService
 }
 
+var _ noncepb.NonceServiceServer = (*Server)(nil)
+
 // Redeem accepts a nonce from a gRPC client and redeems it using the inner nonce service.
 func (ns *Server) Redeem(ctx context.Context, msg *noncepb.NonceMessage) (*noncepb.ValidMessage, error) {
 	return &noncepb.ValidMessage{Valid: ns.inner.Valid(msg.Nonce)}, nil

nonce/nonce_test.go

@@ -5,14 +5,9 @@ import (
 	"testing"
 
 	"github.com/letsencrypt/boulder/metrics"
-	noncepb "github.com/letsencrypt/boulder/nonce/proto"
 	"github.com/letsencrypt/boulder/test"
 )
 
-func TestImplementation(t *testing.T) {
-	test.AssertImplementsGRPCServer(t, &Server{}, noncepb.UnimplementedNonceServiceServer{})
-}
-
 func TestValidNonce(t *testing.T) {
 	ns, err := NewNonceService(metrics.NoopRegisterer, 0, "")
 	test.AssertNotError(t, err, "Could not create nonce service")

publisher/publisher.go

@@ -197,14 +197,16 @@ func initMetrics(stats prometheus.Registerer) *pubMetrics {
 
 // Impl defines a Publisher
 type Impl struct {
-	pubpb.UnimplementedPublisherServer
+	pubpb.UnsafePublisherServer
 	log           blog.Logger
 	userAgent     string
 	issuerBundles map[issuance.NameID][]ct.ASN1Cert
 	ctLogsCache   logCache
 	metrics       *pubMetrics
 }
 
+var _ pubpb.PublisherServer = (*Impl)(nil)
+
 // New creates a Publisher that will submit certificates
 // to requested CT logs
 func New(

publisher/publisher_test.go

@@ -33,10 +33,6 @@ import (
 	"github.com/letsencrypt/boulder/test"
 )
 
-func TestImplementation(t *testing.T) {
-	test.AssertImplementsGRPCServer(t, &Impl{}, pubpb.UnimplementedPublisherServer{})
-}
-
 var log = blog.UseMock()
 var ctx = context.Background()
 

ra/ra.go

@@ -80,7 +80,7 @@ type caaChecker interface {
 // NOTE: All of the fields in RegistrationAuthorityImpl need to be
 // populated, or there is a risk of panic.
 type RegistrationAuthorityImpl struct {
-	rapb.UnimplementedRegistrationAuthorityServer
+	rapb.UnsafeRegistrationAuthorityServer
 	CA        capb.CertificateAuthorityClient
 	OCSP      capb.OCSPGeneratorClient
 	VA        vapb.VAClient
@@ -124,6 +124,8 @@ type RegistrationAuthorityImpl struct {
 	certCSRMismatch             prometheus.Counter
 }
 
+var _ rapb.RegistrationAuthorityServer = (*RegistrationAuthorityImpl)(nil)
+
 // NewRegistrationAuthorityImpl constructs a new RA object.
 func NewRegistrationAuthorityImpl(
 	clk clock.Clock,

ra/ra_test.go

@@ -69,10 +69,6 @@ import (
 	vapb "github.com/letsencrypt/boulder/va/proto"
 )
 
-func TestImplementation(t *testing.T) {
-	test.AssertImplementsGRPCServer(t, &RegistrationAuthorityImpl{}, rapb.UnimplementedRegistrationAuthorityServer{})
-}
-
 func createPendingAuthorization(t *testing.T, sa sapb.StorageAuthorityClient, domain string, exp time.Time) *corepb.Authorization {
 	t.Helper()
 

sa/sa.go

@@ -36,11 +36,6 @@ var (
 // read-only methods provided by the SQLStorageAuthorityRO, those wrapper
 // implementations are in saro.go, next to the real implementations.
 type SQLStorageAuthority struct {
-	// Here we consciously opt out of "forward compatibility", by embedding this
-	// type instead of sapb.UnimplementedStorageAuthorityServer. We make this
-	// trade-off to avoid Go's compile-time "ambiguous selector" error, which it
-	// raises because sapb.UnimplementedStorageAuthorityServer and the embedded
-	// *SQLStorageAuthorityRO both provide methods with the same signatures.
 	sapb.UnsafeStorageAuthorityServer
 
 	*SQLStorageAuthorityRO
@@ -55,6 +50,8 @@ type SQLStorageAuthority struct {
 	rateLimitWriteErrors prometheus.Counter
 }
 
+var _ sapb.StorageAuthorityServer = (*SQLStorageAuthority)(nil)
+
 // NewSQLStorageAuthorityWrapping provides persistence using a SQL backend for
 // Boulder. It takes a read-only storage authority to wrap, which is useful if
 // you are constructing both types of implementations and want to share

sa/sa_test.go

@@ -74,15 +74,6 @@ func (s *fakeServerStream[T]) Context() context.Context {
 	return context.Background()
 }
 
-func TestImplementation(t *testing.T) {
-	test.AssertImplementsGRPCServer(t, &SQLStorageAuthorityRO{}, sapb.UnimplementedStorageAuthorityReadOnlyServer{})
-	// We do not run AssertImplementsGRPCServer for SQLStorageAuthority and
-	// sapb.UnimplementedStorageAuthorityServer, but this is okay: because
-	// SQLStorageAuthority does not embed sapb.UnimplementedStorageauthorityServer
-	// (choosing instead to embed sapb.UnsafeStorageAuthorityServer), we would get
-	// a compile time failure if it did not implement all of those methods.
-}
-
 // initSA constructs a SQLStorageAuthority and a clean up function that should
 // be defer'ed to the end of the test.
 func initSA(t *testing.T) (*SQLStorageAuthority, clock.FakeClock, func()) {

sa/saro.go

@@ -37,7 +37,7 @@ type certCountFunc func(ctx context.Context, db db.Selector, domain string, time
 
 // SQLStorageAuthorityRO defines a read-only subset of a Storage Authority
 type SQLStorageAuthorityRO struct {
-	sapb.UnimplementedStorageAuthorityReadOnlyServer
+	sapb.UnsafeStorageAuthorityReadOnlyServer
 
 	dbReadOnlyMap  *db.WrappedMap
 	dbIncidentsMap *db.WrappedMap
@@ -71,6 +71,8 @@ type SQLStorageAuthorityRO struct {
 	lagFactorCounter *prometheus.CounterVec
 }
 
+var _ sapb.StorageAuthorityReadOnlyServer = (*SQLStorageAuthorityRO)(nil)
+
 // NewSQLStorageAuthorityRO provides persistence using a SQL backend for
 // Boulder. It will modify the given borp.DbMap by adding relevant tables.
 func NewSQLStorageAuthorityRO(

test/asserts.go

@@ -249,44 +249,3 @@ loop:
 	}
 	AssertEquals(t, total, expected)
 }
-
-// AssertImplementsGRPCServer guarantees that impl, which must be a pointer to one of our
-// gRPC service implementation types, implements all of the methods expected by
-// unimpl, which must be the auto-generated gRPC type which is embedded by impl.
-// This function incidentally also guarantees that impl does not have any
-// methods with non-pointer receivers.
-func AssertImplementsGRPCServer(t *testing.T, impl any, unimpl any) {
-	// This type is auto-generated by grpc-go, and has methods which implement
-	// the auto-generated gRPC interface. These methods all have value receivers,
-	// not pointer receivers, so we're not using a pointer type here.
-	unimplType := reflect.TypeOf(unimpl)
-
-	// This is the type we use to manually implement the same auto-generated gRPC
-	// interface. We implement all of our methods with pointer receivers. This
-	// type is required to embed the auto-generated "unimplemented" type above,
-	// so it inherits all of the methods implemented by that type as well. But
-	// we can use the fact that we use pointer receivers while the auto-generated
-	// type uses value receivers to our advantage.
-	implType := reflect.TypeOf(impl).Elem()
-
-	// Iterate over all of the methods which are provided by a *non-pointer*
-	// receiver of our type. These will be only the methods which we *don't*
-	// manually implement ourselves, because when we implement the method ourself
-	// we use a pointer receiver. So this loop will only iterate over those
-	// methods which "fall through" to the embedded "unimplemented" type. Ideally,
-	// this loop executes zero times. If there are any methods at all on the
-	// non-pointer receiver, something has gone wrong.
-	for i := range implType.NumMethod() {
-		method := implType.Method(i)
-		_, ok := unimplType.MethodByName(method.Name)
-		if ok {
-			// If the lookup worked, then we know this is a method which we were
-			// supposed to implement, but didn't. Oops.
-			t.Errorf("%s does not implement method %s", implType.Name(), method.Name)
-		} else {
-			// If the lookup failed, then we have accidentally implemented some other
-			// method with a non-pointer receiver. We probably didn't mean to do that.
-			t.Errorf("%s.%s has non-pointer receiver", implType.Name(), method.Name)
-		}
-	}
-}

va/va.go

@@ -251,8 +251,8 @@ func newDefaultPortConfig() *portConfig {
 
 // ValidationAuthorityImpl represents a VA
 type ValidationAuthorityImpl struct {
-	vapb.UnimplementedVAServer
-	vapb.UnimplementedCAAServer
+	vapb.UnsafeVAServer
+	vapb.UnsafeCAAServer
 	log                blog.Logger
 	dnsClient          bdns.Client
 	issuerDomain       string
@@ -269,6 +269,9 @@ type ValidationAuthorityImpl struct {
 	metrics *vaMetrics
 }
 
+var _ vapb.VAServer = (*ValidationAuthorityImpl)(nil)
+var _ vapb.CAAServer = (*ValidationAuthorityImpl)(nil)
+
 // NewValidationAuthorityImpl constructs a new VA
 func NewValidationAuthorityImpl(
 	resolver bdns.Client,

va/va_test.go

@@ -34,11 +34,6 @@ import (
 	vapb "github.com/letsencrypt/boulder/va/proto"
 )
 
-func TestImplementation(t *testing.T) {
-	test.AssertImplementsGRPCServer(t, &ValidationAuthorityImpl{}, vapb.UnimplementedVAServer{})
-	test.AssertImplementsGRPCServer(t, &ValidationAuthorityImpl{}, vapb.UnimplementedCAAServer{})
-}
-
 var expectedToken = "LoqXcYV8q5ONbJQxbmR7SCTNo3tiAXDfowyjxAjEuX0"
 var expectedKeyAuthorization = "LoqXcYV8q5ONbJQxbmR7SCTNo3tiAXDfowyjxAjEuX0.9jg46WB3rR_AHD-EBXdN7cBkH1WOu0tA3M9fm21mqTI"