VYGR-391: Add opsgenie integrations to notifications by fcobb · Pull Request #120 · atlassian/voyager

fcobb · 2019-02-05T04:28:41Z

What: This PR uses the Opsgenie integration manager client to add Opsgenie integrations into the service-metadata configmap, allowing autowiring functions to make use of the integrations.
Opsgenie integration manager client PR: #119

Manually tested the following scenarios in the playground:

Creating/deploying a service without opsgenie team then adding it, verifying that the configmap was updated.
Services with invalid opsgenie team will return an error back through processing.
Services without Opsgenie team will successfully process.

Remove opsgenie secret work-in-progress

fcobb · 2019-02-15T03:44:36Z


-func (c *Controller) buildNotifications(spec creator_v1.ServiceSpec) (*orch_meta.Notifications, error) {
+func (c *Controller) buildNotifications(spec creator_v1.ServiceSpec) (*orch_meta.Notifications, bool /* retriable */, error) {
+	// Default pagerduty values re-used from Micros config.js


Instead of return this at the end we now instantiate the notifications with defaults and override it if the pagerduty or opsgenie notifications creation is successful.

nilebox · 2019-02-15T04:02:47Z

-		}, nil
+		}
+	}
+	integrations, retriable, err := c.getOpsgenieIntegrations(spec.Metadata.Opsgenie)


what happens if team is not specified in Service Central? Will we get an empty list of integrations?

I do a check at the start of the called function to see if spec.Metadata.Opsgenie is nil

func (c *Controller) getOpsgenieIntegrations(metadata *creator_v1.OpsgenieMetadata) ([]opsgenie.Integration, bool /* retriable */, error) { // Opsgenie is optional if metadata == nil { return nil, true, nil }

nilebox · 2019-02-15T04:06:47Z

 		}

