Go client and CRD definition of SQLUser are inconsistent #598

radoslav-tomov · 2022-01-27T08:07:03Z

Bug Description

SQLUser CRD definiton states:

password:
description: |-
  The password for the user. Can be updated. For Postgres instances this is a Required field, unless type is set to
                  either CLOUD_IAM_USER or CLOUD_IAM_SERVICE_ACCOUNT.
oneOf:
- not:
    required:
    - valueFrom
  required:
  - value
- not:
    required:
    - value
  required:
  - valueFrom
properties:
  value:
    description: Value of the field. Cannot be used if 'valueFrom'
      is specified.
    type: string
  valueFrom:
    description: Source for the field's value. Cannot be used if 'value'
      is specified.
    properties:
      secretKeyRef:
        description: Reference to a value with the given key in the
          given Secret in the resource's namespace.
        properties:
          key:
            description: Key that identifies the value to be extracted.
            type: string
          name:
            description: Name of the Secret to extract a value from.
            type: string
        required:
        - name
        - key
        type: object

Notice, that the secretKeyRef actually is a object with key and name.
The Go struct generated for the SQLUser boils down to

type UserValueFrom struct {
	/* Reference to a value with the given key in the given Secret in the resource's namespace. */
	// +optional
	SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
}

and the ResourceRef

type ResourceRef struct {
	/* The external name of the referenced resource */
	External string `json:"external,omitempty"`
	/* Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names */
	Name string `json:"name,omitempty"`
	/* Namespace of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/ */
	Namespace string `json:"namespace,omitempty"`
}

So when I try to create a SQLUser resource and attempt to reference the password from Secret I get an error.

Additional Diagnostic Information

Kubernetes Cluster Version

❯ kubectl version --short
Client Version: v1.21.2
Server Version: v1.22.4

Config Connector Version

k8s-config-connector v1.71.0

Config Connector Mode

cluster

Log Output

SQLUser.sql.cnrm.cloud.google.com \"alpha\" is invalid: spec.password.valueFrom.secretKeyRef.key: Required value", "errorVerbose": "SQLUser.sql.cnrm.cloud.google.com \"XXXX\" is invalid: spec.password.valueFrom.secretKeyRef.key: Required value

Steps to Reproduce

Steps to reproduce the issue

The text was updated successfully, but these errors were encountered:

mbzomowski · 2022-01-29T02:23:06Z

Hi @radoslav-tomov, sorry to hear you're having trouble with this. I'm having some issues figuring out exactly what problem you're running into here. Would you mind providing the (redacted if necessary) yamls for both your SQLUser CR and your SecretKey CR?

Also, in what context are you using the resourceRef field here? I don't believe that's a valid field in the SQLUser CRD.

radoslav-tomov · 2022-01-29T08:32:01Z

@mbzomowski The problem appears when you try to create SQLUser with a password, which is stored in a secret via the go-client.

Check the Password field in the SQLUser, it boils down to the resourceRef and it is not compatible with the CRD definition. One has key and name fields, the other has name, namespace and external. Check the fix I sugested. here is the definition. SecretKeyRef should be of type, which has name and key fields, which is not the case now.

type UserPassword struct {
	/* Value of the field. Cannot be used if 'valueFrom' is specified. */
	// +optional
	Value *string `json:"value,omitempty"`

	/* Source for the field's value. Cannot be used if 'value' is specified. */
	// +optional
	ValueFrom *UserValueFrom `json:"valueFrom,omitempty"`
}

type UserValueFrom struct {
	/* Reference to a value with the given key in the given Secret in the resource's namespace. */
	// +optional
	SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
}

caieo · 2022-02-01T16:58:36Z

Hi @radoslav-tomov, this is a bug that is an oversight on our part. Our Go-Client code generation assumes that fields with the suffix Ref are a resourceRef type, but this is not necessarily true from the scenario you provided. We will look into fixing this bug on our side and sorry for the inconvenience!

