Repository: virtual-kubelet/virtual-kubelet
Pull request #806 (2020-01-20 21:51:28)

Add support for v1.PodLogOptions
Check if "follow" is requested for container logs, and pass it to the
provider so tailing logs can be supported.

cpuguy83: From what I can tell in the kubelet, there is no supported query param for `follow`.
https://github.com/kubernetes/kubernetes/blob/15c3f1b119ff4b4073c86df05c819a9672b80d68/pkg/kubelet/server/server.go#L542-L583
ldx: There is: https://github.com/kubernetes/kubernetes/blob/15c3f1b119ff4b4073c86df05c819a9672b80d68/staging/src/k8s.io/api/core/v1/types.go#L4808

If you use `kubectl logs -f`, `follow=true` will be present in the API request query.

cpuguy83: Right so we need to decode the log options from the request.
ldx: > Right so we need to decode the log options from the request.

Yes, the same way as with `tailLines`.

Thinking a bit more about this: we could even add support for all fields in `v1.PodLogOptions` here in one go. Let me know, and I can refactor this PR to do that.
cpuguy83: Yes let's support v1.PodLogOptions and convert that to our log config.

Thank you!
ldx: @cpuguy83 I refactored this to include all fields from v1.PodLogOptions.
ldx: @cpuguy83 ping - anything else I can do?

Patch:
diff --git a/node/api/logs.go b/node/api/logs.go
index c45793d4..9602dc72 100644
--- a/node/api/logs.go
+++ b/node/api/logs.go
@@ -18,6 +18,7 @@ import (
 	"context"
 	"io"
 	"net/http"
+	"net/url"
 	"strconv"
 	"time"
 
@@ -33,10 +34,71 @@ type ContainerLogsHandlerFunc func(ctx context.Context, namespace, podName, cont
 // ContainerLogOpts are used to pass along options to be set on the container
 // log stream.
 type ContainerLogOpts struct {
-	Tail       int
-	Since      time.Duration
-	LimitBytes int
-	Timestamps bool
+	Tail         int
+	LimitBytes   int
+	Timestamps   bool
+	Follow       bool
+	Previous     bool
+	SinceSeconds int
+	SinceTime    time.Time
+}
+
+func parseLogOptions(q url.Values) (opts ContainerLogOpts, err error) {
+	if tailLines := q.Get("tailLines"); tailLines != "" {
+		opts.Tail, err = strconv.Atoi(tailLines)
+		if err != nil {
+			return opts, errdefs.AsInvalidInput(errors.Wrap(err, "could not parse \"tailLines\""))
+		}
+		if opts.Tail < 0 {
+			return opts, errdefs.InvalidInputf("\"tailLines\" is %d", opts.Tail)
+		}
+	}
+	if follow := q.Get("follow"); follow != "" {
+		opts.Follow, err = strconv.ParseBool(follow)
+		if err != nil {
+			return opts, errdefs.AsInvalidInput(errors.Wrap(err, "could not parse \"follow\""))
+		}
+	}
+	if limitBytes := q.Get("limitBytes"); limitBytes != "" {
+		opts.LimitBytes, err = strconv.Atoi(limitBytes)
+		if err != nil {
+			return opts, errdefs.AsInvalidInput(errors.Wrap(err, "could not parse \"limitBytes\""))
+		}
+		if opts.LimitBytes < 1 {
+			return opts, errdefs.InvalidInputf("\"limitBytes\" is %d", opts.LimitBytes)
+		}
+	}
+	if previous := q.Get("previous"); previous != "" {
+		opts.Previous, err = strconv.ParseBool(previous)
+		if err != nil {
+			return opts, errdefs.AsInvalidInput(errors.Wrap(err, "could not parse \"previous\""))
+		}
+	}
+	if sinceSeconds := q.Get("sinceSeconds"); sinceSeconds != "" {
+		opts.SinceSeconds, err = strconv.Atoi(sinceSeconds)
+		if err != nil {
+			return opts, errdefs.AsInvalidInput(errors.Wrap(err, "could not parse \"sinceSeconds\""))
+		}
+		if opts.SinceSeconds < 1 {
+			return opts, errdefs.InvalidInputf("\"sinceSeconds\" is %d", opts.SinceSeconds)
+		}
+	}
+	if sinceTime := q.Get("sinceTime"); sinceTime != "" {
+		opts.SinceTime, err = time.Parse(time.RFC3339, sinceTime)
+		if err != nil {
+			return opts, errdefs.AsInvalidInput(errors.Wrap(err, "could not parse \"sinceTime\""))
+		}
+		if opts.SinceSeconds > 0 {
+			return opts, errdefs.InvalidInput("both \"sinceSeconds\" and \"sinceTime\" are set")
+		}
+	}
+	if timestamps := q.Get("timestamps"); timestamps != "" {
+		opts.Timestamps, err = strconv.ParseBool(timestamps)
+		if err != nil {
+			return opts, errdefs.AsInvalidInput(errors.Wrap(err, "could not parse \"timestamps\""))
+		}
+	}
+	return opts, nil
 }
 
 // HandleContainerLogs creates an http handler function from a provider to serve logs from a pod
@@ -55,22 +117,11 @@ func HandleContainerLogs(h ContainerLogsHandlerFunc) http.HandlerFunc {
 		namespace := vars["namespace"]
 		pod := vars["pod"]
 		container := vars["container"]
-		tail := 10
-		q := req.URL.Query()
 
-		if queryTail := q.Get("tailLines"); queryTail != "" {
-			t, err := strconv.Atoi(queryTail)
-			if err != nil {
-				return errdefs.AsInvalidInput(errors.Wrap(err, "could not parse \"tailLines\""))
-			}
-			tail = t
-		}
-
-		// TODO(@cpuguy83): support v1.PodLogOptions
-		// The kubelet decoding here is not straight forward, so this needs to be disected
-
-		opts := ContainerLogOpts{
-			Tail: tail,
+		query := req.URL.Query()
+		opts, err := parseLogOptions(query)
+		if err != nil {
+			return err
 		}
 
 		logs, err := h(ctx, namespace, pod, container, opts)


Test patch:
diff --git a/node/api/logs_test.go b/node/api/logs_test.go
new file mode 100644
index 00000000..99f2ab7c
--- /dev/null
+++ b/node/api/logs_test.go
@@ -0,0 +1,99 @@
+package api
+
+import (
+	"fmt"
+	"net/url"
+	"testing"
+	"time"
+
+	"gotest.tools/assert"
+	is "gotest.tools/assert/cmp"
+)
+
+//func parseLogOptions(q url.Values) (opts ContainerLogOpts, err error)
+func TestParseLogOptions(t *testing.T) {
+	//tailLines
+	//follow
+	//limitBytes
+	//previous
+	//sinceSeconds
+	//sinceTime
+	//timestamps
+	sinceTime, _ := time.Parse(time.RFC3339, "2020-03-20T21:07:34Z")
+	fmt.Printf("%+v\n", sinceTime)
+	testCases := []struct {
+		Values  url.Values
+		Failure bool
+		Result  ContainerLogOpts
+	}{
+		{
+			Values:  url.Values{},
+			Failure: false,
+			Result:  ContainerLogOpts{},
+		},
+		{
+			Values: url.Values{
+				"follow":       {"true"},
+				"limitBytes":   {"123"},
+				"previous":     {"true"},
+				"sinceSeconds": {"10"},
+				"tailLines":    {"99"},
+				"timestamps":   {"true"},
+			},
+			Failure: false,
+			Result: ContainerLogOpts{
+				Follow:       true,
+				LimitBytes:   123,
+				Previous:     true,
+				SinceSeconds: 10,
+				Tail:         99,
+				Timestamps:   true,
+			},
+		},
+		{
+			Values: url.Values{
+				"sinceSeconds": {"10"},
+				"sinceTime":    {"2020-03-20T21:07:34Z"},
+			},
+			Failure: true,
+		},
+		{
+			Values: url.Values{
+				"sinceTime": {"2020-03-20T21:07:34Z"},
+			},
+			Failure: false,
+			Result: ContainerLogOpts{
+				SinceTime: sinceTime,
+			},
+		},
+		{
+			Values: url.Values{
+				"tailLines": {"-1"},
+			},
+			Failure: true,
+		},
+		{
+			Values: url.Values{
+				"limitBytes": {"0"},
+			},
+			Failure: true,
+		},
+		{
+			Values: url.Values{
+				"sinceSeconds": {"-10"},
+			},
+			Failure: true,
+		},
+	}
+	// follow=true&limitBytes=1&previous=true&sinceSeconds=1&sinceTime=2020-03-20T21%3A07%3A34Z&tailLines=1&timestamps=true
+	for i, tc := range testCases {
+		msg := fmt.Sprintf("test case #%d %+v failed", i+1, tc)
+		result, err := parseLogOptions(tc.Values)
+		if tc.Failure {
+			assert.Check(t, is.ErrorContains(err, ""), msg)
+		} else {
+			assert.NilError(t, err, msg)
+			assert.Check(t, is.Equal(result, tc.Result), msg)
+		}
+	}
+}