Adding transformer field to GCPPubSubSource CRD. #398

nachocano · 2019-05-06T22:16:58Z

Proposed Changes

Adding an optional addressable to GCPPubSubSource CRD called transformer to be able to transform GCP events.

Pending

Default transformer.
Cleanup "ce-type" logic.

Release Note

GCPPubSubSource includes an optional `transformer` Addressable field that can be configured to do custom CloudEvents transformations.

GitHub webhooks cannot be created otherwise.

nachocano · 2019-05-06T22:18:39Z

/cc @Harwayne @vaikas-google
still WIP but it would be nice if you can start looking at it...

…nto transformer # Conflicts: # contrib/gcppubsub/pkg/adapter/adapter.go # contrib/gcppubsub/pkg/reconciler/gcppubsubsource.go

contrib/gcppubsub/cmd/receive_adapter/main.go

nachocano · 2019-05-07T17:58:55Z

Removed the default transformer for now until we further agree how it should behave.
Just added the transformer addressable field to GCPPubSubSource.

…nto transformer

nachocano · 2019-05-08T02:08:43Z

contrib/gcppubsub/pkg/adapter/adapter.go

+	event.SetDataContentType(*cloudevents.StringOfApplicationJSON())
+	event.SetSource(a.source)
+	event.SetData(m.Message())
+
 	// TODO: this will break when the upstream sender updates cloudevents versions.
 	// The correct thing to do would be to convert the message to a cloudevent if it is one.
 	et := sourcesv1alpha1.GcpPubSubSourceEventType
 	if override, ok := m.Message().Attributes[eventTypeOverrideKey]; ok {


IMO we should remove this asap. If a particular source wants to do something like this (e.g., the gcs source), it should actually do it in the transformer.

vaikas

Thanks!! Couple of questions/suggestions.

contrib/gcppubsub/config/300-gcppubsubsource.yaml

vaikas · 2019-05-13T17:19:36Z

contrib/gcppubsub/pkg/adapter/adapter.go

+			// Update the event with the transformed one.
+			event = *resp
+		} else {
+			logger.Warnf("cloud event %q was not transformed", event.ID())


Should this return an error?

I was leaving it as in case the transformer does not populate properly the response. If so, then we at least send the //google.pubsub.topic.publish event. As we might not control the transformer code, I'm assuming it can be wrong.
Do you prefer to return an error instead?

I like the idea that the transformer can hide events. For example, events that are not on an allowed list or those that do not pass schema validation. I think our 'canonical' filtering of events is to return a 2XX with an empty body.

So I wouldn't return an error here, I would just return nil.

vaikas · 2019-05-13T17:21:16Z

contrib/gcppubsub/cmd/receive_adapter/main.go

+		TopicID:        env.Topic,
+		SinkURI:        env.Sink,
+		SubscriptionID: env.Subscription,
+		TransformerURI: env.Transformer,


Should we at least validate the URI to make sure it's a valid URI?

we are not doing that for the sinkURI. I guess if it's not valid it will break when trying to create the httpClient. And it can be empty right? in that case we do not create any client...

knative-metrics-robot · 2019-05-13T17:41:22Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-sources-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
contrib/gcppubsub/pkg/adapter/adapter.go	60.0%	63.6%	3.6
contrib/gcppubsub/pkg/apis/sources/v1alpha1/gcp_pubsub_types.go	93.8%	71.4%	-22.3
contrib/gcppubsub/pkg/reconciler/gcppubsubsource.go	90.0%	90.6%	0.6

vaikas · 2019-05-13T18:38:37Z

/lgtm
/approve

knative-prow-robot · 2019-05-13T18:38:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nachocano, vaikas-google

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:

~~contrib/gcppubsub/OWNERS~~ [vaikas-google]

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

Harwayne · 2019-05-13T18:35:42Z

contrib/gcppubsub/cmd/receive_adapter/main.go

@@ -64,11 +58,17 @@ func main() {
 		log.Fatalf("Unable to create logger: %v", err)
 	}

+	var env envConfig
+	if err := envconfig.Process("", &env); err != nil {
+		log.Fatal("Failed to process env var", zap.Error(err))


Is this a sugared logger? If so, it should be Fatalw (my preference is just switch to a desugared logger to make sure these bugs don't occur).

Harwayne · 2019-05-13T18:37:39Z

contrib/gcppubsub/config/300-gcppubsubsource.yaml

            googleCloudProject:
              type: string
+              description: "ID of the Google Cloud Project that the PubSub Topic exists in."


I would add an example here, as historically GCP project name vs ID vs display name has been very confusing.

description: "ID of the Google Cloud Project that the PubSub Topic exists in. E.g. 'my-project-1234' rather than its display name, 'My Project' or its number '1234567890'."

Harwayne · 2019-05-13T18:37:59Z

contrib/gcppubsub/config/300-gcppubsubsource.yaml

            serviceAccountName:
              type: string
+              description: "Name of the ServiceAccount that will be used to run the Receive Adapter Deployment."


Give an example. Is this the full email address? Just the name before the '@'?

contrib/gcppubsub/config/300-gcppubsubsource.yaml

Harwayne · 2019-05-13T18:39:40Z

contrib/gcppubsub/pkg/adapter/adapter.go

+	event.SetDataContentType(*cloudevents.StringOfApplicationJSON())
+	event.SetSource(a.source)
+	event.SetData(m.Message())
+
 	// TODO: this will break when the upstream sender updates cloudevents versions.
 	// The correct thing to do would be to convert the message to a cloudevent if it is one.
 	et := sourcesv1alpha1.GcpPubSubSourceEventType
 	if override, ok := m.Message().Attributes[eventTypeOverrideKey]; ok {


Harwayne · 2019-05-13T18:42:17Z

contrib/gcppubsub/pkg/adapter/adapter.go

+			// Update the event with the transformed one.
+			event = *resp
+		} else {
+			logger.Warnf("cloud event %q was not transformed", event.ID())


I like the idea that the transformer can hide events. For example, events that are not on an allowed list or those that do not pass schema validation. I think our 'canonical' filtering of events is to return a 2XX with an empty body.

So I wouldn't return an error here, I would just return nil.

Harwayne · 2019-05-13T18:43:16Z

contrib/gcppubsub/pkg/apis/sources/v1alpha1/gcp_pubsub_types.go

@@ -83,7 +87,7 @@ const (
 )

 // GetGcpPubSubSource returns the GcpPubSub CloudEvent source value.


GetGcpPubSubSource -> GcpPubSubSource

Harwayne · 2019-05-13T18:44:56Z

contrib/gcppubsub/pkg/apis/sources/v1alpha1/gcp_pubsub_types.go

+	if len(uri) > 0 {
+		gcpPubSubSourceCondSet.Manage(s).MarkTrue(GcpPubSubConditionTransformerProvided)
+	} else {
+		gcpPubSubSourceCondSet.Manage(s).MarkUnknown(GcpPubSubConditionTransformerProvided, "TransformerEmpty", "Transformer has resolved to empty.%s", "")


Why the trailing "%s" that has an empty string arg?

nachocano added 20 commits February 28, 2019 15:09

Adding knative-sources namespace to gcppubsub's ServiceAccount.

f5a9a40

Merge remote-tracking branch 'upstream/master'

61a15f4

Merge remote-tracking branch 'upstream/master'

993f253

Merge remote-tracking branch 'upstream/master'

5d069c1

Merge remote-tracking branch 'upstream/master'

dfc8add

Merge remote-tracking branch 'upstream/master'

2d410d0

Merge remote-tracking branch 'upstream/master'

ed434d6

Merge remote-tracking branch 'upstream/master'

4b44736

Merge remote-tracking branch 'upstream/master'

9288767

Merge remote-tracking branch 'upstream/master'

d171278

Fixing bug introduced during a refactor.

603c57c

GitHub webhooks cannot be created otherwise.

Merge remote-tracking branch 'upstream/master'

9d63463

Merge remote-tracking branch 'upstream/master'

23d8f31

Merge remote-tracking branch 'upstream/master'

e7666f0

Merge remote-tracking branch 'upstream/master'

d6775fa

Merge remote-tracking branch 'upstream/master'

0aa5816

Merge remote-tracking branch 'upstream/master'

c05b9eb

Adding transformer

99ff80c

transformerURI

61f389f

adding transformer for GCP events.

7194a75

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 6, 2019

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 6, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 6, 2019

knative-prow-robot requested review from Harwayne and matzew May 6, 2019 22:17

knative-prow-robot requested a review from vaikas May 6, 2019 22:18

nachocano added 2 commits May 6, 2019 22:02

Merge branch 'transformer' of github.com:nachocano/eventing-sources i…

7577479

…nto transformer # Conflicts: # contrib/gcppubsub/pkg/adapter/adapter.go # contrib/gcppubsub/pkg/reconciler/gcppubsubsource.go

removing transformer implementation for now

25b7005

vaikas reviewed May 7, 2019

View reviewed changes

contrib/gcppubsub/cmd/receive_adapter/main.go Outdated Show resolved Hide resolved

adding stuff

537ab4e

nachocano changed the title ~~[WIP] Adding transformer for GCP events.~~ Adding transformer field to GCPPubSubSource CRD. May 7, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 7, 2019

nachocano added 5 commits May 7, 2019 11:01

Merge remote-tracking branch 'upstream/master' into transformer

3b889b1

adding it back, the skeleton one

6d0dd38

Merge branch 'transformer' of github.com:nachocano/eventing-sources i…

e65f21e

…nto transformer

removing default transformer again!

3de243d

Merge remote-tracking branch 'upstream/master' into transformer

49c6260

nachocano commented May 8, 2019

View reviewed changes

nachocano added 3 commits May 10, 2019 09:33

Merge remote-tracking branch 'upstream/master' into transformer

c3bce54

moving to kelseyhightower

4df252e

Merge remote-tracking branch 'upstream/master' into transformer

66db82d

vaikas reviewed May 13, 2019

View reviewed changes

Adding descriptions

66f7b83

Harwayne mentioned this pull request May 13, 2019

In GcpPubSubSource, map fields to cloud events #393

Closed

knative-prow-robot assigned vaikas May 13, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2019

knative-prow-robot merged commit 34b8477 into knative:master May 13, 2019

Harwayne reviewed May 13, 2019

View reviewed changes

nachocano mentioned this pull request May 15, 2019

GCPPubSub Transformer cosmetic changes #424

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding transformer field to GCPPubSubSource CRD. #398

Adding transformer field to GCPPubSubSource CRD. #398

nachocano commented May 6, 2019 •

edited

Loading

nachocano commented May 6, 2019

nachocano commented May 7, 2019 •

edited

Loading

nachocano May 8, 2019

Harwayne May 13, 2019

vaikas left a comment

vaikas May 13, 2019

nachocano May 13, 2019

Harwayne May 13, 2019

vaikas May 13, 2019

nachocano May 13, 2019

knative-metrics-robot commented May 13, 2019

vaikas commented May 13, 2019

knative-prow-robot commented May 13, 2019

Harwayne May 13, 2019

Harwayne May 13, 2019

Harwayne May 13, 2019

Harwayne May 13, 2019

Harwayne May 13, 2019

Harwayne May 13, 2019

Harwayne May 13, 2019

		@@ -83,7 +87,7 @@ const (
		)

		// GetGcpPubSubSource returns the GcpPubSub CloudEvent source value.

Adding transformer field to GCPPubSubSource CRD. #398

Adding transformer field to GCPPubSubSource CRD. #398

Conversation

nachocano commented May 6, 2019 • edited Loading

Proposed Changes

Pending

nachocano commented May 6, 2019

nachocano commented May 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaikas 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

knative-metrics-robot commented May 13, 2019

vaikas commented May 13, 2019

knative-prow-robot commented May 13, 2019

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

nachocano commented May 6, 2019 •

edited

Loading

nachocano commented May 7, 2019 •

edited

Loading