Terrascan init and config handling refactor #576

devang-gaur · 2021-03-02T13:00:51Z

fixes #550 . fixes #570, fixes #619

This refactor is mainly regarding how config is handled in terrascan code. These above mentioned bug fixes are a ripple effect of this refactor.

Regarding #619 , a warning will be logged every time the user specify only one of the two policy path params, path & rego_subdir . after the warning, the default value will be applied for the unspecified of the two policy path parameter.

Regarding config, the global scoped configuration will be loaded once and only once, into singleton object, to maintain a single source of truth. To access the config values, getter functions such as config.GetPolicyBasePath( ) must be used.

PS : make sure you don't store the config values in a package scoped variable like below or inside the init( ) method,

package demo

var demoPath = config.GetPolicyBasePath( )

func dpDemo( ) {
    ....
}

that won't work as those values are loaded in compile time whereas the config itself is loaded in runtime. So, that compile time loaded variable, demoPath's, value would be blank.

codecov · 2021-03-13T21:26:07Z

Codecov Report

Merging #576 (10d8469) into master (317a536) will decrease coverage by 0.05%.
The diff coverage is 75.49%.

@@            Coverage Diff             @@
##           master     #576      +/-   ##
==========================================
- Coverage   78.13%   78.08%   -0.06%     
==========================================
  Files         103      104       +1     
  Lines        2570     2596      +26     
==========================================
+ Hits         2008     2027      +19     
- Misses        420      422       +2     
- Partials      142      147       +5

Impacted Files	Coverage Δ
pkg/cli/output_writer.go	`33.33% <ø> (ø)`
pkg/cli/register.go	`4.54% <0.00%> (-1.02%)`	⬇️
pkg/config/config-reader.go	`86.36% <0.00%> (ø)`
pkg/http-server/file-scan.go	`81.00% <0.00%> (+6.00%)`	⬆️
pkg/http-server/remote-repo.go	`69.64% <0.00%> (ø)`
pkg/termcolor/termcolor.go	`85.71% <ø> (ø)`
pkg/policy/opa/engine.go	`65.56% <50.00%> (ø)`
pkg/utils/policy.go	`50.00% <50.00%> (ø)`
pkg/initialize/run.go	`64.28% <66.66%> (+2.74%)`	⬆️
pkg/runtime/executor.go	`86.36% <66.66%> (-4.55%)`	⬇️
... and 8 more

kanchwala-yusuf · 2021-03-22T11:37:57Z

pkg/cli/register.go

