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

Fix slice type sensitive fieldpath generation #355

Merged
merged 2 commits into from
Feb 26, 2024

Conversation

sergenyalcin
Copy link
Member

Description of your changes

This PR fixes sensitive fieldpath generation for slices. In the current behavior, we do not have any handling mechanism for slices while calculating the fieldpath. With this PR we will have this.

I have:

  • Read and followed Upjet's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Tested on this PR. And observed two diffs in generated fieldpaths:

modified:   apis/elasticache/v1beta1/zz_user_terraformed.go
modified:   apis/memorydb/v1beta1/zz_user_terraformed.go

Diffs respectively:

-       return map[string]string{"authentication_mode[*].passwords[*]": "spec.forProvider.authenticationMode[*].passwords[*]SecretRef", "passwords[*]": "spec.forProvider.passwords[*]SecretRef"}
+       return map[string]string{"authentication_mode[*].passwords[*]": "spec.forProvider.authenticationMode[*].passwordsSecretRef[*]", "passwords[*]": "spec.forProvider.passwordsSecretRef[*]"}
-       return map[string]string{"authentication_mode[*].passwords[*]": "spec.forProvider.authenticationMode[*].passwords[*]SecretRef"}
+       return map[string]string{"authentication_mode[*].passwords[*]": "spec.forProvider.authenticationMode[*].passwordsSecretRef[*]"}

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thank you @sergenyalcin, left a comment regarding the situation with the maps.

Comment on lines 271 to 277
switch f.FieldType.String() {
case "string", "*string", "map[string]string", "map[string]*string":
cfg.Sensitive.AddFieldPath(fieldPathWithWildcard(f.TerraformPaths), "spec.forProvider."+fieldPathWithWildcard(f.CRDPaths)+sfx)
case "[]string", "[]*string":
f.CRDPaths[len(f.CRDPaths)-2] = f.CRDPaths[len(f.CRDPaths)-2] + sfx
cfg.Sensitive.AddFieldPath(fieldPathWithWildcard(f.TerraformPaths), "spec.forProvider."+fieldPathWithWildcard(f.CRDPaths))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because we already have a validation enumeration under this switch block, how about getting rid of the enumeration here?

Suggested change
switch f.FieldType.String() {
case "string", "*string", "map[string]string", "map[string]*string":
cfg.Sensitive.AddFieldPath(fieldPathWithWildcard(f.TerraformPaths), "spec.forProvider."+fieldPathWithWildcard(f.CRDPaths)+sfx)
case "[]string", "[]*string":
f.CRDPaths[len(f.CRDPaths)-2] = f.CRDPaths[len(f.CRDPaths)-2] + sfx
cfg.Sensitive.AddFieldPath(fieldPathWithWildcard(f.TerraformPaths), "spec.forProvider."+fieldPathWithWildcard(f.CRDPaths))
}
switch f.FieldType.(type) {
case *types.Slice:
f.CRDPaths[len(f.CRDPaths)-2] = f.CRDPaths[len(f.CRDPaths)-2] + sfx
cfg.Sensitive.AddFieldPath(fieldPathWithWildcard(f.TerraformPaths), "spec.forProvider."+fieldPathWithWildcard(f.CRDPaths))
default:
cfg.Sensitive.AddFieldPath(fieldPathWithWildcard(f.TerraformPaths), "spec.forProvider."+fieldPathWithWildcard(f.CRDPaths)+sfx)
}

Also, do we need to handle the maps in a similar manner?

Copy link
Member Author

@sergenyalcin sergenyalcin Feb 26, 2024

Choose a reason for hiding this comment

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

Thank you @ulucinar for that comment. It will be valuable to record an essential piece of information.

When I started to prepare this change, I first evaluated the map type together with the slice type and, put it in the same case, and tried generation. However, I got an index out of range error in this case. Then, in the debugging session I did, I saw that the fieldpath we generate for the fields of the map type does not contain the [*] expression at the end, similar to slices, but only the name of the field, like the string type. Therefore, I put the maps in the same case as the strings.

Calculated fieldpath examples

String: x.y.z
Map: x.y.z
Slice: x.y.z[*]

In short, we do not need to handle the map type here. It seems that the paved library can handle an expression without a wildcard correctly for maps but requires a wildcard for slices. At this point, we need a branch.

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thanks @sergenyalcin, lgtm.

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Copy link

github-actions bot commented Mar 6, 2024

Successfully created backport PR #366 for release-1.2.

Copy link

github-actions bot commented Mar 6, 2024

Successfully created backport PR #367 for release-1.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants