one question though - does this PR makes property mandatory for GetSecretMap? 🤔

@gusfcarvalho Yes, it does. It did that before that as well I believe. 🤔

Are you okay with that? ( before I merge this ).

Also, this is an obvious breaking change in that. But it's okay, because it's alpha.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

It's not really about the breaking change, but the fact this provider will be the only one not in line with other provider implementations. If a property is passed we parse it, but if no property is passed we just parse what we've found at the root of the secret

e.g. given the following content on a given secret provider (vault, aws sm, gcp sm, ...)

{
  "foo" :"bar",
  "nested": {"baz":"bing","bang":"bong"}
  }

if no property is specified we generate this secret:

foo: bar
nested: '{"baz":"bing", "bang":"bong"}'

while if nested property is provided, we generate this secret:

baz: bing
bang: bong

and lastly if foo property is provided we error out because that key does not contain a valid json.

that above is the behavior most providers implement ☝️ so I am having second thoughts if we should allow a provider that would not follow that 🤔

AFAIK the OG implementation is a subset of that (i.e. the property bit was 'not implemented') right?

@gusfcarvalho It definitely was. That's exactly how the old implementation is working.

I agree that it's an outlier.

To be clear, I don't know what to do about it. :D It makes sense to have parity, but yeah, essentially, this is working differently, than other providers.

Does 1password connect behaves differently than that today? 🤔

Nope. It behovass exactly the same way.

@gusfcarvalho Actually... :D Property is not required. If not provided, it will default to fetching fields. :)

Hi, sorry @Skarlso, but i can not figure out the new behavior :)

I used to set a plain json content in a plain note onepassword secret, such as:

{
  "foo": "123",
  "bar": "456"
}

Then, using a dataFrom in spec:

...
  dataFrom:
    - extract:
        key: my_secret/notesPlain
...

I was able to either:

• get all my json keys/values (foo and bar) as secret data without setting a data in my target spec

...
  target:
    deletionPolicy: Retain
    template:
      engineVersion: v2
      type: Opaque

• get some specific json key(s)/value(s) by their name

...
  target:
    deletionPolicy: Retain
    template:
      engineVersion: v2
      type: Opaque
      data:
        foo: {{ `{{ .foo }}` }}
        bar: {{ `{{ .bar }}` }}

Now, in both cases, i receive some events: error processing spec.dataFrom[0].extract, err: key not found

Isn't that the supposed way to do it ?

Hello @nervo. Humm. The code is almost the same. So it should have worked.

They key does exist in your given vault given in your Store, right?

@Skarlso, i finally found out what had changed :)

Right before this pr, the provider behaved just like the Google Cloud "all-keys-example-secret" here https://external-secrets.io/v0.18.2/guides/all-keys-one-secret/ , namely, a single secret item holds a single field, which contains raw json content, this json was unmarshalled, and boom, we got all our keys/values.
In my previous setup, i used to set a "Secure Note" item with plain raw json as single value. The value was accessible via the "notesPlain" field in onepassword api, so i used

  dataFrom:
    - extract:
        key: my_secret/notesPlain

Robust, but not very intuitive :)

Now, after that pr, i can continue to use "Secure Note" items, but i directly set keys/values as fields of my items, which is way more intuitive :) And i just need to declare the name of my item as key, without - obviously - the property|field.

  dataFrom:
    - extract:
        key: my_secret

(note that the "notesPlain" field seems not to be taken into account)

I've struggled a bit to understand this new behavior, digging the code and made many tests, but anyway, it works, and it works quite well ! Thanks for the good and hard job 💪

Thank you! Do you think we should add more tests or some documentation update to make that clearer?

Well, that's the fun part, the documentation did not mention at all the previous behavior with dataFrom, i've discovered it the hard way :)
So i guess you can just only document the new behavior.