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

FR: Core Interceptor plugins #271

Closed
wlynch opened this issue Dec 11, 2019 · 7 comments
Closed

FR: Core Interceptor plugins #271

wlynch opened this issue Dec 11, 2019 · 7 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. maybe-next-milestone
Milestone

Comments

@wlynch
Copy link
Member

wlynch commented Dec 11, 2019

Summary

Expected Behavior

Core Interceptors should be pluggable so that users can choose which are available by default.

Actual Behavior

Interceptors are currently static.

Additional Info

https://github.com/hashicorp/go-plugin is a potential candidate for supporting in-binary plugins.

@dibyom dibyom added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 12, 2019
@dibyom dibyom added this to the Triggers 0.3 milestone Jan 21, 2020
@ncskier
Copy link
Member

ncskier commented Jan 28, 2020

Waiting for design doc

@dibyom
Copy link
Member

dibyom commented Jul 6, 2020

/assign

@dibyom
Copy link
Member

dibyom commented Jul 28, 2020

I wrote up a design proposal: https://docs.google.com/document/d/1zIG295nyWonCXhb8XcOC41q4YVP9o2Hgg8uAoL4NdPA/edit#heading=h.8z1iarlx6yyx

Planning on filing TEP after discussion.

@dibyom
Copy link
Member

dibyom commented Nov 10, 2020

Pending the TEP getting merged, here is the implementation plan:

dibyom added a commit to dibyom/triggers that referenced this issue Nov 13, 2020
This commit adds two new types for InterceptorRequest and
InterceptorResponse. Instead of just forwarding the incoming request,
EventListeners will use these types to send requests to interceptors.

This commit introduces an `extensions` field where Interceptors can add
additional data that is available to TriggerBindings. This replaces the
previous mechanism where Interceptors could freely modify the input
bodies. With this change, the input bodies are immutable, and any extra
fields added by Interceptors can only be done within the `extensions`
field.

This commit simply adds the new interface types and plumbing. Future
commits will change the Interceptors to adopt the new Interface.

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Nov 13, 2020
This commit adds two new types for InterceptorRequest and
InterceptorResponse. Instead of just forwarding the incoming request,
EventListeners will use these types to send requests to interceptors.

This commit introduces an `extensions` field where Interceptors can add
additional data that is available to TriggerBindings. This replaces the
previous mechanism where Interceptors could freely modify the input
bodies. With this change, the input bodies are immutable, and any extra
fields added by Interceptors can only be done within the `extensions`
field.

This commit simply adds the new interface types and plumbing. Future
commits will change the Interceptors to adopt the new Interface.

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Nov 13, 2020
This commit adds two new types for InterceptorRequest and
InterceptorResponse. Instead of just forwarding the incoming request,
EventListeners will use these types to send requests to interceptors.

This commit introduces an `extensions` field where Interceptors can add
additional data that is available to TriggerBindings. This replaces the
previous mechanism where Interceptors could freely modify the input
bodies. With this change, the input bodies are immutable, and any extra
fields added by Interceptors can only be done within the `extensions`
field.

This commit simply adds the new interface types and plumbing. Future
commits will change the Interceptors to adopt the new Interface.

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Nov 13, 2020
This commit adds two new types for InterceptorRequest and
InterceptorResponse. Instead of just forwarding the incoming request,
EventListeners will use these types to send requests to interceptors.

This commit introduces an `extensions` field where Interceptors can add
additional data that is available to TriggerBindings. This replaces the
previous mechanism where Interceptors could freely modify the input
bodies. With this change, the input bodies are immutable, and any extra
fields added by Interceptors can only be done within the `extensions`
field.

This commit simply adds the new interface types and plumbing. Future
commits will change the Interceptors to adopt the new Interface.

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Nov 13, 2020
This commit updates the CEL interceptor to use the new interface that supports
immutable input event bodies. This represents a BREAKING CHANGE for users using
`overlays`. Overlays will no longer modify the request body. Instead `keys`
from overlays will be added to a new top-level `extensions` field that is
accessible to TriggerBindings.

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Nov 13, 2020
This commit updates the CEL interceptor to use the new interface that supports
immutable input event bodies. This represents a BREAKING CHANGE for users using
`overlays`. Overlays will no longer modify the request body. Instead `keys`
from overlays will be added to a new top-level `extensions` field that is
accessible to TriggerBindings.

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Nov 13, 2020
This commit updates the CEL interceptor to use the new interface that supports
immutable input event bodies. This represents a BREAKING CHANGE for users using
`overlays`. Overlays will no longer modify the request body. Instead `keys`
from overlays will be added to a new top-level `extensions` field that is
accessible to TriggerBindings.

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Nov 13, 2020
This commit updates the CEL interceptor to use the new interface that supports
immutable input event bodies. This represents a BREAKING CHANGE for users using
`overlays`. Overlays will no longer modify the request body. Instead `keys`
from overlays will be added to a new top-level `extensions` field that is
accessible to TriggerBindings.

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 8, 2020
1. Change body from []byte to json.RawMessage
2. Remove omitempty tag for boolean type so that it marshals when false
3. Change context.url to context.event_url to match struct type

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 8, 2020
This commit packages the 4 core interceptors into a single HTTP server.
Each interceptor is available at a different path e.g. /cel for CEL etc.

