added kustomize support #378

devang-gaur · 2020-11-11T10:37:01Z

Adds iac-provider that enables scanning directly from kustomize directories.

pkg/iac-providers/kustomize.go

kanchwala-yusuf · 2020-11-11T10:41:30Z

pkg/iac-providers/kubernetes/v1/normalize.go

@@ -85,7 +85,7 @@ func (k *K8sV1) getNormalizedName(kind string) string {
 }

 // normalize takes the input document and normalizes it


Linter error! change normalize to Normalize

kanchwala-yusuf · 2020-11-11T10:42:41Z

pkg/iac-providers/kustomize/v3/load-dir.go

+	return res, nil
+}
+
+/*


Please remove the unwanted code block

kanchwala-yusuf

Testdata folder is added but no tests are added

kanchwala-yusuf · 2020-11-11T11:02:52Z

pkg/utils/path.go

+
+// FindFilesBySuffixInCurrentDir finds all the immediate files within a given directory that have the specified suffixes
+// Returns an array for string pointers as a list of files
+func FindFilesBySuffixInCurrentDir(basePath string, suffixes []string) ([]*string, error) {


Are you searching for files in current dir or in basePath?

May be a better name would be FindFilesBySuffixInCurrentDir

Isn't that the name?

Sorry copy paste mistake.
Can we use the FindFilesBySuffix method here?

No. we don't want a recursive search here. We just want to look up files on a single depth level. No looking into subdirectories.

Updating the function name though

kanchwala-yusuf · 2020-11-11T11:05:23Z

pkg/iac-providers/kustomize/v3/load-dir.go

+)
+
+// LoadIacDir loads the kustomize directory
+func (k *KustomizeV3) LoadIacDir(absRootDir string) (output.AllResourceConfigs, error) {


It is a sincere request to add comments at function level and inside the function as well, so as to help the user understand what's going on. A new reader may not be able to immediately understand what task is being achieved in the function

codecov · 2020-11-11T11:16:10Z

Codecov Report

Merging #378 (25f258c) into master (73d29aa) will decrease coverage by 2.35%.
The diff coverage is 25.49%.

@@            Coverage Diff             @@
##           master     #378      +/-   ##
==========================================
- Coverage   65.90%   63.55%   -2.36%     
==========================================
  Files          80       85       +5     
  Lines        1830     1904      +74     
==========================================
+ Hits         1206     1210       +4     
- Misses        521      591      +70     
  Partials      103      103

Impacted Files	Coverage Δ
pkg/iac-providers/kustomize/v3/load-dir.go	`0.00% <0.00%> (ø)`
pkg/iac-providers/kustomize/v3/load-file.go	`0.00% <0.00%> (ø)`
pkg/iac-providers/kustomize/v3/types.go	`0.00% <0.00%> (ø)`
pkg/iac-providers/register.go	`66.66% <ø> (ø)`
pkg/utils/path.go	`78.57% <0.00%> (-10.62%)`	⬇️
pkg/utils/yaml.go	`61.81% <63.15%> (-11.52%)`	⬇️
pkg/iac-providers/kustomize.go	`100.00% <100.00%> (ø)`
pkg/policy/kustomize.go	`100.00% <100.00%> (ø)`
pkg/iac-providers/kubernetes/v1/normalize.go	`74.46% <0.00%> (ø)`
... and 5 more

kanchwala-yusuf · 2020-11-11T11:17:21Z

pkg/iac-providers/kustomize/v3/types.go

+	}
+}
+
+func getFullFileName(file, ext string) string {


Would AddSuffix be a better name for this functon? And would it make sense to add it to utils package?

yes, makes sense.

kanchwala-yusuf · 2020-11-11T11:23:29Z

pkg/iac-providers/kustomize/v3/load-dir.go

+
+	if len(files) == 0 {
+		err := errors.New("could not find a kustomization.yaml/yml file in the directory")
+		zap.S().Warn("error while searching for iac files", zap.String("root dir", absRootDir), zap.Error(err))


Shouldn't this be an error level log (zap.S().Error)?

kanchwala-yusuf · 2020-11-11T11:23:46Z

pkg/iac-providers/kustomize/v3/load-dir.go

+
+	files, err := utils.FindFilesBySuffixInCurrentDir(absRootDir, KustomizeFileNames())
+	if err != nil {
+		zap.S().Warn("error while searching for iac files", zap.String("root dir", absRootDir), zap.Error(err))


Shouldn't this be an error level log (zap.S().Error)?

kanchwala-yusuf · 2020-11-11T11:27:39Z

pkg/utils/path.go

+
+// FindFilesBySuffixInCurrentDir finds all the immediate files within a given directory that have the specified suffixes
+// Returns an array for string pointers as a list of files
+func FindFilesBySuffixInCurrentDir(basePath string, suffixes []string) ([]*string, error) {


Sorry copy paste mistake.
Can we use the FindFilesBySuffix method here?

kanchwala-yusuf · 2020-11-11T11:30:28Z

pkg/utils/yaml.go

+
+// LoadYAMLString loads a YAML String. Can return one or more IaC Documents.
+// Besides reading in file data, its main purpose is to determine and store line number and filename metadata
+func LoadYAMLString(data, absFilePath string) ([]*IacDocument, error) {


I see a lot of code duplication between LoadYaml and LoadYamlString methods.

Would it make sense to have something like LoadYamlBytes and let LoadYamlString and LoadYaml both call LoadYamBytes?

Yeah good point

kanchwala-yusuf · 2020-11-11T11:34:31Z

pkg/iac-providers/kustomize/v3/load-dir.go

+
+	if len(files) > 1 {
+		err := errors.New("a directory cannot have more than 1 kustomization.yaml/yml file")
+		zap.S().Warn("error while searching for iac files", zap.String("root dir", absRootDir), zap.Error(err))


Shouldn't this be an error level log (zap.S().Error)?

kanchwala-yusuf · 2020-11-11T11:58:30Z

pkg/iac-providers/kustomize/v3/load-dir.go

+
+	var yamlkustomizeobj map[string]interface{}
+	var kustomizeFileName string
+	for _, filename := range KustomizeFileNames() {


Can we use files variable rather than calling KustomizeFileNames?

williepaul · 2020-11-12T01:31:38Z

pkg/iac-providers/kustomize/v3/load-dir.go

+)
+
+const (
+	kustomizedirectory string = "kustomize_directory"


Recommend to use either "kustomization" or "kustomization_file" here instead, as that's the proper terminology as per https://kubectl.docs.kubernetes.io/references/kustomize/glossary/. Also, just a side note on coding style--several linters will flag the variable names if not using camel case, and so usually we like to see that convention followed, otherwise it adds noise to the linter/IDE

williepaul · 2020-11-12T01:34:40Z

pkg/iac-providers/kustomize/v3/load-dir.go

+	}
+
+	if len(files) == 0 {
+		err := errors.New("could not find a kustomization.yaml/yml file in the directory")


Although the compiler is fine, linters will flag the redeclaration of variables (err) in this case and the next

williepaul · 2020-11-12T01:35:45Z

pkg/iac-providers/kustomize/v3/load-dir.go

+	var config output.ResourceConfig
+	config.Type = kustomizedirectory
+	config.Name = filepath.Dir(absRootDir)
+	config.Line = 0


config.Line default should be 1. My original change used 0, so it was a bad example--please change to 1 as the line numbers will start from 1.

williepaul · 2020-11-12T01:41:30Z

pkg/iac-providers/kustomize/v3/load-dir.go

+
+	allResourcesConfig := make(map[string][]output.ResourceConfig)
+
+	files, err := utils.FindFilesBySuffixInCurrentDir(absRootDir, KustomizeFileNames())


Is it difficult to support digesting all kustomizations within a directory hierarchy similarly to kustomize? This will likely end up being a pain point for folks who are heavily using kustomize. If I understand how kustomize works correctly, we should render the entire directory structure, otherwise we may not be scanning output 100% equivalent to the true kustomize output.

kustomize takes care of the directory structure by itself. So I didn't have to do anything special to achieve that.

@williepaul https://github.com/accurics/terrascan/pull/378/files#r522472187

pkg/iac-providers/kustomize/v3/load-dir.go

williepaul · 2020-11-12T01:56:49Z

pkg/iac-providers/kustomize/v3/load-file.go

+	errLoadIacFileNotSupported = fmt.Errorf("load iac file is not supported for kustomize")
+)
+
+// LoadIacFile is not supported for helm. Only loading chart directories are supported


kustomize (not helm)

Aaaah. thanks for pointing out

pkg/iac-providers/kustomize/v3/load-dir.go

pkg/iac-providers/kustomize.go

kanchwala-yusuf · 2020-11-12T03:46:26Z

@dev-gaur, need to rebase this PR branch with master. Some merge conflicts.

devang-gaur · 2020-11-12T08:57:03Z

Hi @kanchwala-yusuf @williepaul I have addressed all the changes request. PTAL.

kanchwala-yusuf · 2020-11-12T09:40:19Z

pkg/utils/yaml.go

+
+// getIacDocumentList provides one or more IaC Documents.
+// Besides reading in file data, its main purpose is to determine and store line number and filename metadata
+func getIacDocumentList(scanner *bufio.Scanner, bytearray []byte, filePath string) ([]*IacDocument, error) {


Would it be better if the function name some how mentions YAML, as yaml documents are being created here?

technically we're passing a scanner object as input

pkg/iac-providers/kustomize/v3/testdata/multibases/kustomization.yaml

williepaul · 2020-11-13T01:46:35Z

pkg/iac-providers/kustomize/v3/load-dir.go

+)
+
+const (
+	kustomizedirectory string = "kustomization_directory"


Can you change this to "kustomization"

sonarqubecloud · 2020-11-13T08:34:36Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
3 Code Smells

No Coverage information
4.4% Duplication

kanchwala-yusuf suggested changes Nov 11, 2020

View reviewed changes

kanchwala-yusuf reviewed Nov 11, 2020

View reviewed changes

devang-gaur force-pushed the add_kustomize_support branch 2 times, most recently from 95ea924 to 2f8eceb Compare November 11, 2020 11:12

kanchwala-yusuf reviewed Nov 11, 2020

View reviewed changes

williepaul suggested changes Nov 12, 2020

View reviewed changes

devang-gaur force-pushed the add_kustomize_support branch from c37b9bc to 92aa420 Compare November 12, 2020 07:13

Devang Gaur added 3 commits November 12, 2020 12:57

added kustomize support

ece2ab1

changes

7e73f26

added vendor/ in .gitignore

442e39a

devang-gaur force-pushed the add_kustomize_support branch from 92aa420 to 442e39a Compare November 12, 2020 07:31

optimized yaml utility function

09d81f8

kanchwala-yusuf suggested changes Nov 12, 2020

View reviewed changes

devang-gaur force-pushed the add_kustomize_support branch from e0c0d6b to 8325926 Compare November 12, 2020 15:38

kanchwala-yusuf previously approved these changes Nov 12, 2020

View reviewed changes

devang-gaur commented Nov 12, 2020

View reviewed changes

pkg/iac-providers/kustomize/v3/testdata/multibases/kustomization.yaml Show resolved Hide resolved

williepaul reviewed Nov 13, 2020

View reviewed changes

last minute fixes

25f258c

devang-gaur dismissed kanchwala-yusuf’s stale review via 25f258c November 13, 2020 08:33

devang-gaur force-pushed the add_kustomize_support branch from 8325926 to 25f258c Compare November 13, 2020 08:33

williepaul approved these changes Nov 13, 2020

View reviewed changes

kanchwala-yusuf approved these changes Nov 13, 2020

View reviewed changes

kanchwala-yusuf merged commit 5c96b2f into tenable:master Nov 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added kustomize support #378

added kustomize support #378

devang-gaur commented Nov 11, 2020

kanchwala-yusuf Nov 11, 2020

kanchwala-yusuf Nov 11, 2020

kanchwala-yusuf left a comment

kanchwala-yusuf Nov 11, 2020

devang-gaur Nov 11, 2020

kanchwala-yusuf Nov 11, 2020

devang-gaur Nov 11, 2020

devang-gaur Nov 11, 2020

kanchwala-yusuf Nov 11, 2020

codecov bot commented Nov 11, 2020 •

edited

Loading

kanchwala-yusuf Nov 11, 2020

devang-gaur Nov 11, 2020

kanchwala-yusuf Nov 11, 2020

kanchwala-yusuf Nov 11, 2020

kanchwala-yusuf Nov 11, 2020

kanchwala-yusuf Nov 11, 2020

devang-gaur Nov 11, 2020

kanchwala-yusuf Nov 11, 2020

kanchwala-yusuf Nov 11, 2020

devang-gaur Nov 11, 2020

williepaul Nov 12, 2020

williepaul Nov 12, 2020

williepaul Nov 12, 2020

williepaul Nov 12, 2020

devang-gaur Nov 12, 2020

devang-gaur Nov 12, 2020

williepaul Nov 12, 2020

devang-gaur Nov 12, 2020

kanchwala-yusuf commented Nov 12, 2020

devang-gaur commented Nov 12, 2020

kanchwala-yusuf Nov 12, 2020

devang-gaur Nov 12, 2020

williepaul Nov 13, 2020

sonarqubecloud bot commented Nov 13, 2020

		@@ -85,7 +85,7 @@ func (k *K8sV1) getNormalizedName(kind string) string {
		}

		// normalize takes the input document and normalizes it


		allResourcesConfig := make(map[string][]output.ResourceConfig)

		files, err := utils.FindFilesBySuffixInCurrentDir(absRootDir, KustomizeFileNames())

added kustomize support #378

added kustomize support #378

Conversation

devang-gaur commented Nov 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanchwala-yusuf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 11, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanchwala-yusuf commented Nov 12, 2020

devang-gaur commented Nov 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 13, 2020

codecov bot commented Nov 11, 2020 •

edited

Loading