-		return &orch_meta.Notifications{
+		notifications = orch_meta.Notifications{


Email remains the same, we could set notifications.PagerdutyEndpoint and notifications.LowPriorityPagerdutyEndpoint to make it more explicit which parts do we want to modify.

…-sync

Resolved

fcobb · 2019-02-18T00:13:51Z

-	return errors.Wrap(err, "unable to parse Deployinator URL")
+	if err != nil {
+		return errors.Wrap(err, "unable to parse Deployinator URL")
+	}


I am not sure why this always returned an error and I cannot identify where this function is called.
I think its called in readAndValidateOptions when yaml.UnmarshalStrict is called, but if thats the case then I'm not sure why we always fail.
https://github.com/atlassian/voyager/blob/master/cmd/synchronization/app/options.go#L67

@ash2k Can you help me understand why we returned an error before?

seems like a bug, not sure how did it work

it should be implicitly called in https://github.com/atlassian/voyager/blob/master/cmd/synchronization/app/options.go#L67

fcobb · 2019-02-18T00:23:41Z

+	EnvTypeDev     EnvType = "dev"
+	EnvTypeStaging EnvType = "staging"
+	EnvTypeProd    EnvType = "prod"
+	EnvTypeGlobal  EnvType = "null" // Intentionally a string called "null" as this is the expected result from opsgenie int manager


cc @jokeyrhyme

Why not global?

I am not sure why its not called "global". @jokeyrhyme Do you know why?

@fcobb Well, it's not that things are global, it's just that they aren't explicitly environment-specific
Maybe this should actually mean Global, and maybe we should actually emit "global" instead of "null"
When we put this together, the most important thing was making sure our CloudWatch alarms (which are environment-specific) were covered, everything else was secondary
I'm wondering whether we should call this Null or something so that we can meaningfully transition to Global in future?

@jokeyrhyme I went for Global as if its not environment specific then its essentially global.
As you own this API I will defer these decisions to you and your team. My preference is to return a specific value for these non-environment specific values rather than null.

fcobb · 2019-02-18T00:24:35Z


-	if found {
-		service.Attributes = append(service.Attributes, ogTeamAttr)
-	}


Previously, we filtered the service attributes before adding them to the service object. We now do that filtering later on.

fcobb · 2019-02-18T00:26:00Z

 	require.Equal(t, 2, handler.RequestSnapshots.Calls())
 }

-func TestGetServiceWithMultipleOpsGenieAttribute(t *testing.T) {


This test does not make sense anymore as it was making sure we errored on multiple OG attributes, this is no longer an error case as we handle multiple attributes later on.

fcobb · 2019-02-18T00:29:15Z

-	maxSyncWorkers = 10
+	maxSyncWorkers             = 10
+	defaultPagerdutyGeneric    = "5d11612f25b840faaf77422edeff9c76"
+	defaultPagerdutyCloudwatch = "https://events.pagerduty.com/adapter/cloudwatch_sns/v1/124e0f010f214a9b9f30b768e7b18e69"


These defaults were in the buildNotifcations function.

These values should come from configuration, not be constants. Please move them.

I'll do this in a follow up PR as this PR is big enough.

fcobb · 2019-02-18T00:30:08Z

 }

-func (c *Controller) buildNotifications(spec creator_v1.ServiceSpec) (*orch_meta.Notifications, error) {
+func (c *Controller) buildNotifications(spec creator_v1.ServiceSpec) (*orch_meta.Notifications, bool /* retriable */, error) {


It is easier to understand the changes to this function if you do a side-by-side diff.

nilebox · 2019-02-18T02:09:48Z

 		service.Spec.Metadata.PagerDuty = pagerDutyMetadata
 	}

+	ogMetadata, _ := GetOpsGenieAttribute(data)


shall we log an error?

shall we log an error?
Considering this is optional, Should this be an error or info message?

ogMetadata, err := GetOpsGenieAttribute(data) // Error ignored as Opsgenie team is optional if err != nil { c.logger.Info("unable to get Opsgenie attribute - ignoring", zap.Error(err)) }

I would say that failure to make a call to Opsgenie integration manager is still an error, so I would log it at the error level. We should react to being unable to fetch Opsgenie integrations rather than completely ignoring them.

@ash2k WDYT?

This is not the path of calling OG int manager, this call will attempt to find any opsgenie attributes within the service object.

We call opsgenie int manager during the createOrUpdateServiceMetadata function when building the notifications. I agree that we should know when calls to OG int manager fail, and we return an error in that case.

attempt to find any opsgenie attributes within the service object

If attribute not found, there won't be an error, but just a nil result AFAICT?
So then the error means that either the attribute format is invalid, or there is some bug in Voyager code, and we should also log with error, I think.

Yep you are correct, an error here is when:

we found an issue with an Opsgenie attribute

we found more than one Opsgenie attribute.

EDIT: As we have found a configured Opsgenie team but failed to process it, I am currently returning an error rather than logging it.

ash2k · 2019-02-18T03:44:52Z

+	EnvTypeDev     EnvType = "dev"
+	EnvTypeStaging EnvType = "staging"
+	EnvTypeProd    EnvType = "prod"
+	EnvTypeGlobal  EnvType = "null" // Intentionally a string called "null" as this is the expected result from opsgenie int manager


ash2k · 2019-02-18T03:53:39Z

+		return nil, nil // Not an error as Opsgenie is optional
+	}
+
+	filtered := make([]opsgenie.Integration, 0, 4)


We usually do not pre-allocate if we don't exactly know the number of items. Just a style thing.

Happy to change this to make our style if you would like. We expect there to be exactly 4 but it is possible that they add more integrations in the future. What would you like to see in this scenario?

ash2k · 2019-02-18T03:55:21Z

-	maxSyncWorkers = 10
+	maxSyncWorkers             = 10
+	defaultPagerdutyGeneric    = "5d11612f25b840faaf77422edeff9c76"
+	defaultPagerdutyCloudwatch = "https://events.pagerduty.com/adapter/cloudwatch_sns/v1/124e0f010f214a9b9f30b768e7b18e69"


These values should come from configuration, not be constants. Please move them.

ash2k · 2019-02-18T03:55:52Z

+	EnvTypeDev     EnvType = "dev"
+	EnvTypeStaging EnvType = "staging"
+	EnvTypeProd    EnvType = "prod"
+	EnvTypeGlobal  EnvType = "null" // Intentionally a string called "null" as this is the expected result from opsgenie int manager


Why not global?

ash2k · 2019-02-18T03:56:17Z


 const (
-	opsGenieIntManURL = "https://micros.prod.atl-paas.net"
+	opsgenieIntManURL = "https://micros.prod.atl-paas.net"


Don't use real data in tests please. Use fake data =)

This is a manual integration test, I don't think fake data is applicable.

ash2k

I think this is fine to be merged. Please address the issues/nits in a follow up PR. I think the manual opsgenie integration test should be moved to the internal repo.

…-sync

nilebox · 2019-02-21T04:20:56Z

As discussed, please add a test for Opsgenie integrations present in ServiceProperties (in entangler_test.go

fcobb · 2019-02-21T05:13:27Z

As discussed, please add a test for Opsgenie integrations present in ServiceProperties (in entangler_test.go

Done

nilebox · 2019-02-21T05:15:50Z

Make sure you block the PR in stash updating dependencies after you merge this change, and fix config.

fcobb requested a review from nilebox February 5, 2019 04:30

ash2k previously requested changes Feb 5, 2019

View reviewed changes

nilebox reviewed Feb 5, 2019

View reviewed changes

fcobb force-pushed the fcobb/VYGR-391-create-opsgenie-int-manager-client branch from f6f5e25 to 04e1d7d Compare February 11, 2019 05:51

fcobb mentioned this pull request Feb 12, 2019

VYGR-391: Add OpsGenie integration manager client #119

Merged

fcobb self-assigned this Feb 12, 2019

VYGR-391: Create OpsGenie integrations secret during sync

005de8c

fcobb force-pushed the fcobb/VYGR-391-save-opsgenie-secret-during-sync branch from 0b5b2f0 to 005de8c Compare February 13, 2019 04:09

fcobb changed the base branch from fcobb/VYGR-391-create-opsgenie-int-manager-client to master February 13, 2019 04:09

fraser-atlassian added 2 commits February 13, 2019 17:30

VYGR-391: Use SecretTypeOpaque for opsgenie secret

f37c6fa

VYGR-391: Add opsgenie integrations into notifications

fcb59fa

Remove opsgenie secret work-in-progress

fcobb changed the title ~~[WIP] VYGR-391: Create opsgenie secret during sync~~ [WIP] VYGR-391: Add opsgenie integrations to notifications Feb 15, 2019

VYGR-391: OpsGenie -> Opsgenie

45259db

fcobb commented Feb 15, 2019

View reviewed changes

Comment thread pkg/synchronization/controller.go Outdated

nilebox reviewed Feb 15, 2019

View reviewed changes

Comment thread pkg/synchronization/controller.go

nilebox reviewed Feb 15, 2019

View reviewed changes

fraser-atlassian added 6 commits February 15, 2019 16:00

VYGR-391: Set Opsgenie metadata in serviceDataToService

dd63115

VYGR-391: Use all service attributes in service object

46e0cfb

VYGR-391: Use constants for default pagerduty notifications

b7e6865

VYGR-391: Override pagerduty defaults instead of new object

cd3ba39

VYGR-391: Pull build opsgenie notifcations into separate function

9022baf

Merge branch 'master' into fcobb/VYGR-391-save-opsgenie-secret-during…

0d61496

…-sync

fcobb commented Feb 18, 2019

View reviewed changes

VYGR-391: Comment on optional opsgenie metadata

4844d76

nilebox reviewed Feb 18, 2019

View reviewed changes

Comment thread pkg/synchronization/controller.go Outdated

fraser-atlassian added 4 commits February 18, 2019 13:16

VYGR-391: Log when unable to get Opsgenie attribute in SC store

533eb33

VYGR-391: Error when Opsgenie filtering encounters unexpected envType

ce4b544

VYGR-391: Return error when we failed to get Opsgenie attr

9368162

VYGR-391: Remove struct method usage for serviceDataToService

d13b742

ash2k suggested changes Feb 18, 2019

View reviewed changes

fraser-atlassian added 2 commits February 18, 2019 15:07

VYGR-391: Reorder url errors to return error earlier

c811811

VYGR-391: Return nil for error cases in GetServiceAttributes

0271bef

ash2k previously approved these changes Feb 18, 2019

View reviewed changes

VYGR-391: Add unit tests for opsgenie buildNotifications

41066cb

fcobb dismissed ash2k’s stale review via 41066cb February 18, 2019 05:37

ash2k previously approved these changes Feb 18, 2019

View reviewed changes

VYGR-391: Update sync bazel

964933c

fcobb dismissed ash2k’s stale review via 964933c February 18, 2019 23:46

fraser-atlassian added 2 commits February 19, 2019 14:31

VYGR-391: Add test for Opsgenie build notifications per env

e3e587b

Merge branch 'master' into fcobb/VYGR-391-save-opsgenie-secret-during…

434198e

…-sync

fcobb force-pushed the fcobb/VYGR-391-save-opsgenie-secret-during-sync branch from fcf889a to 434198e Compare February 20, 2019 05:48

fcobb changed the title ~~[WIP] VYGR-391: Add opsgenie integrations to notifications~~ VYGR-391: Add opsgenie integrations to notifications Feb 20, 2019

fraser-atlassian added 2 commits February 21, 2019 16:01

VYGR-391: Add Opsgenie integrations into entangler test

f229991

VYGR-391: Reorder entangler_test imports

a5ac769

nilebox approved these changes Feb 21, 2019

View reviewed changes

Uh oh!

Conversation

fcobb commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fcobb Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fcobb Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

fcobb Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

fcobb Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fcobb Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fcobb Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

fcobb commented Feb 5, 2019 •

edited

Loading

fcobb Feb 15, 2019 •

edited

Loading

fcobb Feb 18, 2019 •

edited

Loading

fcobb Feb 18, 2019 •

edited

Loading

fcobb Feb 18, 2019 •

edited

Loading

fcobb Feb 18, 2019 •

edited

Loading

fcobb Feb 18, 2019 •

edited

Loading

fcobb Feb 18, 2019 •

edited

Loading