adds general purpose event webhook

[mrparkers]

context: #393

opening this as a draft PR so I can get feedback on the implementation. if this is the correct path to go, I'll add some test cases.

[mrparkers]

@stefanprodan I went ahead and added this as a command line flag instead of a webhook for the Canary resource because I wanted to globally configure flagger to always send events to a webhook instead of doing it individually for each Canary - hope that implementation is alright with you

[stefanprodan]

Can you post here the JSON that gets sent?

[mrparkers]

Sure, here's an example:

{
  metadata: {
    name: 'marketsummary.15e76cd80a706334',
    namespace: 'parker-lab-staging',
    creationTimestamp: null
  },
  involvedObject: {
    kind: 'Canary',
    namespace: 'parker-lab-staging',
    name: 'marketsummary',
    uid: '2d2bfe68-2e78-11ea-b1d3-0e68aa341cb7',
    apiVersion: 'flagger.app/v1alpha3',
    resourceVersion: '24563926'
  },
  reason: 'Synced',
  message: 'Starting canary analysis for marketsummary.parker-lab-staging',
  source: { component: 'flagger' },
  firstTimestamp: null,
  lastTimestamp: null,
  type: 'Normal',
  eventTime: null,
  reportingComponent: '',
  reportingInstance: ''
}

[stefanprodan]

I would like to standardise the webhook calls based the current payload object, right now it looks like this:

{
    "name": "podinfo",
    "namespace": "test",
    "phase": "Progressing", 
    "metadata": {
        "key1":  "val1",
        "key2":  "val2",
    }
}

I would just add the timestamp, message and event type to the metadata. What do you think?

[mrparkers]

I don't mind changing the schema for this, but would it be okay to include involvedObject as well? I need to have a reference to the Canary object that flagger is acting upon.

Or perhaps I'm misunderstanding - in the JSON you posted, does name refer to the Canary object? If so, I can definitely use that schema.

[stefanprodan]

Well that's what the current payload has, the canary name, namespace and current phase.

[stefanprodan]

I'm referring to this object https://github.com/weaveworks/flagger/blob/master/pkg/apis/flagger/v1alpha3/types.go#L165

[mrparkers]

Perfect, I'll use that schema then. Thanks for the feedback!

[stefanprodan]

I think the global webhook address could be override by a webhook of type event when present in the canary spec but this can be address at a later time.

[mrparkers]

@stefanprodan I updated the event payload schema:

{
  name: 'marketsummary',
  namespace: 'parker-lab-staging',
  phase: 'Progressing',
  metadata: {
    eventMessage: 'Advance marketsummary.parker-lab-staging canary weight 75',
    eventType: 'Normal',
    timestamp: '1578417122726798559'
  }
}

The only issue I had when testing is that the "New revision detected" event is sent to the webhook with a phase of "Succeeded" instead of "Progressing". I tried to fix this by moving these lines below c.recorder.SetStatus but that didn't seem to fix it.

I don't personally mind this but I'm willing to dig deeper into this if you'd like me to. Although I could use a few tips for what to look at.

[mrparkers]

@stefanprodan I went ahead and marked the PR as "ready for review". please let me know if you're okay with this approach and I can write some tests for what I did.

[stefanprodan]

@mrparkers SetStatus changes the status in cluster but it doesn't alter the canary in-memory copy. I don't think we should move the notification function from where it is, instead we could make a deep copy of the canary object, change the status in-line and send that copy to the notification function.

[stefanprodan]

@mrparkers the PR looks good, besides tests I think we also need to document how this works and give some json examples.

[mrparkers]

@stefanprodan added tests and docs

[stefanprodan]

LGTM

Thanks @mrparkers awesome work!