Ignore k8s secret not found when ESS enabled

[ezgidemirel]

Description of your changes

In this change, we're ignoring k8s secret not found errors when the external secret store feature is enabled. In Vault implementation, we're not returning any error if the secret doesn't exist. To have the same behavior and allow secret creation at later stages, we start not to return any error in the k8s store as well. This is the root cause of this bug.

Fixes #

I have:

• Read and followed Crossplane's contribution process.
• Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

• consumed the change in crossplane
• enabled both the external store and xfn features
• created a claim which uses Vault as a store
• created a claim which uses k8s as a store
• disabled xfn feature
• created a claim which uses Vault as a store
• created a claim which uses k8s as a store

[pedjak]

if not found, then what would be the value of ks.Data?

could we add a unit test for the new behavior?

If ESS is not enabled, then should we return the error?

[ezgidemirel]

if not found, then what would be the value of ks.Data?

It will be an empty map.

could we add a unit test for the new behavior?

Sure, will update the PR.

If ESS is not enabled, then should we return the error?

This code block is only called when ESS is enabled.

[pedjak]

It will be an empty map.

Can we update the functions docs to reflect that?

[ezgidemirel]

It will be an empty map.

Can we update the functions docs to reflect that?

It shouldn't be necessary. In real world, we shouldn't end up with an empty secret. This change is done to proceed with an intermediate stage where the claim/composite secret is not created yet but it's fetched by the PTF composer to observe the state. Later in the reconciliation loop, the secret will be created as usual and we don't need to handle not found errors.

[phisco]

maybe making it more explicit would be helpful, something like:

Suggested change

	if err := ss.client.Get(ctx, types.NamespacedName{Name: n.Name, Namespace: ss.namespaceForSecret(n)}, ks); resource.IgnoreNotFound(err) != nil {
	err := ss.client.Get(ctx, types.NamespacedName{Name: n.Name, Namespace: ss.namespaceForSecret(n)}, ks)
	if kerrors.IsNotFound(err) {
	return nil
	}
	if err != nil {
	return errors.Wrap(err, errGetSecret)
	}

[phisco]

or is the caller relying on the fact that Metadata is set even on not found?

[ezgidemirel]

I think if we cannot find the secret on the store, we should overwrite the given secret fields with empty/nil values.

[pedjak]

We should return error only if we try to retrieve the secret with the name equal to fakeSecretName. Otherwise, if we do not perform this check, this tests is going to pass even if ReadKeyValues does not properly use the passed ScopedName

[phisco]

yeah, that's the issue with mocking, the mock is never going to be 100% precise with the real behaviour. I don't think it's strictly required here, but could be a separate test case maybe.

[pedjak]

mocks should react for only specific inputs. We are adding here the new test, so why not making it more tight here?

[ezgidemirel]

@pedjak , I think the SecretNotFound test does exactly what it needs to do. I can update the SuccessfulRead test with ScopedName check if you want.

[pedjak]

If you drop fakeSecretName from the setup, and have something like this:

n: store.ScopedName{}

the test is still going to pass.

The mock should return the error only for the given secret name.

[phisco]

But it feels a bit overkill tbh

[ezgidemirel]

The NewMockGetFn function does not use the object key. Which means we cannot check the scope name with that.

[pedjak]

The NewMockGetFn function does not use the object key. Which means we cannot check the scope name with that.

yeah, but we can do something like:

Client: &test.MockClient{
  MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error {
	if key.Name == fakeSecretName {
		return kerrors.NewNotFound(schema.GroupResource{}, "")
	}
	return errors.Errorf("Should return not found error actually for %s", key)
},
},

[ezgidemirel]

I know we can. But my point was, NewMockGetFn function is used for dozens of places without actually checking the name or namespace and I think it's OK. IMHO, if we're mocking an interface, we don't need to worry about the mocker itself. The critical part is checking the behavior after getting the result from the mock.

Anyways, I updated the SuccessfulRead test.

[pedjak]

I know we can. But my point was, NewMockGetFn function is used for dozens of places without actually checking the name or namespace and I think it's OK. IMHO, if we're mocking an interface, we don't need to worry about the mocker itself. The critical part is checking the behavior after getting the result from the mock.

We are not mocking just interface, we are actually mocking its invocations, and we in essence should define a number of invocations as set of (input params, results).

The code under tests should invoke an interface with proper input params, and should react properly on the obtained results. If we do not check the input params, then we verify just partially the code behavior.

[pedjak]

if the test name says that return an empty secret, should then the result be an empty map? If it should remain nil, then perhaps we should rephrase the reason above to reflect that?

[ezgidemirel]

What we want is not the secret, but the error. Therefore, the test is correct

[ezgidemirel]

What we want is not the secret, but the error. Therefore, the test is correct

Nevermind, what I thought is not the one I wrote. I'll update it.

[pedjak]

Not sure that I follow: in this test ReadKeyValues is not going to return the error, and then we compare secrets data with want.result. Do we consider an empty secret the one with Data field being set to nil or to an empty map?

[ezgidemirel]

The zero value of a map is nil. I observed different behaviors like while printing it, I get map[] but in the cmp.Diff method, it considers it as a nil.