HTTP Triggers hookup #1086

zootalures · 2018-06-24T22:08:13Z

This doesn't add invoke yet - and that will necessarily pay down some of the refactoring debt created here but I wanted to get black-box testing and functionality in place for HTTP triggers before we started the larger invoke refactor.

This also adds "unique by appId, source, type" to triggers and tests.

A lot of the changes are an initial refactor of the data store interfaces with a view to removing:

NoPStore from the pure runner
Store calls from the Agent (Agent as a proxy for the DB)
Storage operations from agent.call

None of these are completely payed down yet so it's a bit entangled at the moment - Many of these things can wash out of invoke.

rdallman · 2018-06-26T03:31:45Z

api/datastore/sql/sql.go

@@ -445,15 +511,17 @@ func (ds *SQLStore) GetAppByID(ctx context.Context, appID string) (*models.App,

 // GetApps retrieves an array of apps according to a specific filter.
 func (ds *SQLStore) GetApps(ctx context.Context, filter *models.AppFilter) ([]*models.App, error) {
-	res := []*models.App{}
+	var res []*models.App


for json we were at least in v1 returning an empty list. a nil list will be nil in json. we should probably have a package level var to skip the alloc, but that's the rationale if we're keeping that behavior

ah good point !
I just auto-refactoerd there.

We should check in DS tests here.

rdallman · 2018-06-26T03:35:54Z

api/datastore/sql/sql.go

+}
+
+func (ds *SQLStore) GetFns(ctx context.Context, filter *models.FnFilter) ([]*models.Fn, error) {
+	var res []*models.Fn // for json empty list


res := []*models.Fn{} for json empty list ;)

rdallman · 2018-06-26T03:46:33Z

api/datastore/sql/sql.go

@@ -667,6 +881,48 @@ func (ds *SQLStore) GetRoutesByApp(ctx context.Context, appID string, filter *mo
 	return res, nil
 }

+func (ds *SQLStore) GetFnByID(ctx context.Context, fnID string) (*models.Fn, error) {


open ended question: it may be wise to require an app id as well as an fn id here to avoid security bugs where a user may be able to access another user's function which they shouldn't have access to by supplying the correct id. this would become an issue from some auth middleware, presumably, but would manifest as an issue at this level (because at auth time you wouldn't know the function doesn't exist on a certain other app until you look it up).

for example:

Bob has an app poodles, with function id 4242.

Julie has an app corgis with a few functions on it.

Bob is Julie's sworn nemesis (ok, maybe not relevant).

Julie calls the api at /v2/apps/corgis/fns/4242, is authenticated against the corgis app correctly, and grabs Bob's function 4242 out of the db. Julie then sheathes Bob's beautiful poodle Fifi in the thick of Nordic winter (and maybe adopts it so that this is more of a rescue than a tale of malevolence?).

in any event, I have seen this before! maybe, we'd like to guard against it differently, but it's an issue at some level no less and the cost of adding app id to each of these endpoints is relatively minimal, as its already given in the API and we'd just be passing it down -- maybe, there are better ways though.

edit: apps are referenced by id in this scenario and poodles and corgis should be ids, but it detracts from the story so much I've left this tale of poodles and corgis here - use your imagination to replace the strings with made up ids, the issue is unchanged in this scenario :)

My view is that access control is orthogonal to the API structure - makes more sense (in my per-haps 90s oriented API design mentality) to solve authz on a resource-level properly rather than relying on API to help you here .

Of course - getting the access control right (when you need access to the resource to assert on policy) is a harder problem that I leave as an excercise to the implementor of that feature.

All of this is consolidating at the Data store level right now which is good as it's sound w.r.t most ACL decisions but bad as it adds more complexity there.

I /think/ you can plausibly implement a reasonably power Authz policy based on the data store abstraction by making decision after the subordinate call is complete but before the interceptor returns - this is what I've seen in the past (i.e. you can't make an ACL on a object till you've loaded it)

rdallman · 2018-06-26T03:57:20Z

api/models/datastore.go

@@ -59,8 +56,44 @@ type Datastore interface {
 	// ErrDatastoreEmptyRoutePath when routePath is empty. Returns ErrRoutesNotFound when no route exists.
 	RemoveRoute(ctx context.Context, appID, routePath string) error

-	// GetDatabase returns the underlying sqlx database implementation
-	GetDatabase() *sqlx.DB


the extension stuff (and in particular https://github.com/fnproject/ext-auth) was using this. i'm no fan either, but it may be better to wage that battle another time (up to you)

Hmm good point - I think I chose to keep this broken in the very short term.

One option might be to return the root data source from the stack via this interface and then let the extensions up-cast to something that gives them the DB

rdallman · 2018-06-26T04:01:38Z

api/server/server.go

+			{
+				v2.GET("/apps", s.handleAppList)
+				v2.POST("/apps", s.handleAppCreate)
+				v2.GET("/apps/:appId", s.handleAppGet)


in the process of linting... would appreciate doing appID / fnID / etc. while you're in here (lint will yell), though I can do a sweep afterwards for a low low price for you my friend merchant voice

rdallman · 2018-06-26T04:08:34Z

api/server/runner.go

+// Requires the following in the context:
+// * "app"
+// * "source"
+func (s *Server) handleTriggerHttpFunctionCall2(c *gin.Context) error {


perhaps I missed it or it's still WIP (apologies), I don't see another handler that will allow invoking a function for any trigger type w/ cloud event data, only a handler for http triggers (which will mostly be serviced by an lb and can use the former, but we probably should have for convenience as well for 'getting started'). just didn't see anything and pointing this out in case, sure that something is in the works i'm just not sure of what :)

yep, still WIP , haven't looked at cloud event endpoint - imagine that that would be a different URL

rdallman · 2018-06-26T04:10:12Z

know it's WIP but left a few hopefully helpful comments, sorry if it's just something you hadn't gotten to yet and i'm just nagging, forgive me i was curious

zootalures · 2018-06-27T23:52:34Z

Have rebased down, some of this stuff went in on the triggers merge - so might need fixing frlz on master
(sorry this was distantly branched of the original triggers branch - i rebased and sqashed back which probably explains why @rdallman is the author) - will fix .

zootalures · 2018-06-28T00:02:31Z

Just doing another PR to fix the original feedback here,

fn-bot · 2018-06-28T10:17:59Z

CLA Bot

Thank you for your submission! It appears that the following authors have not signed our Contributor License Agreement:

CI (The email used in the commit is not linked to your GitHub account. As a result we cannot verify that you have signed the CLA. If you have signed already, please let us know in our community Slack. Thanks!)

Please do so now by visiting http://www.oracle.com/technetwork/community/oca-486395.html

Once complete, let us know in our community Slack and we’ll send you an Fn T-shirt.

We are working on modernizing the CLA process into a digital signature but it isn’t quite ready yet.

Thank you for being a part of the Fn Community!

fn-bot · 2018-07-03T09:11:30Z

CLA Bot

All committers have signed the CLA.

rdallman · 2018-07-03T22:31:25Z

api/server/runner_httptrigger.go

+		return err
+	}
+
+	// TODO TRIGGERWIP  Not sure I want this evil magic in my life here - shoud remove


i think we need to just do this content type detection as a middleware for all endpoints on the incoming request and outgoing response if we're going to do it at all. the CRUD api needs it too, it just assumes json and doesn't error on content type or anything. anyway, feel free to remove, but i think some of the examples and maybe tests rely on this behavior for better or for worse so it may not be worth it (we can hide it). it's better than defaulting to json :)

tcoupland · 2018-07-05T12:41:44Z

api/models/call.go

@@ -149,6 +149,12 @@ type Call struct {

 	// App this call belongs to.
 	AppID string `json:"app_id" db:"app_id"`
+
+	// App this call belongs to.


Fix comment

tcoupland · 2018-07-05T12:41:52Z

api/models/call.go

+	// App this call belongs to.
+	TriggerID string `json:"trigger_id" db:"trigger_id"`
+
+	// App this call belongs to.


Fixed (and actually started populating these, which I'd missed)

tcoupland · 2018-07-05T12:43:02Z

api/models/trigger.go

@@ -71,46 +77,63 @@ func ValidTriggerType(a string) bool {
 }

 var (
+	//ErrTriggerIDProvided indicates that a trigger ID was specified when it shouldn't have been


ouch, this looks painful!

golang/lint#263
It's the style guide

tcoupland · 2018-07-05T12:49:24Z

api/server/runner_httptrigger_test.go

+		expectedError error
+	}{
+		{"/t/app/route", "", http.StatusNotFound, models.ErrAppsNotFound},
+		{"/t/myapp/route", "", http.StatusNotFound, models.ErrTriggerNotFound},


No success test?

Yep -about 30 of them further down the file

reclaro

LGTM quite a big change it would be nice to have @hhexo to review it. I am going to add him as reviewer

reclaro · 2018-07-05T13:55:32Z

api/agent/data_access.go

@@ -183,22 +200,18 @@ func (da *directDataAccess) Finish(ctx context.Context, mCall *models.Call, stde
 	if async {
 		// XXX (reed): delete MQ message, eventually
 		// YYY (hhexo): yes, once we have the queued/running/finished mechanics
-		// return da.mq.Delete(ctx, mCall)
+		// return cda.mq.Delete(ctx, mCall)


nit: this no-op if puzzle me. I think the idea here is to highlight the todo but shall we comment the entire 'if' ? The compiler will be grateful to us for saving some work :)

meh-the compiler will do that for free

rdallman

LGTM thanks, this is a good step in carving out most of the hybrid stuff. i think we should aim to get the data access stuff out of the agent eventually, but this may be easier to do later.

rdallman mentioned this pull request Jun 25, 2018

[WIP] fn v0.2 #1038

Closed

rdallman reviewed Jun 26, 2018

View reviewed changes

zootalures force-pushed the triggers-hookup branch from e4e4eed to 76b60e8 Compare June 27, 2018 11:25

zootalures force-pushed the triggers-hookup branch from 586e74e to fb4eb1c Compare July 3, 2018 09:11

rdallman reviewed Jul 3, 2018

View reviewed changes

zootalures force-pushed the triggers-hookup branch from 70c44b8 to ddec183 Compare July 4, 2018 12:52

zootalures added 5 commits July 4, 2018 15:31

Initial suypport for invoking tiggers

6073654

dupe method

b26736c

tighten server constraints

1b94d39

runner tests not working yet

a1f7108

basic route tests passing

18ee3a3

zootalures force-pushed the triggers-hookup branch from ddec183 to 18ee3a3 Compare July 4, 2018 15:02

zootalures added 4 commits July 4, 2018 16:09

post rebase fixes

99fb5c7

add hybrid support for trigger invoke and tests

549dcfc

consoloidate all hybrid evil into one place

65e091f

cleanup and make triggers unique by source

93b30cd

zootalures changed the title ~~WIP: HTTP Triggers hookup~~ HTTP Triggers hookup Jul 5, 2018

zootalures requested review from riconnon and skinowski July 5, 2018 09:00

fix oops with Agent

8656465

linting

cebddac

zootalures requested a review from tcoupland July 5, 2018 12:28

tcoupland reviewed Jul 5, 2018

View reviewed changes

review fixes

6a7422b

reclaro reviewed Jul 5, 2018

View reviewed changes

reclaro requested a review from hhexo July 5, 2018 14:20

rdallman approved these changes Jul 5, 2018

View reviewed changes

rdallman merged commit b8b544e into master Jul 5, 2018

rdallman deleted the triggers-hookup branch July 5, 2018 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP Triggers hookup #1086

HTTP Triggers hookup #1086

zootalures commented Jun 24, 2018 •

edited

Loading

rdallman Jun 26, 2018

zootalures Jun 27, 2018

rdallman Jun 26, 2018

rdallman Jun 26, 2018 •

edited

Loading

zootalures Jun 28, 2018

rdallman Jun 26, 2018

zootalures Jun 28, 2018

rdallman Jun 26, 2018

zootalures Jun 28, 2018

rdallman Jun 26, 2018

zootalures Jun 27, 2018

rdallman commented Jun 26, 2018

zootalures commented Jun 27, 2018

zootalures commented Jun 28, 2018

fn-bot commented Jun 28, 2018

fn-bot commented Jul 3, 2018

rdallman Jul 3, 2018

tcoupland Jul 5, 2018

zootalures Jul 5, 2018

tcoupland Jul 5, 2018

zootalures Jul 5, 2018

tcoupland Jul 5, 2018

zootalures Jul 5, 2018

tcoupland Jul 5, 2018

zootalures Jul 5, 2018

reclaro left a comment

reclaro Jul 5, 2018

zootalures Jul 5, 2018

rdallman left a comment

HTTP Triggers hookup #1086

HTTP Triggers hookup #1086

Conversation

zootalures commented Jun 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdallman Jun 26, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdallman commented Jun 26, 2018

zootalures commented Jun 27, 2018

zootalures commented Jun 28, 2018

fn-bot commented Jun 28, 2018

CLA Bot

fn-bot commented Jul 3, 2018

CLA Bot

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reclaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdallman left a comment

Choose a reason for hiding this comment

zootalures commented Jun 24, 2018 •

edited

Loading

rdallman Jun 26, 2018 •

edited

Loading