@@ -43,8 +46,24 @@ func Execute() {
 	cobra.OnInitialize(func() {
 		// Set up the logger
 		logging.Init(LogType, LogLevel)
+
+		var configfile string


I believe we can completely get rid of configfile variable

kanchwala-yusuf · 2021-03-22T11:38:57Z

pkg/cli/register.go

 )

+const configEnvvarName = "TERRASCAN_CONFIG"


We can Globally export this env variable in config package

kanchwala-yusuf · 2021-03-22T12:08:22Z

pkg/config/global_test.go

 )

 func TestLoadGlobalConfig(t *testing.T) {
 	testConfigFile := "./testdata/terrascan-config.toml"
+	absDefaultBasePolicyPath, absDefaultPolicyRepoPath, _ := utils.GetAbsPolicyConfigPaths(defaultBasePolicyPath, defaultPolicyRepoPath)


It would be better to handle errors here!

kanchwala-yusuf · 2021-03-22T12:15:09Z

pkg/initialize/run.go

-		if isScanCmd {
+	if path, err := os.Stat(config.GetPolicyRepoPath()); err == nil && path.IsDir() {
+
+		zap.S().Debug("EXISTS AND IS A DIR")


Can we please improve this debug message? or remove it?

kanchwala-yusuf · 2021-03-22T12:15:26Z

pkg/initialize/run.go

+
+		zap.S().Debug("EXISTS AND IS A DIR")
+		if isNonInitCmd {
+			zap.S().Debug("IS NON INIT")


Can we please improve this debug message? or remove it?

kanchwala-yusuf · 2021-03-22T12:18:00Z

pkg/notifications/notifiers.go

-	// ErrTomlKeyNotPresent will be returned when config file does not have notificationsConfigKey
-	ErrTomlKeyNotPresent = fmt.Errorf("key not present in toml config")
+	// ErrNotificationNotPresent error is caused when there isn't any notification present in the config
+	ErrNotificationNotPresent = fmt.Errorf("no notification present")


Let's try to indicate that the notifications are not present in config file

kanchwala-yusuf · 2021-03-22T12:19:00Z

pkg/notifications/notifiers.go

-		return notifiers, ErrTomlKeyNotPresent
+	notifications := config.GetNotifications()
+	if len(notifications) == 0 {
+		zap.S().Debug("no notification detected from config")


May be we can improve this message, something like no notification specified in config

kanchwala-yusuf · 2021-03-22T12:25:45Z

pkg/runtime/executor_test.go

@@ -18,16 +18,16 @@ package runtime

 import (
 	"fmt"
+	iacProvider "github.com/accurics/terrascan/pkg/iac-providers"


Please run goimports -e -w pkg/!

thanks for pointing out

kanchwala-yusuf · 2021-03-22T12:28:11Z

pkg/runtime/rules.go

-	if len(configReader.GetSeverity().Level) > 0 {
-		e.severity = configReader.GetSeverity().Level
+	if len(config.GetSeverityLevel()) > 0 {
+		e.severity = config.GetSeverityLevel()


Please check why rules_test.go is deleted?

I recall that rules_test.go was checking how rules are being parsed from the config file. and now that logic has been shifted inside LoadGlobalConfig itself now.

patilpankaj212 · 2021-03-22T12:46:55Z

pkg/initialize/run.go

@@ -64,6 +65,12 @@ func Run(isScanCmd bool) error {

 // DownloadPolicies clones the policies to a local folder
 func DownloadPolicies() error {
+
+	policyBasePath := config.GetPolicyBasePath()


instead of assigning new variables, we can use values directly?

changes made where feasible

patilpankaj212 · 2021-03-22T13:16:48Z

pkg/cli/register.go

+			configfile = os.Getenv(configEnvvarName)
+		}
+
+		zap.S().Debugf("%s=%s", configEnvvarName, os.Getenv(configEnvvarName))


if env variable is not specified, we would log:
TERRASCAN_CONFIG=, is that okay?
also, we are using : when logging a value.

changes made

pkg/initialize/run.go

test/e2e/init/init_test.go

patilpankaj212

added review.

…t specified together

sonarcloud · 2021-03-22T14:27:35Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

devang-gaur requested review from patilpankaj212 and kanchwala-yusuf March 2, 2021 13:00

devang-gaur force-pushed the terrascan_init_refactor branch 2 times, most recently from d03f7c3 to 2d64a97 Compare March 3, 2021 18:39

devang-gaur force-pushed the terrascan_init_refactor branch 2 times, most recently from 1ca848a to 5b9f3fa Compare March 10, 2021 15:24

devang-gaur changed the title ~~Terrascan init refactor~~ Terrascan init and config handling refactor Mar 10, 2021

devang-gaur force-pushed the terrascan_init_refactor branch from 9845a54 to 51d8817 Compare March 13, 2021 21:19

devang-gaur changed the title ~~Terrascan init and config handling refactor~~ [DO NOT MERGE]Terrascan init and config handling refactor Mar 15, 2021

devang-gaur mentioned this pull request Mar 15, 2021

policy download refactor #618

Merged

devang-gaur force-pushed the terrascan_init_refactor branch 2 times, most recently from eafa454 to b2f9759 Compare March 17, 2021 15:30

devang-gaur changed the title ~~[DO NOT MERGE]Terrascan init and config handling refactor~~ Terrascan init and config handling refactor Mar 17, 2021

Terrascan init and config handling refactor

cc3b4bb

devang-gaur force-pushed the terrascan_init_refactor branch 2 times, most recently from fe97d9b to 85e6077 Compare March 18, 2021 00:28

devang-gaur requested a review from jlk March 18, 2021 11:29

kanchwala-yusuf previously approved these changes Mar 22, 2021

View reviewed changes

patilpankaj212 reviewed Mar 22, 2021

View reviewed changes

pkg/initialize/run.go Outdated Show resolved Hide resolved

patilpankaj212 reviewed Mar 22, 2021

View reviewed changes

test/e2e/init/init_test.go Outdated Show resolved Hide resolved

patilpankaj212 suggested changes Mar 22, 2021

View reviewed changes

devang-gaur dismissed kanchwala-yusuf’s stale review via e0c6af6 March 22, 2021 14:08

devang-gaur force-pushed the terrascan_init_refactor branch from 85e6077 to e0c6af6 Compare March 22, 2021 14:08

updated warning message when policy base path and rego sub path aren'…

10d8469

…t specified together

devang-gaur force-pushed the terrascan_init_refactor branch from e0c6af6 to 10d8469 Compare March 22, 2021 14:24

patilpankaj212 approved these changes Mar 22, 2021

View reviewed changes

devang-gaur merged commit a30788a into tenable:master Mar 22, 2021

patilpankaj212 mentioned this pull request Jun 16, 2021

'k8s' key updated multiple times in policy package #439

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Terrascan init and config handling refactor #576

Terrascan init and config handling refactor #576

devang-gaur commented Mar 2, 2021 •

edited

Loading

codecov bot commented Mar 13, 2021 •

edited

Loading

kanchwala-yusuf Mar 22, 2021

kanchwala-yusuf Mar 22, 2021

kanchwala-yusuf Mar 22, 2021

kanchwala-yusuf Mar 22, 2021

kanchwala-yusuf Mar 22, 2021

kanchwala-yusuf Mar 22, 2021

kanchwala-yusuf Mar 22, 2021

kanchwala-yusuf Mar 22, 2021

devang-gaur Mar 22, 2021

kanchwala-yusuf Mar 22, 2021

devang-gaur Mar 22, 2021

patilpankaj212 Mar 22, 2021

devang-gaur Mar 22, 2021

patilpankaj212 Mar 22, 2021

devang-gaur Mar 22, 2021

patilpankaj212 left a comment

sonarcloud bot commented Mar 22, 2021

Terrascan init and config handling refactor #576

Terrascan init and config handling refactor #576

Conversation

devang-gaur commented Mar 2, 2021 • edited Loading

codecov bot commented Mar 13, 2021 • 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

patilpankaj212 left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 22, 2021

devang-gaur commented Mar 2, 2021 •

edited

Loading

codecov bot commented Mar 13, 2021 •

edited

Loading