From 68cce3eec0220cc83772dcf84e38853de9bb5e36 Mon Sep 17 00:00:00 2001 From: Jacob Weinstock Date: Mon, 13 Dec 2021 11:40:06 -0700 Subject: [PATCH 1/8] Add open telemetry to the HTTP service: Brings the HTTP package on par with the TFTP package for open telemetry. Signed-off-by: Jacob Weinstock --- example/main.go | 9 ++++- ihttp/ihttp.go | 99 ++++++++++++++++++++++++++++++++++++++------- ihttp/ihttp_test.go | 72 +++++++++++++++++++++++++++++++-- 3 files changed, 162 insertions(+), 18 deletions(-) diff --git a/example/main.go b/example/main.go index 4ee4e89..8fb6beb 100644 --- a/example/main.go +++ b/example/main.go @@ -15,7 +15,14 @@ import ( ) func main() { - ctx, done := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGHUP, syscall.SIGTERM) + + ctx := context.Background() + /* + ctx, otelShutdown := otelinit.InitOpenTelemetry(ctx, "ipxe") + defer otelShutdown(ctx) + */ + + ctx, done := signal.NotifyContext(ctx, os.Interrupt, syscall.SIGHUP, syscall.SIGTERM) defer done() logger := stdr.New(log.New(os.Stdout, "", log.Lshortfile)) diff --git a/ihttp/ihttp.go b/ihttp/ihttp.go index 2fe05b6..e17794e 100644 --- a/ihttp/ihttp.go +++ b/ihttp/ihttp.go @@ -3,15 +3,21 @@ package ihttp import ( "context" + "fmt" "net" "net/http" "path" "path/filepath" + "regexp" "strings" "unicode/utf8" "github.com/go-logr/logr" "github.com/tinkerbell/ipxedust/binary" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/trace" "inet.af/netaddr" ) @@ -44,9 +50,19 @@ func Serve(_ context.Context, conn net.Listener, h *http.Server) error { return h.Serve(conn) } -func trimFirstRune(s string) string { - _, i := utf8.DecodeRuneInString(s) - return s[i:] +func parseMac(urlPath string) net.HardwareAddr { + p := path.Dir(urlPath) + var name string + if strings.HasPrefix(p, "/") { + s := path.Dir(urlPath) + _, i := utf8.DecodeRuneInString(s) + name = s[i:] + } + mac, err := net.ParseMAC(name) + if err != nil { + return nil + } + return mac } // Handle handles responses to HTTP requests. @@ -58,19 +74,39 @@ func (s Handler) Handle(w http.ResponseWriter, req *http.Request) { } host, port, _ := net.SplitHostPort(req.RemoteAddr) s.Log = s.Log.WithValues("host", host, "port", port) - m := path.Dir(req.URL.Path) - if strings.HasPrefix(m, "/") { - m = trimFirstRune(path.Dir(req.URL.Path)) - } // If a mac address is provided, log it. Mac address is optional. - // Mac address would be useful for logging and tracing. - mac, _ := net.ParseMAC(m) - s.Log = s.Log.WithValues("mac", mac) + mac := parseMac(req.URL.Path) + s.Log = s.Log.WithValues("mac", mac.String()) + filename := filepath.Base(req.URL.Path) + + // clients can send traceparent over HTTP by appending the traceparent string + // to the end of the filename they really want + longfile := filename // hang onto this to report in traces + ctx, shortfile, err := extractTraceparentFromFilename(context.Background(), filename) + if err != nil { + s.Log.Error(err, "failed to extract traceparent from filename") + } + if shortfile != filename { + s.Log.Info("traceparent found in filename", "filename_with_traceparent", longfile, "filename", shortfile) + filename = shortfile + } - got := filepath.Base(req.URL.Path) - file, found := binary.Files[got] + tracer := otel.Tracer("HTTP") + _, span := tracer.Start(ctx, "HTTP get", + trace.WithSpanKind(trace.SpanKindServer), + trace.WithAttributes(attribute.String("filename", filename)), + trace.WithAttributes(attribute.String("requested-filename", longfile)), + trace.WithAttributes(attribute.String("ip", host)), + trace.WithAttributes(attribute.String("mac", mac.String())), + ) + + span.SetStatus(codes.Ok, filename) + span.End() + + s.Log = s.Log.WithValues("filename", filename) + file, found := binary.Files[filename] if !found { - s.Log.Info("requested file not found", "file", got) + s.Log.Info("requested file not found") http.NotFound(w, req) return } @@ -80,5 +116,40 @@ func (s Handler) Handle(w http.ResponseWriter, req *http.Request) { w.WriteHeader(http.StatusInternalServerError) return } - s.Log.Info("file served", "bytes sent", b, "file size", len(file), "file", got) + s.Log.Info("file served", "bytes sent", b, "file size", len(file)) +} + +// extractTraceparentFromFilename takes a context and filename and checks the filename for +// a traceparent tacked onto the end of it. If there is a match, the traceparent is extracted +// and a new SpanContext is constructed and added to the context.Context that is returned. +// The filename is shortened to just the original filename so the rest of boots HTTP can +// carry on as usual. +func extractTraceparentFromFilename(ctx context.Context, filename string) (context.Context, string, error) { + // traceparentRe captures 4 items, the original filename, the trace id, span id, and trace flags + traceparentRe := regexp.MustCompile("^(.*)-[[:xdigit:]]{2}-([[:xdigit:]]{32})-([[:xdigit:]]{16})-([[:xdigit:]]{2})") + parts := traceparentRe.FindStringSubmatch(filename) + if len(parts) == 5 { + traceID, err := trace.TraceIDFromHex(parts[2]) + if err != nil { + return ctx, filename, fmt.Errorf("parsing OpenTelemetry trace id %q failed: %w", parts[2], err) + } + + spanID, err := trace.SpanIDFromHex(parts[3]) + if err != nil { + return ctx, filename, fmt.Errorf("parsing OpenTelemetry span id %q failed: %w", parts[3], err) + } + + // create a span context with the parent trace id & span id + spanCtx := trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: traceID, + SpanID: spanID, + Remote: true, + TraceFlags: trace.FlagsSampled, // TODO: use the parts[4] value instead + }) + + // inject it into the context.Context and return it along with the original filename + return trace.ContextWithSpanContext(ctx, spanCtx), parts[1], nil + } + // no traceparent found, return everything as it was + return ctx, filename, nil } diff --git a/ihttp/ihttp_test.go b/ihttp/ihttp_test.go index 7ef5a22..d1cc939 100644 --- a/ihttp/ihttp_test.go +++ b/ihttp/ihttp_test.go @@ -3,16 +3,21 @@ package ihttp import ( "bytes" "context" + "errors" "fmt" "io" "io/ioutil" + "log" "net/http" "net/http/httptest" + "os" "testing" "github.com/go-logr/logr" + "github.com/go-logr/stdr" "github.com/google/go-cmp/cmp" "github.com/tinkerbell/ipxedust/binary" + "go.opentelemetry.io/otel/trace" "inet.af/netaddr" ) @@ -115,7 +120,7 @@ func TestHandleHTTP_Handler(t *testing.T) { }, { name: "success", - req: req{method: "GET", url: "/snp.efi"}, + req: req{method: "GET", url: "/30:23:03:73:a5:a7/snp.efi-00-23b1e307bb35484f535a1f772c06910e-d887dc3912240434-01"}, want: &http.Response{ StatusCode: http.StatusOK, Body: ioutil.NopCloser(bytes.NewBuffer(binary.Files["snp.efi"])), @@ -140,16 +145,19 @@ func TestHandleHTTP_Handler(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + logger := stdr.New(log.New(os.Stdout, "", log.Lshortfile)) req := httptest.NewRequest(tt.req.method, tt.req.url, tt.req.body) var resp *http.Response if tt.failWrite { w := newFakeResponse() - h := Handler{Log: logr.Discard()} + //h := Handler{Log: logr.Discard()} + h := Handler{Log: logger} h.Handle(w, req) resp = w.Result() } else { w := httptest.NewRecorder() - h := Handler{Log: logr.Discard()} + h := Handler{Log: logger} + //h := Handler{Log: logr.Discard()} h.Handle(w, req) resp = w.Result() } @@ -175,3 +183,61 @@ func TestHandleHTTP_Handler(t *testing.T) { }) } } + +func TestExtractTraceparentFromFilename(t *testing.T) { + tests := map[string]struct { + fileIn string + fileOut string + err error + spanID string + traceID string + }{ + "do nothing when no tp": {fileIn: "undionly.ipxe", fileOut: "undionly.ipxe", err: nil}, + "ignore bad filename": { + fileIn: "undionly.ipxe-00-0000-0000-00", + fileOut: "undionly.ipxe-00-0000-0000-00", + err: nil, + }, + "ignore corrupt tp": { + fileIn: "undionly.ipxe-00-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx-abcdefghijklmnop-01", + fileOut: "undionly.ipxe-00-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx-abcdefghijklmnop-01", + err: nil, + }, + "extract tp": { + fileIn: "undionly.ipxe-00-23b1e307bb35484f535a1f772c06910e-d887dc3912240434-01", + fileOut: "undionly.ipxe", + err: nil, + spanID: "d887dc3912240434", + traceID: "23b1e307bb35484f535a1f772c06910e", + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + ctx, outfile, err := extractTraceparentFromFilename(ctx, tc.fileIn) + if !errors.Is(err, tc.err) { + t.Errorf("filename %q should have resulted in error %q but got %q", tc.fileIn, tc.err, err) + } + if outfile != tc.fileOut { + t.Errorf("filename %q should have resulted in %q but got %q", tc.fileIn, tc.fileOut, outfile) + } + + if tc.spanID != "" { + sc := trace.SpanContextFromContext(ctx) + got := sc.SpanID().String() + if tc.spanID != got { + t.Errorf("got incorrect span id from context, expected %q but got %q", tc.spanID, got) + } + } + + if tc.traceID != "" { + sc := trace.SpanContextFromContext(ctx) + got := sc.TraceID().String() + if tc.traceID != got { + t.Errorf("got incorrect trace id from context, expected %q but got %q", tc.traceID, got) + } + } + }) + } +} From fab68a87b1cf5d99c43aa7dba9fd6e24a4164d8a Mon Sep 17 00:00:00 2001 From: Jacob Weinstock Date: Thu, 16 Dec 2021 17:19:41 -0700 Subject: [PATCH 2/8] update test coverage for func extractTraceparentFromFilename Signed-off-by: Jacob Weinstock --- ihttp/ihttp_test.go | 23 ++++++++++++++++++++++- itftp/itftp_test.go | 16 +++++++++++++++- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/ihttp/ihttp_test.go b/ihttp/ihttp_test.go index d1cc939..ec47663 100644 --- a/ihttp/ihttp_test.go +++ b/ihttp/ihttp_test.go @@ -126,6 +126,14 @@ func TestHandleHTTP_Handler(t *testing.T) { Body: ioutil.NopCloser(bytes.NewBuffer(binary.Files["snp.efi"])), }, }, + { + name: "success but with bad traceparent", + req: req{method: "GET", url: "/30:23:03:73:a5:a7/snp.efi-00-00000000000000000000000000000000-d887dc3912240434-01"}, + want: &http.Response{ + StatusCode: http.StatusNotFound, + Body: nil, + }, + }, { name: "file not found", req: req{method: "GET", url: "/none.efi"}, @@ -203,6 +211,16 @@ func TestExtractTraceparentFromFilename(t *testing.T) { fileOut: "undionly.ipxe-00-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx-abcdefghijklmnop-01", err: nil, }, + "ignore corrupt TraceID": { + fileIn: "undionly.ipxe-00-00000000000000000000000000000000-0000000000000000-01", + fileOut: "undionly.ipxe-00-00000000000000000000000000000000-0000000000000000-01", + err: fmt.Errorf("parsing OpenTelemetry trace id %q failed: %w", "00000000000000000000000000000000", fmt.Errorf("trace-id can't be all zero")), + }, + "ignore corrupt SpanID": { + fileIn: "undionly.ipxe-00-11111111111111111111111111111111-0000000000000000-01", + fileOut: "undionly.ipxe-00-11111111111111111111111111111111-0000000000000000-01", + err: fmt.Errorf("parsing OpenTelemetry span id %q failed: %w", "0000000000000000", fmt.Errorf("span-id can't be all zero")), + }, "extract tp": { fileIn: "undionly.ipxe-00-23b1e307bb35484f535a1f772c06910e-d887dc3912240434-01", fileOut: "undionly.ipxe", @@ -217,7 +235,10 @@ func TestExtractTraceparentFromFilename(t *testing.T) { ctx := context.Background() ctx, outfile, err := extractTraceparentFromFilename(ctx, tc.fileIn) if !errors.Is(err, tc.err) { - t.Errorf("filename %q should have resulted in error %q but got %q", tc.fileIn, tc.err, err) + if diff := cmp.Diff(fmt.Sprint(err), fmt.Sprint(tc.err)); diff != "" { + t.Errorf(diff) + t.Errorf("filename %q should have resulted in error %q but got %q", tc.fileIn, tc.err, err) + } } if outfile != tc.fileOut { t.Errorf("filename %q should have resulted in %q but got %q", tc.fileIn, tc.fileOut, outfile) diff --git a/itftp/itftp_test.go b/itftp/itftp_test.go index 54cfdd2..294b94b 100644 --- a/itftp/itftp_test.go +++ b/itftp/itftp_test.go @@ -3,6 +3,7 @@ package itftp import ( "context" "errors" + "fmt" "io" "net" "os" @@ -170,6 +171,16 @@ func TestExtractTraceparentFromFilename(t *testing.T) { fileOut: "undionly.ipxe-00-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx-abcdefghijklmnop-01", err: nil, }, + "ignore corrupt TraceID": { + fileIn: "undionly.ipxe-00-00000000000000000000000000000000-0000000000000000-01", + fileOut: "undionly.ipxe-00-00000000000000000000000000000000-0000000000000000-01", + err: fmt.Errorf("parsing OpenTelemetry trace id %q failed: %w", "00000000000000000000000000000000", fmt.Errorf("trace-id can't be all zero")), + }, + "ignore corrupt SpanID": { + fileIn: "undionly.ipxe-00-11111111111111111111111111111111-0000000000000000-01", + fileOut: "undionly.ipxe-00-11111111111111111111111111111111-0000000000000000-01", + err: fmt.Errorf("parsing OpenTelemetry span id %q failed: %w", "0000000000000000", fmt.Errorf("span-id can't be all zero")), + }, "extract tp": { fileIn: "undionly.ipxe-00-23b1e307bb35484f535a1f772c06910e-d887dc3912240434-01", fileOut: "undionly.ipxe", @@ -184,7 +195,10 @@ func TestExtractTraceparentFromFilename(t *testing.T) { ctx := context.Background() ctx, outfile, err := extractTraceparentFromFilename(ctx, tc.fileIn) if !errors.Is(err, tc.err) { - t.Errorf("filename %q should have resulted in error %q but got %q", tc.fileIn, tc.err, err) + if diff := cmp.Diff(fmt.Sprint(err), fmt.Sprint(tc.err)); diff != "" { + t.Errorf(diff) + t.Errorf("filename %q should have resulted in error %q but got %q", tc.fileIn, tc.err, err) + } } if outfile != tc.fileOut { t.Errorf("filename %q should have resulted in %q but got %q", tc.fileIn, tc.fileOut, outfile) From 13c101cba54909472e0f51652abaa4f892edc316 Mon Sep 17 00:00:00 2001 From: Jacob Weinstock Date: Tue, 21 Dec 2021 10:17:37 -0700 Subject: [PATCH 3/8] Update logger to be a local variable: Updating a local variable instead of the handler field. Signed-off-by: Jacob Weinstock --- ihttp/ihttp.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/ihttp/ihttp.go b/ihttp/ihttp.go index e17794e..1241864 100644 --- a/ihttp/ihttp.go +++ b/ihttp/ihttp.go @@ -73,10 +73,10 @@ func (s Handler) Handle(w http.ResponseWriter, req *http.Request) { return } host, port, _ := net.SplitHostPort(req.RemoteAddr) - s.Log = s.Log.WithValues("host", host, "port", port) + log := s.Log.WithValues("host", host, "port", port) // If a mac address is provided, log it. Mac address is optional. mac := parseMac(req.URL.Path) - s.Log = s.Log.WithValues("mac", mac.String()) + log = log.WithValues("mac", mac.String()) filename := filepath.Base(req.URL.Path) // clients can send traceparent over HTTP by appending the traceparent string @@ -84,10 +84,10 @@ func (s Handler) Handle(w http.ResponseWriter, req *http.Request) { longfile := filename // hang onto this to report in traces ctx, shortfile, err := extractTraceparentFromFilename(context.Background(), filename) if err != nil { - s.Log.Error(err, "failed to extract traceparent from filename") + log.Error(err, "failed to extract traceparent from filename") } if shortfile != filename { - s.Log.Info("traceparent found in filename", "filename_with_traceparent", longfile, "filename", shortfile) + log.Info("traceparent found in filename", "filename_with_traceparent", longfile, "filename", shortfile) filename = shortfile } @@ -103,20 +103,20 @@ func (s Handler) Handle(w http.ResponseWriter, req *http.Request) { span.SetStatus(codes.Ok, filename) span.End() - s.Log = s.Log.WithValues("filename", filename) + log = log.WithValues("filename", filename) file, found := binary.Files[filename] if !found { - s.Log.Info("requested file not found") + log.Info("requested file not found") http.NotFound(w, req) return } b, err := w.Write(file) if err != nil { - s.Log.Error(err, "error serving file") + log.Error(err, "error serving file") w.WriteHeader(http.StatusInternalServerError) return } - s.Log.Info("file served", "bytes sent", b, "file size", len(file)) + log.Info("file served", "bytes sent", b, "file size", len(file)) } // extractTraceparentFromFilename takes a context and filename and checks the filename for From b01fe32c5d213186238f7a14cd9391ebebfec9c2 Mon Sep 17 00:00:00 2001 From: Jacob Weinstock Date: Thu, 23 Dec 2021 10:48:25 -0700 Subject: [PATCH 4/8] Update logging fields to not contain spaces: Logging fields with spaces have a potential to affect parsing. This removes spacing from field names. Signed-off-by: Jacob Weinstock --- example/main.go | 1 - ihttp/ihttp.go | 15 +++++++-------- ihttp/ihttp_test.go | 2 -- itftp/itftp.go | 16 ++++++++-------- 4 files changed, 15 insertions(+), 19 deletions(-) diff --git a/example/main.go b/example/main.go index 8fb6beb..1fc2f71 100644 --- a/example/main.go +++ b/example/main.go @@ -15,7 +15,6 @@ import ( ) func main() { - ctx := context.Background() /* ctx, otelShutdown := otelinit.InitOpenTelemetry(ctx, "ipxe") diff --git a/ihttp/ihttp.go b/ihttp/ihttp.go index 1241864..87a1e80 100644 --- a/ihttp/ihttp.go +++ b/ihttp/ihttp.go @@ -51,12 +51,10 @@ func Serve(_ context.Context, conn net.Listener, h *http.Server) error { } func parseMac(urlPath string) net.HardwareAddr { - p := path.Dir(urlPath) var name string - if strings.HasPrefix(p, "/") { - s := path.Dir(urlPath) - _, i := utf8.DecodeRuneInString(s) - name = s[i:] + if p := path.Dir(urlPath); strings.HasPrefix(p, "/") { + _, i := utf8.DecodeRuneInString(p) + name = p[i:] } mac, err := net.ParseMAC(name) if err != nil { @@ -78,6 +76,7 @@ func (s Handler) Handle(w http.ResponseWriter, req *http.Request) { mac := parseMac(req.URL.Path) log = log.WithValues("mac", mac.String()) filename := filepath.Base(req.URL.Path) + log = log.WithValues("filename", filename) // clients can send traceparent over HTTP by appending the traceparent string // to the end of the filename they really want @@ -87,7 +86,8 @@ func (s Handler) Handle(w http.ResponseWriter, req *http.Request) { log.Error(err, "failed to extract traceparent from filename") } if shortfile != filename { - log.Info("traceparent found in filename", "filename_with_traceparent", longfile, "filename", shortfile) + log = log.WithValues("shortfile", shortfile) + log.Info("traceparent found in filename", "filenameWithTraceparent", longfile) filename = shortfile } @@ -103,7 +103,6 @@ func (s Handler) Handle(w http.ResponseWriter, req *http.Request) { span.SetStatus(codes.Ok, filename) span.End() - log = log.WithValues("filename", filename) file, found := binary.Files[filename] if !found { log.Info("requested file not found") @@ -116,7 +115,7 @@ func (s Handler) Handle(w http.ResponseWriter, req *http.Request) { w.WriteHeader(http.StatusInternalServerError) return } - log.Info("file served", "bytes sent", b, "file size", len(file)) + log.Info("file served", "bytesSent", b, "fileSize", len(file)) } // extractTraceparentFromFilename takes a context and filename and checks the filename for diff --git a/ihttp/ihttp_test.go b/ihttp/ihttp_test.go index ec47663..fb07114 100644 --- a/ihttp/ihttp_test.go +++ b/ihttp/ihttp_test.go @@ -158,14 +158,12 @@ func TestHandleHTTP_Handler(t *testing.T) { var resp *http.Response if tt.failWrite { w := newFakeResponse() - //h := Handler{Log: logr.Discard()} h := Handler{Log: logger} h.Handle(w, req) resp = w.Result() } else { w := httptest.NewRecorder() h := Handler{Log: logger} - //h := Handler{Log: logr.Discard()} h.Handle(w, req) resp = w.Result() } diff --git a/itftp/itftp.go b/itftp/itftp.go index 12d7e5f..1d64d24 100644 --- a/itftp/itftp.go +++ b/itftp/itftp.go @@ -54,23 +54,23 @@ func (t Handler) HandleRead(filename string, rf io.ReaderFrom) error { full := filename filename = path.Base(filename) - l := t.Log.WithValues("event", "get", "filename", filename, "uri", full, "client", client) + log := t.Log.WithValues("event", "get", "filename", filename, "uri", full, "client", client) // clients can send traceparent over TFTP by appending the traceparent string // to the end of the filename they really want longfile := filename // hang onto this to report in traces ctx, shortfile, err := extractTraceparentFromFilename(context.Background(), filename) if err != nil { - l.Error(err, "") + log.Error(err, "failed to extract traceparent from filename") } if shortfile != filename { - l = l.WithValues("filename", shortfile) // flip to the short filename in logs - l.Info("client requested filename '", filename, "' with a traceparent attached and has been shortened to '", shortfile, "'") + log = log.WithValues("shortfile", shortfile) + log.Info("traceparent found in filename", "filenameWithTraceparent", longfile) filename = shortfile } // If a mac address is provided, log it. Mac address is optional. mac, _ := net.ParseMAC(path.Dir(full)) - l = l.WithValues("mac", mac.String()) + log = log.WithValues("mac", mac.String()) tracer := otel.Tracer("TFTP") _, span := tracer.Start(ctx, "TFTP get", @@ -87,17 +87,17 @@ func (t Handler) HandleRead(filename string, rf io.ReaderFrom) error { content, ok := binary.Files[filepath.Base(shortfile)] if !ok { err := fmt.Errorf("file unknown: %w", os.ErrNotExist) - l.Error(err, "file unknown") + log.Error(err, "file unknown") return err } ct := bytes.NewReader(content) b, err := rf.ReadFrom(ct) if err != nil { - l.Error(err, "file serve failed", "b", b, "content size", len(content)) + log.Error(err, "file serve failed", "b", b, "contentSize", len(content)) return err } - l.Info("file served", "bytes sent", b, "content size", len(content)) + log.Info("file served", "bytesSent", b, "contentSize", len(content)) return nil } From 98231b05e1f19b73a6360e758d1c455bee3d5a43 Mon Sep 17 00:00:00 2001 From: Jacob Weinstock Date: Thu, 23 Dec 2021 11:00:45 -0700 Subject: [PATCH 5/8] Add code coverage for TFTP: Also, update test names to follow `TestXxx` format. Ref: https://pkg.go.dev/testing Signed-off-by: Jacob Weinstock --- ihttp/ihttp_test.go | 4 ++-- itftp/itftp_test.go | 14 ++++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/ihttp/ihttp_test.go b/ihttp/ihttp_test.go index fb07114..41c5234 100644 --- a/ihttp/ihttp_test.go +++ b/ihttp/ihttp_test.go @@ -99,7 +99,7 @@ func TestListenAndServeHTTP(t *testing.T) { } } -func TestHandleHTTP_Handler(t *testing.T) { +func TestHandle(t *testing.T) { type req struct { method string url string @@ -127,7 +127,7 @@ func TestHandleHTTP_Handler(t *testing.T) { }, }, { - name: "success but with bad traceparent", + name: "fail with bad traceparent", req: req{method: "GET", url: "/30:23:03:73:a5:a7/snp.efi-00-00000000000000000000000000000000-d887dc3912240434-01"}, want: &http.Response{ StatusCode: http.StatusNotFound, diff --git a/itftp/itftp_test.go b/itftp/itftp_test.go index 294b94b..52937c0 100644 --- a/itftp/itftp_test.go +++ b/itftp/itftp_test.go @@ -93,7 +93,7 @@ func TestListenAndServeTFTP(t *testing.T) { } } -func TestHandlerTFTP_ReadHandler(t *testing.T) { +func TestHandleRead(t *testing.T) { tests := []struct { name string fileName string @@ -110,16 +110,19 @@ func TestHandlerTFTP_ReadHandler(t *testing.T) { fileName: "snp.efi-00-23b1e307bb35484f535a1f772c06910e-d887dc3912240434-01", want: binary.Files["snp.efi"], }, + { + name: "fail with bad traceparent", + fileName: "snp.efi-00-00000000000000000000000000000000-d887dc3912240434-01", + wantErr: os.ErrNotExist, + }, { name: "fail - not found", fileName: "not-found", - want: []byte{}, wantErr: os.ErrNotExist, }, { name: "failure - with read error", fileName: "snp.efi", - want: []byte{}, wantErr: net.ErrClosed, }, } @@ -136,6 +139,9 @@ func TestHandlerTFTP_ReadHandler(t *testing.T) { if !errors.Is(err, tt.wantErr) { t.Fatalf("error mismatch, got: %T, want: %T", err, tt.wantErr) } + if tt.wantErr != nil { + tt.want = []byte{} + } if diff := cmp.Diff(rf.content, tt.want); diff != "" { t.Fatal(diff) } @@ -143,7 +149,7 @@ func TestHandlerTFTP_ReadHandler(t *testing.T) { } } -func TestHandlerTFTP_WriteHandler(t *testing.T) { +func TestHandleWrite(t *testing.T) { ht := &Handler{Log: logr.Discard()} rf := &fakeReaderFrom{addr: net.UDPAddr{IP: net.IPv4(127, 0, 0, 1), Port: 9999}} err := ht.HandleWrite("snp.efi", rf) From f5c5062b5b53d6b65d635251dc8a2e8827a48336 Mon Sep 17 00:00:00 2001 From: Jacob Weinstock Date: Tue, 4 Jan 2022 10:54:07 -0700 Subject: [PATCH 6/8] Update package comment to match new package name. Signed-off-by: Jacob Weinstock --- ihttp/ihttp.go | 2 +- ipxedust.go | 2 +- itftp/itftp.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ihttp/ihttp.go b/ihttp/ihttp.go index 87a1e80..0e36e97 100644 --- a/ihttp/ihttp.go +++ b/ihttp/ihttp.go @@ -1,4 +1,4 @@ -// Package http implements an HTTP server for iPXE binaries. +// Package ihttp implements an HTTP server for iPXE binaries. package ihttp import ( diff --git a/ipxedust.go b/ipxedust.go index 724f7e3..9886bd8 100644 --- a/ipxedust.go +++ b/ipxedust.go @@ -1,4 +1,4 @@ -// Package ipxe implements the iPXE tftp and http serving. +// Package ipxedust implements the iPXE tftp and http serving. package ipxedust import ( diff --git a/itftp/itftp.go b/itftp/itftp.go index 1d64d24..9e00a2f 100644 --- a/itftp/itftp.go +++ b/itftp/itftp.go @@ -1,4 +1,4 @@ -// Package tftp implements a TFTP server for iPXE binaries. +// Package itftp implements a TFTP server for iPXE binaries. package itftp import ( From c1148165f999b8c8832e6495f0594a370a3a4c8b Mon Sep 17 00:00:00 2001 From: Jacob Weinstock Date: Wed, 5 Jan 2022 18:20:55 -0700 Subject: [PATCH 7/8] update mac parsing, update logging, and error handling. Signed-off-by: Jacob Weinstock --- ihttp/ihttp.go | 18 ++---------------- itftp/itftp.go | 4 ++-- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/ihttp/ihttp.go b/ihttp/ihttp.go index 0e36e97..41c5db2 100644 --- a/ihttp/ihttp.go +++ b/ihttp/ihttp.go @@ -10,7 +10,6 @@ import ( "path/filepath" "regexp" "strings" - "unicode/utf8" "github.com/go-logr/logr" "github.com/tinkerbell/ipxedust/binary" @@ -50,19 +49,6 @@ func Serve(_ context.Context, conn net.Listener, h *http.Server) error { return h.Serve(conn) } -func parseMac(urlPath string) net.HardwareAddr { - var name string - if p := path.Dir(urlPath); strings.HasPrefix(p, "/") { - _, i := utf8.DecodeRuneInString(p) - name = p[i:] - } - mac, err := net.ParseMAC(name) - if err != nil { - return nil - } - return mac -} - // Handle handles responses to HTTP requests. func (s Handler) Handle(w http.ResponseWriter, req *http.Request) { s.Log.V(1).Info("handling request", "method", req.Method, "path", req.URL.Path) @@ -73,8 +59,8 @@ func (s Handler) Handle(w http.ResponseWriter, req *http.Request) { host, port, _ := net.SplitHostPort(req.RemoteAddr) log := s.Log.WithValues("host", host, "port", port) // If a mac address is provided, log it. Mac address is optional. - mac := parseMac(req.URL.Path) - log = log.WithValues("mac", mac.String()) + mac, _ := net.ParseMAC(strings.TrimPrefix(path.Dir(req.URL.Path), "/")) + log = log.WithValues("macFromURI", mac.String()) filename := filepath.Base(req.URL.Path) log = log.WithValues("filename", filename) diff --git a/itftp/itftp.go b/itftp/itftp.go index 9e00a2f..89c1116 100644 --- a/itftp/itftp.go +++ b/itftp/itftp.go @@ -70,7 +70,7 @@ func (t Handler) HandleRead(filename string, rf io.ReaderFrom) error { } // If a mac address is provided, log it. Mac address is optional. mac, _ := net.ParseMAC(path.Dir(full)) - log = log.WithValues("mac", mac.String()) + log = log.WithValues("macFromURI", mac.String()) tracer := otel.Tracer("TFTP") _, span := tracer.Start(ctx, "TFTP get", @@ -86,7 +86,7 @@ func (t Handler) HandleRead(filename string, rf io.ReaderFrom) error { content, ok := binary.Files[filepath.Base(shortfile)] if !ok { - err := fmt.Errorf("file unknown: %w", os.ErrNotExist) + err := fmt.Errorf("file [%v] unknown: %w", filepath.Base(shortfile), os.ErrNotExist) log.Error(err, "file unknown") return err } From 67de65bbf47ce7ba3c84da26a5b5f729a4811861 Mon Sep 17 00:00:00 2001 From: Jacob Weinstock Date: Thu, 6 Jan 2022 09:13:38 -0700 Subject: [PATCH 8/8] add comments around mac address in URI being optional. Signed-off-by: Jacob Weinstock --- ihttp/ihttp.go | 9 +++++---- itftp/itftp.go | 9 +++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/ihttp/ihttp.go b/ihttp/ihttp.go index 41c5db2..47f4039 100644 --- a/ihttp/ihttp.go +++ b/ihttp/ihttp.go @@ -58,9 +58,10 @@ func (s Handler) Handle(w http.ResponseWriter, req *http.Request) { } host, port, _ := net.SplitHostPort(req.RemoteAddr) log := s.Log.WithValues("host", host, "port", port) - // If a mac address is provided, log it. Mac address is optional. - mac, _ := net.ParseMAC(strings.TrimPrefix(path.Dir(req.URL.Path), "/")) - log = log.WithValues("macFromURI", mac.String()) + // If a mac address is provided (/0a:00:27:00:00:02/snp.efi), parse and log it. + // Mac address is optional. + optionalMac, _ := net.ParseMAC(strings.TrimPrefix(path.Dir(req.URL.Path), "/")) + log = log.WithValues("macFromURI", optionalMac.String()) filename := filepath.Base(req.URL.Path) log = log.WithValues("filename", filename) @@ -83,7 +84,7 @@ func (s Handler) Handle(w http.ResponseWriter, req *http.Request) { trace.WithAttributes(attribute.String("filename", filename)), trace.WithAttributes(attribute.String("requested-filename", longfile)), trace.WithAttributes(attribute.String("ip", host)), - trace.WithAttributes(attribute.String("mac", mac.String())), + trace.WithAttributes(attribute.String("mac", optionalMac.String())), ) span.SetStatus(codes.Ok, filename) diff --git a/itftp/itftp.go b/itftp/itftp.go index 89c1116..19a7f78 100644 --- a/itftp/itftp.go +++ b/itftp/itftp.go @@ -68,9 +68,10 @@ func (t Handler) HandleRead(filename string, rf io.ReaderFrom) error { log.Info("traceparent found in filename", "filenameWithTraceparent", longfile) filename = shortfile } - // If a mac address is provided, log it. Mac address is optional. - mac, _ := net.ParseMAC(path.Dir(full)) - log = log.WithValues("macFromURI", mac.String()) + // If a mac address is provided (0a:00:27:00:00:02/snp.efi), parse and log it. + // Mac address is optional. + optionalMac, _ := net.ParseMAC(path.Dir(full)) + log = log.WithValues("macFromURI", optionalMac.String()) tracer := otel.Tracer("TFTP") _, span := tracer.Start(ctx, "TFTP get", @@ -78,7 +79,7 @@ func (t Handler) HandleRead(filename string, rf io.ReaderFrom) error { trace.WithAttributes(attribute.String("filename", filename)), trace.WithAttributes(attribute.String("requested-filename", longfile)), trace.WithAttributes(attribute.String("ip", client.IP.String())), - trace.WithAttributes(attribute.String("mac", mac.String())), + trace.WithAttributes(attribute.String("mac", optionalMac.String())), ) span.SetStatus(codes.Ok, filename)