Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconsider use of Golang templates #112

Closed
scothis opened this issue Sep 28, 2020 · 13 comments
Closed

Reconsider use of Golang templates #112

scothis opened this issue Sep 28, 2020 · 13 comments

Comments

@scothis
Copy link
Member

scothis commented Sep 28, 2020

Golang templates are used by mappings to create a value for a new key composed of existing keys. During a review it was brought up that Golang templates are not standardized and are inherently part of the Golang language. They are subject to the same evolution and deprecation as Golang features. A deprecation in the implementation language should not also implicitly be a deprecation in the Service Binding spec.

We should reconsider the use of Golang templates in favor of an independently scoped template language with multiple language implementations, or define our own minimal grammar.

The templating language was less of a concern when mappings were a spec extension, but since we promoted it to core, the choice has broader impacts.

@baijum
Copy link
Contributor

baijum commented Sep 28, 2020

The Go 1 compatibility promise is applicable to the standard library also. So I would suggest mandating the Go template syntax as defined in Go 1 standard library. This way implementors can choose an alternative implementation of the Go 1 template if required in the future.

@navidsh navidsh added the RC3 label Sep 29, 2020
@navidsh
Copy link
Contributor

navidsh commented Sep 29, 2020

Discussed during the interlock call. Continuing the discussion...

@baijum
Copy link
Contributor

baijum commented Oct 1, 2020

On a closer look at the use case for the template, a string substitution with a method to escape the URL query would be sufficient.
I think we can restrict the use of the current Go template with grammar, this way it will work for existing syntax. If a new use case arises, the grammar can be amended conforming to the template. Here is an attempt to define grammar using Parsing expression grammar:

from parsimonious.grammar import Grammar

grammar = Grammar(r"""
    mappings = ws* (string* keyref string*)+ ws*
    keyref   = (opening ws* urlquery? key ws* closing)
    opening  = '{{'
    closing  = '}}'
    urlquery = 'urlquery' ws+
    key      = ~r'.[A-Z0-9-_]*'i
    string   = ~r'[ A-Z0-9.-_:/\\@~#$%^&*]*'i
    ws       = ~"\s"
    """)

if __name__ == "__main__":
    out = grammar.parse("https://{{ urlquery .username }}:{{ urlquery .password }}@{{ .host }}:{{ .port }}/{{ .path }}")
    print(out)

P.S: Recently Python switched from EBNF to PEG to define its own grammar. I think PEG would be a good choice to define the grammar. Here is an online playground in JS: https://pegjs.org/online

@nebhale
Copy link
Member

nebhale commented Oct 13, 2020

I want to register two points that come to mind, but I’m not sure I’d consider them blockers:

  1. If we standardize on a subset of Golang templates, will users (who won’t read the spec) be confused when the full expressive power of Golang templates are not available?
  2. If we standardize on a subset of Golang templates, will implementations simply implement the spec using Golang templates (implementations that do more than the spec requires are perfectly legal) negating the choice to specify a subset?

For some context, the recommendation to get away from Golang templates comes from someone really focused on what Kubernetes looks like a decade from now (so Golang 2 kind of timeframe). There’s some question if Kubernetes as a whole could ever make that jump (given how implementations commonly include one another as Golang libraries), but we should at the very least be cognizant of tying the standard to a community that we have no influence in.

@baijum
Copy link
Contributor

baijum commented Oct 15, 2020

@nebhale Those are valid points about a subset of the Go template!

A subset of the Go template will lead to unportable configuration files, as is evident from your second point.

I think the use cases are limited to string substitution and a function to escape for the URL query. The input to process the mapping is a Secret data of map from string to string values. So we can come up with a simple syntax for this usage.

I have noticed IBM Cloud provides the Secret values as a JSON object! See the docs: https://cloud.ibm.com/docs/containers?topic=containers-service-binding#bind-services

There is an issue reported about the same in Red Hat's Service Binding Operator: redhat-developer/service-binding-operator#717

If we consider it a fair use case, a function to extract values from the JSON object would be excellent.

When it comes to extracting values from a JSON object, we could adopt one of these:

  1. JSON Pointer: https://tools.ietf.org/html/rfc6901 (The is a PROPOSED STANDARD)
    Very limited and not sure if it can meet all needs.
  2. JAMESPath: https://jmespath.org/
    Very advanced and well defined through a formal specification. I think this will be slow compared to other alternatives.
  3. JSONPath: https://datatracker.ietf.org/doc/draft-goessner-dispatch-jsonpath/
    Very widely adopted with around 40 implementations in many languages.
    There is an attempt to standardize going on.

Use Cases and Syntax for Mapping

Considering the above three use cases, this is my proposal:

  1. Reference value for a given key in the Secret

    {% key %}
    
  2. Evaluate the value as JSONPath for a given key in the Secret

    {% "JSONPath" key %}
    
  3. Escape for URL Query.

    a. Escape the value for the given key

    {% ! key %}
    

    b. Escape the JSONPath evaluated value for the given key

    {% ! "JSONPath" key %}
    

PEG Syntax of Service Binding Mapping


MappingString <- (AnyChar Expression AnyChar)+ !.

Expression <- Opening Escape? JSONPath? Key Closing

Opening <- '{%' Whitespace+

Closing <- Whitespace+ '%}'

Escape <- '!' Whitespace+

JSONPath <- Whitespace* '"' JSONPathExpr '"' Whitespace+

JSONPathExpr <- (!'"' .)+

Key <- ([a-z] / [A-Z] / [0-9] / '-' / '_' / '.')+

Whitespace <- [ \n\r\t]

AnyChar <- (!'{%' .)*

Programming API

For the convenience of the implementors, we can provide libraries that implement parsers for the above syntax. This is not part of the spec.

Go API

type Template struct { ... }

func NewTemplate(t string) *Template { ... }

func (tmpl *Template) Render(data map[string]string) string { ... }

Python API


def NewTemplate(t: string) -> Template:

class Template:

    def Render(self, data: dict[str, str]) -> str:

Update: Added programming API (not going to be part of the spec)

@sbose78
Copy link
Contributor

sbose78 commented Oct 15, 2020

The templating language was less of a concern when mappings were a spec extension

Given the discussion above, I wonder if mappings could be moved out of the core, at the very least, while this area matures ?

@scothis
Copy link
Member Author

scothis commented Oct 15, 2020

Given the discussion above, I wonder if mappings could be moved out of the core, at the very least, while this area matures ?

Mappings could be implemented as a separate resource that consumes a provisioned service and exposes a transformed provisioned service. That alternate resource can use whatever syntax it deems appropriate without any fear of breaking the ServiceBinding resource down the road. This is conceptual similar to the Composite/MultiService proposal in #120

@baijum
Copy link
Contributor

baijum commented Oct 16, 2020

  1. JSON Pointer: https://tools.ietf.org/html/rfc6901 (The is a PROPOSED STANDARD)
    Very limited and not sure if it can meet all needs.

JSON Pointer supports identifying a specific value within a JSON document. On second thought, I think this would be sufficient to meet the use cases. Reasons to use JSON Pointer:

  1. The RFC 6901 is a "PROPOSED STANDARD". There won't be any change in the future.
  2. Implementing the JSON Pointer would be easy.
  3. Depending on a smaller spec is better than supporting the heavier alternatives.

@baijum
Copy link
Contributor

baijum commented Oct 16, 2020

FYI, I created a proposal to add EvalJSONPointer function to support RFC 6901 in Go.

@arthurdm
Copy link
Member

Discussed this at today's hangout and our recommendation is to move mappings into the proposed ComposedService CR (or different name), so that it is an extension and not part of core. Please add comments / feedback about this recommendation here.

@arthurdm
Copy link
Member

Spoke again at today's hangout and the previous recommendation of moving mappings to an extension is dependant on whether or not we think that mappings is not a very common use case.

@sbose78
Copy link
Contributor

sbose78 commented Nov 24, 2020

mappings feel like a great escape hatch to me ( and that's my hunch is that it will remain popular), given that the ecosystem to have things just work with binding isn't there yet.

@baijum
Copy link
Contributor

baijum commented Jun 16, 2021

Since we merged #154, we can close this issue.

@scothis scothis closed this as completed Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants