Automatically update pipelines webhook #48

Skarlso · 2018-07-20T21:39:55Z

This currently a working end-point for accepting and parsing the webhooks. I'm opening a PR for conversation purposes. I'm debating whether to make the user create a webhook or have that authentication process in Gaia complete with callback.

I'm all for making it as easy as possible for a user and probably a user would say the same thing. Which means Gaia will have to redirect and ask for access to the repository in order to send a create webhook request to Github.

Thoughts?

Skarlso · 2018-07-21T07:46:28Z

@michelvocks Okay, so I went with the easiest solution here and the least possible impact in either the workflow or the user experience. I'm using a github token which the user needs to provide for now as an environment property later on from a secret file of some sort or maybe both.

Right now everything is hardcoded as I was testing if the authentication and the creation is working fine. Here you can see that the hook is created for said repository:

This also means that I have to provide a callback url. Which means I will need to be able to determine where and under what address gaia is currently deployed at. Which I'm sure I can. :)

Also pinging @rmb938 for the glory of the Sontaran Empire.

And to see if you are satisfied with the approach laid out. :)

Skarlso · 2018-07-21T07:47:46Z

Makefile

@@ -4,7 +4,7 @@ GO_LDFLAGS_STATIC=-ldflags "-s -w -extldflags -static"
 default: dev

 dev:
-	go run ./cmd/gaia/main.go -homepath=${PWD}/tmp -dev=true -poll=true


I haven't noticed that I put this in. 😊

I guess you can leave that in. Especially when you do pipeline testing this is really helpful! 😄

Skarlso · 2018-07-21T07:49:09Z

handlers/hook.go

+		return c.JSON(400, e)
+	}
+
+	gaia.Cfg.Logger.Info("received: ", h.Event)


Actually, I don't care about the event at all. And in any other case, I'm thinking it's only enough to verify the signature. Since the hook is only for one event, this data here is meaningless. The hook will only ever be called on a push. If there is an event, we build the specific pipeline.

Right. We just have to make sure that this event triggers the right pipeline 😃

Skarlso · 2018-07-21T07:50:05Z

pipeline/create_pipeline.go

+		// TODO: Add repo VCS type information?
+		// TODO: Q: Also, save the webhook so we can later find the pipeline associated with it?
+		// A: Don't think so, because the hook will contain the name of the repository.
+		subscribeRepoWebhook(&p.Pipeline.Repo)


@michelvocks This also means that webhook automatic building will never be available for drop-in pipelines as this needs repository information. Are you okay with this? Or shall we try and do some workaround magic?

No, this is absolutely fine! If you just drop your pipeline binary into the pipeline folder, this pipeline cannot use the poll functionality as well as the webhook. 👍

Skarlso · 2018-07-21T08:19:01Z

pipeline/git.go

+	)
+	tc := oauth2.NewClient(ctx, ts)
+	config := make(map[string]interface{})
+	config["url"] = "https://example.com/callback"


So, unless echo is running and a request was received I can't determine where gaia is deployed at from the back-end side. Maybe the admin site can't, I'm not big on NPM. However, that leaves me with config options maybe. :/ Which further convolutes the huge configuration flags by adding -host=example.com or something like that.

I see the config refactoring approaching ever faster. :) What say you?

Good point. I know from many tools (gitlab for example) that you have to provide the domain name where the application is accessible from. So I guess adding a configuration flag is fine here.

I actually think we should do both. The admin can provide the domain name (which will be used for sure) and if it's not provided we try to determine it after the first request came in. What do you think?

I agree that admin can provide a hostname which overwrites the one provided by the flag. I guess we can try. The first request would be a user login?

michelvocks

Good job so far! 🤗
This is really nice for github users. But I think we should also support non-github users. So we should also provide the end-user just an URL and a secret and that's it. 😄

I'm also not a fan of providing the github token via environment variable/flag etc. What happens if you have multiple github accounts?

I think it should be possible to provide a github token per pipeline via the UI. What do you think?

michelvocks · 2018-07-21T15:21:31Z

pipeline/create_pipeline.go

+		// TODO: Add repo VCS type information?
+		// TODO: Q: Also, save the webhook so we can later find the pipeline associated with it?
+		// A: Don't think so, because the hook will contain the name of the repository.
+		subscribeRepoWebhook(&p.Pipeline.Repo)


No, this is absolutely fine! If you just drop your pipeline binary into the pipeline folder, this pipeline cannot use the poll functionality as well as the webhook. 👍

michelvocks · 2018-07-21T15:28:09Z

pipeline/git.go

+	)
+	tc := oauth2.NewClient(ctx, ts)
+	config := make(map[string]interface{})
+	config["url"] = "https://example.com/callback"


Good point. I know from many tools (gitlab for example) that you have to provide the domain name where the application is accessible from. So I guess adding a configuration flag is fine here.

I actually think we should do both. The admin can provide the domain name (which will be used for sure) and if it's not provided we try to determine it after the first request came in. What do you think?

rmb938 · 2018-07-21T16:27:20Z

Looks good so far! I do have a few nitpicks though (of course lol).

Since this webhook is specific to github could we have it under the /pipeline/github/hook path?

I.e someone may want to add in the future a gitlab hook, or a bitbucket hook, ect..
This would also allow us to add a "Generic" hook that someone could just call from curl or something.

Actually thinking about it maybe put it under pipeline/github/build-hook or something since the hook is specific to rebuilding the code vs triggering a pipeline 🤷‍♂️

Also I don't know if we actually need to register the hook in github during pipeline creation, doing that kinda locks us to github specifically and will probably add some more config options for github creds. Unless we want to have this be a global config option to say which git system we are using.

Skarlso · 2018-07-21T18:21:34Z

@rmb938 @michelvocks Thanks guys. This is all very good feedback and I agree with the both of you. :)

Skarlso · 2018-07-21T18:22:35Z

@michelvocks I agree with you on the admin thingy. And I probably need you help with that since I'm not very big on front-end stuff and vue. :D

Also yeah on the UI github token and the separation of github.

@rmb938 Also also agree on the whole github thing being restricted into it's own environment.

And also, also, also on the fact that it might not should create a hook on pipeline create. The UI should probably have a flag for that or something like Michel is saying.

michelvocks · 2018-07-21T20:47:22Z

@Skarlso Yes sure. Just let me know where I can help explicitly.

Skarlso · 2018-07-22T19:17:54Z

So, what I'm going to do in this Pr is the following... @michelvocks @rmb938.

Not going to save the git token as there is, for now, no reason for it. It's only used once, and that's when the pipeline is created and the hook is sent.
Modify the UI to have a checkbox that the user want's a webhook, and if it's checked, popup a field where a github TOKEN is requested. ( make apparent that it's not saved at this time ). (( I imagine that this would be a different set of fields for a bitbucket hook. For git a token is enough )).
This means deleting a hook is not handled currently ( that will be a future feature after we save the token in the form of issue Secret store for passwords, tokens, api keys and more #51 (handling secrets via a vault).
Modify the auto-updater to be opt-in and save a flag for a pipeline saying (autoupdate - true) because maybe you don't always want all your pipeline to update. Thoughts on this?
Add a flag called hostname to use for the callback url.

I think that's all.

codecov-io · 2018-07-22T19:29:56Z

Codecov Report

Merging #48 into master will increase coverage by 0.47%.
The diff coverage is 70.5%.

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
+ Coverage   64.07%   64.55%   +0.47%     
==========================================
  Files          21       22       +1     
  Lines        1751     1879     +128     
==========================================
+ Hits         1122     1213      +91     
- Misses        512      535      +23     
- Partials      117      131      +14

Impacted Files	Coverage Δ
security/vault.go	`76.69% <0%> (ø)`	⬆️
handlers/handler.go	`85.13% <100%> (+1.07%)`	⬆️
pipeline/create_pipeline.go	`50% <28.57%> (-2.84%)`	⬇️
handlers/hook.go	`62.5% <62.5%> (ø)`
pipeline/git.go	`69.33% <79.41%> (+8.22%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 312ae68...ee8b7b5. Read the comment docs.

Skarlso · 2018-07-22T21:10:17Z

So this works right now:

2018/07/22 23:08:47 hook created:  "web" 201 Created
2018/07/22 23:08:47 hook url:  https://api.github.com/repos/Skarlso/go-example/hooks/39614928

button:

The only two problems are that the token is saved if the pipeline errors out and second, I still have no way of providing and saving a secret which brings us back to #51. :/

@michelvocks?

Skarlso · 2018-07-22T21:33:54Z

Yeah so I think this pr is blocked until I can inject a secret from a managed secret store via #51.

michelvocks · 2018-07-23T06:15:15Z

The only two problems are that the token is saved if the pipeline errors out and second, I still have no way of providing and saving a secret which brings us back to #51. :/

What do you mean exactly with "pipeline errors out"?
Especially when an error happened during the create pipeline step, the github token should not be stored at all.

Not going to save the git token as there is, for now, no reason for it. It's only used once, and that's when the pipeline is created and the hook is sent.

I like that a lot until we have #51 implemented! 🤗

Modify the UI to have a checkbox that the user want's a webhook, and if it's checked, popup a field where a github TOKEN is requested. ( make apparent that it's not saved at this time ). (( I imagine that this would be a different set of fields for a bitbucket hook. For git a token is enough )).

Awesome! Maybe move the button next to the "Add credentials" button and change the icon (I know this is fine tuning and will be done later 👍 ).

This means deleting a hook is not handled currently ( that will be a future feature after we save the token in the form of issue #51 (handling secrets via a vault).

That's fine I guess. We also have to think about what happens when the flag "hostname" is changed? We might need to update all webhooks in this case.

Modify the auto-updater to be opt-in and save a flag for a pipeline saying (autoupdate - true) because maybe you don't always want all your pipeline to update. Thoughts on this?

Like that. We somehow need to integrate that into the create pipeline view. 😄

Add a flag called hostname to use for the callback url.

Perfect. ❤️

Skarlso · 2018-07-23T09:35:43Z

That's fine I guess. We also have to think about what happens when the flag "hostname" is changed? We might need to update all webhooks in this case.

Oh f*ck. :D That didn't occur to me before. :)

rmb938 · 2018-07-24T13:20:11Z

Well like jenkins, drone ci, ect... if the admin updates the hostname the hooks just break. No one is trying to fix that (because it's hard) so I don't think we should try to.

rmb938 · 2018-07-24T13:21:46Z

Also I don't think we need a hostname flag, I know gin has a middleware for getting the request's hostname https://github.com/gin-contrib/location so I'm sure echo has it/won't be hard to add it.

Skarlso · 2018-07-26T12:49:02Z

@rmb938 It does, it just feels hacky to place such a thing on the first whatever request that gets fired off. I don't think it's easily testable, or maintainable. What if the end-point gets moved and the person moving it doesn't realize that there is a critical thing in there for the webhooks. Namely, detect the host and set it. Or even worse, they put it into a different location but that happens to be after a request for webhook creation is fired off.

I think it's just easier and cleaner to have a setting where you explicitly set it, and that's that. No magic, no fuss, no muss. Clean.

That's how I feel about it at least.

rmb938 · 2018-07-26T13:15:36Z

Makes sense :)

I don't really have a strong preference one way or another tbh.

michelvocks · 2018-07-26T19:00:23Z

I agree with @Skarlso 😄
We cannot 100% rely on the detection. And if there is an error or a user has a problem, we can always point to this parameter. 🤗

But I actually like that we don't support the update stuff for the webhooks. If the hostname changes, all webhooks are broken. Updating can be hard for sure.

bidhan-a · 2018-08-14T18:04:19Z

Correct me if I'm wrong but this PR doesn't support non-Github webhooks?

Skarlso · 2018-08-14T19:26:33Z

@bidhan-a You are not wrong. Since non-github webhook require a complete different way of registering them with a different library and api, this PR deals with github only.

Other hooks, like gitlab and bitbucket will be in a separate PR. :)

michelvocks

Just a few remarks 🤗 Looks already good!

michelvocks · 2018-08-15T09:50:58Z

cmd/gaia/main.go

@@ -37,6 +37,7 @@ func init() {
 	// command line arguments
 	flag.StringVar(&gaia.Cfg.ListenPort, "port", "8080", "Listen port for gaia")
 	flag.StringVar(&gaia.Cfg.HomePath, "homepath", "", "Path to the gaia home folder")
+	flag.StringVar(&gaia.Cfg.Hostname, "hostname", "https://localhost", "The host's name under which gaia is deployed at exp: https://gaia-pipeline.com")


what does exp mean? 😄

Example. :D

maybe e.g.? or for example? 😄

Yeah that's definitely better. :D

michelvocks · 2018-08-15T09:52:26Z

frontend/client/views/pipeline/create.vue

@@ -32,6 +32,12 @@
                  </span>
                  <span>Add credentials</span>
                </a>
+                <a class="button is-primary" v-on:click="showGitHubWebHookModal">
+                  <span class="icon">
+                    <i class="fa fa-certificate"></i>


Would be awesome if you could change the icon for user experience 😃 see https://fontawesome.com/icons?d=gallery most of them are available

Maybe https://fontawesome.com/icons/wrench?style=solid ?+

Sure thing. :) Thanks! :)

Skarlso · 2018-08-15T19:47:03Z

@michelvocks Before doing anything I wanted to run a quick manual. Turns out I had a few bugs in there.. One, I was generating the password at a wrong location. :/ Second, I was sending off the wrong URL for the webhook callback. 🗡

Unfortunately, because I needed the API version I had to shift the constant into gaia.go where the other constants are lying. That's why there are now more edits.

Skarlso · 2018-08-15T19:49:15Z

@michelvocks umm... dude... The scheduler threw a data race. :/

2018-08-15T21:47:24.501+0200 [DEBUG] Gaia: error during job execution: error="job failed" job="{2296842551 Job1  [] running}"
panic: send on closed channel

goroutine 13 [running]:
github.com/gaia-pipeline/gaia/scheduler.(*Scheduler).resolveDependencies(0xc420020f00, 0x89e70aca, 0x11e6932, 0x4, 0x0, 0x0, 0xc42000e2d0, 0x1, 0x1, 0x11ea8ba, ...)
        .../golang/src/github.com/gaia-pipeline/gaia/scheduler/scheduler.go:376 +0x2f0
github.com/gaia-pipeline/gaia/scheduler.(*Scheduler).resolveDependencies(0xc420020f00, 0x8ae70c5d, 0x11e6936, 0x4, 0x0, 0x0, 0xc42000e2d8, 0x1, 0x1, 0x11ea8ba, ...)
        .../golang/src/github.com/gaia-pipeline/gaia/scheduler/scheduler.go:358 +0x223
created by github.com/gaia-pipeline/gaia/scheduler.(*Scheduler).executeScheduler
        ...golang/src/github.com/gaia-pipeline/gaia/scheduler/scheduler.go:429 +0x2c9

Skarlso · 2018-08-15T19:52:16Z

To be fair running it with --race only failed once out of 10 runs. :/ Which is quiet shitty. But it still failed. So we'll have to take care of that. But to be faire, I'm not going to do this in this pr if you don't mind.

…so fixed the url it was sending.

Skarlso · 2018-08-16T05:06:34Z

I'm getting invalid signature on the real deal.. I need to investigate why. :/

Skarlso · 2018-08-16T06:41:23Z

Everything works!! Yiiss!!

Couple of things:

Password generated wasn't accepted because it had special characters.
The pipeline was actually not being built because it didn't select the branch which was selected
The webhook URL was not open to github. It's open now which is safe as it has it's own authentication procedures using secret and header checks.

Skarlso · 2018-08-16T06:42:41Z

2018-08-16T08:39:55.517+0200 [DEBUG] Gaia: checking pipeline: : asdf=<unknown>
2018-08-16T08:39:55.517+0200 [DEBUG] Gaia: selected branch : : refs/heads/dependson=<unknown>
2018-08-16T08:39:56.709+0200 [DEBUG] Gaia: updating pipeline: : asdf=<unknown>
2018-08-16T08:39:58.052+0200 [DEBUG] Gaia: successfully updated: : asdf=<unknown>

POST /api/v1/pipeline/githook  200 OK

That image is from the webhook's webpage in github. It shows that it successfully sent a push event request. I'm using ngrok to create and open tunnel to the local service.

michelvocks · 2018-08-17T06:37:12Z

pipeline/git.go

+		return err
+	}
+	gaia.Cfg.Logger.Info("hook created: ", github.Stringify(hook.Name), resp.Status)
+	gaia.Cfg.Logger.Info("hook url: ", hook.Geturl(""))


should be gaia.Cfg.Logger.Info("hook url ", "url", hook.Geturl(""))
Otherwise we get ugly log output like hook url: : asdf=<unknown> 😄
It's always (initial_log_message, key=string, value)

michelvocks · 2018-08-17T06:38:20Z

pipeline/git.go

 	})
 	if err != nil {
 		// It's also an error if the repo is already up to date so we just move on.
-		gaia.Cfg.Logger.Error("error while doing a pull request : ", err.Error())
+		message := "error while doing a pull request: " + err.Error()
+		gaia.Cfg.Logger.Error(message)


gaia.Cfg.Logger.Error("error while doing a pull request", "error", err.Error())

michelvocks · 2018-08-17T06:39:22Z

pipeline/git.go

+func createGithubWebhook(token string, repo *gaia.GitRepo, gitRepo GithubRepoService) error {
+	vault, err := services.VaultService(nil)
+	if err != nil {
+		gaia.Cfg.Logger.Error("unable to initialize vault: " + err.Error())


gaia.Cfg.Logger.Error("unable to initialize vault", "error", err.Error())

michelvocks · 2018-08-17T06:39:45Z

pipeline/git.go

+
+	err = vault.LoadSecrets()
+	if err != nil {
+		gaia.Cfg.Logger.Error("unable to open vault: " + err.Error())


gaia.Cfg.Logger.Error("unable to open vault", "error", err.Error())

Addressed Comments.

Skarlso added 3 commits July 20, 2018 23:33

POC: Implement handling of push event.

69cdfda

Some additional comments.

9e70848

Working hook creation.

e1643fe

Skarlso commented Jul 21, 2018

View reviewed changes

michelvocks reviewed Jul 21, 2018

View reviewed changes

Added go-github dependency.

1da13f1

Working hook creation through a UI provided github token.

7c04afc

Skarlso changed the title ~~[POC][WIP] Automatically update pipelines webhook~~ [BLOCKED][WIP] Automatically update pipelines webhook Jul 23, 2018

Skarlso mentioned this pull request Jul 28, 2018

A secret vault storage with AES encryption using a certificate #56

Merged

5 tasks

Skarlso changed the title ~~[BLOCKED][WIP] Automatically update pipelines webhook~~ Automatically update pipelines webhook Aug 2, 2018

Skarlso added the Needs Work The PR still requires some work from the submitter. label Aug 2, 2018

Skarlso requested a review from michelvocks August 14, 2018 16:12

michelvocks reviewed Aug 15, 2018

View reviewed changes

Skarlso force-pushed the automatically_update_pipelines_webhook branch from 35e95ea to 7b69b60 Compare August 15, 2018 19:52

Fixed the password generation issue. Now it is in the right place. Al…

030a068

…so fixed the url it was sending.

Skarlso force-pushed the automatically_update_pipelines_webhook branch from 7b69b60 to 030a068 Compare August 15, 2018 20:03

Skarlso added Needs Work The PR still requires some work from the submitter. and removed needs review labels Aug 16, 2018

Skarlso added Ready To Merge PR is ready to be merged into master Needs Work The PR still requires some work from the submitter. and removed Needs Work The PR still requires some work from the submitter. Ready To Merge PR is ready to be merged into master labels Aug 16, 2018

Fixed tests and correct creation of secret so github accepts it.

61938f4

Skarlso force-pushed the automatically_update_pipelines_webhook branch from 90e32d5 to 61938f4 Compare August 16, 2018 08:58

michelvocks previously requested changes Aug 17, 2018

View reviewed changes

Addressed comments.

5a524ed

michelvocks added 2 commits August 18, 2018 20:13

Merge branch 'master' into automatically_update_pipelines_webhook

bce7cae

dep ensure again

ee8b7b5

michelvocks merged commit ee6f795 into gaia-pipeline:master Aug 18, 2018

Skarlso mentioned this pull request Aug 19, 2018

Automatically rebuild the pipeline when a new commit is pushed to the git repo #31

Closed

Automatically update pipelines webhook #48

Automatically update pipelines webhook #48

Uh oh!

Conversation

Skarlso commented Jul 20, 2018

Uh oh!

Skarlso commented Jul 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michelvocks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmb938 commented Jul 21, 2018

Uh oh!

Skarlso commented Jul 21, 2018

Uh oh!

Skarlso commented Jul 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michelvocks commented Jul 21, 2018

Uh oh!

Skarlso commented Jul 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Skarlso commented Jul 22, 2018

Uh oh!

Skarlso commented Jul 22, 2018

Uh oh!

michelvocks commented Jul 23, 2018

Uh oh!

Skarlso commented Jul 23, 2018

Uh oh!

rmb938 commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmb938 commented Jul 24, 2018

Uh oh!

Skarlso commented Jul 26, 2018

Uh oh!

rmb938 commented Jul 26, 2018

Uh oh!

michelvocks commented Jul 26, 2018

Uh oh!

bidhan-a commented Aug 14, 2018

Uh oh!

Skarlso commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michelvocks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Skarlso commented Jul 21, 2018 •

edited

Loading

Skarlso commented Jul 22, 2018 •

edited

Loading

codecov-io commented Jul 22, 2018 •

edited

Loading

rmb938 commented Jul 24, 2018 •

edited

Loading

Skarlso commented Aug 14, 2018 •

edited

Loading

Skarlso commented Aug 15, 2018 •

edited

Loading

Skarlso commented Aug 16, 2018 •

edited

Loading