Tool for generating k8s types wrapping Istio types

[rcernich]

Signed-off-by: rcernich rcernich@redhat.com

[istio-testing]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: geeknoid

If they are not already assigned, you can assign the PR to them by writing /assign @geeknoid in a comment when ready.

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS

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

[geeknoid]

Sorry for the dumb questions, but I'm a bit confused by what this PR is for exactly.

What is the exact input of the tool? A proto package or a Go package?

And what's the output, and what is that used for?

[rcernich]

Hey @geeknoid, I provided a README with the PR. The tool is used to generate Kubernetes types representing the Custom Resource Definitions used by Istio. This tool simply generates a wrapper around this Istio type, e.g.

type Policy struct {
	v1.TypeMeta `json:",inline"`
	// +optional
	v1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`

	// Spec defines the implementation of this definition.
	// +optional
	Spec v1alpha1.Policy `json:"spec,omitempty" protobuf:"bytes,2,opt,name=spec"`
}

Where v1alpha1.Policy is the native Istio type.

The generated code can then be run through other Kubernetes code generators to produce Kubernetes clients, informers, listers, etc., making it easier for folks to work with the Istio resources within Kubernetes.

Here are examples of some of the generated files:

authentication/v1alpha1/types.go
authentication/v1alpha1/register.go

HTH

[geeknoid]

Thanks, I saw the README, but it just mentions an "input package", which given the context could mean a Go package or a proto package.

The work I was expecting in this space was a protoc plugin that takes as input an Istio .proto file and produces as output an OpenAPI definition representing the proto. From this OpenAPI definition, we could then generate all the downstream k8s artifacts.

How would you contrast your approach vs. what I describe? Is your approach preferable overall?

[rcernich]

Sorry... it works off a go package and the go files emitted by protoc. Any types which need to have k8s counterparts are annotated (comment tags) appropriately. Those comments should also include tags to drive the other generators (e.g. client-gen, openapi-gen, etc.)

I can't say what the benefits are of this approach over the protoc plugin approach. I was only focused on getting k8s style apis generated and was not looking at OpenAPI definitions for the raw types. That said, there is a k8s code generator for openapi as well (//+k8s:openapi-gen). If you like, I can update istio/api#764 to include generation of openapi specs.

The k8s code generation framework was easier for me to wrap my head around, as I've never used protoc. Both approaches use the .proto files as the source (comment tags are copied over to the generated go files, then used by the other code generators). One difference was using the package comments in doc.go to identify which packages are scanned and for specifying the target k8s group/version for the types in the package. To me, this seemed less error prone than specifying group/version within each type/message, but does assume that all types in a package will belong to a single k8s group/version. Also, using comment tags to drive code generation seemed like a k8s standard, and thus seemed like it fit in with what folks might already be familiar with (assuming the target audience was k8s devs).

HTH

[rcernich]

Here are the diffs for containing the annotations used to drive all the code generation for k8s, istio/api@c2154e2. Focus on the changes to the .proto files and the new doc.go files. The .pb.go files are just duplicates coming out of protoc.

[geeknoid]

Couple more questions:

• Is doc.go generated or hand-edited? If it's generated, what is doing that work? If its hand-edited, then why is there corresponding comments also in the .proto file?

• We use comments on the protos to generate docs for istio.io. As is, I think all these comments will make it into the generated .pb.html files and so will show up on istio.io. We need to prevent that. Check out this https://github.com/istio/tools/blob/master/protoc-gen-docs/README.md for info on how we process proto comments.

It would be good to turn on OpenAPI generation on istio/api. We need those files for other things downstream.

Now that I understand better, I'll try and get to the full review later today.

Thanks.

[ozevren]

There are a few things to consider: - We've ran into a few serialization related issues between protos and K8s API machinery, particularly around oneof serialization. We need to make sure that these work, tests here would help. This might be a concern for OpenAPI spec generation as well. - The annotations themselves will carry over to documentation, I believe. They need to be hid. It looks like this approach would work for versioning: When we have multiple API Groups, then we will need to create multiple proto packages. As long as the root package names of the generated/top-level code can accommodate that, it should work. The one main downside is, we will loose the opportunity to integrate with protoc-gen-validate, to produce more detailed OpenAPI Spec.

…

On Thu, Jan 24, 2019 at 7:43 AM Rob Cernich ***@***.***> wrote: Here are the diffs for containing the annotations used to drive all the code generation for k8s, ***@***.*** <istio/api@c2154e2>. Focus on the changes to the .proto files and the new doc.go files. The .pb.go files are just duplicates coming out of protoc. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#37 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQR8I_g2NSVXANt6SKGKQsWS3pxhqkSWks5vGdSxgaJpZM4aLUUJ> .

[rcernich]

doc.go is hand edited. The reason for the duplicates in the type/message comments is that not all types are mapped to k8s api.

I can modify the tool such that generator tags are above the type/message doc section. The k8s code generation tools scan comments above the type comments. That said, I'll need to verify that those comments are copied over by protoc. Another option may be to modify the doc plugin to filter comment lines with go-style tags.

There should not be any issues with serialization, but there was some manual work required to get DeepCopy() working. The workaround I came up with was to provide custom DeepCopyInto() functions for the types used in the k8s apis. The code is rather simple, although probably sub-optimal:

https://github.com/rcernich/istio-api/blob/istio-8772/mixer/v1/config/client/zz_custom.deepcopy.go
https://github.com/rcernich/istio-api/blob/istio-8772/util/deepcopy.go

I did attempt to constrain this to individual fields, but that seemed a bit more difficult. That said, I can revisit that approach to constrain marshal/unmarshal to specific fields. (I bailed as it was simpler to just run the whole type through. This is certainly something that can be optimized.)

I also had to rework the proto imports to make them compatible with the k8s go-to-protobuf generator. This required adding generated.proto files, which are used by go-to-protobuf for locating message definitions. This allows go-to-protobuf to delegate to the existing .pb.go code for the wrapped types. (Which reminds me, I need to filter those files from the make targets.)

It sounds like I need to update the readme in the api project to account for all these details. Summarizing:

To generate k8s api types from istio types:

1. Add a doc.go file to the containing package, specifying tags for group/version and base target package name.
2. Add a generated.proto file to the containing package. This file should contain import public "<type.proto>"; lines, importing the message definitions for the types being generated.
3. Add generator tags to the comments of the message types that should be mapped.
4. If the type contains oneof fields, a custom DeepCopyInto() function should be provided. (Note, this is easy to detect as there will be compilation errors if it is missing.)

It seems like this may be a lot of manual effort, but other than the DeepCopy() and generated.proto bits, I think any other solution would require a similar amount of effort (i.e. you'll need to identify which types should be mapped and where they should go), and from this point forward, any effort would be incremental.

[rcernich]

One additional note, there may be some issues with types that are mapped to multiple k8s types, e.g. namespaced vs clustered resources. These would require two sets of k8s tags to drive downstream code generation (e.g. +genclient:nonNamespaced vs nothing, where MeshPolicy would want the former and Policy the latter, but both are generated from the istio Policy type).

[geeknoid]

Thanks for all the clarifying answers. I've got one more.

I still don't understand why some of the special comments are in doc.go and some are in the .proto file. Wouldn't it be possible to put everything in the .proto file? The protoc compiler carries all the comments forward into the generated go files. I prefer this approach as it makes the .proto the source of truth for our API definitions, as opposed to being only a partial source of truth.

As for hiding the comments from the generated docs, please see if you can make that work with the tools as-is. Otherwise, I can modify the doc gen tool to support skipping blocks within comments.

Finally, it'd be great to have some tests checked in alongside this tool. Something that exercises the core features, including serialization.

Thanks.

[rcernich]

doc.go provides a single place to define the target group/version and package for the generated files. I could move that specification into the types, but thought having a single place for this would reduce errors. The main difference would be that +kubetype-gen:package and +kubetype-gen:groupVersion would be specified for every type (which is why I chose to go this way). That said, the alternate is also more flexible, as it would allow types in a package to be split across different groups/versions, but I'm guessing this wouldn't be very common. Perhaps, split the difference and use the package settings as defaults if they're not specified on individual types. I'm indifferent.

Testing...sure, I'll look at adding some tests.

[geeknoid]

I don't think we need (or maybe even want) the flexibility to split the API like you describe. I'd rather force people to split up the package in the first place.

If you move the content of doc.go as an "unattached" comment in a proto file, then I think the protoc compiler will just carry everything as-is into the go file, also unattached to anything. So you end up with semantically the same thing you have now, except that the source of truth is the proto.

[rcernich]

I can do that. It will mean the +kubetype-gen:groupVersion and +kubetype-gen:package tags will be duplicated for every type/message in the package. This was the reason I put them in doc.go, to prevent duplication.

[rcernich]

Updated with tests.

I removed the package flag and instead use the base of the group (i.e. either group or group of group.some.name).

I added support for adding tags for specific generated types (e.g. genclient:nonNamespaced for cluster scoped resources using the same type as namespaced resources like Policy and MeshPolicy).

I still have to update the PR for the api project using the new generator. This will move the generator comments to the comment section above the type comments and will include the proper tags for generating the clientsets for cluster resources. We'll see how that goes.

[geeknoid]

I'm going through the code now.

[geeknoid]

Sorry about that, I clicked the wrong button

[rcernich]

Currently, protoc isn't copying over the detached comments for the types, which means the generator tags need to be in the message comments. I'll keep digging to see if there's a workaround. (I just got builds working in api, so I haven't done much investigating up to this point.)

[rcernich]

It looks like protobuf has changed pretty significantly in the last few months and there are many diffs in the generated .pb.go files. The protoc image is using master for golang/protobuf and gogo/protobuf and I'm wondering if these should be using a tag or ref when building the image. Opinions?

[ozevren]

I believe we're using a tagged version in the master branch. We should do the same for istio/api. Following 1.1 release, we can update the tag #. I think this is it: https://github.com/istio/api/blob/825044c7e15f6723d558b7b878855670663c2e1e/tools/protoc/Dockerfile#L8

…

On Tue, Jan 29, 2019 at 7:33 AM Rob Cernich ***@***.***> wrote: It looks like protobuf has changed pretty significantly in the last few months and there are many diffs in the generated .pb.go files. The protoc image is using master for golang/protobuf and gogo/protobuf and I'm wondering if these should be using a tag or ref when building the image. Opinions? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#37 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQR8I57eGyYL004zjGENa52d_SB1-LXYks5vIGnNgaJpZM4aLUUJ> .

[rcernich]

It's actually this that's the problem. I was able to narrow down to specific commit ids that seem to correlate with the last build of the image.

[geeknoid]

The code looks good in general. The only issue is the copyright injection in the output.

Thanks.

[geeknoid]

I don't believe it's correct to put the Istio copyright on the output of the tool. This would be like putting a copyright from a compiler vendor into produced binaries. The copyright to the material is actually with the author of the original content. I suggest just going with something like:

// Code generated by protoc-gen-gogo. DO NOT EDIT.
// source: authentication/v1alpha1/policy.proto

[rcernich]

I can do that. I was just following the convention of the other code generators. I just about have the api PR polished up, which should provide a good example of the output of the tool.

Thanks for taking the time to review!

[rcernich]

This should be good to go now. Please refer to the individual commits in istio/api#764 to see it in action.

Changes to isio/api source: istio/api@1e4498c

Generated k8s code: istio/api@152cda4

I still looking at cleaning up the Makefile in istio/api. That said, this change needs to be committed first, so I can get the proper info in the Gopkg.lock file.

[geeknoid]

Thanks. Looks good.