From 912035662bc3b436c93539fd14eaf9e1b152c01a Mon Sep 17 00:00:00 2001 From: DRAGON Date: Wed, 19 Jul 2023 20:29:37 +0530 Subject: [PATCH 1/6] fix: --- kubescape fix Signed-off-by: DRAGON --- core/pkg/fixhandler/fixhandler.go | 21 +++++ core/pkg/fixhandler/fixhandler_test.go | 81 ++++++++++++++++++- ...c-12-00-begin-with-document-separator.yaml | 10 +++ .../testdata/inserts/tc-12-01-expected.yaml | 12 +++ 4 files changed, 121 insertions(+), 3 deletions(-) create mode 100644 core/pkg/fixhandler/testdata/inserts/tc-12-00-begin-with-document-separator.yaml create mode 100644 core/pkg/fixhandler/testdata/inserts/tc-12-01-expected.yaml diff --git a/core/pkg/fixhandler/fixhandler.go b/core/pkg/fixhandler/fixhandler.go index d0e8d3ff..39c0ff21 100644 --- a/core/pkg/fixhandler/fixhandler.go +++ b/core/pkg/fixhandler/fixhandler.go @@ -208,6 +208,8 @@ func (h *FixHandler) ApplyChanges(ctx context.Context, resourcesToFix []Resource continue } + fileAsString = sanitizeYaml(fileAsString) + fixedYamlString, err := ApplyFixToContent(ctx, fileAsString, yamlExpression) if err != nil { @@ -217,6 +219,8 @@ func (h *FixHandler) ApplyChanges(ctx context.Context, resourcesToFix []Resource updatedFiles[filepath] = true } + fixedYamlString = revertSanitizeYaml(fixedYamlString) + err = writeFixesToFile(filepath, fixedYamlString) if err != nil { @@ -368,3 +372,20 @@ func determineNewlineSeparator(contents string) string { return unixNewline } } + +// Handle the case where the resource file starts with --- +// causes yaml.Node to misinterpret the resources +func sanitizeYaml(fileAsString string) string { + if fileAsString[:3] == "---" { + fileAsString = "# " + fileAsString + } + return fileAsString +} + +// For the --- case to ensure correct output file format +func revertSanitizeYaml(fixedYamlString string) string { + if fixedYamlString[:5] == "# ---" { + fixedYamlString = fixedYamlString[2:] + } + return fixedYamlString +} \ No newline at end of file diff --git a/core/pkg/fixhandler/fixhandler_test.go b/core/pkg/fixhandler/fixhandler_test.go index 81d049f1..47e3cc51 100644 --- a/core/pkg/fixhandler/fixhandler_test.go +++ b/core/pkg/fixhandler/fixhandler_test.go @@ -101,6 +101,13 @@ func getTestCases() []indentationTestCase { "inserts/tc-11-01-expected.yaml", }, + // Starts with --- + { + "inserts/tc-12-00-begin-with-document-separator.yaml", + "select(di==0).spec.containers[0].securityContext.allowPrivilegeEscalation |= false", + "inserts/tc-12-01-expected.yaml", + }, + // Removal Scenarios { "removals/tc-01-00-input.yaml", @@ -118,10 +125,10 @@ func getTestCases() []indentationTestCase { "removals/tc-03-01-expected.yaml", }, { - "removes/tc-04-00-input.yaml", + "removals/tc-04-00-input.yaml", `del(select(di==0).spec.containers[0].securityContext) | del(select(di==1).spec.containers[1])`, - "removes/tc-04-01-expected.yaml", + "removals/tc-04-01-expected.yaml", }, // Replace Scenarios @@ -182,7 +189,9 @@ func TestApplyFixKeepsFormatting(t *testing.T) { want := string(wantRaw) expression := tc.yamlExpression - got, _ := ApplyFixToContent(context.TODO(), string(input), expression) + fileAsString := sanitizeYaml(string(input)) + fixedYamlString, _ := ApplyFixToContent(context.TODO(), fileAsString, expression) + got := revertSanitizeYaml(fixedYamlString) assert.Equalf( t, want, got, @@ -245,3 +254,69 @@ func Test_fixPathToValidYamlExpression(t *testing.T) { }) } } + +func Test_sanitizeYaml(t *testing.T) { + type args struct { + fileAsString string + } + tests := []struct{ + name string + args args + want string + }{ + { + name: "sanitize yaml starting with ---", + args: args{ + fileAsString: "---\nlabel: test", + }, + want: "# ---\nlabel: test", + }, + { + name: "sanitize yaml not starting with ---", + args: args{ + fileAsString: "label: test", + }, + want: "label: test", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := sanitizeYaml(tt.args.fileAsString); got != tt.want { + t.Errorf("sanitizeYaml() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_revertSanitizeYaml(t *testing.T) { + type args struct { + fixedYamlString string + } + tests := []struct{ + name string + args args + want string + }{ + { + name: "revert sanitized yaml starting with ---", + args: args{ + fixedYamlString: "# ---\nlabel: test", + }, + want: "---\nlabel: test", + }, + { + name: "revert sanitized yaml not starting with ---", + args: args{ + fixedYamlString: "label: test", + }, + want: "label: test", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := revertSanitizeYaml(tt.args.fixedYamlString); got != tt.want { + t.Errorf("revertSanitizeYaml() = %v, want %v", got, tt.want) + } + }) + } +} \ No newline at end of file diff --git a/core/pkg/fixhandler/testdata/inserts/tc-12-00-begin-with-document-separator.yaml b/core/pkg/fixhandler/testdata/inserts/tc-12-00-begin-with-document-separator.yaml new file mode 100644 index 00000000..61d8bee3 --- /dev/null +++ b/core/pkg/fixhandler/testdata/inserts/tc-12-00-begin-with-document-separator.yaml @@ -0,0 +1,10 @@ +--- +apiVersion: v1 +kind: Pod +metadata: + name: begin-with-document-separator + +spec: + containers: + - name: nginx_container + image: nginx diff --git a/core/pkg/fixhandler/testdata/inserts/tc-12-01-expected.yaml b/core/pkg/fixhandler/testdata/inserts/tc-12-01-expected.yaml new file mode 100644 index 00000000..8116502f --- /dev/null +++ b/core/pkg/fixhandler/testdata/inserts/tc-12-01-expected.yaml @@ -0,0 +1,12 @@ +--- +apiVersion: v1 +kind: Pod +metadata: + name: begin-with-document-separator + +spec: + containers: + - name: nginx_container + image: nginx + securityContext: + allowPrivilegeEscalation: false From a4478ba89912061be6c9b9c3086502037e0a1d09 Mon Sep 17 00:00:00 2001 From: Vlad Klokun Date: Fri, 21 Jul 2023 19:45:43 +0300 Subject: [PATCH 2/6] style(fixhandler): newlines and spacing Ran with `go fmt`. Signed-off-by: Vlad Klokun --- core/pkg/fixhandler/fixhandler.go | 2 +- core/pkg/fixhandler/fixhandler_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/core/pkg/fixhandler/fixhandler.go b/core/pkg/fixhandler/fixhandler.go index 39c0ff21..399e2aac 100644 --- a/core/pkg/fixhandler/fixhandler.go +++ b/core/pkg/fixhandler/fixhandler.go @@ -388,4 +388,4 @@ func revertSanitizeYaml(fixedYamlString string) string { fixedYamlString = fixedYamlString[2:] } return fixedYamlString -} \ No newline at end of file +} diff --git a/core/pkg/fixhandler/fixhandler_test.go b/core/pkg/fixhandler/fixhandler_test.go index 47e3cc51..f2c36b02 100644 --- a/core/pkg/fixhandler/fixhandler_test.go +++ b/core/pkg/fixhandler/fixhandler_test.go @@ -259,7 +259,7 @@ func Test_sanitizeYaml(t *testing.T) { type args struct { fileAsString string } - tests := []struct{ + tests := []struct { name string args args want string @@ -292,7 +292,7 @@ func Test_revertSanitizeYaml(t *testing.T) { type args struct { fixedYamlString string } - tests := []struct{ + tests := []struct { name string args args want string @@ -319,4 +319,4 @@ func Test_revertSanitizeYaml(t *testing.T) { } }) } -} \ No newline at end of file +} From 1503e984f839a5c05d19f5dc99d88f44ef61def6 Mon Sep 17 00:00:00 2001 From: Vlad Klokun Date: Fri, 21 Jul 2023 19:55:03 +0300 Subject: [PATCH 3/6] tests(fixhandler): fail test if unable to open test data file Previously when there was a typo in a test file name, we silently failed. This commit makes the test explicitly fail if a test data file was not found. Signed-off-by: Vlad Klokun --- core/pkg/fixhandler/fixhandler_test.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/core/pkg/fixhandler/fixhandler_test.go b/core/pkg/fixhandler/fixhandler_test.go index f2c36b02..5df11774 100644 --- a/core/pkg/fixhandler/fixhandler_test.go +++ b/core/pkg/fixhandler/fixhandler_test.go @@ -176,16 +176,23 @@ func getTestCases() []indentationTestCase { func TestApplyFixKeepsFormatting(t *testing.T) { testCases := getTestCases() + getTestDataPath := func(filename string) string { + currentFile := "testdata/" + filename + return filepath.Join(testutils.CurrentDir(), currentFile) + } for _, tc := range testCases { t.Run(tc.inputFile, func(t *testing.T) { - getTestDataPath := func(filename string) string { - currentFile := "testdata/" + filename - return filepath.Join(testutils.CurrentDir(), currentFile) + inputFilename := getTestDataPath(tc.inputFile) + input, err := os.ReadFile(inputFilename) + if err != nil { + t.Fatalf(`Unable to open file %s due to: %v`, inputFilename, err) + } + expectedFilename := getTestDataPath(tc.expectedFile) + wantRaw, err := os.ReadFile(expectedFilename) + if err != nil { + t.Fatalf(`Unable to open file %s due to: %v`, expectedFilename, err) } - - input, _ := os.ReadFile(getTestDataPath(tc.inputFile)) - wantRaw, _ := os.ReadFile(getTestDataPath(tc.expectedFile)) want := string(wantRaw) expression := tc.yamlExpression From 22c412ce7fa4ffe393b9abb6e80a288dd6bef57e Mon Sep 17 00:00:00 2001 From: Vlad Klokun Date: Fri, 21 Jul 2023 20:17:33 +0300 Subject: [PATCH 4/6] refactor(fixhandler): sanitize YAML inside ApplyFixToContent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit External observers don’t need to be aware of the fact we need to sanitize leading document separators in YAML files. This should be hidden inside our public function - `ApplyFixToContent()`. Signed-off-by: Vlad Klokun --- core/pkg/fixhandler/fixhandler.go | 6 ++--- core/pkg/fixhandler/fixhandler_test.go | 21 ++++++++++-------- .../tc-05-00-input-leading-doc-separator.yaml | 22 +++++++++++++++++++ .../testdata/hybrids/tc-05-01-expected.yaml | 22 +++++++++++++++++++ 4 files changed, 58 insertions(+), 13 deletions(-) create mode 100644 core/pkg/fixhandler/testdata/hybrids/tc-05-00-input-leading-doc-separator.yaml create mode 100644 core/pkg/fixhandler/testdata/hybrids/tc-05-01-expected.yaml diff --git a/core/pkg/fixhandler/fixhandler.go b/core/pkg/fixhandler/fixhandler.go index 399e2aac..dbe1e049 100644 --- a/core/pkg/fixhandler/fixhandler.go +++ b/core/pkg/fixhandler/fixhandler.go @@ -208,8 +208,6 @@ func (h *FixHandler) ApplyChanges(ctx context.Context, resourcesToFix []Resource continue } - fileAsString = sanitizeYaml(fileAsString) - fixedYamlString, err := ApplyFixToContent(ctx, fileAsString, yamlExpression) if err != nil { @@ -219,8 +217,6 @@ func (h *FixHandler) ApplyChanges(ctx context.Context, resourcesToFix []Resource updatedFiles[filepath] = true } - fixedYamlString = revertSanitizeYaml(fixedYamlString) - err = writeFixesToFile(filepath, fixedYamlString) if err != nil { @@ -247,6 +243,7 @@ func (h *FixHandler) getFilePathAndIndex(filePathWithIndex string) (filePath str } func ApplyFixToContent(ctx context.Context, yamlAsString, yamlExpression string) (fixedString string, err error) { + yamlAsString = sanitizeYaml(yamlAsString) newline := determineNewlineSeparator(yamlAsString) yamlLines := strings.Split(yamlAsString, newline) @@ -268,6 +265,7 @@ func ApplyFixToContent(ctx context.Context, yamlAsString, yamlExpression string) fixedYamlLines := getFixedYamlLines(yamlLines, fixInfo, newline) fixedString = getStringFromSlice(fixedYamlLines, newline) + fixedString = revertSanitizeYaml(fixedString) return fixedString, nil } diff --git a/core/pkg/fixhandler/fixhandler_test.go b/core/pkg/fixhandler/fixhandler_test.go index 5df11774..a372451b 100644 --- a/core/pkg/fixhandler/fixhandler_test.go +++ b/core/pkg/fixhandler/fixhandler_test.go @@ -169,6 +169,12 @@ func getTestCases() []indentationTestCase { select(di==0).spec.securityContext.runAsRoot |= false`, "hybrids/tc-04-01-expected.yaml", }, + { + "hybrids/tc-05-00-input-leading-doc-separator.yaml", + `del(select(di==0).spec.containers[0].securityContext) | + select(di==0).spec.securityContext.runAsRoot |= false`, + "hybrids/tc-05-01-expected.yaml", + }, } return indentationTestCases @@ -196,9 +202,8 @@ func TestApplyFixKeepsFormatting(t *testing.T) { want := string(wantRaw) expression := tc.yamlExpression - fileAsString := sanitizeYaml(string(input)) - fixedYamlString, _ := ApplyFixToContent(context.TODO(), fileAsString, expression) - got := revertSanitizeYaml(fixedYamlString) + fileAsString := string(input) + got, _ := ApplyFixToContent(context.TODO(), fileAsString, expression) assert.Equalf( t, want, got, @@ -288,9 +293,8 @@ func Test_sanitizeYaml(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := sanitizeYaml(tt.args.fileAsString); got != tt.want { - t.Errorf("sanitizeYaml() = %v, want %v", got, tt.want) - } + got := sanitizeYaml(tt.args.fileAsString) + assert.Equal(t, tt.want, got) }) } } @@ -321,9 +325,8 @@ func Test_revertSanitizeYaml(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := revertSanitizeYaml(tt.args.fixedYamlString); got != tt.want { - t.Errorf("revertSanitizeYaml() = %v, want %v", got, tt.want) - } + got := revertSanitizeYaml(tt.args.fixedYamlString) + assert.Equal(t, tt.want, got) }) } } diff --git a/core/pkg/fixhandler/testdata/hybrids/tc-05-00-input-leading-doc-separator.yaml b/core/pkg/fixhandler/testdata/hybrids/tc-05-00-input-leading-doc-separator.yaml new file mode 100644 index 00000000..9a6bc1bf --- /dev/null +++ b/core/pkg/fixhandler/testdata/hybrids/tc-05-00-input-leading-doc-separator.yaml @@ -0,0 +1,22 @@ +# Fix to Apply: +# REMOVE: +# "del(select(di==0).spec.containers[0].securityContext)" + +# INSERT: +# select(di==0).spec.securityContext.runAsRoot: false + + +--- +apiVersion: v1 +kind: Pod +metadata: + name: insert_to_mapping_node_1 + +spec: + containers: + - name: nginx_container + + image: nginx + + securityContext: + runAsRoot: true diff --git a/core/pkg/fixhandler/testdata/hybrids/tc-05-01-expected.yaml b/core/pkg/fixhandler/testdata/hybrids/tc-05-01-expected.yaml new file mode 100644 index 00000000..5beb28d4 --- /dev/null +++ b/core/pkg/fixhandler/testdata/hybrids/tc-05-01-expected.yaml @@ -0,0 +1,22 @@ +# Fix to Apply: +# REMOVE: +# "del(select(di==0).spec.containers[0].securityContext)" + +# INSERT: +# select(di==0).spec.securityContext.runAsRoot: false + + +--- +apiVersion: v1 +kind: Pod +metadata: + name: insert_to_mapping_node_1 + +spec: + containers: + - name: nginx_container + + image: nginx + securityContext: + runAsRoot: false + From 4763f0d69d55378f96bb3f92113b785c467ace9d Mon Sep 17 00:00:00 2001 From: Vlad Klokun Date: Fri, 21 Jul 2023 20:28:18 +0300 Subject: [PATCH 5/6] docs(fixhandler): follow Go Doc comments convention in sanitization func Signed-off-by: Vlad Klokun --- core/pkg/fixhandler/fixhandler.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/core/pkg/fixhandler/fixhandler.go b/core/pkg/fixhandler/fixhandler.go index dbe1e049..578a1812 100644 --- a/core/pkg/fixhandler/fixhandler.go +++ b/core/pkg/fixhandler/fixhandler.go @@ -371,8 +371,14 @@ func determineNewlineSeparator(contents string) string { } } -// Handle the case where the resource file starts with --- -// causes yaml.Node to misinterpret the resources +// sanitizeYaml receives a YAML file as a string, sanitizes it and returns the result +// +// Callers should remember to call the corresponding revertSanitizeYaml function. +// +// It applies the following sanitization: +// +// - Since `yaml/v3` fails to serialize documents starting with a document +// separator, we comment it out to be compatible. func sanitizeYaml(fileAsString string) string { if fileAsString[:3] == "---" { fileAsString = "# " + fileAsString @@ -380,7 +386,9 @@ func sanitizeYaml(fileAsString string) string { return fileAsString } -// For the --- case to ensure correct output file format +// revertSanitizeYaml receives a sanitized YAML file as a string and reverts the applied sanitization +// +// For sanitization details, refer to the sanitizeYaml() function. func revertSanitizeYaml(fixedYamlString string) string { if fixedYamlString[:5] == "# ---" { fixedYamlString = fixedYamlString[2:] From bc131efd91eb010334d5039e01b1622327f32b0c Mon Sep 17 00:00:00 2001 From: Vlad Klokun Date: Fri, 21 Jul 2023 20:29:04 +0300 Subject: [PATCH 6/6] tests(fixhandler): remove tests of an unexported sanitization method Signed-off-by: Vlad Klokun --- core/pkg/fixhandler/fixhandler_test.go | 64 -------------------------- 1 file changed, 64 deletions(-) diff --git a/core/pkg/fixhandler/fixhandler_test.go b/core/pkg/fixhandler/fixhandler_test.go index a372451b..d598dd83 100644 --- a/core/pkg/fixhandler/fixhandler_test.go +++ b/core/pkg/fixhandler/fixhandler_test.go @@ -266,67 +266,3 @@ func Test_fixPathToValidYamlExpression(t *testing.T) { }) } } - -func Test_sanitizeYaml(t *testing.T) { - type args struct { - fileAsString string - } - tests := []struct { - name string - args args - want string - }{ - { - name: "sanitize yaml starting with ---", - args: args{ - fileAsString: "---\nlabel: test", - }, - want: "# ---\nlabel: test", - }, - { - name: "sanitize yaml not starting with ---", - args: args{ - fileAsString: "label: test", - }, - want: "label: test", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := sanitizeYaml(tt.args.fileAsString) - assert.Equal(t, tt.want, got) - }) - } -} - -func Test_revertSanitizeYaml(t *testing.T) { - type args struct { - fixedYamlString string - } - tests := []struct { - name string - args args - want string - }{ - { - name: "revert sanitized yaml starting with ---", - args: args{ - fixedYamlString: "# ---\nlabel: test", - }, - want: "---\nlabel: test", - }, - { - name: "revert sanitized yaml not starting with ---", - args: args{ - fixedYamlString: "label: test", - }, - want: "label: test", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := revertSanitizeYaml(tt.args.fixedYamlString) - assert.Equal(t, tt.want, got) - }) - } -}