[Bug]: issue CRD secrets keyVault

DrummyFloyd commented 4 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Affected Resource(s)

apiVersion: keyvault.azure.upbound.io/v1beta1 kind: Secret

Resource MRs required to reproduce the bug

apiVersion: keyvault.azure.upbound.io/v1beta1
kind: Secret

Steps to Reproduce

---
apiVersion: pkg.crossplane.io/v1
kind: Provider
metadata:
  name: provider-azure-keyvault
spec:
  package: xpkg.upbound.io/upbound/provider-azure-keyvault:v1.4.0
---
apiVersion: keyvault.azure.upbound.io/v1beta1
kind: Secret
metadata:
  name: test-secret
spec:
  forProvider:
    keyVaultIdSelector:
      matchLabels:
        xvault.azure.xor.io/name: "vault-1245azer8907"
    valueSecretRef:
      key: test
      name: test
      namespace: crossplane
---
# https://kubernetes.io/docs/concepts/configuration/secret/
apiVersion: v1
kind: Secret
metadata:
  name: mysecret
type: Opaque
data:
  test: test

What happened?

secret/mysecret unchanged
The Secret "test-secret" is invalid: 
* spec: Required value
* <nil>: Invalid value: "null": some validation rules were not checked because the object was invalid; correct the existing errors to complete validation

can't achieve to get the crd read correctly crd

❯ k get crds |rg azure
accesspolicies.keyvault.azure.upbound.io                             2024-07-19T15:01:08Z
certificatecontacts.keyvault.azure.upbound.io                        2024-07-19T15:01:08Z
certificateissuers.keyvault.azure.upbound.io                         2024-07-19T15:01:08Z
certificates.keyvault.azure.upbound.io                               2024-07-19T15:01:08Z
keys.keyvault.azure.upbound.io                                       2024-07-19T15:01:08Z
managedhardwaresecuritymodules.keyvault.azure.upbound.io             2024-07-19T15:01:08Z
managedstorageaccounts.keyvault.azure.upbound.io                     2024-07-19T15:01:08Z
managedstorageaccountsastokendefinitions.keyvault.azure.upbound.io   2024-07-19T15:01:08Z
providerconfigs.azure.upbound.io                                     2024-07-19T15:01:06Z
providerconfigusages.azure.upbound.io                                2024-07-19T15:01:06Z
resourcegroups.azure.upbound.io                                      2024-07-19T15:01:06Z
resourceproviderregistrations.azure.upbound.io                       2024-07-19T15:01:06Z
secrets.keyvault.azure.upbound.io                                    2024-07-19T15:01:08Z => present 
storeconfigs.azure.upbound.io                                        2024-07-19T15:01:06Z
subscriptions.azure.upbound.io                                       2024-07-19T15:01:06Z
vaults.keyvault.azure.upbound.io                                     2024-07-19T15:01:08Z
xvaults.azure.xor.io

Relevant Error Output Snippet

No response

Crossplane Version

1.16

Provider Version

1.4

Kubernetes Version

1.28

Kubernetes Distribution

OVH

Additional Info

No response

naimadswdn commented 2 months ago

The issue is related to the spec.initProvider.valueSecretRef started to being required with the 1.3.0 release.

I just tried to update the provider version from 1.2.0 to 1.5.0 and I am suffering such an error: spec.initProvider.valueSecretRef: Required value, <nil>: Invalid value: "null": some validation rules were not checked because the object was invalid; correct the existing errors to complete validation

For the 1.2.0 provider it does works as expected. It is even visible in the docs:

for 1.2.0 the value is not required, because it doesn't even exist: https://marketplace.upbound.io/providers/upbound/provider-azure-keyvault/v1.2.0/resources/keyvault.azure.upbound.io/Secret/v1beta1#doc:spec-initProvider
for > 1.3.0 the value is required (a bug): https://marketplace.upbound.io/providers/upbound/provider-azure-keyvault/v1.3.0/resources/keyvault.azure.upbound.io/Secret/v1beta1#doc:spec-initProvider-valueSecretRef

I see it has been added by the @gravufo on the 1st of June: https://github.com/crossplane-contrib/provider-upjet-azure/blame/main/package/crds/keyvault.azure.upbound.io_secrets.yaml#L321

Is the fix so simple as removing the:

required:
- valueSecretRef

from the CRD definition? If yes, then I can create a PR. Otherwise, I am not into golang to dig into that :(

EDIT: For the newly created resources it does works, probably because the LateInitialize - it only stops to work with already existing resources. Of course, I could add this value (spec.initProvider.valueSecretRef) in order to finish the provider update, but its clearly an unwanted behavior.

gravufo commented 2 months ago

@naimadswdn thanks for tagging me. I checked my changes and I don't think I voluntarily introduced this. In fact, all of these files are auto-generated and the changes I have done in that PR should not have affected this. My guess is that the generator has been modified somehow between the last time it was run and the time I did my PR, so I ended up having those changes at the same time. Unfortunately, removing the line from the CRD will not be a permanent solution. Any subsequent PR that runs make generate will reintroduce the issue.

I think someone more familiar with Upjet may have a better idea of where to do a fix.

naimadswdn commented 2 months ago

Thanks @gravufo for the meaningful response. 👍 I see that there were changes in the build submodule on the 30th of May, so maybe @turkenf can help us to understand what happen here.

naimadswdn commented 2 months ago

Any response about it? I would like to understand why one of the initProvider (that is still in the beta!!) fields become Required.

The issue can be easily reproduced.
First create a new Secret object with provider 1.2.0. Then, update the provider to higher version (whatever, it may be currently highest 1.5.1): The Secret is not going to be Synced, because of missing initProvider.valueSecretRef.

CCOLLOT commented 2 weeks ago

Any news? This issue is making the provider hard to use

DrummyFloyd commented 2 weeks ago

Any news? This issue is making the provider hard to use

tbh honest on my side, i just created a css & a PushSecret (external secret operator) to push the secret directly to vault

gravufo commented 2 weeks ago

There's an upjet meeting today, you may want to bring this issue up. Here is the google docs agenda: https://docs.google.com/document/d/1QJfsAk-pdo_j2cKsJtG6W-BOQh_3ilhiRVTYVIGkDXM/edit?tab=t.0#heading=h.cfsfte4lw0gh

The zoom link: https://upbound-io.zoom.us/j/82277438090?pwd=pgJE4lGa1KkXyZZ80N6GwvHNZhuave.1r

The call takes place at 08:00 PT/16:00 UTC.

crossplane-contrib / provider-upjet-azure