feat: allow masking output on comments

[GMartinez-Sisti]

what

Part of #163 (comment).

why

I have the requirements to mask some values that are passed to the comments posted by Atlantis, building up on strip_refreshing I added two new output configurations that will allow this via a regex configured on the step. There is an assumption that users that shouldn't see secrets/sensitive values won't have access to the URL jobs, where the plan outputs are shown untouched.

The output key can now contain a string, []stringor[]any`, this was we ensure compatibility while adding new possibilities to it.

Example (added to the docs):

workflows:
  terragrunt:
    plan:
      steps:
      - run:
          command: terragrunt plan -input=false -out=$PLANFILE
          output:
            - strip_refreshing
            # Filters text matching 'mySecret: "aaa"' -> 'mySecret: "<redacted>"'
            - filter_regex: "((?i)secret:\\s\")[^\"]*"

Note that the changes related to mocks were made manually since make go-generate is currently broken (#4664).

tests

• Running all the tests locally and adding coverage for the new feature
• Build and deployed this version with the new config from feature and tested that atlantis plan provides the desired masked output on GitHub 😄

references

Possibly solves #163.

[jamengual]

did you test tfmask? or any other tool?

[GMartinez-Sisti]

did you test tfmask? or any other tool?

I did, also terrahelp and even plain sed. The problem is that we are sending the output straight to the $planfile, so we can’t act on it. I even tried to change the $showfile, and while that works, Atlantis doesn’t use it for the comment.

[jamengual]

I see ok, it make sense on doing the pre-processing

[anryko]

I like the feature and find it very useful. However, IMHO, the API could be better.
To maintain backward compatibility and allow for extending this API I would suggest the following implementation:

workflows:
  terragrunt:
    plan:
      steps:
      - run:
          command: terragrunt plan -input=false -out=$PLANFILE
          output: strip_refreshing|show|hide
      - run:
          command: terragrunt plan -input=false -out=$PLANFILE
          output:
            - show
            - strip_refreshing
            - filter_regex: "((?i)secret:\\s\")[^\"]*"

This would allow us to support previous string values and in case of the list[string], apply multiple filters sequentially, as well as add more filters in the future without breaking the API.
The filter_regex is a better name because it matches <action> | <action>_<topic> naming pattern of already supported strip_refreshing|show|hide output actions.

[GMartinez-Sisti]

I like the feature and find it very useful. However, IMHO, the API could be better. To maintain backward compatibility and allow for extending this API I would suggest the following implementation:

workflows:
  terragrunt:
    plan:
      steps:
      - run:
          command: terragrunt plan -input=false -out=$PLANFILE
          output: strip_refreshing|show|hide
      - run:
          command: terragrunt plan -input=false -out=$PLANFILE
          output:
            - show
            - strip_refreshing
            - filter_regex: "((?i)secret:\\s\")[^\"]*"

This would allow us to support previous string values and in case of the list[string], apply multiple filters sequentially, as well as add more filters in the future without breaking the API. The filter_regex is a better name because it matches <action> | <action>_<topic> naming pattern of already supported strip_refreshing|show|hide output actions.

Hi, thanks for the feedback 😃

I've been using this to support terraform for 100+ environments on the three major clouds with zero issues so far. I adjusted the regex to "((?i)secret.*:\\s\")[^\"]*" so it includes pretty much all fields with secret on the name.

I have to rebase this soon, I'll take a stab at making it work the way you suggested and see how it behaves.

[X-Guardian]

Hi @GMartinez-Sisti, are you able to look at the suggestions from @anryko. It would be great to get this merged.

[GMartinez-Sisti]

I've been thinking about the suggested API, the suggested output accepts either:

• a string
• a list of strings and a list of maps

I think this is not ideal and might create some confusion, we can support multiple types but only one at a time and act accordingly. This is my suggestion:

workflows:
  terragrunt:
    plan:
      steps:
      - run:
          command: terragrunt plan -input=false -out=$PLANFILE
          output: strip_refreshing|show|hide
      - run:
          command: terragrunt plan -input=false -out=$PLANFILE
          output:
            - show
            - strip_refreshing
            - filter_regex
          regex_expression: "((?i)secret:\\s\")[^\"]*"

• output accepts either string or map[string]string
• regex_expression has a default value and we can optionally define it to set a different value

WDYT @anryko and @X-Guardian ?

[anryko]

The api I suggested would provide an option to apply a sequence of simple regexps one after another. It would make your feature more powerful. I understand the added implementation complexity you are referring to and believe that this would be a bit easier to implement on top of the changes done for this feature, which "loosens" the config unmarshaling.

[GMartinez-Sisti]

The api I suggested would provide an option to apply a sequence of simple regexps one after another. It would make your feature more powerful. I understand the added implementation complexity you are referring to and believe that this would be a bit easier to implement on top of the changes done for this feature, which "loosens" the config unmarshaling.

I see it, while being more verbose it will be more flexible indeed. I'll wait for #5024 to be merged then so I can leverage the new map[string]map[string]interface{}.

[X-Guardian]

Hi @GMartinez-Sisti, #5024 is now merged. Can you resolve the conflicts on this?

[GMartinez-Sisti]

Hi @GMartinez-Sisti, #5024 is now merged. Can you resolve the conflicts on this?

I've fixed the conflicts, but haven't updated the logic to match the suggestions.

[github-actions]

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.

[GMartinez-Sisti]

Will still work on this! ☺️

[jamengual]

@GMartinez-Sisti still working on this? Thanks.

[GMartinez-Sisti]

@GMartinez-Sisti still working on this? Thanks.

Yes! Just haven't add the time to focus and update it. But not forgotten.

[github-actions]

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.

[GMartinez-Sisti]

I'll pick this up this weekend. Sorry for the delay.

[GMartinez-Sisti]

@X-Guardian @jamengual @anryko I finally got some time to work on this! Please take a look when possible :) 🙏

[lukemassa]

I don't think this warrants a "utility" function. Its purpose seems to be just to change the zero object from the empty string regex to a nil. I'd prefer this happens at the call site, since it's not a "general" behavior we have. Additionally this would allow

testRegexDotStar, _ := utils.ParseRegex(".*")

to be written in the simpler and more idiomatic

testRegexDotStar := regexp.MustCompile(".*")

If we have to have this utility function, I'd prefer its name be changed to reflect the way its semantics differ from that of the standard library regexp.Compile, or at the very least its godoc comment updated to explain the difference.

[GMartinez-Sisti]

Moving it closer to the call site makes sense. When I started this I thought it would have to be reused across multiple packages, but in the end it didn't so I can move it.

Regarding MustCompile, this will panic if it fails to parse, it's true that we already validated it when we parsed the configs so we can change it, was just trying to play it safe 😅 .

[lukemassa]

Oh yeah sorry to be clear, I'm only recommending MustCompile() in cases where you pass a literal string to it, like in tests.

[GMartinez-Sisti]

This func is used in multiple in the raw package:

→ ag ParseRegex
server/core/config/raw/step.go
311:                                                    _, err := utils.ParseRegex(v)
450:                                                                    r, _ := utils.ParseRegex(t)
454:                                                                            r, _ := utils.ParseRegex(e)

server/core/config/raw/step_test.go
508:    testRegexDotStar, _ := utils.ParseRegex(".*")
509:    testRegexSecret, _ := utils.ParseRegex("((?i)secret:\\s\")[^\"]*")

server/core/runtime/run_step_runner_test.go
26:     testRegexSecret, _ := utils.ParseRegex(`((?i)Secret:\s")[^"]*`)

server/core/runtime/plan_step_runner_test.go
654:            r, err := utils.ParseRegex(c.regex)

server/utils/regex.go
7:// ParseRegex validates and returns a [Regexp] object
8:func ParseRegex(pattern string) (*regexp.Regexp, error) {

What is your suggestion here? Just move the regex.go file to the raw package? Or add the func to a random file from that package?

[lukemassa]

It seems like the logic here is that we keep the first and second capture group, if they exist, and put them before and after <redacted>. I don't see this in the documentation, can this be added?

What happens if there are zero, one, or more than two capture groups? We should have test cases in TestCustomRegexFromPlanOutputFromPlanOutput to show the expectations here.

[GMartinez-Sisti]

It will just match everything and return it without any change.

Also if it helps I have been running this in production since I made the PR :) And also already deployed this latest version and it's working fine.

[github-actions]

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.