Get rid of sandboxed fmtserver.
The current sandbox is excess complexity, needs manual steps to setup,
and doesn't isolate different users of a formatting server from each other.
diff --git a/Dockerfile b/Dockerfile
deleted file mode 100644
index 849d506..0000000
--- a/Dockerfile
+++ /dev/null
@@ -1,20 +0,0 @@
-FROM openjdk:8-jre
-
-WORKDIR /app
-
-# ADD /app/google-java-format.jar
-
-# https://github.com/google/google-java-format/releases/download/google-java-format-1.7/google-java-format-1.7-all-deps.jar
-COPY google-java-format-1.7-all-deps.jar /usr/bin/google-java-format-all-deps.jar
-
-# go build github.com/google/gerritfmt/cmd/fmtserver
-COPY fmtserver /usr/bin/fmtserver
-
-# go get github.com/bazelbuild/buildtools/buildifier
-COPY buildifier /usr/bin/buildifier
-
-# ??
-COPY gofmt /usr/bin/gofmt
-
-EXPOSE 80
-CMD ["fmtserver", "-port=80"]
diff --git a/README.md b/README.md
index 365f22e..b50b7b9 100644
--- a/README.md
+++ b/README.md
@@ -6,27 +6,25 @@
This is a style verifier intended to be used with the Gerrit checks
plugin.
-It consists of the following components
-
- * fmtserver: an RPC server that reformats source code. It is
- intended to be run in a Docker container that is secured with
- gvisor.
-
- * checker: a daemon that contacts gerrit, and sends pending changes
- to fmtserver to validate correct formatting
-
-
TODO
====
* handle file types (symlink) and deletions
- * use the full checker API
-
* more formatters: clang-format, typescript, jsformat, ... ?
- * isolate each formatter run with a separate gvisor/docker
- container?
+ * isolate each formatter to run with a separate gvisor/docker
+ container.
+
+ * tests: the only way to test this reliably is to spin up a gerrit server,
+ and create changes against the server.
+
+
+SECURITY
+========
+
+This currently runs the formatters without sandboxing. Critical bugs in
+formatters can be escalated to obtain the OAuth2 token used for authentication.
DISCLAIMER
diff --git a/cmd/checker/checker.go b/cmd/checker/checker.go
index 930a1bb..f45b0c9 100644
--- a/cmd/checker/checker.go
+++ b/cmd/checker/checker.go
@@ -30,8 +30,7 @@
// gerritChecker run formatting checks against a gerrit server.
type gerritChecker struct {
- server *gerrit.Server
- fmtClient *rpc.Client
+ server *gerrit.Server
// UUID => language
checkerUUIDs []string
@@ -48,11 +47,10 @@
return fields[0], true
}
-func NewGerritChecker(server *gerrit.Server, fmtClient *rpc.Client) (*gerritChecker, error) {
+func NewGerritChecker(server *gerrit.Server) (*gerritChecker, error) {
gc := &gerritChecker{
- server: server,
- fmtClient: fmtClient,
- todo: make(chan *gerrit.PendingChecksInfo, 5),
+ server: server,
+ todo: make(chan *gerrit.PendingChecksInfo, 5),
}
if out, err := ListCheckers(server); err != nil {
@@ -92,7 +90,7 @@
}
rep := gerritfmt.FormatReply{}
- if err := c.fmtClient.Call("Server.Format", &req, &rep); err != nil {
+ if err := gerritfmt.Format(&req, &rep); err != nil {
_, ok := err.(rpc.ServerError)
if ok {
return nil, fmt.Errorf("server returned: %s", err)
@@ -170,7 +168,7 @@
statusUnset: "UNSET",
statusIrrelevant: "IRRELEVANT",
statusRunning: "RUNNING",
- statusFail: "FAIL",
+ statusFail: "FAILED",
statusSuccessful: "SUCCESSFUL",
}[s]
}
diff --git a/cmd/checker/main.go b/cmd/checker/main.go
index 1e643f2..451e049 100644
--- a/cmd/checker/main.go
+++ b/cmd/checker/main.go
@@ -23,7 +23,6 @@
"fmt"
"io/ioutil"
"log"
- "net/rpc"
"net/url"
"os"
"strings"
@@ -101,7 +100,6 @@
func main() {
gerritURL := flag.String("gerrit", "", "URL to gerrit host")
- addr := flag.String("addr", "", "Address of the fmtserver")
register := flag.Bool("register", false, "Register with the host")
update := flag.Bool("update", false, "Update an existing checker on the host")
list := flag.Bool("list", false, "List pending checks")
@@ -186,15 +184,8 @@
log.Printf("CreateChecker result: %v", ch)
os.Exit(0)
}
- if *addr == "" {
- log.Fatal("must set --addr")
- }
- fmtClient, err := rpc.DialHTTP("tcp", *addr)
- if err != nil {
- log.Fatal(err)
- }
- gc, err := NewGerritChecker(g, fmtClient)
+ gc, err := NewGerritChecker(g)
if err != nil {
log.Fatal(err)
}
diff --git a/server.go b/server.go
index e4cc445..b2692a0 100644
--- a/server.go
+++ b/server.go
@@ -28,34 +28,71 @@
"strings"
)
-// Formatter is a definition of a formatting
-type Formatter struct {
+// Formatter is a definition of a formatting engine
+type Formatter interface {
+ // Format returns the files but formatted. All files are
+ // assumed to have the same language.
+ Format(in []File, outSink io.Writer) (out []FormattedFile, err error)
+}
+
+// FormatterConfig defines the mapping configurable
+type FormatterConfig struct {
// Regex is the typical filename regexp to use
Regex *regexp.Regexp
// Query is used to filter inside Gerrit
Query string
+
+ // The formatter
+ Formatter Formatter
}
// Formatters holds all the formatters supported
-var Formatters = map[string]*Formatter{
- "java": {
- Regex: regexp.MustCompile(`\.java$`),
- Query: "ext:java",
- },
- "bzl": {
- Regex: regexp.MustCompile(`(\.bzl|/BUILD|^BUILD)$`),
- Query: "(ext:bzl OR file:BUILD OR file:WORKSPACE)",
- },
- "go": {
- Regex: regexp.MustCompile(`\.go$`),
- Query: "ext:go",
- },
+var Formatters = map[string]*FormatterConfig{
"commitmsg": {
- Regex: regexp.MustCompile(`^/COMMIT_MSG$`),
+ Regex: regexp.MustCompile(`^/COMMIT_MSG$`),
+ Formatter: &commitMsgFormatter{},
},
}
+func init() {
+ gjf, err := exec.LookPath("google-java-format")
+ if err == nil {
+ Formatters["java"] = &FormatterConfig{
+ Regex: regexp.MustCompile(`\.java$`),
+ Query: "ext:java",
+ Formatter: &toolFormatter{
+ bin: "java",
+ args: []string{"-jar", gjf, "-i"},
+ },
+ }
+ }
+
+ bzl, err := exec.LookPath("buildifier")
+ if err == nil {
+ Formatters["bzl"] = &FormatterConfig{
+ Regex: regexp.MustCompile(`(\.bzl|/BUILD|^BUILD)$`),
+ Query: "(ext:bzl OR file:BUILD OR file:WORKSPACE)",
+ Formatter: &toolFormatter{
+ bin: bzl,
+ args: []string{"-mode=fix"},
+ },
+ }
+ }
+
+ gofmt, err := exec.LookPath("gofmt")
+ if err == nil {
+ Formatters["go"] = &FormatterConfig{
+ Regex: regexp.MustCompile(`\.go$`),
+ Query: "ext:go",
+ Formatter: &toolFormatter{
+ bin: gofmt,
+ args: []string{"-w"},
+ },
+ }
+ }
+}
+
// IsSupported returns if the given language is supported.
func IsSupported(lang string) bool {
_, ok := Formatters[lang]
@@ -72,30 +109,6 @@
return r
}
-// Server holds the settings for a server.
-type Server struct {
- JavaJar string
- Buildifier string
- Gofmt string
- formatterMap map[string]formatterFunc
-}
-
-type formatterFunc func(in []File, out io.Writer) ([]FormattedFile, error)
-
-// NewServer constructs a new server.
-func NewServer() *Server {
- s := &Server{}
-
- s.formatterMap = map[string]formatterFunc{
- "java": s.javaFormat,
- "go": s.goFormat,
- "bzl": s.bazelFormat,
- "commitmsg": s.commitCheck,
- }
-
- return s
-}
-
func splitByLang(in []File) map[string][]File {
res := map[string][]File{}
for _, f := range in {
@@ -104,8 +117,8 @@
return res
}
-// Format is the formatserver RPC endpoint.
-func (s *Server) Format(req *FormatRequest, rep *FormatReply) error {
+// Format formats all the files in the request for which a formatter exists.
+func Format(req *FormatRequest, rep *FormatReply) error {
for _, f := range req.Files {
if f.Language == "" {
return fmt.Errorf("file %q has empty language", f.Name)
@@ -114,7 +127,10 @@
for language, fs := range splitByLang(req.Files) {
var buf bytes.Buffer
- out, err := s.formatterMap[language](fs, &buf)
+ entry := Formatters[language]
+ log.Println("init", Formatters)
+
+ out, err := entry.Formatter.Format(fs, &buf)
if err != nil {
return err
}
@@ -127,8 +143,10 @@
return nil
}
-func (s *Server) commitCheck(in []File, outSink io.Writer) (out []FormattedFile, err error) {
- complaint := s.checkCommitMessage(string(in[0].Content))
+type commitMsgFormatter struct{}
+
+func (f *commitMsgFormatter) Format(in []File, outSink io.Writer) (out []FormattedFile, err error) {
+ complaint := checkCommitMessage(string(in[0].Content))
ff := FormattedFile{}
ff.Name = in[0].Name
if complaint != "" {
@@ -140,7 +158,7 @@
return out, nil
}
-func (s *Server) checkCommitMessage(msg string) (complaint string) {
+func checkCommitMessage(msg string) (complaint string) {
lines := strings.Split(msg, "\n")
if len(lines) < 2 {
return "must have multiple lines"
@@ -161,42 +179,14 @@
return ""
}
-func (s *Server) javaFormat(in []File, outSink io.Writer) (out []FormattedFile, err error) {
- if _, err := os.Stat(s.JavaJar); err != nil {
- return nil, fmt.Errorf("Stat(%q): %v", s.JavaJar, err)
- }
- cmd := exec.Command(
- "java",
- "-jar",
- s.JavaJar,
- "-i",
- )
- return s.inlineFixTool(cmd, in, outSink)
+type toolFormatter struct {
+ bin string
+ args []string
}
-func (s *Server) bazelFormat(in []File, outSink io.Writer) (out []FormattedFile, err error) {
- if _, err := os.Stat(s.Buildifier); err != nil {
- return nil, fmt.Errorf("Stat(%q): %v", s.Buildifier, err)
- }
- cmd := exec.Command(
- s.Buildifier,
- "-mode=fix",
- )
- return s.inlineFixTool(cmd, in, outSink)
-}
+func (f *toolFormatter) Format(in []File, outSink io.Writer) (out []FormattedFile, err error) {
+ cmd := exec.Command(f.bin, f.args...)
-func (s *Server) goFormat(in []File, outSink io.Writer) (out []FormattedFile, err error) {
- if _, err := os.Stat(s.Buildifier); err != nil {
- return nil, fmt.Errorf("Stat(%q): %v", s.Buildifier, err)
- }
- cmd := exec.Command(
- s.Gofmt,
- "-w",
- )
- return s.inlineFixTool(cmd, in, outSink)
-}
-
-func (s *Server) inlineFixTool(cmd *exec.Cmd, in []File, outSink io.Writer) (out []FormattedFile, err error) {
tmpDir, err := ioutil.TempDir("", "gerritfmt")
if err != nil {
return nil, err
