arabold / aws-to-slack

Forward AWS CloudWatch Alarms and other notifications from Amazon SNS to Slack.
MIT License
294 stars 111 forks source link

An INCLUDE_AWS_LINKS true/false env var to indicate preferred parser behavior #67

Closed whereisaaron closed 5 years ago

whereisaaron commented 5 years ago

Many of the parsers assume the Slack receiver has AWS Console access to the resources mentioned and include back-links into AWS. Even generic SNS messages assume the receiver can view/manage SNS topics.

Sometimes the people at the Slack target don't have any access to the AWS resources generating the events. It would be nice to have the option to either

I suggest an INCLUDE_AWS_LINKS environment variable that tells that deployment whether or not including back-links is preferred.

Multiple deployments for different targets works fine, and I can set this global preference so that I don't have to fork the code-base for internal/external audiences. I've implemented something like this to test it out, and could extract a PR later if this is a desirable feature.

homeyjd commented 5 years ago

Seems like it could be helpful!

We've had some requests for even more links (like "See Logs" that links to the time of alert for log-metric-based CloudWatch alarms). As parsers are added, links might "leak". Do you see a danger there?

The average user of this repo probably has access to the Console, but a feature-add to make them optional would be welcome as long as the default is true. :)

whereisaaron commented 5 years ago

No, no danger I think @homeyjd, as the links don't confer any access. They will just be useless (and/or confusing) for users without suitable AWS access. That's why I called it a 'preference' setting and 'preferred parser behavior', I don't expect every parser will comply, only the ones that support the preference.

Once the preference exists, it would nice if new/updated parsers considered it, and just left out the links if INCLUDE_AWS_LINKS=false. But if INCLUDE_AWS_LINKS=true then go to town and link all the things 😄

I just added to eventdef.js:

/** Whether to include links to AWS resources in Slack messages **/
const includeAwsLinks = (_.isString(process.env.INCLUDE_AWS_LINKS) && _.lowerCase(process.env.INCLUDE_AWS_LINKS) === 'true');
global.includeAwsLinks = includeAwsLinks;

And then in parsers I just wrap linked chunks or alternatives in if (includeAwsLinks):

        if (initiator) {
                fields.push({
                        title: "Triggered By",
                        value: initiator,
                        short: true
                });
        }

        if (includeAwsLinks) {
                fields.push({
                        title: "Logs",
                        value: `<${logsUrl}|View Logs>`,
                        short: true
                });
                fields.push({
                        title: "Build",
                        value: `<${phaseUrl}|View Build>`,
                        short: true
                });
        }
homeyjd commented 5 years ago

Since the intent is to leave default=true, I might suggest changing that behavior to HIDE_AWS_LINKS.

global.HIDE_AWS_LINKS = /true|1/i.test(process.env.HIDE_AWS_LINKS || "");

The feature-gate could be written in a less-brittle way:

// brittle, if variable is not defined (such as during a test?), script will always fail
if (!HIDE_AWS_LINKS) { /* add link */ }
// less brittle
if (!global.HIDE_AWS_LINKS) { /* add link */ }

As a possible alternative, eventdef.js could have a getConsoleLink(urlPath, urlQuery) method that returns undefined if when inactive. That would eliminate the need for another global. :)

    getConsoleLink(urlPath, urlQuery, region = "us-east-1") {
        if (/true|1/i.test(process.env.HIDE_AWS_LINKS || "")) return undefined;
        return `https://${region}.console.aws.amazon.com${urlPath}?${urlQuery}`;
    }
whereisaaron commented 5 years ago

I prefer a simple boolean test (as well), but that could be a method.

   isHideAwsLinks() {
        return (/true|1/i.test(process.env.HIDE_AWS_LINKS || ""));
   }

You could have getConsoleLink too but that might not cover all possible URLs and then you'll be adding more and more for variations. Though link creating methods that automatically handled thing like amazonaws.cn regions etc. could be handy.

homeyjd commented 5 years ago

I added this for now. Open to a PR if you'd like. https://github.com/arabold/aws-to-slack/commit/62bd18178f761b619ce6be43b8285a597d9d876c