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

1.30 adds tags that break imagePullSecrets #124540

Closed
pmalek opened this issue Apr 25, 2024 · 14 comments · Fixed by #124553
Closed

1.30 adds tags that break imagePullSecrets #124540

pmalek opened this issue Apr 25, 2024 · 14 comments · Fixed by #124553
Labels
needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@pmalek
Copy link
Contributor

pmalek commented Apr 25, 2024

Problem statement

Using code-generator's client-gen in version 1.30 adds several new fields to generated CRDs:

                           imagePullSecrets:
                             description: |-
                               ImagePullSecrets is an optional list of references to secrets in the same namespace to use for pulling any of the images used by this PodSpec.
                               If specified, these secrets will be passed to individual puller implementations for them to use.
                               More info: https://kubernetes.io/docs/concepts/containers/images#specifying-imagepullsecrets-on-a-pod
                             items:
                               description: |-
                                 LocalObjectReference contains enough information to let you locate the
                                 referenced object inside the same namespace.
                               properties:
                                 name:
                                   description: |-
                                     Name of the referent.
                                     More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
                                     TODO: Add other useful fields. apiVersion, kind, uid?
                                   type: string
                               type: object
                               x-kubernetes-map-type: atomic
                             type: array
+                            x-kubernetes-list-map-keys:
+                            - name
+                            x-kubernetes-list-type: map

This in turn makes the CRDs fail to be applied on the cluster (note: the CRD in question has PodTemplateSpec embedded as a field)

Error from server (Invalid): CustomResourceDefinition.apiextensions.k8s.io "dataplanes.gateway-operator.konghq.com" is invalid: [spec.validation.openAPIV3Schema.properties[spec].properties[deployment].properties[podTemplateSpec].properties[spec].properties[hostAliases].items.properties[ip].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property, spec.validation.openAPIV3Schema.properties[spec].properties[deployment].properties[podTemplateSpec].properties[spec].properties[imagePullSecrets].items.properties[name].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property]

Proposed solution

It would seem that LocalObjectReference (which is the underlying type of ImagePullSecrets)
is missing some code markers? Not sure at this moment.

@liggitt
Copy link
Member

liggitt commented Apr 25, 2024

/transfer kubernetes

@k8s-ci-robot k8s-ci-robot transferred this issue from kubernetes/code-generator Apr 25, 2024
@k8s-ci-robot k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 25, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@liggitt
Copy link
Member

liggitt commented Apr 25, 2024

from https://github.com/kubernetes/kubernetes/pull/121759/files#diff-5ffbebb40a29f8e491f4ff1e07c618eb3edcc0d5f00a1a71445272d6767d7883R3652

cc @thockin

/sig api-machinery

even though these list types accurately reflect the structure / validation in built-in types, fields used as map keys must be required or defaulted when used in a CRD schema

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 25, 2024
@thockin
Copy link
Member

thockin commented Apr 25, 2024

@pmalek If you add // +required to LocalObjectRef.Name, does the error go away? Or maybe that's imposing too much - can you point me at your CRD and I can poke around?

I'm not sure if the right answer is to make it required -- it SEEMS correct, considering it is the only field in the type :) -- or to rmeove that listMap (despite being correct in context)

@pmalek
Copy link
Contributor Author

pmalek commented Apr 25, 2024

One of the types can be found here : https://github.com/Kong/gateway-operator/blob/main/api%2Fv1beta1%2Fdataplane_types.go

I can do some testing when I find a moment.

@thockin
Copy link
Member

thockin commented Apr 25, 2024

Which type in particular?

@pmalek
Copy link
Contributor Author

pmalek commented Apr 25, 2024

The embedded PodTemplateSpec here https://github.com/Kong/gateway-operator/blob/main/api%2Fv1beta1%2Fshared_types.go#L35 seems to be causing issues.

@thockin
Copy link
Member

thockin commented Apr 26, 2024

I'm trying to repro real quickly, but I can't get it to build.

$ make generate
bash: line 1: mise: command not found
mise - https://github.com/jdx/mise - not found. Please install it.
make: *** [Makefile:45: mise] Error 1

So I installed mise into /tmp

$ PATH="$PATH:/tmp" make generate
bash: line 1: yq: command not found
mise /tmp/gateway-operator/bin/plugins/kube-controller-tools/bin/install failed
mise /tmp/gateway-operator/bin/plugins/kube-controller-tools/bin/install exited with non-zero status: exit code 1
mise Run with --verbose or MISE_VERBOSE=1 for more information
make: *** [Makefile:64: controller-gen] Error 1

OK, so I installed yq:

$ sudo apt-get install yq
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
The following NEW packages will be installed:
  yq
0 upgraded, 1 newly installed, 0 to remove and 0 not upgraded.
Need to get 0 B/29.8 kB of archives.
After this operation, 73.7 kB of additional disk space will be used.
Selecting previously unselected package yq.
(Reading database ... 357393 files and directories currently installed.)
Preparing to unpack .../archives/yq_3.1.0-3_all.deb ...
Unpacking yq (3.1.0-3) ...
Setting up yq (3.1.0-3) ...

But still no luck:

$ PATH="$PATH:/tmp" make generate
jq: Unknown option -ojson
Use jq --help for help with command-line options,
or see the jq manpage, or online docs  at https://jqlang.github.io/jq
mise /tmp/gateway-operator/bin/plugins/kube-controller-tools/bin/install failed
mise /tmp/gateway-operator/bin/plugins/kube-controller-tools/bin/install exited with non-zero status: exit code 1
mise Run with --verbose or MISE_VERBOSE=1 for more information
make: *** [Makefile:64: controller-gen] Error 1

I am out of time for the moment.

@rashedkvm
Copy link

rashedkvm commented Apr 26, 2024

Even though marked as +optional it is behaving as required due to the map
https://github.com/kubernetes/api/blob/fb932d2704f6db73ad667467c071fa0021b4f11a/core/v1/types.go#L3706-L3711. I believe the same issue is observed in few other types as well.

From my CR, validation errors

6:10:07PM:  ^ Reconcile failed:  (message: kapp: Error: create customresourcedefinition/podintents.conventions.carto.run (apiextensions.k8s.io/v1) cluster:
  Creating resource customresourcedefinition/podintents.conventions.carto.run (apiextensions.k8s.io/v1) cluster:
    API server says:
      CustomResourceDefinition.apiextensions.k8s.io "podintents.conventions.carto.run" is invalid:
  - spec.validation.openAPIV3Schema.properties[spec].properties[template].properties[spec].properties[imagePullSecrets].items.properties[name].default: Required value: this property is in x-kubernetes-list-map-keys
  - so it must have a default or be a required property
  - spec.validation.openAPIV3Schema.properties[spec].properties[template].properties[spec].properties[hostAliases].items.properties[ip].default: Required value: this property is in x-kubernetes-list-map-keys
  - so it must have a default or be a required property
  - spec.validation.openAPIV3Schema.properties[status].properties[template].properties[spec].properties[hostAliases].items.properties[ip].default: Required value: this property is in x-kubernetes-list-map-keys
  - so it must have a default or be a required property
  - spec.validation.openAPIV3Schema.properties[status].properties[template].properties[spec].properties[imagePullSecrets].items.properties[name].default: Required value: this property is in x-kubernetes-list-map-keys
  - so it must have a default or be a required property
 (reason: Invalid))
kapp: Error: waiting on reconcile packageinstall/cartographer-conventions-controller (packaging.carvel.dev/v1alpha1) namespace: default:

  Finished unsuccessfully (Reconcile failed:  (message: kapp: Error: create customresourcedefinition/podintents.conventions.carto.run (apiextensions.k8s.io/v1) cluster:
  Creating resource customresourcedefinition/podintents.conventions.carto.run (apiextensions.k8s.io/v1) cluster:
    API server says:
      CustomResourceDefinition.apiextensions.k8s.io "podintents.conventions.carto.run" is invalid:
  - spec.validation.openAPIV3Schema.properties[spec].properties[template].properties[spec].properties[imagePullSecrets].items.properties[name].default: Required value: this property is in x-kubernetes-list-map-keys
  - so it must have a default or be a required property
  - spec.validation.openAPIV3Schema.properties[spec].properties[template].properties[spec].properties[hostAliases].items.properties[ip].default: Required value: this property is in x-kubernetes-list-map-keys
  - so it must have a default or be a required property
  - spec.validation.openAPIV3Schema.properties[status].properties[template].properties[spec].properties[hostAliases].items.properties[ip].default: Required value: this property is in x-kubernetes-list-map-keys
  - so it must have a default or be a required property
  - spec.validation.openAPIV3Schema.properties[status].properties[template].properties[spec].properties[imagePullSecrets].items.properties[name].default: Required value: this property is in x-kubernetes-list-map-keys
  - so it must have a default or be a required property
 (reason: Invalid)))