aw185176 · 2022-03-09T18:39:55Z

Echoing that this issue is useful to us as well at NCR, any ETA on a fix?

xiaobaitusi · 2022-03-09T22:16:13Z

Hi @aw185176 thanks for providing another data point.

Unfortunately our team hasn't gotten around to it yet, I couldn't provide a good ETA at this point. It has been tracked in our backlog and we will update this thread when we have more update.

Sorry for the inconvenience! Could you please let us know the impact of this issue? Is it currently a blocker for you? This will help us to better prioritize our tasks.

b4nst · 2022-07-21T14:42:11Z

This issue prevent us from using automatically generated cue definitions to validate our configuration.

zealws · 2023-04-24T14:33:32Z

It looks like the SecretKeyRef fields should probably be this SecretKeyReference struct instead (godocs).

Here's all the references to *v1alpha1.ResourceRef that looks like they might be wrong:

❯ git grep -r '*v1alpha1.ResourceRef' | grep Secret
pkg/clients/generated/apis/alloydb/v1alpha1/alloydbcluster_types.go:    SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/bigqueryconnection/v1alpha1/bigqueryconnectionconnection_types.go:   SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/bigquerydatatransfer/v1alpha1/bigquerydatatransferconfig_types.go:   SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/certificatemanager/v1alpha1/certificatemanagercertificate_types.go:  SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/compute/v1alpha1/computebackendbucketsignedurlkey_types.go:  SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/compute/v1alpha1/computebackendservicesignedurlkey_types.go: SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/compute/v1beta1/computebackendservice_types.go:      SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/compute/v1beta1/computedisk_types.go:        SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/compute/v1beta1/computeinstance_types.go:    SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/compute/v1beta1/computesnapshot_types.go:    SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/compute/v1beta1/computesslcertificate_types.go:      SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/compute/v1beta1/computevpntunnel_types.go:   SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/container/v1beta1/containercluster_types.go: SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/datastream/v1alpha1/datastreamconnectionprofile_types.go:    SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/identityplatform/v1beta1/identityplatformconfig_types.go:    SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/identityplatform/v1beta1/identityplatformoauthidpconfig_types.go:    SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/identityplatform/v1beta1/identityplatformtenantoauthidpconfig_types.go:      SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/kms/v1alpha1/kmssecretciphertext_types.go:   SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/monitoring/v1beta1/monitoringnotificationchannel_types.go:   SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/monitoring/v1beta1/monitoringuptimecheckconfig_types.go:     SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/networkservices/v1alpha1/networkservicesedgecachekeyset_types.go:    SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/run/v1beta1/runservice_types.go:     SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/secretmanager/v1beta1/secretmanagersecretversion_types.go:   SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/sql/v1beta1/sqlinstance_types.go:    SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/sql/v1beta1/sqluser_types.go:        SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`
pkg/clients/generated/apis/storagetransfer/v1beta1/storagetransferjob_types.go: SecretKeyRef *v1alpha1.ResourceRef `json:"secretKeyRef,omitempty"`

I haven't tried this, but I think a work-around for this would be to use the kube unstructured API instead, but you lose out on type safety using the Go SDK, which is problematic.

Our Go-Client code generation assumes that fields with the suffix Ref are a resourceRef type

I don't know exactly how the generator for this code works, but if it's picking up the Ref name suffix and assuming the associated type should be *v1alpha1.ResourceRef, would changing the name of those fields to SecretKeyReference be enough to fix it?

If that's not enough, you could rename the fields SecretKeyLocator or something else instead (pick your favorite synonym).

zealws · 2023-04-24T14:51:03Z

Also, if the Ref suffix is causing the problem, then every reference to SecretKeyRef in the codebase would be broken by extension, since they'd all be using the wrong type.

Spot-checking that assumption...

There's only 1 reference to the correct v1alpha1.SecretKeyReference struct in the codebase (in k8s.GetSecretVal).
k8s.GetSecretVal is referenced in 4 places.
all 4 calls to k8s.GetSecretVal load unstructured data to get a v1alpha1.SecretKeyReference, rather than using the Go SDK resource types which contain a v1alpha1.SecretKeyReference.

So there's no code at all in this codebase that uses a Go SDK resource type containing a v1alpha1.SecretKeyReference.

I think that's a pretty good indicator that all the go resource types which contain SecretKeyRef fields are unusable in the current version of the Go SDK (at least, if you want to use the SecretKeyRef field).

That's 26 resource types (from the git grep -r '*v1alpha1.ResourceRef' | grep Secret in the previous comment), for reference.

diviner524 · 2023-04-24T23:40:14Z

@zealws Thanks for the research! Yes it is coming from the incorrect assumption here:

https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/scripts/generate-go-crd-clients/generate-types-file.go#L327

We apologize that we may not be able to address the issue immediately due to current capacity limitations. However, if you're interested in contributing to fix the issue, we would be happy to provide some guidance on how to generate and validate the Go client. Please let us know if you'd like to get involved, and we'll share some pointers to help you get started!

radoslav-tomov added the bug Something isn't working label Jan 27, 2022

radoslav-tomov mentioned this issue Jan 27, 2022

Introducing secretKeyRef to match SQLUser CRD #599

Open

pantheon-chris mentioned this issue Apr 22, 2023

Go client and CRD definition for ComputeSSLCertificate are inconsistent #803

Open

zealws mentioned this issue Apr 25, 2023

Fix ResourceReference fields that are not ResourceReferences #804

Draft

haorenfsa mentioned this issue Nov 14, 2023

Fix secret key ref #1012

Closed

8 tasks

jijinp mentioned this issue Nov 22, 2023

fixes SecretKeyRef on Go client and CRD definition #1025

Merged

2 tasks

google-oss-prow bot closed this as completed in #1025 Nov 22, 2023

This was referenced Nov 27, 2023

[Snyk] Fix for 15 vulnerabilities Matthelonianxl/k8s-config-connector#30

Open

[Snyk] Fix for 1 vulnerabilities Matthelonianxl/k8s-config-connector#33

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go client and CRD definition of SQLUser are inconsistent #598

Go client and CRD definition of SQLUser are inconsistent #598

radoslav-tomov commented Jan 27, 2022

mbzomowski commented Jan 29, 2022 •

edited

Loading

radoslav-tomov commented Jan 29, 2022 •

edited

Loading

caieo commented Feb 1, 2022

aw185176 commented Mar 9, 2022

xiaobaitusi commented Mar 9, 2022 •

edited

Loading

b4nst commented Jul 21, 2022

zealws commented Apr 24, 2023 •

edited

Loading

zealws commented Apr 24, 2023

diviner524 commented Apr 24, 2023

Go client and CRD definition of SQLUser are inconsistent #598

Go client and CRD definition of SQLUser are inconsistent #598

Comments

radoslav-tomov commented Jan 27, 2022

Bug Description

Additional Diagnostic Information

Kubernetes Cluster Version

Config Connector Version

Config Connector Mode

Log Output

Steps to Reproduce

Steps to reproduce the issue

mbzomowski commented Jan 29, 2022 • edited Loading

radoslav-tomov commented Jan 29, 2022 • edited Loading

caieo commented Feb 1, 2022

aw185176 commented Mar 9, 2022

xiaobaitusi commented Mar 9, 2022 • edited Loading

b4nst commented Jul 21, 2022

zealws commented Apr 24, 2023 • edited Loading

zealws commented Apr 24, 2023

diviner524 commented Apr 24, 2023

mbzomowski commented Jan 29, 2022 •

edited

Loading

radoslav-tomov commented Jan 29, 2022 •

edited

Loading

xiaobaitusi commented Mar 9, 2022 •

edited

Loading

zealws commented Apr 24, 2023 •

edited

Loading