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

fixes: recursive loop when parent and child module has same local block #900

Merged
merged 2 commits into from
Jul 2, 2021

Conversation

Rchanger
Copy link
Contributor

fixes #851

  • resolution of the local variable from the parent module
  • resolution of reference of map type.

@codecov
Copy link

codecov bot commented Jun 29, 2021

Codecov Report

Merging #900 (5d8ed50) into master (b8fda7c) will increase coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #900      +/-   ##
==========================================
+ Coverage   78.41%   78.42%   +0.01%     
==========================================
  Files         168      168              
  Lines        4470     4501      +31     
==========================================
+ Hits         3505     3530      +25     
- Misses        741      747       +6     
  Partials      224      224              
Impacted Files Coverage Δ
...ac-providers/terraform/commons/local-references.go 70.73% <72.72%> (-1.15%) ⬇️
...providers/terraform/commons/variable-references.go 74.68% <81.25%> (-3.78%) ⬇️
pkg/iac-providers/terraform/commons/references.go 85.96% <100.00%> (+2.29%) ⬆️
...c-providers/terraform/commons/lookup-references.go 76.00% <0.00%> (ø)
...c-providers/terraform/commons/module-references.go 47.50% <0.00%> (+7.50%) ⬆️

@@ -86,8 +87,20 @@ func (r *RefResolver) ResolveLocalRef(localRef, callerRef string) interface{} {
}

// replace the local value reference string with actual value
if reflect.TypeOf(val).Kind() == reflect.String {
valStr := val.(string)
if reflect.TypeOf(val).Kind() == reflect.String || reflect.TypeOf(val).Kind() == reflect.Map {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of invoking reflect.TypeOf(val).Kind() 3 times, you can store invoke once and store the value in a local variable. reflect function calls are generally heavy and slow from what I've read and experienced.

if err == nil {
value = temp
}
zap.S().Debugf("resolved value is not a json object", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message should be more generic here like ("failed to unmarshal json string %s", valueStr). because in some cases the problem can be with the temp struct too.

@@ -87,8 +88,20 @@ func (r *RefResolver) ResolveVarRef(varRef, callerRef string) interface{} {
}
zap.S().Debugf("resolved variable ref '%v', value: '%v'", varRef, val)

if reflect.TypeOf(val).Kind() == reflect.String {
valStr := val.(string)
if reflect.TypeOf(val).Kind() == reflect.String || reflect.TypeOf(val).Kind() == reflect.Map {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same. use a variable to store reflect values

"type": "aws_iam_user",
"config": {
"name": "joe != \"\" ? \"joe-fred\" : fred",
"tags": "${merge(${merge({\n CreatedBy = \"Terraform\"\n }, {\"this\":\"that\"})},\n {\n Name = joe != \"\" ? \"joe-fred\" : fred\n })}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not possible because here it is a string with an expression that needs to be evaluated. Once we add support for functions this will also change.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@patilpankaj212 patilpankaj212 merged commit cee9ab4 into tenable:master Jul 2, 2021
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.

Recursive Loop Scanning Terraform
4 participants