Improve documentation, remove dead code plus some minor refactorings
This improves our code-to-comments ratio by a lot.
On the refactorings:
- Simplify the cluster join mode to just a single protobuf message -
a node can either join an existing cluster or bootstrap a new one.
All of the node-level setup like hostname and trust backend is done
using the setup call, since those are identical for both cases.
- We don't need a node name separate from the hostname. Ideally, we would
get rid of IP addresses for etcd as well.
- Google API design guidelines suggest the `List` term (vs. `Get`).
- Add username to comments for consistency. I think the names provide
useful context, but git blame is a thing. What do you think?
- Fixed or silenced some ignored error checks in preparation of using
an errcheck linter. Especially during early boot, many errors are
obviously not recoverable, but logging them can provide useful debugging info.
- Split up the common package into smaller subpackages.
- Remove the audit package (this will be a separate service that probably
uses it own database, rather than etcd).
- Move storage constants to storage package.
- Remove the unused KV type.
I also added a bunch of TODO comments with discussion points.
Added both of you as blocking reviewers - please comment if I
misunderstood any of your code.
Test Plan: Everything compiles and scripts:launch works (for whatever that's worth).
X-Origin-Diff: phab/D235
GitOrigin-RevId: 922fec5076e8d683e1138f26d2cb490de64a9777
diff --git a/core/internal/api/BUILD.bazel b/core/internal/api/BUILD.bazel
index 6e3cb2b..b7aa48d 100644
--- a/core/internal/api/BUILD.bazel
+++ b/core/internal/api/BUILD.bazel
@@ -4,7 +4,7 @@
name = "go_default_library",
srcs = [
"cluster.go",
- "main.go",
+ "server.go",
"setup.go",
],
importpath = "git.monogon.dev/source/nexantic.git/core/internal/api",
@@ -12,8 +12,9 @@
deps = [
"//core/api/api:go_default_library",
"//core/internal/common:go_default_library",
+ "//core/internal/common/grpc:go_default_library",
+ "//core/internal/common/service:go_default_library",
"//core/internal/consensus:go_default_library",
- "@com_github_casbin_casbin//:go_default_library",
"@org_golang_google_grpc//:go_default_library",
"@org_uber_go_zap//:go_default_library",
],
diff --git a/core/internal/api/cluster.go b/core/internal/api/cluster.go
index 64a757f..3b45c40 100644
--- a/core/internal/api/cluster.go
+++ b/core/internal/api/cluster.go
@@ -22,7 +22,7 @@
"encoding/hex"
"fmt"
schema "git.monogon.dev/source/nexantic.git/core/generated/api"
- "git.monogon.dev/source/nexantic.git/core/internal/common"
+ "git.monogon.dev/source/nexantic.git/core/internal/common/grpc"
"errors"
@@ -35,7 +35,7 @@
func (s *Server) AddNode(ctx context.Context, req *schema.AddNodeRequest) (*schema.AddNodeResponse, error) {
// Setup API client
- c, err := common.NewSmalltownAPIClient(fmt.Sprintf("%s:%d", req.Host, req.ApiPort))
+ c, err := grpc.NewSmalltownAPIClient(fmt.Sprintf("%s:%d", req.Addr, s.config.Port))
if err != nil {
return nil, err
}
@@ -60,45 +60,44 @@
return nil, ErrAttestationFailed
}
- consensusCerts, err := s.consensusService.IssueCertificate(req.Host)
+ consensusCerts, err := s.consensusService.IssueCertificate(req.Addr)
if err != nil {
return nil, err
}
- // Provision cluster info locally
- memberID, err := s.consensusService.AddMember(ctx, req.Name, fmt.Sprintf("https://%s:%d", req.Host, req.ConsensusPort))
+ // TODO(leo): fetch remote hostname rather than using the addr
+ name := req.Addr
+
+ // Add new node to local etcd cluster.
+ memberID, err := s.consensusService.AddMember(ctx, name, fmt.Sprintf("https://%s:%d", req.Addr, s.config.Port))
if err != nil {
return nil, err
}
- s.Logger.Info("Added new node to consensus cluster; provisioning external node now",
- zap.String("host", req.Host), zap.Uint32("port", req.ApiPort),
- zap.Uint32("consensus_port", req.ConsensusPort), zap.String("name", req.Name))
+ s.Logger.Info("Added new node to consensus cluster; sending cluster join request to node",
+ zap.String("addr", req.Addr), zap.Uint16("port", s.config.Port))
- // Provision cluster info externally
- _, err = c.Setup.ProvisionCluster(ctx, &schema.ProvisionClusterRequest{
+ // Send JoinCluster request to new node to make it join.
+ _, err = c.Setup.JoinCluster(ctx, &schema.JoinClusterRequest{
InitialCluster: s.consensusService.GetInitialClusterString(),
- ProvisioningToken: req.Token,
- ExternalHost: req.Host,
- NodeName: req.Name,
- TrustBackend: req.TrustBackend,
+ ProvisioningToken: req.ProvisioningToken,
Certs: consensusCerts,
})
if err != nil {
- err3 := s.consensusService.RevokeCertificate(req.Host)
- if err3 != nil {
- s.Logger.Error("Failed to revoke a certificate after rollback, potential security risk", zap.Error(err3))
+ errRevoke := s.consensusService.RevokeCertificate(req.Addr)
+ if errRevoke != nil {
+ s.Logger.Error("Failed to revoke a certificate after rollback - potential security risk", zap.Error(errRevoke))
}
- // Revert Consensus add member - might fail if consensus cannot be established
- err2 := s.consensusService.RemoveMember(ctx, memberID)
- if err2 != nil || err3 != nil {
- return nil, fmt.Errorf("Rollback failed after failed provisioning; err=%v; err_rb=%v; err_revoke=%v", err, err2, err3)
+ // Revert etcd add member - might fail if consensus cannot be established.
+ errRemove := s.consensusService.RemoveMember(ctx, memberID)
+ if errRemove != nil || errRevoke != nil {
+ return nil, fmt.Errorf("rollback failed after failed provisioning; err=%v; err_rb=%v; err_revoke=%v", err, errRemove, errRevoke)
}
return nil, err
}
s.Logger.Info("Fully provisioned new node",
- zap.String("host", req.Host), zap.Uint32("port", req.ApiPort),
- zap.Uint32("consensus_port", req.ConsensusPort), zap.String("name", req.Name),
+ zap.String("host", req.Addr),
+ zap.Uint16("apiPort", s.config.Port),
zap.Uint64("member_id", memberID))
return &schema.AddNodeResponse{}, nil
@@ -108,7 +107,7 @@
panic("implement me")
}
-func (s *Server) GetNodes(context.Context, *schema.GetNodesRequest) (*schema.GetNodesResponse, error) {
+func (s *Server) ListNodes(context.Context, *schema.ListNodesRequest) (*schema.ListNodesResponse, error) {
nodes := s.consensusService.GetNodes()
resNodes := make([]*schema.Node, len(nodes))
@@ -121,7 +120,7 @@
}
}
- return &schema.GetNodesResponse{
+ return &schema.ListNodesResponse{
Nodes: resNodes,
}, nil
}
diff --git a/core/internal/api/main.go b/core/internal/api/server.go
similarity index 89%
rename from core/internal/api/main.go
rename to core/internal/api/server.go
index 20c3a3a..715e99e 100644
--- a/core/internal/api/main.go
+++ b/core/internal/api/server.go
@@ -20,8 +20,8 @@
"fmt"
schema "git.monogon.dev/source/nexantic.git/core/generated/api"
"git.monogon.dev/source/nexantic.git/core/internal/common"
+ "git.monogon.dev/source/nexantic.git/core/internal/common/service"
"git.monogon.dev/source/nexantic.git/core/internal/consensus"
- "github.com/casbin/casbin"
"go.uber.org/zap"
"google.golang.org/grpc"
"net"
@@ -29,9 +29,8 @@
type (
Server struct {
- *common.BaseService
+ *service.BaseService
- ruleEnforcer *casbin.Enforcer
setupService common.SetupService
grpcServer *grpc.Server
@@ -52,7 +51,7 @@
consensusService: consensusService,
}
- s.BaseService = common.NewBaseService("api", logger, s)
+ s.BaseService = service.NewBaseService("api", logger, s)
grpcServer := grpc.NewServer()
schema.RegisterClusterManagementServer(grpcServer, s)
@@ -75,7 +74,7 @@
s.Logger.Error("API server failed", zap.Error(err))
}()
- s.Logger.Info("GRPC listening", zap.String("host", listenHost))
+ s.Logger.Info("gRPC listening", zap.String("host", listenHost))
return nil
}
diff --git a/core/internal/api/setup.go b/core/internal/api/setup.go
index 6aeda40..f317534 100644
--- a/core/internal/api/setup.go
+++ b/core/internal/api/setup.go
@@ -34,66 +34,27 @@
)
func (s *Server) Setup(c context.Context, r *schema.SetupRequest) (*schema.SetupResponse, error) {
-
- switch r.Request.(type) {
- case *schema.SetupRequest_JoinCluster:
- token, err := s.enterJoinCluster(r.GetJoinCluster())
- if err != nil {
- return nil, err
- }
-
- return &schema.SetupResponse{
- Response: &schema.SetupResponse_JoinCluster{
- JoinCluster: &schema.JoinClusterResponse{
- ProvisioningToken: token,
- },
- },
- }, nil
-
- case *schema.SetupRequest_NewCluster:
- return &schema.SetupResponse{
- Response: &schema.SetupResponse_NewCluster{
- NewCluster: &schema.NewClusterResponse{},
- },
- }, s.setupNewCluster(r.GetNewCluster())
- }
-
return &schema.SetupResponse{}, nil
}
-func (s *Server) enterJoinCluster(r *schema.JoinClusterRequest) (string, error) {
- err := s.setupService.EnterJoinClusterMode()
- if err != nil {
- return "", err
- }
-
- return s.setupService.GetJoinClusterToken(), nil
+func (s *Server) BootstrapNewCluster(context.Context, *schema.BootstrapNewClusterRequest) (*schema.BootstrapNewClusterResponse, error) {
+ err := s.setupService.SetupNewCluster()
+ return &schema.BootstrapNewClusterResponse{}, err
}
-func (s *Server) setupNewCluster(r *schema.NewClusterRequest) error {
- if len(r.NodeName) < MinNameLength {
- return ErrInvalidNameLength
- }
- return s.setupService.SetupNewCluster(r.NodeName, r.ExternalHost)
-}
-
-func (s *Server) ProvisionCluster(ctx context.Context, req *schema.ProvisionClusterRequest) (*schema.ProvisionClusterResponse, error) {
- if len(req.NodeName) < MinNameLength {
- return nil, ErrInvalidNameLength
- }
-
+func (s *Server) JoinCluster(ctx context.Context, req *schema.JoinClusterRequest) (*schema.JoinClusterResponse, error) {
// Verify provisioning token
if s.setupService.GetJoinClusterToken() != req.ProvisioningToken {
return nil, ErrInvalidProvisioningToken
}
// Join cluster
- err := s.setupService.JoinCluster(req.NodeName, req.InitialCluster, req.ExternalHost, req.Certs)
+ err := s.setupService.JoinCluster(req.InitialCluster, req.Certs)
if err != nil {
return nil, err
}
- return &schema.ProvisionClusterResponse{}, nil
+ return &schema.JoinClusterResponse{}, nil
}
func (s *Server) Attest(c context.Context, r *schema.AttestRequest) (*schema.AttestResponse, error) {