Add tool code #1

corest · 2019-01-17T17:16:48Z

Towards https://github.com/giantswarm/giantswarm/issues/3103

corest · 2019-01-17T17:35:32Z

endpoint/endpoint.go

+		response, _ = json.Marshal(healthCheckResponse{Status: "webhook request received"})
+		writeJSONResponse(w, http.StatusOK, response)
+
+		if h.Event.Ref == masterRef && stringInSlice(h.Event.Repository.Name, o.repositories) {


rule is created only for merges into master and for configured repositories

corest · 2019-01-17T17:35:59Z

endpoint/endpoint.go

+		},
+	}
+
+	ttl := time.Now().Add(routingRuleTTL).UTC().Unix()


default ttl for automatically created rules is 1h

corest · 2019-01-17T17:36:53Z

endpoint/endpoint.go

+		return microerror.Maskf(userNotFoundError, event.Pusher.Name)
+	}
+	routingRule := &opsgenie.RoutingRule{
+		Name:       fmt.Sprintf("autooncall-%s-%s", event.HeadCommit.ID[:5], strconv.FormatInt(ttl, 10)),


rule name is prefixed with auto-oncall+first 5 symbols of the head commit

corest · 2019-01-17T17:37:22Z

endpoint/endpoint.go

+
+	conditions := []opsgenie.Rule{
+		opsgenie.Rule{
+			Value: event.Repository.Name,


conditions list include only the repository name for now. That can be extended later

corest · 2019-01-17T17:37:53Z

helm chart be added in separate PR

MarcelMue

I assume that you didn't want to use microkit because of the overhead? Would be cool to align this a bit more, also with some health endpoint maybe and then deploy it to ginger or something.

Nice work overall!

MarcelMue · 2019-01-17T20:46:17Z

LICENSE

+      same "printed page" as the copyright notice for easier
+      identification within third-party archives.
+
+   Copyright 2016-2017 Giant Swarm GmbH


Should be something else, I think 2016-2019? Not sure right now.

MarcelMue · 2019-01-17T20:51:15Z

main.go

+var help = flag.Bool("help", false, "show help for this tool")
+
+func main() {
+	flag.Set("logtostderr", "true")


You could just use the viper / cobra standard magic boilerplate here. Could safe you some hassle later.

As there is only one flag I really need, defining flag structure for viper wasn't a time-saver

MarcelMue · 2019-01-17T20:53:56Z

main.go

+		panic(fmt.Sprintf("%#v", err))
+	}
+
+	var config endpoint.Config


It would also be neat to have a /version endpoint which we can use to check if the project is coming up with architect and so on.

Frankly speaking, I didn't even plan to setup ci for this tool. Just build once, push into quay and run.

Please setup CI and CD for this. Doesn't make sense to handle projects differently.

I'm fine with CI, but not sure about CD. It is installed as application into tenant cluster, so imo docs in wiki for support process with configuration example should be enough. I don't want to make a part of control-plane deployment.

docs and website are also deployed automatically. imo CD is a must.

service/githubhook/githubhook.go

Co-Authored-By: corest <[email protected]>

corest · 2019-01-17T21:17:48Z

I've deployed it into our production site cluster and configured webhook handler for repository https://github.com/giantswarm/test-oncall. You can play with it by merging your PRs into that repository and check routingrules after

…development

corest · 2019-01-18T06:34:49Z

For the health I've added default handler as it also required for letsencrypt to validate certs

kopiczko

Fist batch. I focused mostly on Hook struct.

service/githubhook/githubhook.go

kopiczko · 2019-01-18T08:22:14Z

service/githubhook/hook.go

+//
+// Implements validation described in github's documentation:
+// https://developer.github.com/webhooks/securing/
+func (h *Hook) signedBy(secret []byte) bool {


This shouldn't be a method. This should be a function. We should avoid adding methods to pure data types. Exceptions may be things like String or MarshalToJson.

kopiczko · 2019-01-18T08:22:45Z

service/githubhook/hook.go

+}
+
+// New reads a Hook from an incoming HTTP Request.
+func New(req *http.Request, secret []byte) (hook *Hook, err error) {


This should not return a pointer unless memory is a concern (which it isn't).

corest · 2019-01-18T10:40:50Z

As I understood @xh3b4sd meant using microkit/microserver, server/service structure eg following api repo. I just didn’t want to bring all that complexity I don’t need in this project. But for the sake of standardization, I’m fine to rewrite it with general approach. Overall didn’t want to invest here that much time, but that also true it will be hard to support that in case we’ll need to update something.

Thx for review points. I’ll refactor it following all the suggestions

xh3b4sd · 2019-01-18T10:45:27Z

Sounds good. Let me know when you need anything.

corest · 2019-01-18T21:23:30Z

server/endpoint/webhook/endpoint.go

+			response.StatusCode = http.StatusOK
+		}
+
+		go e.Service.Webhook.Process(h)


I'm not sure about this approach. I didn't find an example of async stuff. After receiving a hook I don't want to wait for opsgenie and also Github shouldn't care if routingrule was created. It just needs approve that webhook was delivered

To me it looks like we should rather implement an operator. We either process a request synchronously or create a CR in the first place and let it reconcile. Turning the request here into some background job will cause magic failures eventhough responses from the endpoint where successful. Then users try to do it again because nothing happened and then the system is in an inconsistent state.

What is the desired workflow this new service should be part of? Can you describe that or provide a simple diagram? We can maybe also hang and talk a bit to sort this out.

Operator? No. That's would be too much for this.
Deployment event happens -> github sends webhook to auto-oncall -> app dispatches payload and responds to github it received valid webhook -> async process creates routingrule

I don't see why the processing has to be split out of the sync request. I think it is just fine then to remove the goroutine and add some backoff to be a bit more safe.

Processing is separated from the sync, because the success of creating a routing rule is not a part of the response to github about webhook state. Github cares about the delivery of the webhook with payload, not about internal processing. E.g., github shouldn't get error about webhook delivery if opsgenie is down. At least that's how I see it

That makes sense. Then let's go with this and iterate in it. I think for the future it would make sense to create a CR synchronously and have an operator reconcile it accordingly. That way all logic is more or less guaranteed. Right now it is not, will fail once and be forgotten.

👍
all other review points have been addressed.

corest · 2019-01-18T21:32:40Z

Refactored a bit. Now I couldn't tell that is this project for. But structure was followed :D

corest · 2019-01-19T14:43:38Z

Also I changed logic.
Now instead of push events, deployment events are used.
If deployment event was created by taylorbot - app will get author of reference commit and create for him routing rule. If you used your own github api key and created deployment event with opsctl - routingrule will be automatically created for your deployment.

teemow · 2019-01-21T07:18:08Z

Nice! 👍

corest · 2019-01-21T09:27:23Z

@xh3b4sd ptal

xh3b4sd

Note the binary is committed. Let's remove it and have a .gitignore.

xh3b4sd · 2019-01-21T13:28:30Z

Gopkg.toml

+#   go-tests = true
+#   unused-packages = true
+
+[prune]


Here are no dependencies at all. Maybe try the init again.

xh3b4sd · 2019-01-21T13:32:03Z

README.md

+auto-oncall application is a webhook handler, responsible for creating new Opsgenie routing rules on every deployment event.
+
+# configuration
+Configuration requires next data to be configured in `values.yaml` of the helm chart:


That looks like it should be a secret and not a configmap. As which is it used?

It is a secret, that's just a values file for helm, which used for generating templates.

helm comes in separate branch https://github.com/giantswarm/auto-oncall/blob/add_hel/helm/auto-oncall-chart/templates/secret.yaml

server/endpoint/endpoint.go

service/service.go

xh3b4sd · 2019-01-21T19:49:47Z

service/webhook/event.go

@@ -0,0 +1,29 @@
+package webhook


The naming of the file is a bit off. There is no Event type in event.go. We usually call files in which we gather specs and interfaces spec.go, especially when there are lots of different types.

xh3b4sd · 2019-01-21T19:56:34Z

service/webhook/service.go

+func (s *Service) createRoutingRule(event DeploymentEvent) error {
+	var err error
+
+	var opsGenieService *opsgenie.OpsGenie


The opsgenie service should be configured as a dependency at program boot. There is no reason to do it over and over again at runtime.

xh3b4sd · 2019-01-21T19:57:02Z

service/webhook/service.go

+		User:       user,
+	}
+
+	err = opsGenieService.CreateEscalation(routingRule)


The different calls could be performed in parallel if performance is an issue for the service.

nope, escalation and routing rule are related entities. Routing rule depends on escalation

Co-Authored-By: corest <[email protected]>

…development

xh3b4sd

Pretty nice work. I think that goes into the right direction.

xh3b4sd · 2019-01-22T08:43:31Z

Gopkg.toml

+
+[[constraint]]
+  name = "github.com/giantswarm/opsctl"
+  version = "9923241.0.0"


Is that right? It does not look right.

That is what dep generated for me

tuommaki

Bit late to the parties, but added a notion of possibility of using string map for users. Not compulsory, but could improve maintainability of that user map.

tuommaki · 2019-01-22T08:50:57Z

main.go

+	daemonCommand := newCommand.DaemonCommand().CobraCommand()
+
+	daemonCommand.PersistentFlags().String(f.Service.Oncall.OpsgenieToken, "", "Opsgenie API token.")
+	daemonCommand.PersistentFlags().String(f.Service.Oncall.Users, "", "github_id:opsgenie_id mapppings, separated by comma.")


Would it make sense to use map[string]string for Users? See https://godoc.org/github.com/spf13/pflag#FlagSet.GetStringToString and spf13/pflag#133 so it should work --service.oncall.users=github_id0=opsgenie_id0,github_id1=opsgenie_id1,... - then one can write user mappings as a YAML map in installations file and do something similar to what we did with string slices here: https://github.com/giantswarm/aws-operator/blob/master/helm/aws-operator-chart/templates/03-configmap.yaml#L18

Later when querying for value, I think you would just do: viper.GetStringMapString(f.Service.Oncall.Users) and get a map of users.

I'm using map[string]string for Users on the service level. But with flag this magic didn't work for me. It just fails to automatically dispatch that flag into map[string]string

corest added 4 commits January 17, 2019 19:10

Add github hook handler

3a50eb6

Add endpoint code

bc1860e

Add vendoring

1bd2bf2

Add licenses

6189d5e

corest self-assigned this Jan 17, 2019

Add readme file

dc6a30e

corest force-pushed the development branch from 5ae6c2e to dc6a30e Compare January 17, 2019 17:19

corest added 3 commits January 17, 2019 19:21

Vendor latest

8940e16

Fix dockerfile

860ba37

Remove circleci

1e077f5

corest requested review from a team January 17, 2019 17:34

corest commented Jan 17, 2019

View reviewed changes

corest added 2 commits January 17, 2019 22:13

Fix main.go

d365e02

Add default handler

3cd798f

MarcelMue reviewed Jan 17, 2019

View reviewed changes

corest and others added 2 commits January 17, 2019 23:08

Fix license

0cbc110

Apply suggestions from code review

eeb97b4

Co-Authored-By: corest <[email protected]>

corest added 3 commits January 17, 2019 23:54

Add more logging

0f816de

Merge branch 'development' of github.com:giantswarm/auto-oncall into …

8a9941a

…development

Change name format

c082d0e

kopiczko reviewed Jan 18, 2019

View reviewed changes

Address review

873bbde

kopiczko reviewed Jan 18, 2019

View reviewed changes

corest added 7 commits January 18, 2019 17:08

devcommit

99d0098

Add circle configuration

8114866

Refactor code

8902f60

Vendor latest

9e92209

User standard configs approach

c426eb1

Add config tags

254a6ed

Fix circleci folder name

17c9532

corest force-pushed the development branch from e29d30d to 17c9532 Compare January 18, 2019 21:20

Remove env vars

27128e6

corest commented Jan 18, 2019

View reviewed changes

corest added 3 commits January 19, 2019 00:58

Fix endpoint

d534c12

Use deployment events

d444404

Update logic

2a32fd7

xh3b4sd reviewed Jan 21, 2019

View reviewed changes

xh3b4sd and others added 6 commits January 21, 2019 22:15

Apply suggestions from code review

49202e4

Co-Authored-By: corest <[email protected]>

Remove binary

144a3d7

Merge branch 'development' of github.com:giantswarm/auto-oncall into …

1eee11f

…development

Rename spec file

286d438

Update vendoring

86060c7

Move opsgenie service definition into service

bc6703c

xh3b4sd approved these changes Jan 22, 2019

View reviewed changes

tuommaki approved these changes Jan 22, 2019

View reviewed changes

corest merged commit 1e93fa1 into master Jan 22, 2019

corest deleted the development branch January 22, 2019 09:19

Add tool code #1

Add tool code #1

Conversation

corest commented Jan 17, 2019 • edited Loading

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

corest commented Jan 17, 2019

MarcelMue 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corest commented Jan 17, 2019

corest commented Jan 18, 2019

kopiczko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corest commented Jan 18, 2019

xh3b4sd commented Jan 18, 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

corest Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corest commented Jan 18, 2019

corest commented Jan 19, 2019

teemow commented Jan 21, 2019

corest commented Jan 21, 2019

xh3b4sd left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xh3b4sd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuommaki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corest commented Jan 17, 2019 •

edited

Loading

corest Jan 21, 2019 •

edited

Loading

xh3b4sd left a comment •

edited

Loading