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

feat: add custom strfmt.Registry support #42

Closed
wants to merge 1 commit into from

Conversation

eddycharly
Copy link
Contributor

@eddycharly eddycharly commented May 11, 2023

This PR adds custom strfmt.Registry support.

Format byte does not allow empty strings (see comes from https://github.com/asaskevich/govalidator/blob/a9d515a09cc289c60d55064edec5ef189859f172/patterns.go#L27).

This makes the secret below invalid:

apiVersion: v1
kind: Secret
metadata:
  name: secret
data:
  key: ''

Now, one could supply it's own registry like this to workaround the problem:

	registry := strfmt.NewFormats()
	b64 := strfmt.Base64([]byte(nil))
	registry.Add("byte", &b64, func(s string) bool {
		if len(s) == 0 {
			return true
		}
		return govalidator.IsBase64(s)
	})

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 11, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: eddycharly
Once this PR has been reviewed and has the lgtm label, please assign alexzielenski for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 11, 2023
@alexzielenski
Copy link
Contributor

alexzielenski commented May 12, 2023

This is an interesting bug. In upstream apiserver, native k8s types do not undergo schema validation so the empty string was allowed. CRDs on the other hand do, and use strfmt.Default.

I just tested this by making a CRD with a field with type: string and format: byte like Secret. K8s apiserver rejected it with:

* bytesField: Invalid value: "": bytesField in body must be of type byte: ""

So to fix this for native types causes CRD validation to be incorrect (unless we hardcoded some logic to decide which strfmt to use depending on the GVK, but thats not nice).

I wonder if this is a bug that should be fixed upstream, so that in k8s CRDs are allowed to have empty byte values

@eddycharly
Copy link
Contributor Author

I wonder if this is a bug that should be fixed upstream, so that in k8s CRDs are allowed to have empty byte values

I wondered the same. Dunno maybe it was done intentionally.

@eddycharly
Copy link
Contributor Author

I thought it was acceptable to allow providing a custom registry, WDYT ?

@eddycharly
Copy link
Contributor Author

Or maybe a registry provider that can return a registry depending on the resource being validated ?

@alexzielenski
Copy link
Contributor

alexzielenski commented May 12, 2023

I thought it was acceptable to allow providing a custom registry, WDYT ?

I dont think so, at least for a single registry, since the custom registry will either be wrong for native types, or wrong for CRDs. It would never be correct for both unless you provided a registry depending on the type.

Or maybe a registry provider that can return a registry depending on the resource being validated ?

Yeah, In the worst case scenario we can use the registry conditionally for native types, but I'd like to avoid that if we can. Unfortunately patching upstream seems the solution I can think of.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2023
@eddycharly
Copy link
Contributor Author

Unfortunately patching upstream seems the solution I can think of.

You mean changing the format validator for byte upstream ?

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2023
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2023
@eddycharly
Copy link
Contributor Author

Closing, became useless with recent changes.

@eddycharly eddycharly closed this Aug 31, 2023
@eddycharly eddycharly deleted the registry branch August 31, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants