Improve GetTimestamp parsing #35402

thaJeztah · 2017-11-03T16:43:57Z

GetTimestamp() "assumed" values it could not parse to be a valid unix timestamp, and would use invalid values ("hello world") as-is (even testing that it did so).

This patch validates unix timestamp to be a valid numeric value, and makes other values invalid.

I noticed this while reviewing #32914 (comment)

- Description for the changelog

dnephin

LGTM

minor nits on tests

dnephin · 2017-11-03T16:56:41Z

api/types/time/timestamp_test.go

@@ -49,8 +49,7 @@ func TestGetTimestamp(t *testing.T) {
 		{"1.5h", fmt.Sprintf("%d", now.Add(-90*time.Minute).Unix()), false},
 		{"1h30m", fmt.Sprintf("%d", now.Add(-90*time.Minute).Unix()), false},

-		// String fallback
-		{"invalid", "invalid", false},
+		{"invalid", "", true},


expectedErr should probably the a string with the expected error instead of just a boolean, but this is existing so doesn't need to be fixed in this PR.

dnephin · 2017-11-03T16:57:33Z

client/container_logs_test.go

-		if err != nil {
-			t.Fatal(err)
+		if logCase.expectedError != "" {
+			require.EqualError(t, err, logCase.expectedError)


Should probably be assert.EqualError() in this case so it runs the other cases even if one case fails.

yes, I can update that; I was trying to keep the existing behaviour, but agree updating would make sense.

thaJeztah · 2017-11-03T22:13:53Z

Looks like I missed updating one test;

17:01:03 2017/11/03 17:01:03 RoundTripper returned a response & error; ignoring response
17:01:03 --- FAIL: TestServiceLogs (0.00s)
17:01:03 	service_logs_test.go:106: failed to parse value as time or duration: invalid but valid
17:01:03 FAIL

thaJeztah · 2017-11-03T23:57:42Z

client/service_logs_test.go

 			},
 		},
+		{
+			options: types.ContainerLogsOptions{


Synced the client/service_logs_... files with client/container_logs_.. ones.

Looking at this type, I wonder if we should create a types.ServiceLogsOptions, because service-logs and container-logs are now different in functionality (can possibly addressed, but there's no guarantee that they always have feature parity)

@dnephin wdyt?

cpuguy83 · 2017-11-04T12:59:59Z

What are the point of these functions? Reading the doc strings doesn't seem to indicate much.
Why aren't we using the various parsing functions in the time package?

cpuguy83 · 2017-11-04T13:01:32Z

I mean, I can't believe the time package doesn't handle all the cases we need.

thaJeztah · 2017-11-04T13:05:44Z

I can do some digging to see what values dont work for the time package, otherwise just wrap those errors directly.

Which reminds me; I have to check the API status code that's returned for invalid values

thaJeztah · 2017-12-01T22:42:37Z

@cpuguy83 @dnephin PTAL

I looked if Go's time package could parse this automatically, but looks I think we really need the pre-processing steps to determin what format is provided (which is what this function does) 😞 .

I did make one change, and instead of parsing the "%d.%09d" timestamps using strconf.ParseFloat(), I'm now using the parsing from ParseTimestamps(). For that, I extracted the actual parsing from ParseTimestamps() into a new parseTimestamp() function;

diff --git a/api/types/time/timestamp.go b/api/types/time/timestamp.go
index f96b562a7..f97f04e12 100644
--- a/api/types/time/timestamp.go
+++ b/api/types/time/timestamp.go
@@ -86,8 +86,8 @@ func GetTimestamp(value string, reference time.Time) (string, error) {
                if strings.Contains(value, "-") {
                        return "", err // was probably an RFC3339 like timestamp but the parser failed with an error
                }
-               if _, err := strconv.ParseFloat(value, 64); err != nil {
-                       return "", fmt.Errorf("failed to parse value as time or duration: %s", value)
+               if _, _, err := parseTimestamp(value); err != nil {
+                       return "", fmt.Errorf("failed to parse value as time or duration: %q", value)
                }
                return value, nil // unix timestamp in and out case (meaning: the value passed at the command line is already in the right format for passing to the server)
        }
@@ -107,6 +107,10 @@ func ParseTimestamps(value string, def int64) (int64, int64, error) {
        if value == "" {
                return def, 0, nil
        }
+       return parseTimestamp(value)
+}
+
+func parseTimestamp(value string) (int64, int64, error) {
        sa := strings.SplitN(value, ".", 2)
        s, err := strconv.ParseInt(sa[0], 10, 64)
        if err != nil {
diff --git a/api/types/time/timestamp_test.go b/api/types/time/timestamp_test.go
index ca49eb9cb..10241ec40 100644
--- a/api/types/time/timestamp_test.go
+++ b/api/types/time/timestamp_test.go
@@ -50,6 +50,7 @@ func TestGetTimestamp(t *testing.T) {
                {"1h30m", fmt.Sprintf("%d", now.Add(-90*time.Minute).Unix()), false},
 
                {"invalid", "", true},
+               {"", "", true},
        }
 
        for _, c := range cases {

vdemeester

LGTM 🐸

vdemeester · 2018-01-19T19:34:26Z

client/service_logs.go

@@ -26,7 +27,7 @@ func (cli *Client) ServiceLogs(ctx context.Context, serviceID string, options ty
 	if options.Since != "" {
 		ts, err := timetypes.GetTimestamp(options.Since, time.Now())
 		if err != nil {
-			return nil, err
+			return nil, errors.Wrap(err, "invalid value for \"since\"")


nit:

`invalid value for "since"`

Updated 👍

thaJeztah · 2018-01-19T20:05:24Z

oh, looks like there's a test I need to update as well; I'll update later

thaJeztah · 2018-01-20T18:39:39Z

humph

17:53:07 ----------------------------------------------------------------------
17:53:07 FAIL: docker_cli_plugins_test.go:386: DockerTrustSuite.TestPluginUntrustedInstall
17:53:07 
17:53:07 docker_cli_plugins_test.go:391:
17:53:07     // install locally and push to private registry
17:53:07     cli.DockerCmd(c, "plugin", "install", "--grant-all-permissions", "--alias", pluginName, pNameWithTag)
17:53:07 /go/src/github.com/docker/docker/vendor/github.com/gotestyourself/gotestyourself/icmd/command.go:65:
17:53:07     t.Fatalf(err.Error() + "\n")
17:53:07 ... Error: 
17:53:07 Command:  /usr/local/bin/docker plugin install --grant-all-permissions --alias 127.0.0.1:5000/dockercliuntrusted/plugintest:latest tiborvass/sample-volume-plugin:latest
17:53:07 ExitCode: 1
17:53:07 Error:    exit status 1
17:53:07 Stdout:   
17:53:07 Stderr:   Error response from daemon: Get https://registry-1.docker.io/v2/: net/http: TLS handshake timeout
17:53:07 
17:53:07 
17:53:07 Failures:
17:53:07 ExitCode was 1 expected 0
17:53:07 Expected no error
17:53:07

restarting Janky

`GetTimestamp()` "assumed" values it could not parse to be a valid unix timestamp, and would use invalid values ("hello world") as-is (even testing that it did so). This patch validates unix timestamp to be a valid numeric value, and makes other values invalid. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2018-05-20T11:11:34Z

Rebased (now using gotestyourself); @cpuguy83 @vdemeester this ok to merge?

cpuguy83

LGTM

codecov · 2018-05-22T10:23:01Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f9dd74d). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #35402   +/-   ##
=========================================
  Coverage          ?   35.25%           
=========================================
  Files             ?      615           
  Lines             ?    46223           
  Branches          ?        0           
=========================================
  Hits              ?    16297           
  Misses            ?    27786           
  Partials          ?     2140

thaJeztah requested a review from dnephin as a code owner November 3, 2017 16:43

GordonTheTurtle added the status/0-triage label Nov 3, 2017

dnephin approved these changes Nov 3, 2017

View reviewed changes

thaJeztah force-pushed the fix-GetTimestamp-parsing branch from 87214c4 to f7edecd Compare November 3, 2017 23:42

thaJeztah added impact/api status/2-code-review and removed status/0-triage labels Nov 3, 2017

thaJeztah commented Nov 3, 2017

View reviewed changes

thaJeztah force-pushed the fix-GetTimestamp-parsing branch from f7edecd to 40a7bf4 Compare November 7, 2017 15:08

GordonTheTurtle assigned vdemeester Nov 18, 2017

thaJeztah force-pushed the fix-GetTimestamp-parsing branch 2 times, most recently from d191e77 to 5ddcbec Compare December 1, 2017 22:39

vdemeester approved these changes Jan 19, 2018

View reviewed changes

thaJeztah force-pushed the fix-GetTimestamp-parsing branch 2 times, most recently from 318ac34 to eecddb8 Compare January 20, 2018 16:21

thaJeztah added the rebuild/janky label Jan 20, 2018

GordonTheTurtle removed the rebuild/janky label Jan 20, 2018

thaJeztah force-pushed the fix-GetTimestamp-parsing branch from eecddb8 to c8ea1b5 Compare January 20, 2018 18:48

thaJeztah force-pushed the fix-GetTimestamp-parsing branch from c8ea1b5 to 48cfe3f Compare May 20, 2018 11:09

cpuguy83 approved these changes May 21, 2018

View reviewed changes

thaJeztah added the status/4-merge label May 21, 2018

thaJeztah added rebuild/* and removed status/2-code-review labels May 21, 2018

GordonTheTurtle removed the rebuild/* label May 21, 2018

vdemeester added the rebuild/powerpc label May 22, 2018

GordonTheTurtle removed the rebuild/powerpc label May 22, 2018

vdemeester merged commit da99009 into moby:master May 22, 2018

thaJeztah deleted the fix-GetTimestamp-parsing branch May 22, 2018 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve GetTimestamp parsing #35402

Improve GetTimestamp parsing #35402

thaJeztah commented Nov 3, 2017

dnephin left a comment

dnephin Nov 3, 2017

dnephin Nov 3, 2017

thaJeztah Nov 3, 2017

thaJeztah commented Nov 3, 2017

thaJeztah Nov 3, 2017

cpuguy83 commented Nov 4, 2017

cpuguy83 commented Nov 4, 2017

thaJeztah commented Nov 4, 2017

thaJeztah commented Dec 1, 2017

vdemeester left a comment

vdemeester Jan 19, 2018

thaJeztah Jan 20, 2018

thaJeztah commented Jan 19, 2018

thaJeztah commented Jan 20, 2018

thaJeztah commented May 20, 2018

cpuguy83 left a comment

codecov bot commented May 22, 2018

Improve GetTimestamp parsing #35402

Improve GetTimestamp parsing #35402

Conversation

thaJeztah commented Nov 3, 2017

dnephin left a comment

Choose a reason for hiding this comment

dnephin Nov 3, 2017

Choose a reason for hiding this comment

dnephin Nov 3, 2017

Choose a reason for hiding this comment

thaJeztah Nov 3, 2017

Choose a reason for hiding this comment

thaJeztah commented Nov 3, 2017

thaJeztah Nov 3, 2017

Choose a reason for hiding this comment

cpuguy83 commented Nov 4, 2017

cpuguy83 commented Nov 4, 2017

thaJeztah commented Nov 4, 2017

thaJeztah commented Dec 1, 2017

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester Jan 19, 2018

Choose a reason for hiding this comment

thaJeztah Jan 20, 2018

Choose a reason for hiding this comment

thaJeztah commented Jan 19, 2018

thaJeztah commented Jan 20, 2018

thaJeztah commented May 20, 2018

cpuguy83 left a comment

Choose a reason for hiding this comment

codecov bot commented May 22, 2018

Codecov Report