Update Webhooks Extension to use Triggers 0.4

[a-roberts]

Expected Behavior

Webhooks extension should work with Triggers 0.4

Actual Behavior

It likely won't, lots of breaking changes so far - @dibyom @ncskier I'm sure I've seen a doc somewhere, can you provide a list of what's breaking so far please? I looked through the 0.3 release notes and didn't see anything deprecated but I don't think I'm imagining it

[ncskier]

Looking at what's on the v0.4.0 Milestone and what's been committed already, it looks like this will be the only breaking change:

• Proposal: Change API Group for triggers triggers#332

We're planning on changing the APIGroup to triggers.tekton.dev. If I understand correctly, users will have to delete and recreate their Triggers resources with the new APIGroup when upgrading to the latest Triggers version after this change.

[a-roberts]

All good on Docker Desktop with what's in Triggers so far (master) with 12 webhooks, having a look at OpenShift next with GitLab I'll deploy myself to be sure that's ok too, it'd be handy to have a big many webhook tester, pretty sure you've got a script somewhere @ncskier so wondering if we could make that more official. I'm using my own pipelines hotel branch at my fork with identical (except in name) pipeline/templates/bindings.

I'm doing push events, pull requests, and a tag push.

Note that as part of the testing I'm not taking into account displaying any new Triggers 0.4 features as that'd be Dashboard related, @ncskier are there any that spring to mind?

I realise this is pretty early but this also motivates me to get GitLab deployed for further testing once the Triggers changes do land so we can be ready.

[ncskier]

it'd be handy to have a big many webhook tester, pretty sure you've got a script somewhere @ncskier so wondering if we could make that more official.

My script was pretty rudimentary, and it didn't actually test triggering a large number of triggers (although you could easily modify it to do so). Here's the issue where I posted my script: tektoncd/triggers#356 (comment). Since this script is pretty rudimentary, I don't really think it's worth publishing it/making it more official. However, we definitely should have something official for testing Triggers at a large scale. There's an existing issue for this here tektoncd/triggers#406. If you're interested in working on testing Triggers at a large scale & getting performance metrics, feel free to contribute to this issue 👍

Note that as part of the testing I'm not taking into account displaying any new Triggers 0.4 features as that'd be Dashboard related, @ncskier are there any that spring to mind?

This one is already in master; it probably won't affect the Dashboard, but here it is just in case: tektoncd/triggers#470.

This one hasn't been completed yet, so I don't know if it will make it into the v0.4.0 release or not, but it probably would affect the Dashboard: tektoncd/triggers#371

This one will be a breaking change, but I'm pretty sure it won't affect the Dashboard (also it has not been implemented yet): tektoncd/triggers#332.

There might be some additional features that get merged that are not under the Milestone right now, and I'll try to keep this issue updated in that case.

[a-roberts]

Triggers 0.4 has its first release candidate now so the next step is to get the pipelines in our hotel working, they're at https://github.com/pipeline-hotel/example-pipelines.git. I'll give this a go.

[a-roberts]

Putting this on hold while I look at #502

[a-roberts]

Current issue after moving the k8s dependencies up in our Gopkg.toml:

2020/04/03 15:34:04 Unexpected error running "go build": exit status 2
# github.com/tektoncd/experimental/webhooks-extension/pkg/endpoints
pkg/endpoints/webhook.go:934:39: cannot use client (type "github.com/tektoncd/experimental/webhooks-extension/vendor/k8s.io/client-go/kubernetes/typed/certificates/v1beta1".CertificateSigningRequestInterface) as type context.Context in argument to csr.WaitForCertificate:
        "github.com/tektoncd/experimental/webhooks-extension/vendor/k8s.io/client-go/kubernetes/typed/certificates/v1beta1".CertificateSigningRequestInterface does not implement context.Context (missing Deadline method)
pkg/endpoints/webhook.go:934:39: cannot use csrRecord (type *"github.com/tektoncd/experimental/webhooks-extension/vendor/k8s.io/api/certificates/v1beta1".CertificateSigningRequest) as type "github.com/tektoncd/experimental/webhooks-extension/vendor/k8s.io/client-go/kubernetes/typed/certificates/v1beta1".CertificateSigningRequestInterface in argument to csr.WaitForCertificate:
        *"github.com/tektoncd/experimental/webhooks-extension/vendor/k8s.io/api/certificates/v1beta1".CertificateSigningRequest does not implement "github.com/tektoncd/experimental/webhooks-extension/vendor/k8s.io/client-go/kubernetes/typed/certificates/v1beta1".CertificateSigningRequestInterface (missing Create method)
pkg/endpoints/webhook.go:934:63: cannot use 3600 * time.Second (type time.Duration) as type *"github.com/tektoncd/experimental/webhooks-extension/vendor/k8s.io/api/certificates/v1beta1".CertificateSigningRequest in argument to csr.WaitForCertificate

With @ncskier's help there are no actual Tekton related errors now in our code which so far sits at #503

(I dep ensure -v then kustomize build overlays/development | ko apply -f -.

Perhaps there's a new method we need to use or another dependency to pin.

@akihikokuroda FYI as it's to do with the certificate creation goodies you added a while back, which was all good using

  name = "k8s.io/client-go"
  version = "kubernetes-1.13.4"

instead of

name = "k8s.io/client-go"
version = "v0.16.5"

🤔

[ncskier]

I think that the method signature might've changed (see csr GoDoc).

I think that this fixes the problem... does it achieve the same functionality we had before?

ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(3600*time.Second))
// Even though ctx will be expired, it is good practice to call its
// cancellation function in any case. Failure to do so may keep the
// context and its parent alive longer than necessary.
defer cancel()
csrdata, err = csr.WaitForCertificate(ctx, client, csrRecord)

(this was copied from the context GoDoc)
(this would replace the code here)

[ncskier]

I was able to kustomize build overlays/development | ko apply -f - with the code on my branch here: master...ncskier:triggers-0.4.0-rc1

[a-roberts]

Thanks @ncskier! Running with this now

[a-roberts]

#504 for the Pipeline Hotel changes, almost done making them.

Done - monitor task changes to fix up next, flows are working all fine otherwise. I'm using @AlanGreene's Dashboard PR too for rendering resources OK.

[a-roberts]

all done and working well on OCP 4.3 with Triggers 0.4 RC1, Pipelines 0.11, with my changes (so to both the example pipelines and our webhooks extension code - including RBAC, Go changes and a little JS).

I'll test scalability next making a dozen webhooks with various pipelines.

[a-roberts]

Nearly there, seeing

Checking pipelinerun buildah-pipeline-10-run-5qtn2 in namespace tekton-pipelines
Traceback (most recent call last):
  File "<stdin>", line 74, in <module>
ValueError: list.remove(x): x not in list

when i have lots of webhooks though. I'll address this. Not seeing any Triggers specific problems though.

That's weird since the code has:

    if missingDataEntry in runsMissing:
                runsMissing.remove(data)

data should be getting added to it, so I'll debug and print what data is compared to missingDataEntry. Also, my run hasn't even gone missing.

[a-roberts]

Deletion has a problem too (at least in bulk) so have to fix this too. Specifically, the error notification appears but they actually are gone. Nothing interesting in the logs.

[a-roberts]

Got it working a treat, happy with Triggers 0.4 @ncskier. I'm going to look into the issue @dibbles raised around deletion of webhooks with the same name next, and why an error is reported deleting many webhooks at once.

I've also fixed a bug (it really should be runsMissing.remove(missingDataEntry)), not data.

Then once this is merged and the dashboard changes are in, we can do a 0.6.1 release.

[a-roberts]

Merged so closing