Error: Process completed with exit code 1.

@pmalek
Copy link
Contributor Author

pmalek commented Apr 26, 2024

@thockin Not sure what's up with jq: Unknown option -ojson as there are no calls to jq that would be used when install code-generator and generating CRDs.

I'd assume this was a problem with using yq v3 instead of v4 which has a different set of flags.

In any case we've merged Kong/gateway-operator#230 to make sure to make no assumptions about the yq (which is used for the above) that's installed on the system.


I believe the culprit of this is as @thockin suggested Name not being required but also removing the omitempty json tag since that's rendering the required tag noop.

I've pushed #124553 as an attempt to fix this.

@thockin
Copy link
Member

thockin commented Apr 26, 2024

debian yq is an alias to jq, but if I uninstall that, I just get yq: command not found and I can't figure how to get mise (never used it) to install whatever variant of yq it expects?

@thockin
Copy link
Member

thockin commented Apr 26, 2024

Did you confirm that the +required actually satisfies your CRD?

@pmalek
Copy link
Contributor Author

pmalek commented Apr 26, 2024

... and I can't figure how to get mise (never used it) to install whatever variant of yq it expects?

mise plugin install --yes -q yq
mise install -q yq@4.43.1

but that's already sorted out in latest main.

Did you confirm that the +required actually satisfies your CRD?

I've basically tried embedding

type MyType struct {
	// ImagePullSecrets is a list of references to secrets in the same namespace to use for pulling any images
	// in pods that reference this ServiceAccount. ImagePullSecrets are distinct from Secrets because Secrets
	// can be mounted in the pod, but ImagePullSecrets are only accessed by the kubelet.
	// More info: https://kubernetes.io/docs/concepts/containers/images/#specifying-imagepullsecrets-on-a-pod
	// +optional
	// +listType=atomic
	ImagePullSecrets []LocalObjectReference `json:"imagePullSecrets,omitempty" protobuf:"bytes,3,rep,name=imagePullSecrets"`

	// ImagePullSecrets is an optional list of references to secrets in the same namespace to use for pulling any of the images used by this PodSpec.
	// If specified, these secrets will be passed to individual puller implementations for them to use.
	// More info: https://kubernetes.io/docs/concepts/containers/images#specifying-imagepullsecrets-on-a-pod
	// +optional
	// +patchMergeKey=name
	// +patchStrategy=merge
	// +listType=map
	// +listMapKey=name
	ImagePullSecretsPodSpec []LocalObjectReference `json:"imagePullSecretsPodSpec,omitempty" patchStrategy:"merge" patchMergeKey:"name" protobuf:"bytes,15,rep,name=imagePullSecretsPodSpec"`

}

// LocalObjectReference contains enough information to let you locate the
// referenced object inside the same namespace.
// +structType=atomic
type LocalObjectReference struct {
	// Name of the referent.
	// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
	// TODO: Add other useful fields. apiVersion, kind, uid?
	// +required
	Name string `json:"name" protobuf:"bytes,1,opt,name=name"`
}

Which exist in k8s tree in PodTemplateSpec and then my changing the optional to required + removing omitempty I was able to apply the generated CRD without errors.

@JorTurFer
Copy link

I'm bumping deps to v0.30.0 in KEDA and I'm facing the same issue during the CRD apply:

"CustomResourceDefinition.apiextensions.k8s.io \"scaledjobs.keda.sh\" is invalid: [spec.validation.openAPIV3Schema.properties[spec].properties[jobTargetRef].properties[template].properties[spec].properties[hostAliases].items.properties[ip].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property, spec.validation.openAPIV3Schema.properties[spec].properties[jobTargetRef].properties[template].properties[spec].properties[imagePullSecrets].items.properties[name].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property]"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants