Handle sensitive fields

[turkenh]

Description of your changes

This PR resolves handling sensitive fields in resource definition by generating appropriate schema (i.e. a secretRef for sensitive inputs and dropping fields from status), building connection details secret content, and constructing terraform attributes by gathering data from input/connection secrets.

Task list:

• Extract field paths for fields marked as sensitive in resource schema
• Get values matching field paths
• Developers can add custom fields to be included in addition to whats is coming with schema
• Generate GetConnectionDetails function in terraformed and verify sensitive fields landing in connection details secret
• Adjust CRD schema for sensitive fields
  • If the sensitive field is a parameter, generate secret key ref in the schema
  • If the sensitive field is an observation, drop the field from the schema
• Implement sensitive getting sensitive information back
  • Generate GetSensitiveObservation - returns data in connection details as terraform state attributes
  • Generate GetSensitiveParameters - returns data in the secrets referenced from parameter fields

Fixes #35

I have:

• Read and followed Crossplane'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 with the following provider-tf-aws PR: crossplane-contrib/provider-jet-aws#24

[muvaf]

Awesome to see the whole sensitive information issue including user input getting resolved!

[muvaf]

If I understood correctly, this function takes an expanded JSON path and returns a TF path that you can use to set a value on a TF file. I wonder if we can get away with some string manipulation here to replace the field names. Do you think something like this worth testing?

[turkenh]

Thanks for the suggestion and for preparing the code snippet.

I'll try if I can simplify following an approach you suggested, however, one caveat here is, fieldpath.Paved does not print keys of maps inside [ ] if the key contains dots. See this as an example. So, initially, I attempted some regex replace kind of solution but I ended up parsing them using the same parsing not to miss something or build a complex method to handle edge cases like I mentioned.

[turkenh]

I had another look. Actual complexity came from identifying expandedJSON is for which wildcarded fieldpath due to the inconsistent printing behaviour I mentioned above (see possible values for expandedJSON), which is currently handled by expandedFor function.
I think it wouldn't worth to use replacer solution given we already have to parse fieldpath to segments.

[muvaf]

Hmm I didn't think of the case where there is a string as index actually. We'd have to make it not consider strings between [ and ] during replace, which could be more complex as you said. Thanks for testing it!

[muvaf]

Would it make sense to use map[string]string instead of []string as type of CustomFieldPaths, then merge it into fieldPaths directly? It could be easier to work with if there was a single map that we need to use here. I didn't try to see whether it works, just an idea you might want to consider.

[turkenh]

This was something I considered but I didn't like that users would need to provide a map in that case and the value part of the map (xp/json path) is even unknown before the code generation. We could expect them to set some placeholder (or empty?) values but that sounded hacky.
But anyway, as we discussed today, I will need to revisit getting custom configuration since we would need the distinction of include into connection details but not sensitive type of fields.

[turkenh]

Thanks for the review @muvaf! Resolved/responded to all your comments.

The two open items I need to add to PR is:

• Always include id in connection details
• Ability to configure fields to get into connection but not mark as sensitive (i.e. no schema change)

[ulucinar]

nit:
Just for consideration: If the field name already ends with secret, we will have something like xyzSecretSecretRef.

[turkenh]

Yeah, but not sure xyzSecretRef would be better in such a case or are there any better alternative

[muvaf]

LGTM! Thanks @turkenh !

[muvaf]

Right now it looks like the following:

map[string]string{"master_password": "masterPasswordSecretRef"}

In TFState, master_password is at the top level in HCL of the resource definition, whereas spec.forProvider.masterPasswordSecretRef would have been the fully qualified path for the Crossplane definition. Both work but the latter is correct in the sense that it's ready to be used in all possible contexts whereas if we have only masterPasswordSecretRef, we'd need to know the prefix that's needed to use to actually access the data.

[muvaf]

Hmm I didn't think of the case where there is a string as index actually. We'd have to make it not consider strings between [ and ] during replace, which could be more complex as you said. Thanks for testing it!