markers to skip fields that does not have json tags for crd generation

[yuzisun]

controller-tools version: 0.2.2

Currently I am getting this error after upgrading controller-tools from 0.2.2 from 0.1.9 because in our CRD status we included a go struct URL (https://github.com/knative/pkg/blob/43ca049cdbdad4892b0c926e7c6536d064efde72/apis/duck/v1beta1/addressable_types.go#L40) which does not have json tags and this blocks generating the crd manifests.

/usr/local/go/src/net/url/url.go:357:2: encountered struct field "Scheme" without JSON tag in type "URL"
/usr/local/go/src/net/url/url.go:358:2: encountered struct field "Opaque" without JSON tag in type "URL"
/usr/local/go/src/net/url/url.go:359:2: encountered struct field "User" without JSON tag in type "URL"
/usr/local/go/src/net/url/url.go:360:2: encountered struct field "Host" without JSON tag in type "URL"
/usr/local/go/src/net/url/url.go:361:2: encountered struct field "Path" without JSON tag in type "URL"
/usr/local/go/src/net/url/url.go:362:2: encountered struct field "RawPath" without JSON tag in type "URL"
/usr/local/go/src/net/url/url.go:363:2: encountered struct field "ForceQuery" without JSON tag in type "URL"
/usr/local/go/src/net/url/url.go:364:2: encountered struct field "RawQuery" without JSON tag in type "URL"
/usr/local/go/src/net/url/url.go:365:2: encountered struct field "Fragment" without JSON tag in type "URL"

We want a way to skip fields when generating crd manifests.

[DirectXMan12]

/kind feature
/priority important-longterm

I'm assuming URL has custom serialization. We need a good way to detect that a type has custom serialization and use that instead of the fields.

[DirectXMan12]

/help

[k8s-ci-robot]

@DirectXMan12:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

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.

[vishvananda]

I'm running into the same issue. Is there a reasonable workaround for this?

[vishvananda]

I've managed to work around the specific knative issues with the following patches:

master...vishvananda:fix-known

There must be a better way to do this than adding custom exceptions for every type, but I'm not sure of the best approach. Both of these objects have a MarshalJSON that writes the data as a string.

[DirectXMan12]

The fix should be to check during schema generation if a type implements the {Marshal,Unmarshal}JSON interfaces, and if so treat it as opaque. We have type-checking information, so we can do this.

[yuzisun]

Do you mean here?

                if !hasTag {
                        // if the field doesn't have a JSON tag, it doesn't belong in output (and shouldn't exist in a serialized type)
+                       ctx.pkg.AddError(loader.ErrFromNode(fmt.Errorf("encountered struct field %q without JSON tag in type %q", field.Name, ctx.info.Name), field.RawField))
                        continue
                }

[skaslev]

The fix should be to check during schema generation if a type implements the {Marshal,Unmarshal}JSON interfaces, and if so treat it as opaque. We have type-checking information, so we can do this.

@DirectXMan12 I'm interested in working on fix for this. Can you elaborate on what do you mean by treat them as opaque? Should schema generated for types with custom json serialization get declared as Any type?

[DirectXMan12]

So, our code generator is built on introspection: for each type, visit its sub-types, etc, mapping them to jsonschema. Some types, however, can't or shouldn't be introspected. For these types, introspection produces bad results, because their serialized form is not equivalent to their Go form under our normal generation rules. Instead, we have to treat these types as if they're "opaque" -- i.e. we cannot see into them to look at their fields. Instead, we want to treat them as if they were type Foo <blackbox> (instead of type Foo struct), and derive their jsonschema entirely from markers.

That's a really longwinded way of saying "treat them like primitive types".

[axsaucedo]

@yuzisun I have also run into this issue attempting to add ducktyping into our CRD status (ie SeldonIO/seldon-core#1572). Is there a workaround for this?

[yoichiwo7]

I have tried controller-gen built with #427 in my Kubernetes v1.15.3 cluster.
It works fine and generated YAML also works fine with kubectl commands such as apply, create, and delete. But after the CRD is installed, I get following error when I try to run kubectl diff with any resources. (looks like the error comes from kubernetes/kube-openapi)

error: SchemaError(MyCRD.spec.tasks.withItems): Unknown primitive type: "Any"

I have also tried on Kubernetes v1.17.0 cluster and kubectl diff works fine.

[DirectXMan12]

You'll have to manually override the type when you use that capability. e.g.

// +kubebuilder:validation:type=string
// +kubebuilder:validation:pattern="[😀-🙏]"
type SerializesAsEmoji uint8
func (v SerializesAsEmoji) MarshalJSON() ([]byte, error) {
  return []byte(`"` + string(rune(0x1f600+v)) + `"`)
}

[alvaroaleman]

/reopen
The fix in #427 is not valid, Any is not allowed
Revert of #427 which claims to fix this but just introduces a different issue without fixing it in #505

[k8s-ci-robot]

@alvaroaleman: Reopened this issue.

In response to this:

/reopen
The fix in #427 is not valid, Any is not allowed
Revert of #427 which claims to fix this but just introduces a different issue without fixing it in #505

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.

[DirectXMan12]

So, I think the intent of #427 was that custom marshallers generate an invalid type (but doesn't traverse further), which you then must specify a valid type for.

[alvaroaleman]

So, I think the intent of #427 was that custom marshallers generate an invalid type (but doesn't traverse further), which you then must specify a valid type for.

The description doesn't sound like that to me:

With this change we check if a type implements custom JSON marshalling and if it
does, we output schema for Any type. This still allows the validation type to
overriden with a marker.

and IMHO generating invalid schema is not a great error handling strategy. On top of that, it irrevocably breaks everyone who has type with a custom marshaller as a dependency in their api.

[DirectXMan12]

Fair, I think we were operating under the assumption that it wasn't possible before, but had overlooked the case of "you use all the normal JSON tags, but are using jsoniterator for performance" (which is the only case where we could generate valid openapi)

[DirectXMan12]

(FWIW, with the revert, there's currently no way to make use of certain classes of custom-serialized types, even with markers)

[alvaroaleman]

(FWIW, with the revert, there's currently no way to make use of certain classes of custom-serialized types, even with markers)

But how did this work before the revert? We generated Any by default (which is invalid) and otherwise used the markers. So this also didn't work before the revert or am I missing something?

[DirectXMan12]

Before the revert, you could do something like

// +kubebuilder:validation:Type=object
type Weird map[string]interface{} // map[string]interface{} is normally invalid, but...

// ... custom marshalling means we *just* use the markers
func (w *Weird) UnmarshalJSON(data []byte) error { ... }
func (w *Weird) MarshalJSON() ([]byte, error) { ... }

Which would get interpreted as:

{type: "Any"} --> apply marker "kubebulider:validation:Type" --> {type: "object"}

So it supported cases where you don't want an object or you just want a catch-all object, but didn't support cases where you want a partially-specified object, or a fully specified object, which was an oversight.

For whatever we merge, we should attempt to support both usecases.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

[robbie-demuth]

Like yuzisun, I'm trying to add a field of type net.URL to my CRD. In other projects, I've wanted to use fields of type time.Time and time.Duration. After doing a bit of digging, I stumbled across metav1.Time and metav1.Duration. These types seem to do exactly what's wanted here. Does anyone know how? I've tried wrapping a type around net.URL with custom serialization, but haven't had much success

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

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.

[Ruminateer]

Any update on this? I am working on a project using Knative and I have the exact same issue with OP. I am using controller-gen v0.5.0.

/home/zzy/go/bin/controller-gen "crd:maxDescLen=0" rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases
/usr/lib/go-1.13/src/net/url/url.go:357:2: encountered struct field "Scheme" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:358:2: encountered struct field "Opaque" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:359:2: encountered struct field "User" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:360:2: encountered struct field "Host" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:361:2: encountered struct field "Path" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:362:2: encountered struct field "RawPath" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:363:2: encountered struct field "ForceQuery" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:364:2: encountered struct field "RawQuery" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:365:2: encountered struct field "Fragment" without JSON tag in type "URL"
ElasticServing/pkg/apis/elasticserving/v1:-: conflicting types in allOf branches in schema: string vs object
Error: not all generators ran successfully

As #427 is reverted, is there a workaround for the issue? Or should this be an issue with Knative?

Can I reopen this?
/reopen

[k8s-ci-robot]

@Ruminateer: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

Any update on this? I am working on a project using Knative and I have the exact same issue with OP. I am using controller-gen v0.5.0.

/home/zzy/go/bin/controller-gen "crd:maxDescLen=0" rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases
/usr/lib/go-1.13/src/net/url/url.go:357:2: encountered struct field "Scheme" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:358:2: encountered struct field "Opaque" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:359:2: encountered struct field "User" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:360:2: encountered struct field "Host" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:361:2: encountered struct field "Path" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:362:2: encountered struct field "RawPath" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:363:2: encountered struct field "ForceQuery" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:364:2: encountered struct field "RawQuery" without JSON tag in type "URL"
/usr/lib/go-1.13/src/net/url/url.go:365:2: encountered struct field "Fragment" without JSON tag in type "URL"
ElasticServing/pkg/apis/elasticserving/v1:-: conflicting types in allOf branches in schema: string vs object
Error: not all generators ran successfully

As #427 is reverted, is there a workaround for the issue? Or should this be an issue with Knative?

Can I reopen this?
/reopen

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.

[njhill]

It looks like this may now be addressed in knative: knative/pkg#2431