Add go function invocation runtime as a package #1451

lance · 2022-11-30T21:26:43Z

Changes

This change takes the Golang function invocation framework from boson-project/packs and adds that code as a package to func itself. We should consider instead if it makes more sense to move this to https://github.com/knative/pkg.

🎁 Adds the Go function invocation framework from https://github.com/boson-project/packs/tree/main/buildpacks/go as a package at knative.dev/func/runtime
🎁 Updates the Go function templates to use the knative.dev/func/runtime package

Motivation

Doing this will remove some of the "magic" from how a Golang function works. It also solves a number of problems.

Eliminates the need to maintain a function-specific buildpack in https://github.com/boson-project/packs
Allows us to default to the paketo golang buildpack alone, and for users to use any go-capable buildpack they want
Enables on-cluster builds for Go functions, which is currently prevented by limitations in the Tekton task related to specifying additional buildpacks
Enables s2i builds for Go functions, as the framework is now simply a library dependency instead of being applied in the buildpack
Enables the function developer to run the function locally without requiring a container image and docker daemon
Makes the function invocation mechanism for Go functions more transparent/open for greater potential community input

Areas of concern

Requires a minimal main() to be included in the Go function templates (this PR adds faas/main.go)
Requires either go.sum in the Go function templates, or the user must generate it before an initial func build (this PR adds go.sum)
Adds gorilla/mux as a dependency to func (unless the package is moved to knative.dev/pkg/runtime)

TODO

Remove check preventing on-cluster builds for golang
Update go.mod replace directives if/when this PR lands (is there a better way to do this?)
Update template README.md docs to provide instructions on how to run the function locally

/kind enhancement

Fixes: #809
Related: #984
Related: #1445

Release notes

- Adds the Go function invocation framework from https://github.com/boson-project/packs/tree/main/buildpacks/go as a package at knative.dev/func/runtime
- Updates the Go function templates to use the knative.dev/func/runtime package

knative-prow · 2022-11-30T21:26:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [lance]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov · 2022-11-30T21:39:26Z

Codecov Report

Base: 63.00% // Head: 63.27% // Increases project coverage by +0.26% 🎉

Coverage data is based on head (b8b111b) compared to base (4fbabb0).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head b8b111b differs from pull request most recent head 3c706da. Consider uploading reports for the commit 3c706da to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1451      +/-   ##
==========================================
+ Coverage   63.00%   63.27%   +0.26%     
==========================================
  Files          74       75       +1     
  Lines       10783    10785       +2     
==========================================
+ Hits         6794     6824      +30     
+ Misses       3430     3404      -26     
+ Partials      559      557       -2

Flag	Coverage Δ
integration-tests	`53.03% <0.00%> (+0.02%)`	⬆️
unit-tests	`?`
unit-tests-macos-latest	`53.43% <100.00%> (?)`
unit-tests-ubuntu-latest	`54.83% <100.00%> (?)`
unit-tests-windows-latest	`53.45% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pipelines/tekton/validate.go	`90.00% <100.00%> (ø)`
knative/logs.go	`80.41% <0.00%> (-4.13%)`	⬇️
knative/deployer.go	`61.55% <0.00%> (-0.17%)`	⬇️
docker/docker_client_nonlinux.go	`0.00% <0.00%> (ø)`
client.go	`62.57% <0.00%> (+0.61%)`	⬆️
docker/creds/credentials.go	`73.06% <0.00%> (+1.34%)`	⬆️
docker/docker_client.go	`84.87% <0.00%> (+23.52%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lance · 2022-12-01T20:48:51Z

/cc @knative/func-reviewers @knative/func-writers @knative/functions-wg-leads

This change takes the Golang function invocation framework from boson-project/packs and adds that code as a package to func itself. We should consider instead if it makes more sense to move this to https://github.com/knative/pkg. 🎁 Adds the Go function runtime from https://github.com/boson-project/packs/tree/main/buildpacks/go 🎁 Updates templates to use the func package /kind enhancement Signed-off-by: Lance Ball <lball@redhat.com>

lance · 2022-12-02T04:38:33Z

Consider backporting to release-1.8.

lance · 2022-12-02T18:44:25Z

Note that currently, the s2i golang image does not support non-standard build targets. See: sclorg/golang-container#48

lance · 2022-12-02T19:10:57Z

This allows, for example, a user to use the Google builder with their Go functions. A working example with no changes to the code outside of this PR...

....
....
Adding cache layer 'google.go.gomod:gopath'
Layer 'google.go.gomod:gopath' SHA: sha256:3b524f2223d745a5a2027195fa62bfbfde642332a037b7268b751a728ddaa69b
Reading buildpack directory: /layers/google.go.build
Reading buildpack directory item: bin
Reading buildpack directory item: bin.toml
Reading buildpack directory item: gocache
Reading buildpack directory item: gocache.toml
Reading buildpack directory item: launch.toml
Reading buildpack directory: /layers/google.utils.label
🙌 Function image built: quay.io/lanceball/go-google:latest

~/demo/go-s2i via 🐹 v1.19.1 on ☁️  lanceball@gmail.com took 48s
❯ func run
Function already built.  Use --build to force a rebuild.
Function started on port 8080

Here is the same Go function being built and deployed on-cluster using the Google builder.

func deploy --remote --verbose
Creating Pipeline resources
Creating Pipeline resources
Running Pipeline with the Function
Running Pipeline: Building function image on the cluster
Running Pipeline: Deploying function to the cluster
Function deployed in namespace "default" and exposed at URL:
http://go-test.default.127.0.0.1.sslip.io

matejvasek · 2022-12-02T20:03:27Z

Note that currently, the s2i golang image does not support non-standard build targets

What would we need to do to make it work.
Wouldn't it suffice to put main pkg into root and move Handle() to another dir/pkg?

lance · 2022-12-02T20:46:22Z

Note that currently, the s2i golang image does not support non-standard build targets

What would we need to do to make it work. Wouldn't it suffice to put main pkg into root and move Handle() to another dir/pkg?

Yes that would fix it, but I really do like having main() out of the way like this. Greater sophistication on the part of the s2i builder image's build script would be nice. For example, the paketo builder accepts a BP_GO_TARGETS environment variable, and the Google builder image uses go list to figure out where main() lives. (See: https://github.com/GoogleCloudPlatform/buildpacks/blob/cb94a4f0db650902151419fc3a167cad7831b6b6/cmd/go/build/main.go#L132-L135).

lance · 2022-12-02T22:08:14Z

There has also been some talk of mapping knative-sandbox/func-go to http://knative.dev/runtime with an HTTP entry for go get and moving this invocation code to a new repo. That might make the chicken/egg problem in go.mod easier to deal with.

lkingland · 2022-12-06T18:44:31Z

Please do not merge this just yet. I think with some changes it will be quite a nice addition, but in this current form it's got a little bit more to add to be complete. I am so sorry, but I am out sick with covid, and can not provide much details. In short, this is a nice step in the right direction, but wrapping a function in a process boundary is part of the deployment proces. A function's source code should have no hard dependencies on ops infrastructure. Create a main wrapper from within func is good. But do this as a transient artifact of the build/deploy process. For illustration: what should the dependency tree be of a Function which says "hello world" to an HTTP request?

lance · 2022-12-06T20:05:38Z

I am proposing that we create a new repository https://github.com/knative-sandbox/func-go mapping to knative.dev/runtime. We can iterate on the runtime characteristics there, and this PR will then become a set of much smaller changes primarily to the golang templates. Let's see how that plays out this week.

See: knative/community#1220

lance · 2022-12-06T20:05:47Z

/hold

knative-prow-robot · 2022-12-10T07:34:14Z

@lance: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lkingland · 2022-12-12T02:45:23Z

I like that this pulls the Go function invocation code out of the buildpack, and how it is now nicely placed right here as a library. Also nice to see the simplification of the template manifest. What I think we need to discuss is at which stage in a function's lifecycle the function's source code is "wrapped" in the process boundary and placed inside a container. In particular, this is part of the build step. So instead of using the template system to do this during function creation, let's do something similar but on build of the container, and as a transient artifact in .func/build[N]

lance · 2022-12-12T18:00:57Z

instead of using the template system to do this during function creation, let's do something similar but on build of the container, and as a transient artifact in .func/build[N]

I like this suggestion, but I have two concerns.

I do think it's useful for a Go function developer to be able to run their function locally without requiring a container. All other runtimes allow for this. If we construct a main() during the build phase, we need to be sure to explicitly exclude any other main() that the user has defined.
It would be ideal if we could aim for allowing manually constructed pipelines which do not originate from a func deploy command, but are instead applied by the user in, for example, a CI/CD pipeline. If we are constructing the main() function during func deploy, how does this get replicated in a Pipeline that has been manually created?

zroubalik

A great improvement!

zroubalik · 2022-12-14T15:20:54Z

go.mod

@@ -53,7 +53,10 @@ require (
 	knative.dev/serving v0.35.1-0.20221114131921-874ccebb8063
 )

-require github.com/sabhiram/go-gitignore v0.0.0-20210923224102-525f6e181f06
+require (
+	github.com/gorilla/mux v1.8.0


https://github.com/gorilla#gorilla-toolkit

not sure if we should depend on this package

Yes I agree. For the time being, however, I think we can keep it as is until we come up with a suitable alternative.

@zroubalik also, note that I've moved this code over to https://github.com/knative-sandbox/func-go which will change this PR to just be modifications to the templates.

zroubalik · 2022-12-14T15:24:09Z

templates/go/cloudevents/faas/main.go

+	"knative.dev/func/runtime"
+)
+
+func main() {


We should add some comment here pointing users to handle.go. So they don't try to write the function code here.

zroubalik · 2022-12-14T15:24:31Z

templates/go/http/faas/main.go

+	"knative.dev/func/runtime"
+)
+
+func main() {


We should add some comment here pointing users to handle.go. So they don't try to write the function code here.

templates/go/manifest.yaml

lkingland · 2023-02-27T07:42:43Z

Since this is a bit bigger of a topic, here is a discussion we could use to explore

lance · 2023-02-27T20:11:59Z

Since this is a bit bigger of a topic, here is a discussion we could use to explore

Very much appreciate your effort to outline this in a discussion.

lance · 2023-03-21T13:42:00Z

Closing as obsolete

knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/enhancement labels Nov 30, 2022

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2022

knative-prow bot requested a review from maximilien November 30, 2022 21:26

knative-prow bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 30, 2022

knative-prow bot requested a review from vyasgun November 30, 2022 21:26

lance changed the title ~~[WIP] Add runtime~~ [WIP] Add go function invocation runtime as a package Nov 30, 2022

lance force-pushed the add-runtime branch from e07dd1a to 6340e37 Compare December 1, 2022 20:44

lance changed the title ~~[WIP] Add go function invocation runtime as a package~~ Add go function invocation runtime as a package Dec 1, 2022

lance marked this pull request as ready for review December 1, 2022 20:47

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2022

knative-prow bot requested a review from nainaz December 1, 2022 20:48

knative-prow bot requested review from a team December 1, 2022 20:48

lance requested review from lkingland and matejvasek December 1, 2022 20:54

matejvasek mentioned this pull request Dec 1, 2022

allowing on cluster build for go runtime #1445

Merged

lance force-pushed the add-runtime branch from b8b111b to 3c706da Compare December 2, 2022 04:29

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 6, 2022

lance mentioned this pull request Dec 7, 2022

Go template dependencies are outdated #1460

Closed

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 10, 2022

lance mentioned this pull request Dec 12, 2022

Better go module reflection of GOPATH #1459

Closed

zroubalik reviewed Dec 14, 2022

View reviewed changes

lance added this to the 1.9.0 Release milestone Jan 8, 2023

lance modified the milestones: 1.9.0 Release, Release 1.10 Feb 14, 2023

lance closed this Mar 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add go function invocation runtime as a package #1451

Add go function invocation runtime as a package #1451

lance commented Nov 30, 2022 •

edited

Loading

knative-prow bot commented Nov 30, 2022

codecov bot commented Nov 30, 2022 •

edited

Loading

lance commented Dec 1, 2022

lance commented Dec 2, 2022

lance commented Dec 2, 2022

lance commented Dec 2, 2022 •

edited

Loading

matejvasek commented Dec 2, 2022

lance commented Dec 2, 2022 •

edited

Loading

lance commented Dec 2, 2022

lkingland commented Dec 6, 2022 •

edited

Loading

lance commented Dec 6, 2022 •

edited

Loading

lance commented Dec 6, 2022

knative-prow-robot commented Dec 10, 2022

lkingland commented Dec 12, 2022

lance commented Dec 12, 2022

zroubalik left a comment

zroubalik Dec 14, 2022

lance Dec 14, 2022

lance Dec 14, 2022

zroubalik Dec 14, 2022

zroubalik Dec 14, 2022

lkingland commented Feb 27, 2023

lance commented Feb 27, 2023

lance commented Mar 21, 2023

Add go function invocation runtime as a package #1451

Add go function invocation runtime as a package #1451

Conversation

lance commented Nov 30, 2022 • edited Loading

Changes

Motivation

Areas of concern

TODO

Release notes

knative-prow bot commented Nov 30, 2022

codecov bot commented Nov 30, 2022 • edited Loading

Codecov Report

lance commented Dec 1, 2022

lance commented Dec 2, 2022

lance commented Dec 2, 2022

lance commented Dec 2, 2022 • edited Loading

matejvasek commented Dec 2, 2022

lance commented Dec 2, 2022 • edited Loading

lance commented Dec 2, 2022

lkingland commented Dec 6, 2022 • edited Loading

lance commented Dec 6, 2022 • edited Loading

lance commented Dec 6, 2022

knative-prow-robot commented Dec 10, 2022

lkingland commented Dec 12, 2022

lance commented Dec 12, 2022

zroubalik left a comment

Choose a reason for hiding this comment

zroubalik Dec 14, 2022

Choose a reason for hiding this comment

lance Dec 14, 2022

Choose a reason for hiding this comment

lance Dec 14, 2022

Choose a reason for hiding this comment

zroubalik Dec 14, 2022

Choose a reason for hiding this comment

zroubalik Dec 14, 2022

Choose a reason for hiding this comment

lkingland commented Feb 27, 2023

lance commented Feb 27, 2023

lance commented Mar 21, 2023

lance commented Nov 30, 2022 •

edited

Loading

codecov bot commented Nov 30, 2022 •

edited

Loading

lance commented Dec 2, 2022 •

edited

Loading

lance commented Dec 2, 2022 •

edited

Loading

lkingland commented Dec 6, 2022 •

edited

Loading

lance commented Dec 6, 2022 •

edited

Loading