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

Prevent secretInjectionConfigs from setting empty value for key that is not present #81

Open
schaschko opened this issue Feb 14, 2025 · 4 comments

Comments

@schaschko
Copy link

schaschko commented Feb 14, 2025

Is there a way to use secretInjectionConfigs in combination with Request, but don't have a key in the Secret updated if it's not present in the response anymore?

E.g. this API will return a field secret only on CREATE. Afterwards (OBSERVE) this field is just not in the response at all. Is there any way, to have a resource like this managed by Request (not DisposableRequest) while keeping the response value that was returned on the first CREATE request?

Or stated differently: if there is a field (e.g. secret like in this example) that's seen in the (first) response, but not on subsequent requests, should it be updated at all when it's not even there anymore? At the moment it is, meaning the corresponding key's value in the K8s Secret is emptied.

@klaudworks
Copy link

I see two different solutions to deal with APIs that do not return every field on every request e.g Stripe:

Solution 1: If a field is not returned from a request just leave the secret as it is. This is not really backwards compatible though and in most cases it is probably intended behavior to assume if something is not returned it is set to "null".

Solution 2: We could add a extra field for the SecretInjectionConfigs to deal with such APIs. That would be backwards compatible.

Possible solutions could be:

  1. add a sync policy that specifies if updates/deletes should be performed.
---
apiVersion: http.crossplane.io/v1alpha2
kind: Request
metadata:
  name: stripe-example
spec:
    ...
    secretInjectionConfigs:
        secretRef:
          name: example-secret
          namespace: example
        keyMappings:
          - secretKey: secret
            responseJQ: ".body.secret"
            syncPolicy:
              update: true
              delete: false
  1. add a field preserveExisting to prevent deletion of existing fields if the counterpart is not explicitly returned by the API.
---
apiVersion: http.crossplane.io/v1alpha2
kind: Request
metadata:
  name: stripe-example
spec:
    ...
    secretInjectionConfigs:
        secretRef:
          name: example-secret
          namespace: example
        keyMappings:
          - secretKey: secret
            responseJQ: ".body.secret"
            preserveExisting: true

I'd be open to implement this behavior if one of the maintainers approves the API change.

@arielsepton
Copy link
Member

arielsepton commented Feb 20, 2025

Thanks for the suggestions! @klaudworks I really like the idea behind the first solution, but I think the syncPolicy naming could be improved. What do you think about this approach?

apiVersion: http.crossplane.io/v1alpha2
kind: Request
metadata:
  name: stripe-example
spec:
  ...
  secretInjectionConfigs:
    secretRef:
      name: example-secret
      namespace: example
    keyMappings:
      - secretKey: secret
        responseJQ: ".body.secret"
        preserveOnMissing: true  # Preserves the existing value if the field is missing in the response
        preserveOnDelete: true   # Prevents deletion if the field is no longer returned

I’d be happy to approve this if you’re interested in implementing it!

@klaudworks
Copy link

klaudworks commented Feb 20, 2025

@arielsepton Thanks for your input. I see 3 behaviors that we wanna cover for the case secret field exists + response field missing:

  1. secret field value is set null -> preserveOnDeletion: true would work
  2. secret field is deleted -> should be the default
  3. secret field value is preserved -> preserveOnMissing: true

There is one problem though: Setting both to true would be ambiguous because do we set the field to null or preserve the value.
A cleaner solution could be to use a single field with 3 different strategies to cover each of the 3 behaviors when a field is missing:

missingFieldStrategy: preserve / setNull / delete

This would allow us to cover all the different cases. Happy to change the name of the field if you have a better suggestion. What do you think about the solution?

@arielsepton
Copy link
Member

@klaudworks

I really like this idea!
I think the three strategies you’ve defined make perfect sense.

I’m happy to approve this solution! Let me know if you need anything else from my side.

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

No branches or pull requests

3 participants