This does not wire up the implementation of the server into the
EventListener which will be done in a future commit.

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 8, 2020
This commit packages the 4 core interceptors into a single HTTP server.
Each interceptor is available at a different path e.g. /cel for CEL etc.

This does not wire up the implementation of the server into the
EventListener which will be done in a future commit.

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 9, 2020
1. Change body from []byte to json.RawMessage
2. Remove omitempty tag for boolean type so that it marshals when false
3. Change context.url to context.event_url to match struct type

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 9, 2020
This commit packages the 4 core interceptors into a single HTTP server.
Each interceptor is available at a different path e.g. /cel for CEL etc.

This does not wire up the implementation of the server into the
EventListener which will be done in a future commit.

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 9, 2020
1. Change body from []byte to json.RawMessage
2. Remove omitempty tag for boolean type so that it marshals when false
3. Change context.url to context.event_url to match struct type

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 9, 2020
This commit packages the 4 core interceptors into a single HTTP server.
Each interceptor is available at a different path e.g. /cel for CEL etc.

This does not wire up the implementation of the server into the
EventListener which will be done in a future commit.

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 10, 2020
1. Change body from []byte to json.RawMessage
2. Remove omitempty tag for boolean type so that it marshals when false
3. Change context.url to context.event_url to match struct type

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 10, 2020
This commit packages the 4 core interceptors into a single HTTP server.
Each interceptor is available at a different path e.g. /cel for CEL etc.

This does not wire up the implementation of the server into the
EventListener which will be done in a future commit.

Part of tektoncd#271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
tekton-robot pushed a commit that referenced this issue Dec 10, 2020
1. Change body from []byte to json.RawMessage
2. Remove omitempty tag for boolean type so that it marshals when false
3. Change context.url to context.event_url to match struct type

Part of #271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
tekton-robot pushed a commit that referenced this issue Dec 10, 2020
This commit packages the 4 core interceptors into a single HTTP server.
Each interceptor is available at a different path e.g. /cel for CEL etc.

This does not wire up the implementation of the server into the
EventListener which will be done in a future commit.

Part of #271

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 22, 2020
Storing the body as json.RawMessagae can lead to loss of information about
spaces in the incoming body when the InterceptorRequest is marshaled as JSON
before sending it over HTTP.

This is because Go's `json.Marshal` will encode `[]byte` as a base64 string and
base64 does not preserve spaces. Adding a custom `MarshalJSON` does not help
here since `MarshalJSON` will return a `[]byte` that will then get compacted as
base64 by `json.Marshal`.

This loss of spaces can be problematic for some use cases. For instance, the
GitHub payload signature validation requires us to compare using the exact body
as it was sent. Otherwise, the validation fails. Note that this will only be a
problem when we marshal the InterceptorRequest i.e. when we move the core
interceptors out to its own server.

Using a string means that the string will be quoted e.g. `{\"foo\": \"bar\"}`.
Go should take care of unquoting it during the unmarshaling process. However,
intercetpor authors using a different language will have to manually unquote
the string by parsing it as a JSON object.

Part of tektoncd#271, 867

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 22, 2020
Storing the body as json.RawMessagae can lead to loss of information about
spaces in the incoming body when the InterceptorRequest is marshaled as JSON
before sending it over HTTP.

This is because Go's `json.Marshal` will encode `[]byte` as a base64 string and
base64 does not preserve spaces. Adding a custom `MarshalJSON` does not help
here since `MarshalJSON` will return a `[]byte` that will then get compacted as
base64 by `json.Marshal`.

This loss of spaces can be problematic for some use cases. For instance, the
GitHub payload signature validation requires us to compare using the exact body
as it was sent. Otherwise, the validation fails. Note that this will only be a
problem when we marshal the InterceptorRequest i.e. when we move the core
interceptors out to its own server.

Using a string means that the string will be quoted e.g. `{\"foo\": \"bar\"}`.
Go should take care of unquoting it during the unmarshaling process. However,
intercetpor authors using a different language will have to manually unquote
the string by parsing it as a JSON object.

Part of tektoncd#271, 867

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 22, 2020
Storing the body as `json.RawMessage` can lead to loss of information about
spaces in the incoming body when the InterceptorRequest is marshaled as JSON
before sending it over HTTP.

This is because Go's `json.Marshal` will encode `[]byte` as a base64 string and
base64 does not preserve spaces. Adding a custom `MarshalJSON` does not help
here since `MarshalJSON` will return a `[]byte` that will then get compacted as
base64 by `json.Marshal`.

This loss of spaces can be problematic for some use cases. For instance, the
GitHub payload signature validation requires us to compare using the exact body
as it was sent. Otherwise, the validation fails. Note that this will only be a
problem when we marshal the InterceptorRequest i.e. when we move the core
interceptors out to its own server.

Using a string means that the string will be quoted e.g. `{\"foo\": \"bar\"}`.
Go should take care of unquoting it during the unmarshaling process. However,
intercetpor authors using a different language will have to manually unquote
the string by parsing it as a JSON object.

Part of tektoncd#271, 867

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 22, 2020
Storing the body as `json.RawMessage` can lead to loss of information about
spaces in the incoming body when the InterceptorRequest is marshaled as JSON
before sending it over HTTP.

This is because Go's `json.Marshal` will encode `[]byte` as a base64 string and
base64 does not preserve spaces. Adding a custom `MarshalJSON` does not help
here since `MarshalJSON` will return a `[]byte` that will then get compacted as
base64 by `json.Marshal`.

This loss of spaces can be problematic for some use cases. For instance, the
GitHub payload signature validation requires us to compare using the exact body
as it was sent. Otherwise, the validation fails. Note that this will only be a
problem when we marshal the InterceptorRequest i.e. when we move the core
interceptors out to its own server.

Using a string means that the string will be quoted e.g. `{\"foo\": \"bar\"}`.
Go should take care of unquoting it during the unmarshaling process. However,
intercetpor authors using a different language will have to manually unquote
the string by parsing it as a JSON object.

Part of tektoncd#271, tektoncd#867

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 22, 2020
Storing the body as `json.RawMessage` can lead to loss of information about
spaces in the incoming body when the InterceptorRequest is marshaled as JSON
before sending it over HTTP.

This is because Go's `json.Marshal` will encode `[]byte` as a base64 string and
base64 does not preserve spaces. Adding a custom `MarshalJSON` does not help
here since `MarshalJSON` will return a `[]byte` that will then get compacted as
base64 by `json.Marshal`.

This loss of spaces can be problematic for some use cases. For instance, the
GitHub payload signature validation requires us to compare using the exact body
as it was sent. Otherwise, the validation fails. Note that this will only be a
problem when we marshal the InterceptorRequest i.e. when we move the core
interceptors out to its own server.

Using a string means that the string will be quoted e.g. `{\"foo\": \"bar\"}`.
Go should take care of unquoting it during the unmarshaling process. However,
intercetpor authors using a different language will have to manually unquote
the string by parsing it as a JSON object.

Part of tektoncd#271, tektoncd#867

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Dec 23, 2020
Storing the body as `json.RawMessage` can lead to loss of information about
spaces in the incoming body when the InterceptorRequest is marshaled as JSON
before sending it over HTTP.

This is because Go's `json.Marshal` will encode `[]byte` as a base64 string and
base64 does not preserve spaces. Adding a custom `MarshalJSON` does not help
here since `MarshalJSON` will return a `[]byte` that will then get compacted as
base64 by `json.Marshal`.

This loss of spaces can be problematic for some use cases. For instance, the
GitHub payload signature validation requires us to compare using the exact body
as it was sent. Otherwise, the validation fails. Note that this will only be a
problem when we marshal the InterceptorRequest i.e. when we move the core
interceptors out to its own server.

Using a string means that the string will be quoted e.g. `{\"foo\": \"bar\"}`.
Go should take care of unquoting it during the unmarshaling process. However,
intercetpor authors using a different language will have to manually unquote
the string by parsing it as a JSON object.

Part of tektoncd#271, tektoncd#867

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
tekton-robot pushed a commit that referenced this issue Dec 23, 2020
Storing the body as `json.RawMessage` can lead to loss of information about
spaces in the incoming body when the InterceptorRequest is marshaled as JSON
before sending it over HTTP.

This is because Go's `json.Marshal` will encode `[]byte` as a base64 string and
base64 does not preserve spaces. Adding a custom `MarshalJSON` does not help
here since `MarshalJSON` will return a `[]byte` that will then get compacted as
base64 by `json.Marshal`.

This loss of spaces can be problematic for some use cases. For instance, the
GitHub payload signature validation requires us to compare using the exact body
as it was sent. Otherwise, the validation fails. Note that this will only be a
problem when we marshal the InterceptorRequest i.e. when we move the core
interceptors out to its own server.

Using a string means that the string will be quoted e.g. `{\"foo\": \"bar\"}`.
Go should take care of unquoting it during the unmarshaling process. However,
intercetpor authors using a different language will have to manually unquote
the string by parsing it as a JSON object.

Part of #271, #867

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
@dibyom dibyom modified the milestones: Triggers v0.11, Triggers v0.12 Jan 13, 2021
@dibyom dibyom modified the milestones: Triggers v0.12, Triggers v0.13 Mar 3, 2021
@dibyom dibyom modified the milestones: Triggers v0.13, Triggers Beta Apr 6, 2021
@dibyom
Copy link
Member

dibyom commented May 12, 2021

Shipped in v0.13. Closing.

@dibyom
Copy link
Member

dibyom commented May 12, 2021

/close

@tekton-robot
Copy link

@dibyom: Closing this issue.

In response to this:

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. maybe-next-milestone
Projects
None yet
Development

No branches or pull requests

4 participants