Add a template server #3600

arielshaqed · 2022-07-03T16:24:00Z

See design at template-server.md.

Has only the minimal functions required to build the MVP.

Implements #3540 .

See template-server/template-server.md for design, and ttv-wizard.mvp for (first) application.

arielshaqed · 2022-07-04T09:38:51Z

Guide to reviewing

Apologies for a large PR, but it actually may be easier to review than it seems at first.

Please review by commits:
- c935081 is the templater.
- 7aa8754 hooks it up to the API controller.
- 9786efa adds API tests.
- Other commits are small.
Ignore all generated files: everything under java/ and python/, as well as the copy of swagger.yml.
Ignore lint and similar errors that may appear on commits halfway through; there are 2 commits to fix those (and you can just rely on the fact that the linter action is green).

Jonathan-Rosenberg · 2022-07-04T15:17:34Z

api/swagger.yml

+            type: string
+            description: URL of the template; must be relative (to a URL configured on the server).
+        - in: query
+          name: params


Does that mean that if I want to pass additional params I would need to add something like:
<URL+PATH>?params.a=1&params.b=2...
?
Is it a common practice to do this?
Wouldn't be more cleaner to pass <URL+PATH>?a=1&b=2... ?

Jonathan-Rosenberg · 2022-07-04T15:49:55Z

pkg/templater/expander.go

+type Expander interface {
+	// Prepare checks whether the template expansion will succeed.  Call
+	// it before Expand to ensure nothing is written when expansion fails.
+	Prepare(params *Params) error


Why do you need this TPE (two-phase-expand)?
Can't you use Expand to try and Execute to a temporary bytes.Buffer{}, return error if it doesn't succeed, and write to the given Writer if it does succeed?

We could, but I am not sure that it would be better than this. One disadvantage: I worry that the buffer might be large. As an example, suppose someone abused template to serve a large object (maybe even static, say an image). Then we would hold it in process memory, which is not fun for the process.

Are there advantages other than simpler implementation?

I do think that simpler implementation (thus better maintainability) is important enough, but the concerns you're raising are more important.
Regardless you can still collapse it to a single step process and do the validation step as a first step of the expansion... I think that it's not the role of the client to check if the template is expandable or not.

Sounds good! I shall rework the logic from service.Expand into a single call here.

pkg/templater/expander.go

arielshaqed

Thanks!

I think we should really decide on 2-phase vs. 1-phase processing. What's better about 1-phase, other than simpler implementation?

arielshaqed · 2022-07-05T06:43:34Z

pkg/templater/expander.go

+type Expander interface {
+	// Prepare checks whether the template expansion will succeed.  Call
+	// it before Expand to ensure nothing is written when expansion fails.
+	Prepare(params *Params) error


We could, but I am not sure that it would be better than this. One disadvantage: I worry that the buffer might be large. As an example, suppose someone abused template to serve a large object (maybe even static, say an image). Then we would hold it in process memory, which is not fun for the process.

Are there advantages other than simpler implementation?

pkg/templater/expander.go

Jonathan-Rosenberg · 2022-07-05T14:18:55Z

pkg/templater/funcmap.go

+
+// WrapFuncMapWithData wraps a funcMap to a FuncMap that passes data as a
+// first element when calling each element.
+func WrapFuncMapWithData(funcMap template.FuncMap, data interface{}) template.FuncMap {


Trying to figure out why is it necessary.
The output of this function is an array of functions (FuncMap) that contains functions that hold some data (that would be the controlled data in the implementation), and that appends that "hold-data" to the given argument-data, then calls the underlying function on the total-data.
So a number of questions:

Is my understanding correct?

Why not initialize the template structure with the original FuncMap, and pass the total-data (controlled + uncontrolled) to Execute()?

If it's still necessary to wrap the functions, can we wrap those functions and values in functions instead of reflections to make it more understandable (and probably maintainable)?

By "wrapping with functions" I mean something like:

ret[k] = func(args ...interface{}) (interface{}, error) { argVals := make([]interface{}, 0, len(args)+1) argVals = append(argVals, data) argVals = append(argVals, args...) return v(argVals) }

(holding the controlled data and the underlying function)

By points...

(1) Yes, this is what this function does.

(2) Execute() is somewhat broken: I could find no mechanism to pass side-data into functions. The 2nd arg data does not work for this, because any exported members of it are visible not only to functions but also to the template being expanded. And I am afraid of that access in templates, I have less control over them and they are a DSL that is difficult to analyze. In particular note the template function call, and consider how much damage a user allowed to create new templates will be able to do with the auth service, if that is supplied on data.

(3) I am not sure why wrapping functions manually is less error-prone than performing it automatically. I prefer to pay the added price of having to understand reflection now but once, to having to check each definition of a function wrapper. Also I don't think that your example will compile without redefining v to take all arguments as interface{}. The way to do it would probably be to specify explicitly the types of all arguments to v, and just copy the argument list into the call to v.

Jonathan-Rosenberg · 2022-07-05T14:41:39Z

api/swagger.yml

+              schema:
+                format: binary
+        401:
+          $ref: "#/components/responses/Unauthorized"


What about 403 Forbidden?
The user might be authenticated but not authorized to fetch the template or use the requested function

For some reason we never specify 403 in this file. @nopcoder do you know why? In any case, I'm not blocking on this.

api/swagger.yml

Jonathan-Rosenberg

Really awesome!!!

Not requesting changes but rather some explanations (no such radio button yet)...

Just have a single Expand call that expands twice, once to `io.Discard` and once to the real writer.

arielshaqed

Thanks!

arielshaqed · 2022-07-05T15:01:18Z

api/swagger.yml

+              schema:
+                format: binary
+        401:
+          $ref: "#/components/responses/Unauthorized"


For some reason we never specify 403 in this file. @nopcoder do you know why? In any case, I'm not blocking on this.

arielshaqed · 2022-07-05T15:02:40Z

pkg/templater/expander.go

+type Expander interface {
+	// Prepare checks whether the template expansion will succeed.  Call
+	// it before Expand to ensure nothing is written when expansion fails.
+	Prepare(params *Params) error


Sounds good! I shall rework the logic from service.Expand into a single call here.

arielshaqed · 2022-07-05T15:20:25Z

pkg/templater/funcmap.go

+
+// WrapFuncMapWithData wraps a funcMap to a FuncMap that passes data as a
+// first element when calling each element.
+func WrapFuncMapWithData(funcMap template.FuncMap, data interface{}) template.FuncMap {


By points...

(1) Yes, this is what this function does.

(2) Execute() is somewhat broken: I could find no mechanism to pass side-data into functions. The 2nd arg data does not work for this, because any exported members of it are visible not only to functions but also to the template being expanded. And I am afraid of that access in templates, I have less control over them and they are a DSL that is difficult to analyze. In particular note the template function call, and consider how much damage a user allowed to create new templates will be able to do with the auth service, if that is supplied on data.

(3) I am not sure why wrapping functions manually is less error-prone than performing it automatically. I prefer to pay the added price of having to understand reflection now but once, to having to check each definition of a function wrapper. Also I don't think that your example will compile without redefining v to take all arguments as interface{}. The way to do it would probably be to specify explicitly the types of all arguments to v, and just copy the argument list into the call to v.

arielshaqed · 2022-07-05T15:23:26Z

You're still requesting changes, so asking for a re-review :-)

templates/content/spark.conf.tt

Jonathan-Rosenberg

Thanks!
I think that the template itself should be changed...

This should makes it immediately and directly usable. Co-authored-by: Jonathan Rosenberg <96974219+Jonathan-Rosenberg@users.noreply.github.com>

* Move `template_location` to the URL path. This makes the OpenAPI interface cleaner: the URL query now contains just the parameters of the template. * [CR] Make the `spark.conf.tt` template the finished required form including `lakefs_url` in it. * Fix **bug** in OpenAPI golang code generation (on both client and server sides) with freeform query params in the specification: it does not understand `expand: true`. This is documented as how to do it, generates correct URLs in online OpenAPI editors, but code generated is incorrect (seems to think the dummy params object is actually where it will place the parameters, when in fact it does not.

arielshaqed · 2022-07-07T10:46:37Z

Thanks! I think that the template itself should be changed...

Did that, which uncovered a BUG in OpenAPI code generation (and that the previous code never really worked). Updated version includes the requested template and works.

PTAL...

arielshaqed · 2022-07-10T16:40:45Z

Thanks!

arielshaqed added 2 commits July 3, 2022 19:24

Add templater: a facility for expanding templates on demand

c935081

See template-server/template-server.md for design, and ttv-wizard.mvp for (first) application.

Add template server endpoint

7aa8754

arielshaqed force-pushed the feature/3540-malta-shablona branch from d7bd5eb to 7aa8754 Compare July 3, 2022 16:26

arielshaqed added 2 commits July 4, 2022 09:40

Lint fixes (including local load test compilation)

b59123b

Add tests

9786efa

arielshaqed marked this pull request as ready for review July 4, 2022 09:04

arielshaqed added the include-changelog PR description should be included in next release changelog label Jul 4, 2022

arielshaqed added 2 commits July 4, 2022 12:16

Update auto-generated files

dc1eb7b

Fix linter errors

6961349

Jonathan-Rosenberg self-requested a review July 4, 2022 15:08

Jonathan-Rosenberg reviewed Jul 4, 2022

View reviewed changes

pkg/templater/expander.go Show resolved Hide resolved

arielshaqed commented Jul 5, 2022

View reviewed changes

Jonathan-Rosenberg reviewed Jul 5, 2022

View reviewed changes

api/swagger.yml Show resolved Hide resolved

Jonathan-Rosenberg requested changes Jul 5, 2022

View reviewed changes

[CR] Remove requirement to Prepare-and-Expand

b69e6f5

Just have a single Expand call that expands twice, once to `io.Discard` and once to the real writer.

arielshaqed commented Jul 5, 2022

View reviewed changes

arielshaqed requested a review from Jonathan-Rosenberg July 5, 2022 15:23

make gen

eabbcc2

Jonathan-Rosenberg reviewed Jul 6, 2022

View reviewed changes

templates/content/spark.conf.tt Outdated Show resolved Hide resolved

Jonathan-Rosenberg requested changes Jul 6, 2022

View reviewed changes

Include S3A endpoint and path-style access in Spark config template

5727ba4

This should makes it immediately and directly usable. Co-authored-by: Jonathan Rosenberg <96974219+Jonathan-Rosenberg@users.noreply.github.com>

Jonathan-Rosenberg approved these changes Jul 6, 2022

View reviewed changes

arielshaqed requested a review from Jonathan-Rosenberg July 7, 2022 10:48

Jonathan-Rosenberg approved these changes Jul 10, 2022

View reviewed changes

arielshaqed merged commit a5ebd9c into master Jul 10, 2022

arielshaqed deleted the feature/3540-malta-shablona branch July 10, 2022 16:40

arielshaqed mentioned this pull request Jul 11, 2022

TTV: Operative Templating service #3540

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a template server #3600

Add a template server #3600

arielshaqed commented Jul 3, 2022

arielshaqed commented Jul 4, 2022

Jonathan-Rosenberg Jul 4, 2022

Jonathan-Rosenberg Jul 4, 2022 •

edited

Loading

arielshaqed Jul 5, 2022

Jonathan-Rosenberg Jul 5, 2022

arielshaqed Jul 5, 2022

arielshaqed left a comment

arielshaqed Jul 5, 2022

Jonathan-Rosenberg Jul 5, 2022

arielshaqed Jul 5, 2022

Jonathan-Rosenberg Jul 5, 2022

arielshaqed Jul 5, 2022

Jonathan-Rosenberg left a comment

arielshaqed left a comment

arielshaqed Jul 5, 2022

arielshaqed Jul 5, 2022

arielshaqed Jul 5, 2022

arielshaqed commented Jul 5, 2022

Jonathan-Rosenberg left a comment

arielshaqed commented Jul 7, 2022

arielshaqed commented Jul 10, 2022

Add a template server #3600

Add a template server #3600

Conversation

arielshaqed commented Jul 3, 2022

arielshaqed commented Jul 4, 2022

Guide to reviewing

Choose a reason for hiding this comment

Jonathan-Rosenberg Jul 4, 2022 • 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

arielshaqed 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

Choose a reason for hiding this comment

Jonathan-Rosenberg left a comment

Choose a reason for hiding this comment

arielshaqed 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

arielshaqed commented Jul 5, 2022

Jonathan-Rosenberg left a comment

Choose a reason for hiding this comment

arielshaqed commented Jul 7, 2022

arielshaqed commented Jul 10, 2022

Jonathan-Rosenberg Jul 4, 2022 •

edited

Loading