Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Support custom functions for data pre-processing #260

Open
AaronME opened this issue Mar 9, 2022 · 4 comments
Open

Support custom functions for data pre-processing #260

AaronME opened this issue Mar 9, 2022 · 4 comments
Labels
enhancement New feature or request wontfix This will not be worked on

Comments

@AaronME
Copy link

AaronME commented Mar 9, 2022

What problem are you facing?

The vault_generic_secret resource expects data to be formatted as JSON. It uses the Keys of this json to name secret fields when submitting to vault.

An example Kubernetes Secret:

apiVersion: v1
kind: Secret
metadata:
  name: example-secret
  namespace: default
stringData:
  exampleKey: '{"keyone": "valueone", "keytwo":"valuetwo"}'
type: Opaque

An example Vault Secret:

---
apiVersion: generic.vault.jet.crossplane.io/v1alpha1
kind: Secret
metadata:
  name: example-vault-secret
spec:
  forProvider:
    path: "secret/foo"
    dataJsonSecretRef:
      key: exampleKey
      name: example-secret
      namespace: default

The resulting secret in vault would look like this:

$ vault kv get secret/foo

===== Data =====
Key        Value
---        -----
keyone     valueone
keytwo     valuetwo

If the secret is not json-formatted, it will fail apply and not be created.

In terraform, there are built-in functions which can re-format data as it is submitted to new resources. In a provider, we would need to perform this re-formatting in the controller.

This creates several issues:

  1. json-formatted secrets (such as gcp credentials files) are often intended to be consumed as a string and instead are being decomposed into multiple secret fields.
  2. ConnectionDetails which are not json formatted are failing apply.

How could Terrajet help solve your problem?

Allow injecting formatting logic for certain fields. In this example, we would like to be able to specifying a pre-format function in our config.go file. This could be added as "Formatted" option on the TerraformResource Schema:

    p.AddResourceConfigurator("vault_generic_secret", func(r *config.Resource) {
        r.Formatted = config.Formatted {
          "data_json": {
				FormatFn: jsonFormat(data),
        }
    }

This would result would appear in the deepCopyInto method for parameters:

func (in *SecretParameters) DeepCopyInto(out *SecretParameters) {
	*out = *in
	if in.DataJSONSecretRef != nil {
		in, out := &in.DataJSONSecretRef, &out.DataJSONSecretRef
		*out = new(v1.SecretKeySelector)
		**out = jsonFormat(**in)
	}
...
@AaronME AaronME added the enhancement New feature or request label Mar 9, 2022
@muvaf
Copy link
Member

muvaf commented Mar 10, 2022

The conversion from CR to TF input file happens in GetParameters function of the given type. I see the following options for injecting a function:

  1. Let user override GetParameters function completely since it's already simple enough.
  2. Accept a function in config and call it before or after GetParameters call here.
  3. Accept a function and call it in the function block of GetParameters.
  4. Add injection points to the TJ controller, i.e. preCreate, preUpdate etc similar to ACK.

As opposed to external name config functions, I don't think we're seeing many cases that people need to change the schema of parameters that are used by TF (most of the time, they are additional and not input to TF itself). So, if we do it, I'd prefer it to be less intrusive as possible, for example it shouldn't change the way others are using Terrajet.

I think it'd be best if we can let user write a GetParameters function in a file in API package and we see it and skip generating GetParameters. Something like the following would be written in apis/<group>/v1alpha1/secretparameters_override.go:

// GetParameters of this Repository
func (tr *SecretParameters) GetParameters() (map[string]interface{}, error) {
	// your logic
	p, err := json.TFParser.Marshal(tr.Spec.ForProvider)
	if err != nil {
		return nil, err
	}
	base := map[string]interface{}{}
	return base, json.TFParser.Unmarshal(p, &base)
}

Once Terrajet sees that *SecretParameters already has a GetParameters() (map[string]interface{}, error) implemented, it'd not generate it. This way we could allow override of any of those functions without affecting anyone. Though I remember it was problematic to read the existing package for our type builder since the type definitions (type SecretParameters struct...) are deleted before generation, hence having a func (tr *SecretParameters) GetParameters() (map[string]interface{}, error implementation threw compilation errors which made the read operation fail (hence we create a package from scratch here). But I hadn't spend so much time on it back then, so it's worth experimenting by changing that NewPackage call with something that reads the package, like this.

@AaronME
Copy link
Author

AaronME commented Mar 15, 2022

@muvaf Are we sure that GetParameters is the right place to look?

I've been playing around with the generated code to see the results of interacting with GetParameters. I'm not seeing the contents of the SecretRef when I print out p in GetParameters. I only see Parameters that were parsed from the spec.forProvider field.

When I examine the Resource contents, I can see the SecretRef (secret name, key, etc), but I haven't yet found where we grab the secret data. Is this still in GetParameters?

@AaronME
Copy link
Author

AaronME commented Mar 15, 2022

I think we want to provide the ability to inject a function here. This would enable providers to "sanitize" connection secrets coming from other providers.

@stale
Copy link

stale bot commented Aug 13, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Aug 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants