publish notification to GCP Pub/Sub

[honnix]

TL;DR

Add support of GCP Pub/Sub for publishing workflow execution events.

Type

• Bug Fix
• Feature
• Plugin

Are all requirements met?

• Code completed
• Smoke tested
• Unit tests added
• Code documentation added
• Any pending items have an associated Issue

Complete description

Tested on a GCP project with configuration:

notifications:
  type: gcp
  projectId: "flyte-test"
  publisher:
    topicName: "flyte"
  emailer:
    subject: "Notice: Execution \"{{ name }}\" has {{ phase }} in \"{{ domain }}\"."
    sender:  "[email protected]"
    body: >
      Execution \"{{ name }}\" has {{ phase }} in \"{{ domain }}\". View details at
      <a href=\http://example.com/projects/{{ project }}/domains/{{ domain }}/executions/{{ name }}>
      http://example.com/projects/{{ project }}/domains/{{ domain }}/executions/{{ name }}</a>. {{ error }}

Subscriber and emailer are NOT implemented in this PR, and the emailer configuration is only use to verify the correctness of message body.

Tracking Issue

flyteorg/flyte#268

Follow-up issue

NA

[honnix]

I have a thought around the publishing part.

To me tailoring the message (with rigid email structure) already at publishing stage is a bit too early. I can understand this notification was probably designed for emailing purpose only, however I think it could be used for other things if the message only contains informative fields, e.g. project, domain, error, name, and the subscriber can decide how to reshape it later on.

[katrogan]

To me tailoring the message (with rigid email structure) already at publishing stage is a bit too early

Agreed, we built out the notifications feature with the original intent of just being able to produce email notifications but the eventual goal is to have a rich notification system that spans beyond emails and includes additional user-topic queues and of course, flexible notification message structures. This is all on the back-log unfortunately :(

[kumare3]

@honnix yes, eventual goal is to publish subscribable notifications to some pubsub system. I think the biggest thing we need to do is define the egress structure. But, this is awesome, 💯

[kumare3]

Ideally we should have 2 different structures.
AWS {
Region:
}

GCP {
projectID
}

etc

[honnix]

Yeah that makes sense. Unfortunately that would be a breaking change. How do we handle this kind of change?

[honnix]

Also there are a few other places having Region defined, should we change them all? It does feel out of scope of this PR though.

[honnix]

Played with this idea a bit. PTAL honnix#1

[honnix]

Ping @kumare3

[kumare3]

It is out of the scope of this PR. But, if you can create a follow up issue we are golden :)

[honnix]

I tried a bit to add unit tests, but it seems this is where dependency injection happens, so I guess it is fine to skip unit test than adding another layer of injection?

[honnix]

@katrogan @kumare3 We configured the message body to be a JSON, but due to no proper escaping, it's like playing with fire (as an example we cannot put error there):

  emailer:
    ...
    body: >-
      {
        "project": "{{ project }}",
        "domain": "{{ domain }}",
        "name": "{{ name }}",
        "phase": "{{ phase }}",
        "workflowProject": "{{ workflow.project }}",
        "workflowDomain": "{{ workflow.domain }}",
        "workflowName": "{{ workflow.name }}",
        "workflowVersion": "{{ workflow.version }}",
        "launchPlanProject": "{{ launch_plan.project }}",
        "launchPlanDomain": "{{ launch_plan.domain }}",
        "launchPlanName": "{{ launch_plan.name }}",
        "launchPlanVersion": "{{ launch_plan.version }}"
      }

[kumare3]

@katrogan ^^ did you see the mess

@katrogan @kumare3 We configured the message body to be a JSON, but due to no proper escaping, it's like playing with fire (as an example we cannot put error there):

  emailer:
    ...
    body: >-
      {
        "project": "{{ project }}",
        "domain": "{{ domain }}",
        "name": "{{ name }}",
        "phase": "{{ phase }}",
        "workflowProject": "{{ workflow.project }}",
        "workflowDomain": "{{ workflow.domain }}",
        "workflowName": "{{ workflow.name }}",
        "workflowVersion": "{{ workflow.version }}",
        "launchPlanProject": "{{ launch_plan.project }}",
        "launchPlanDomain": "{{ launch_plan.domain }}",
        "launchPlanName": "{{ launch_plan.name }}",
        "launchPlanVersion": "{{ launch_plan.version }}"
      }

@katrogan did you see this message?

[kumare3]

@honnix can we take this as a separate issue and merge this one. I think your changes look good.

[katrogan]

+1 to filing a separate issue for message formatting

[honnix]

@katrogan @kumare3 Absolutely as it should be. I will create an issue to track that. Thanks for reviewing the PR!

[honnix]

Here it is flyteorg/flyte#293