Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: mask environment variables from outputs #463

Merged
merged 2 commits into from
Nov 9, 2022

Conversation

carnei-ro
Copy link
Contributor

@carnei-ro carnei-ro commented Jul 14, 2021

fixes #462

@carnei-ro carnei-ro requested a review from a team as a code owner July 14, 2021 22:49
@CLAassistant
Copy link

CLAassistant commented Jul 14, 2021

CLA assistant check
All committers have signed the CLA.

@carnei-ro carnei-ro force-pushed the feat/mask-env-var-value-from-output branch 3 times, most recently from 8352c00 to b516e5f Compare July 20, 2021 12:55
@dancallaghan
Copy link

Just wondering what the status was on this PR? We noticed that the diff command was resulting in secrets being leaked in our CI.

@hbagdi hbagdi requested review from GGabriele and removed request for GGabriele February 24, 2022 14:37
@thesheps
Copy link

Just wondering what the status was on this PR? We noticed that the diff command was resulting in secrets being leaked in our CI.

Can confirm this is also causing us problem across all our environments. More than happy to contribute or review any supporting PRs if that's of any use?

@hbagdi
Copy link
Member

hbagdi commented Mar 18, 2022

This looks promising. Could you please add an integration test for this?

@carnei-ro carnei-ro force-pushed the feat/mask-env-var-value-from-output branch from b516e5f to 370749b Compare August 2, 2022 17:07
@carnei-ro carnei-ro requested a review from a team as a code owner August 2, 2022 17:07
@carnei-ro carnei-ro force-pushed the feat/mask-env-var-value-from-output branch 5 times, most recently from 083ef36 to 63cb183 Compare August 3, 2022 13:34
@carnei-ro
Copy link
Contributor Author

@hbagdi TBH I have no idea how to create a "diff" integration test that actually looks to the diff output 😞
Is there any "CONTRIBUTING" doc to help me out?
I've rebased, created a simple unit test, and edited the existing integration test. It is masking the value.

=== RUN   Test_Diff_Workspace
=== RUN   Test_Diff_Workspace/diff_with_not_existent_workspace_doesn't_error_out
creating workspace test
creating service svc1  {
+  "connect_timeout": 60000
+  "host": "mockbin.org"
+  "id": "d0d2ec9b-fe41-4516-be12-50766a770523"
+  "name": "[masked]"
+  "protocol": "http"
+  "read_timeout": 60000
+  "write_timeout": 60000
 }

@carnei-ro carnei-ro changed the title [WIP] feat: mask environment variables from outputs feat: mask environment variables from outputs Aug 3, 2022
@hbagdi
Copy link
Member

hbagdi commented Aug 12, 2022

@GGabriele Could you help out, please?

@GGabriele
Copy link
Collaborator

Hi @carnei-ro ,

thanks for this very nice feature AND apologies for the super late response!

The trick here would be to capture the output from the decK command and compare it to some expected one. Behind the scenes, decK uses github.com/fatih/color to generate diff reports, and specifically this method to print them out.

In order to redirect the output to something we can use during tests, we can overwrite color.Output in our diff utils function with something like this:

func diff(kongFile string, opts ...string) (string, error) {
	deckCmd := cmd.NewRootCmd()
	args := []string{"diff", "-s", kongFile}
	if len(opts) > 0 {
		args = append(args, opts...)
	}
	deckCmd.SetArgs(args)

	// overwrite default standard output
	r, w, _ := os.Pipe()
	color.Output = w

	// execute decK command
	cmdErr := deckCmd.ExecuteContext(context.Background())

	// read command output
	w.Close()
	out, _ := ioutil.ReadAll(r)

	return string(out), cmdErr
}

At that point, you can compare out in the test case with some output you expect to get (you'd need to hardcode the entities IDs in the kong.yaml, otherwise they will get re-generated every time):

			out, err := diff(tc.stateFile)
			assert.NoError(t, err)
			assert.Equal(t, out, expected)

@carnei-ro carnei-ro force-pushed the feat/mask-env-var-value-from-output branch 2 times, most recently from b7aac32 to d1c00da Compare October 12, 2022 19:12
@carnei-ro
Copy link
Contributor Author

Thanks @GGabriele it really did the job. Masked and Unmasked for Kong < 3 and Kong >= 3 tests are there!

@carnei-ro carnei-ro temporarily deployed to Configure ci October 13, 2022 09:00 Inactive
@carnei-ro carnei-ro force-pushed the feat/mask-env-var-value-from-output branch from d1c00da to a137fdd Compare October 13, 2022 19:33
@carnei-ro carnei-ro force-pushed the feat/mask-env-var-value-from-output branch from a137fdd to df7bd57 Compare October 13, 2022 19:53
diff/diff.go Outdated Show resolved Hide resolved
@carnei-ro carnei-ro force-pushed the feat/mask-env-var-value-from-output branch from df7bd57 to e65a146 Compare October 14, 2022 12:20
cmd/common.go Outdated Show resolved Hide resolved
@GGabriele
Copy link
Collaborator

@carnei-ro I just added a couple of more comments which would require a bit of refactoring (sorry for not pushing these earlier).

@rainest can you take a look at this PR too? I want to make sure KIC is good with these changes (and the ones I proposed)

@carnei-ro carnei-ro requested review from GGabriele and removed request for rainest October 14, 2022 13:35
@carnei-ro carnei-ro temporarily deployed to Configure ci October 14, 2022 14:07 Inactive
@GGabriele GGabriele merged commit 6487bfd into Kong:main Nov 9, 2022
@GGabriele
Copy link
Collaborator

@carnei-ro Awesome job here! Merging this now, thanks a lot again!

@carnei-ro
Copy link
Contributor Author

@GGabriele, thanks for merging it! Does decK project have custom contributor T-shirts? I'm collecting them - I already have Kong, KIC and it would be great to have one of decK.

@GGabriele
Copy link
Collaborator

Does decK project have custom contributor T-shirts?

Unfortunately it doesn't :( not yet at least

GGabriele added a commit that referenced this pull request Dec 13, 2022
This commit fixes a 'visual' regression introduced
in #463, which is
making decK print full diff documents for 'create'
and 'delete' actions. This can result in very
verbose output in case of configuration files
containing hundreds/thousands of resources.
GGabriele added a commit that referenced this pull request Dec 13, 2022
This commit fixes a 'visual' regression introduced
in #463, which is
making decK print full diff documents for 'create'
and 'delete' actions. This can result in very
verbose output in case of configuration files
containing hundreds/thousands of resources.
GGabriele added a commit that referenced this pull request Dec 15, 2022
This commit fixes a 'visual' regression introduced
in #463, which is
making decK print full diff documents for 'create'
and 'delete' actions. This can result in very
verbose output in case of configuration files
containing hundreds/thousands of resources.
AntoineJac pushed a commit that referenced this pull request Jan 23, 2024
This commit fixes a 'visual' regression introduced
in #463, which is
making decK print full diff documents for 'create'
and 'delete' actions. This can result in very
verbose output in case of configuration files
containing hundreds/thousands of resources.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redact value from placeholders
6 participants