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

CRD generation via generate openapi does not surface errors for invalid tags #2042

Closed
hasbro17 opened this issue Oct 10, 2019 · 15 comments · Fixed by #2201
Closed

CRD generation via generate openapi does not surface errors for invalid tags #2042

hasbro17 opened this issue Oct 10, 2019 · 15 comments · Fixed by #2201
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@hasbro17
Copy link
Contributor

Bug Report

What did you do?
Run operator-sdk generate openapi for the following pkg/apis/..._types.go file that has the invalid marker comment // +kubebuilder:resource:foo=bar.

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// Memcached is the Schema for the memcacheds API
// +k8s:openapi-gen=true
// +kubebuilder:subresource:status
// +kubebuilder:resource:path=memcacheds,scope=Namespaced
// +kubebuilder:resource:shortName=mc
// +kubebuilder:resource:foo=bar
type Memcached struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`

	Spec   MemcachedSpec   `json:"spec,omitempty"`
	Status MemcachedStatus `json:"status,omitempty"`
}

What did you expect to see?
An error message on which marker is invalid.

What did you see instead? Under which circumstances?
No error from the actual controller-gen CRD generator is being surfaced.

$ operator-sdk generate openapi --verbose
DEBU[0000] Debug logging is set
INFO[0000] Running OpenAPI code-generation for Custom Resource group versions: [cache:[v1alpha1], ]
Error: error generating CRD for Group cache Version v1alpha1 Kind Memcached
Usage:
  operator-sdk generate openapi [flags]

Flags:
  -h, --help   help for openapi

Global Flags:
      --verbose   Enable verbose logging

Environment

  • operator-sdk version: "v0.10.0-84-g8132d358", commit: "8132d3588c988b12907e541e5069e9ff7a9fcc55", go version: "go1.12.7 darwin/amd64"

  • go version: 1.12.7

Additional context
The wrapped error message Error: error generating CRD ... suggests that this might just be a lack of context being surfaced from the CRD generator.

ctx.OutputRule = r.OutputRules.ForGenerator(gens[0])
if err := gens[0].Generate(&ctx); err != nil {
return errors.Wrapf(err, "error generating CRDs")
}

https://github.com/kubernetes-sigs/controller-tools/blob/83f6193614de0a51c4bd5af02650c577034c6a4c/pkg/crd/gen.go#L54

Going to test this directly with controller-gen to see if the issue is upstream.

@hasbro17 hasbro17 added the kind/bug Categorizes issue or PR as related to a bug. label Oct 10, 2019
@hasbro17 hasbro17 self-assigned this Oct 10, 2019
@hasbro17
Copy link
Contributor Author

/cc @estroz

@robbie-demuth
Copy link

I believe I'm running into something similar. After upgrading to v0.11.0, I had to add the +listType tag to several fields in my *_types.go file to fix API rule violation: list_type_missing errors. After doing so, however, the errors go away, but operator-sdk generate openapi still fails without any indication as to what went wrong

@camilamacedo86
Copy link
Contributor

Hi @robbie-demuth,

Really thank you for your contribution. We will check this issue. However, in order to help you solve the problems in your project, I'd recommend checking the steps in the migration doc in order to ensure that nothing is missing. Mainly the required annotations. Another good approach which could be helpful is to create a new project as described in the QuickStart here just to compare the annotations and CRD's so far.

Also, I'd like to share a PR with migration from 0.10 to 0.11 / master in a personal project which may help you find out what is missing. See here

I hope that it helps you.

@robbie-demuth
Copy link

I scaffolded two new projects using v0.9.0 and v0.11.0 to compare the differences and applied them to the project I'm working on, so I thought I got them all. That being said, I tried building out some simple types in a new v0.11.0 project and it seems to be working, so I'll try building from the ground up and seeing where I went wrong. I'll report back here once I've got more info. Thanks!

@robbie-demuth
Copy link

robbie-demuth commented Oct 12, 2019

So after a bit of digging, I figured out my issue. There were two things going on:

  1. I was using +kubebuilder:validation:Pattern, but my regular expressions had commas in them. I needed to surround the expressions in backticks
  2. I was using +kubebuilder:validation:Enum on fields representing Kubernetes types. I was doing this to ensure that fields used to specify Service types, for example, were one of ClusterIP, LoadBalancer, etc. I'm not sure why this caused an issue, but I've removed the validation for the time being to work around it

EDIT

The second issue was caused by a change in the way that Enums need to be defined. Previously, separating Enum values via commas was fine, but now, they need to be enclosed in curly braces or separated using semicolons https://book.kubebuilder.io/reference/markers.html

@camilamacedo86
Copy link
Contributor

Hi @robbie-demuth,

Really tks to share it. Just to register for it be easier for the others. So, from the SDK which is missing is just shown what is wrong(output error msgs). Also, maybe we need to check the Enum type as well.

@jpkrohling
Copy link

Thanks @robbie-demuth for sharing your results. I was also bitten by the change to the Enums and you saved me from a debugging session :-)

@aklinkert
Copy link

Okay, folks this needs a word in the upgrade guide, please. I was updating the operator-sdk from 0.10 tu 0.11 and this Enum change described by @robbie-demuth took me 1,5 days to figure out / find this issue. ☹️ Please please please, document this update in the upgrade guide. 🥇

@camilamacedo86
Copy link
Contributor

Hi @apinnecke,

The #2111 was created for it. Feel free to follow up.

@estroz
Copy link
Member

estroz commented Oct 31, 2019

@robbie-demuth @jpkrohling @apinnecke out of curiosity, do you use the generated Go OpenAPI spec (pkg/apis/<group>/<version>/zz_generated.openapi.go), or is that extra code just sitting around?

@robbie-demuth
Copy link

To be honest, I haven't ever really looked at it. I don't believe our organization uses it. We mainly just need the generated DeepCopy... methods which I believe are generated by operator-sdk generate k8s. I assumed the generated OpenAPI code was used as an interim step to generate the CRD manifest

@aklinkert
Copy link

@camilamacedo86 No, those zz_generated.* files are not used by us at all. We only use the generated CRD manifests for deployment.

@CharlyF
Copy link

CharlyF commented Nov 13, 2019

I faced this issue using the wrong type in my *_types.go file.
With the operator-sdk 0.9.0, this:

	// +kubebuilder:validation:Minimum=0.01
	// +kubebuilder:validation:Maximum=0.99
	Tolerance float64 `json:"tolerance,omitempty"`

Would work, but as I bumped to 0.12.0:
I saw the error:

INFO[0000] Running OpenAPI code-generation for Custom Resource group versions: [XXX:[v1alpha1], ]
Error: error generating CRD for Group XXX Version v1alpha1 Kind XXX
[...]

It was wrong from the start, but it took me some time to find it out, give the error message.

@estroz
Copy link
Member

estroz commented Nov 13, 2019

@CharlyF thanks for sharing your experience; the lack of errors being surfaced is a bad experience for sure. Ensuring generate commands surface errors correctly is being worked on.

@estroz
Copy link
Member

estroz commented Dec 12, 2019

This should be fixed by #2201, which directly uses controller-gen crd API and surfaces errors appropriately.

Note that generate openapi has been deprecated in favor of generate crds. A lot of these issues will go away now that openapi-gen is not longer run by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants