Added URL support for --from-file command line flag #726

tkeech1 · 2018-05-01T02:28:43Z

Issue Ref: Issue 705

Description:

This PR adds support for URLs in the --from-file command line flag which allows the user to specify a a raw code file or zip file when calling function deploy or function update. For example:

function deploy foo --runtime python2.7 --from-file https://raw.githubusercontent.com/<USER>/<REPO>/<BRANCH>/<FILE> --handler test.foobar

Thanks.

TODOs:

Ready to review
Automated Tests
Docs

andresmgot

Hi @tkeech1, thank you for sending a PR for this!

I would suggest a different approach here. Instead of downloading the file with the CLI and setting the function content in the field Function it would be better to:

Set the content type to url.
Set the function content to the URL given by the user.
In the provision container (see the method getProvisionContainer in k8sutil.go), if the content type is an url, download the file with curl and decode or unzip it if the content type isurl+base64 or url+zip.

That way we avoid the limitation of 1MB that the CRD object has and we fix as well #655.

Another couple of small suggestions:

You can still download the file in the CLI as well to obtain the sha256.
We don't need to support URLs pointing to a Zip file from the CLI in the first PR, we can assume that --from-url will point to a plain text file for the moment.

Let us know if you need any help with that.

andresmgot · 2018-05-01T10:24:32Z

cmd/kubeless/function/deploy.go

+		if err != nil {
+			logrus.Fatal(err)
+		}
+
 		file, err := cmd.Flags().GetString("from-file")


we would need to throw an error if --from-url and --from-file are both set

did you miss this comment? (not sure if it has been answered elsewhere). Actually, thinking about it, it would be better to support that the flag --from-file supports to specify an URL. That is more similar to kubectl that allows you to do kubectl create -f https://... we can parse the path and discover if it is an URL.

andresmgot · 2018-05-01T10:28:42Z

cmd/kubeless/function/function.go

@@ -120,6 +121,16 @@ func getFileSha256(file string) (string, error) {
 	return "sha256:" + checksum, err
 }

+func getHTTPSha256(bytes []byte) (string, error) {


this can be called getSha256, there is nothing specific about HTTP here

tkeech1 · 2018-05-02T00:15:23Z

Hi @andresmgot,
Thanks for the feedback. I'll try to make the changes you've suggested.

…Container

gkarthiks · 2018-05-07T05:04:13Z

Hi @tkeech1 , I have one more question. If the URL is say the gitlab or github URL, which needs an authentication, how are we handling the authentication?

andresmgot

Great! thank you for applying the changes. I see that the CI is working now. Can you add an integration test to check that the feature is working in a real environment?

For doing so you need to add, in the examples/Makefile, a couple of tasks:

get-python-url:
    kubeless function deploy get-python --runtime python2.7 --handler helloget.foo --from-file https://raw.githubusercontent.com/kubeless/kubeless/v1.0.0-alpha.1/examples/python/helloget.py

get-python-url-verify:
    kubeless function call get-python |egrep hello.world

And then enable the test in the bats file tests/integration-tests.bats

@test "Deploy functions to evaluate" {
  deploy_function get-python
  ...
+ deploy_function get-python-url
  ...
}
...
+@test "Test function: get-python-url" {
+  verify_function get-python-url
+  kubeless_function_delete get-python-url
+}

andresmgot · 2018-05-07T07:29:22Z

pkg/utils/k8sutil.go

@@ -433,6 +433,10 @@ func getProvisionContainer(function, checksum, fileName, handler, contentType, r
 		decodedFile := "/tmp/func.decoded"
 		prepareCommand = appendToCommand(prepareCommand, fmt.Sprintf("base64 -d < %s > %s", originFile, decodedFile))
 		originFile = decodedFile
+	} else if strings.Contains(contentType, "url") {
+		fromURLFile := "/tmp/func.fromurl"
+		prepareCommand = appendToCommand(prepareCommand, fmt.Sprintf("curl %s --silent --output %s", function, fromURLFile))


Lets add -L to follow redirects

andresmgot · 2018-05-07T07:31:10Z

cmd/kubeless/function/deploy.go

+		if err != nil {
+			logrus.Fatal(err)
+		}
+
 		file, err := cmd.Flags().GetString("from-file")


did you miss this comment? (not sure if it has been answered elsewhere). Actually, thinking about it, it would be better to support that the flag --from-file supports to specify an URL. That is more similar to kubectl that allows you to do kubectl create -f https://... we can parse the path and discover if it is an URL.

andresmgot · 2018-05-07T07:38:13Z

pkg/utils/k8sutil_test.go

+	if !strings.HasPrefix(c.Args[0], "curl https://raw.githubusercontent.com/test/test/test/test.py --silent --output /tmp/func.fromurl") {
+		t.Errorf("Unexpected command: %s", c.Args[0])
+	}
+


can you add another unit test that checks that if the type is url+zip it downloads and unzip the file?

Hi @andresmgot,
Can you add a .zip file that can be used for integration testing to examples/nodejs/? My local integration tests use https://github.com/tkeech1/kubelessfunction/blob/master/nodejs/helloFunctions.zip but I would like to update the tests to download the zip file from the kubeless repo to avoid creating a dependency on my repo. Thanks.

It's okay to have it there for the moment, just upload the file with this PR. We can modify the URL after this PR gets merged.

andresmgot · 2018-05-07T07:53:56Z

Hi @tkeech1 , I have one more question. If the URL is say the gitlab or github URL, which needs an authentication, how are we handling the authentication?

Let's track that in a different Issue/PR.

Also, we need to support URLs for the flag --dependencies as well. It would be weird to allow remote files for the function code but no for its dependencies.

tkeech1 · 2018-05-08T03:21:30Z

Hi @andresmgot,
I like the idea of supporting URLs directly in the --from-file flag. I'll also add URL support for --dependencies and include the other changes you mentioned. Thanks.

* pv for kafka * Update README.md Fix some typos and links

* <script/binary>: print more information when building * Update binary Fix typo

…#742)

* Add response object to function event * Add docs

* CircleCI integration * Fix builder image ref * Remove debug code * Minor review * Store artifacts

…Container

andresmgot

This is great, thank you for working on this! I just made a few minor comments now.

It also seems that you need to update the unit tests in function_test.go. Once you fix that and apply the changes we can merge this PR.

Also can you update the docs at docs/advanced-function-deployment.md? There is a small description of the supported content types.

andresmgot · 2018-05-15T08:11:00Z

cmd/kubeless/function/function.go

+			}
+			function.Spec.FunctionContentType = "url"
+			// strip any query params to determine if the file is a zip
+			if path.Ext(strings.Split(functionURL.String(), "?")[0]) == ".zip" {


I would move the check if strings.Index(file, "http://") and if path.Ext(strings.Split(functionURL.String(), "?")[0]) == ".zip" to the function getContentType of the function.go file so the same function always returns the full contentType

andresmgot · 2018-05-15T08:17:22Z

cmd/kubeless/function/function.go

+			} else {
+				function.Spec.Function = base64.StdEncoding.EncodeToString(functionBytes)
+			}
+			function.Spec.Checksum, err = getFileSha256(file)


I would create another function here that given a content type it returns the content and its checksum:

func parseContent(content, type) (string, string, error)

that function would read a file in case the type is text do the base64 in case it is a zip or download the file in case it is an URL. That way you can reuse it for the dependencies file.

andresmgot · 2018-05-15T08:21:08Z

pkg/utils/k8sutil_test.go

+	if !strings.HasPrefix(c.Args[0], "curl https://raw.githubusercontent.com/test/test/test/test.py --silent --output /tmp/func.fromurl") {
+		t.Errorf("Unexpected command: %s", c.Args[0])
+	}
+


It's okay to have it there for the moment, just upload the file with this PR. We can modify the URL after this PR gets merged.

andresmgot · 2018-05-15T08:23:58Z

examples/Makefile

+get-python-url-deps:
+	kubeless function deploy get-python-url-deps --runtime python2.7 --handler helloget.foo --from-file https://raw.githubusercontent.com/kubeless/kubeless/v1.0.0-alpha.1/examples/python/hellowithdeps.py --dependencies https://raw.githubusercontent.com/kubeless/kubeless/v1.0.0-alpha.1/examples/python/requirements.txt
+
+get-python-url-deps-verify:


This is very complete but these kind of tests take a long time to be executed in the CI. Can you merge get-python-url, get-python-url-filedep and get-python-url-deps in just one test? Basically having just get-python-url-deps since that one covers the previous two.

andresmgot · 2018-05-16T08:22:15Z

Thank you for addressing all the comments! We can merge this now.

tkeech1 added 3 commits April 30, 2018 21:54

Added --from-url command line flag

cce20f2

modified method name to getHTTPSha256

887de6e

modified method name to getHTTPSha256 in function call

0f1fb97

andresmgot suggested changes May 1, 2018

View reviewed changes

knightXun and others added 4 commits May 4, 2018 14:57

change some terms in architecture.md (#728)

13da68b

modified PR 726 to use curl to retrieve function code in getProvision…

c6e5068

…Container

code formatting changes to pass build

e9ef9fb

fix cmd/kubeless/autoscale some typo (#729)

8d1966c

andresmgot reviewed May 7, 2018

View reviewed changes

pv for kafka (#735)

d9c23be

* pv for kafka * Update README.md Fix some typos and links

andresmgot mentioned this pull request May 8, 2018

Utils refactor #734

Merged

2 tasks

Andres and others added 14 commits May 8, 2018 15:05

Point to latest release in quick-start guide

147424d

<script/binary>: print more information when building (#739)

082209d

* <script/binary>: print more information when building * Update binary Fix typo

<cmd/function>: make user be able to choose image-pull-policy (#741)

e2e9bcf

<cmd/kubeless/logs>: change some typo and judge all containers in pod (…

8e0e006

…#742)

Add response object to function event (#745)

8b266a2

* Add response object to function event * Add docs

CircleCI integration (#743)

1d2f73a

* CircleCI integration * Fix builder image ref * Remove debug code * Minor review * Store artifacts

Add ca-certificates to Golang runtime image (#749)

8f011e4

modified --from-file and --dependencies to support URLs

8dd171d

Added --from-url command line flag

dbe4a1e

modified method name to getHTTPSha256

9d3da2e

modified method name to getHTTPSha256 in function call

44ea323

modified PR 726 to use curl to retrieve function code in getProvision…

fbed818

…Container

code formatting changes to pass build

28a0cfc

modified --from-file and --dependencies to support URLs

d9be1d2

andresmgot mentioned this pull request May 15, 2018

1mb etcd limit #655

Closed

andresmgot approved these changes May 15, 2018

View reviewed changes

tkeech1 added 2 commits May 16, 2018 00:02

created parseContent function

541f66a

created parseContent function

9950f42

andresmgot merged commit a650c4c into vmware-archive:master May 16, 2018

andresmgot changed the title ~~Added --from-url command line flag~~ Added URL support for --from-file command line flag May 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added URL support for --from-file command line flag #726

Added URL support for --from-file command line flag #726

tkeech1 commented May 1, 2018 •

edited by andresmgot

andresmgot left a comment •

edited

andresmgot May 1, 2018

andresmgot May 7, 2018

andresmgot May 1, 2018

tkeech1 commented May 2, 2018

gkarthiks commented May 7, 2018

andresmgot left a comment •

edited

andresmgot May 7, 2018

andresmgot May 7, 2018

andresmgot May 7, 2018

tkeech1 May 13, 2018

andresmgot May 15, 2018

andresmgot commented May 7, 2018

tkeech1 commented May 8, 2018

andresmgot left a comment

andresmgot May 15, 2018

andresmgot May 15, 2018

andresmgot May 15, 2018

andresmgot May 15, 2018

andresmgot commented May 16, 2018

Added URL support for --from-file command line flag #726

Added URL support for --from-file command line flag #726

Conversation

tkeech1 commented May 1, 2018 • edited by andresmgot

andresmgot left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkeech1 commented May 2, 2018

gkarthiks commented May 7, 2018

andresmgot left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot commented May 7, 2018

tkeech1 commented May 8, 2018

andresmgot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot commented May 16, 2018

tkeech1 commented May 1, 2018 •

edited by andresmgot

andresmgot left a comment •

edited

andresmgot left a comment •

edited