Handle external API types explicitly in create api

[estroz]

I want to create a controller for an external type, ex. core/v1.Pod or route.openshift.io/v1.Route, and have kubebuilder be aware that this type is external (for commands I run after create api).

Currently external types are supported, but are treated as custom types, which can cause problems with scaffolding controller files. Additionally the edge case of the "core" group, which is "" in actual GVKs, isn't handled well.

From #1772:

Regarding creating a new command: The most common use case by far (anecdotal evidence only) is creating both a controller and resource. I do sometimes see one or the other (or neither) being created to support external types, so having the choice of either or neither mode is still useful. Having a command for each mode confounds the currently straightforward workflow. Therefore I vote to leave the create api command as-is, except invert flag defaults to true.

Regarding the extra flag on create api: Having an extra flag makes sense, since we need to know where the external types are coming from as they aren't in api/<version> or apis/<group>/<version>. I like @camilamacedo86's idea of supplying a module/import path, perhaps with a more descriptive flag like --external-import. kubebuilder create api shouldn't be doing any go get stuff though; that'll be done by calling make targets. The input would also have to describe the exact import path, and --group would have to be the full API group:

kubebuiler create api \
	--external-import=github.com/openshift/api/route/[email protected] \
	--group=route.openshift.io \
	--kind=Route \
	--version=v1

We might also want to add the resource to resources in the config with an external: <boolean> qualifier.

Related: #1975 (loosely), #1772

/kind feature

[camilamacedo86]

I'd like to make a few considerations over this one:

Handle external API types explicitly in create api

Iin POV the command create webhook would also accept:

kubebuiler create webhook \
	--external-import=github.com/openshift/api/route/[email protected] \
	--group=route \
        --domain= openshift.io \ 
	--kind=Route \
	--version=v1

Where the webhook would upstatde and the PROJECT file will have the resource info without api and a controller.

Currently external types are supported, but are treated as custom types,

Kubebuilder only supports core types and the APIs scaffolded in the project by default unless you manually change the files you will be unable to work with external-types. I mean, it is not really supported by the tool since users are able to do that only if they customize the scaffolds manually via workarounds.

• The example command is passing the domain in the group. IMO the users will need to pass the group and the domain in this case

kubebuiler create api \
	--external-import=github.com/openshift/api/route/[email protected] \
	--group=route \
        --domain= openshift.io \ 
	--kind=Route \
	--version=v1

• IMO: The expected result over this scenario would be:

PROJECT file

• new attr external-import to store: github.com/openshift/api/route/[email protected]
• path attr will be the import in the go files: github.com/openshift/api/route/v1

post subcommand action

• go get -u external-import would be called after the scaffolds

controller
By following up the above suggestion I believe that the controller will be done properly already and will not need to do any change.

main.go

We will need to add the schema before starts the manager.

suite_test.go for controller and webhooks
We will need to add a conditional to add the schema in this scenario, following an example:

import (
	...
	routev1 "github.com/openshift/api/route/v1"  // see {{path}} 
	...
)
....
var _ = BeforeSuite(func(done Done) {
...
       	err = routev1.AddToScheme(scheme.Scheme) 
	Expect(err).NotTo(HaveOccurred())
...
}

Also, shows that we need to do a change in the CRDDirectoryPath for this case: See kubernetes-sigs/controller-runtime#1191

CRDDirectoryPaths: []string{
			filepath.Join("..", "config", "crd", "bases"),
			filepath.Join(build.Default.GOPATH, "pkg", "mod", "github.com", "owner", "theirs@version", "config", "crd", "bases"),
		},

We need to check that.

[camilamacedo86]

That requires a few more caveats. It was identified in the scenario (operator-framework/operator-sdk#4747)

To scaffold a webhook for an external/core type:

• that will be required multi-group support otherwise is not possible to create the webhook file in the right place
• should not be required to create-api before that ith resource=true

[Adirio]

Webhooks can not be created for external types with the higher level abstraction of controller-runtime (which create webhook uses) as it implements a method of a type and that is not supported in Go (you cant define a method for a type of different package). What we may want to do is provide some scaffolding using the lower abstraction layer in case of external types, but that would be a different topic.

[camilamacedo86]

@Adirio,

Could you describe why not?

An external type is exactly the same as any api/type created in the operator. The only difference is that is not defined in the project and outside. However, both are CRDs.

Are you trying to describe that webhooks have a limitation for core types? could you please add more references about that here to share your thoughts?

[estroz]

@camilamacedo86 you cannot define methods on an external type, since the Go project does not define the type. For example, you cannot define

var _ admission.Validator = &corev1.Pod{}

func (t *corev1.Pod) ValidateCreate() error {
	...
}

[camilamacedo86]

HI @estroz, @Adirio.

The &corev1.Pod{} is not an external type. That is a core-type.
External type or third-party(as called in other places) is any api whose owner is not the operator itself and is not a core type. Core types are the k8s api resource types. Third-party/external types are CRDs that are defined in other projects or for example the OCP route api.

Note that indeed we need an behaviour/implementation-specific for each scenario,

[estroz]

All core types are external types in this context. The problem encountered by the above example is the same for all external types.

[camilamacedo86]

HI @estroz, @Adirio

All core types are external types in this context

Sorry, I am afraid that I do not agree with that.

For both scenarios, we are able to accomplish the goal. However, for each one, we need to address its caveats. For example, see here the kb documentation over how to create webhooks for core types. (https://book.kubebuilder.io/reference/webhook-for-core-types.html)

[estroz]

Can you explain why they are different (not considering how they are represented in PROJECT right now)?

[kopiczko]

For example, see here the kb documentation over how to create webhooks for core types. (https://book.kubebuilder.io/reference/webhook-for-core-types.html)

Just saying this documentation is incomplete. kubebuilder won't generate many kustomize resources if the project's only webhook is the one handling core types (or types not fulfilling interfaces). Possibly it was written with the assumption there is already a type with the associated webhook generated in the project.

It would be great to support 'core types'/'types not fulfilling interfaces'. It's quite doable ATM but requires some manual work. I'm happy to help with that if you need a hand.

[kopiczko]

Coming from #2141 (comment)

We can change here: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/v3/webhook.go#L107 for:

• If is core type then update the PROJECT file accordingly and scaffold the webhook
• if is external-type (we need a new flag for that) then, do the same.

Do I understand correctly that external types not implementing Defaulter or Validator should be supported as well?

[camilamacedo86]

Hi @Adirio, @estroz, @kopiczko,

My point here is that an external type is NOT a core type and they have not the same bahaviour. For example:

External type can be a CRD that is implemented in Operator A and used in the B. In this common case, the scaffold for the external types can be exactly the same as that one that is done for the APIs/CRD owned by the project. However, please let me know if you see any reason for that does not be true.

Then, in POV we have the following scenarios:

• Core-types: (K8s ones e.g Pod/Deployment): That requires a different scaffold/options
• External-types: (CRDs/third-party): if they are an API/CRD that seems to be == the internal/owned omes. However, then for this case, we also have for example the scenario where the API can be a third-party as the OCP route added above. For the third-party scenarios, I am not sure what implementations we could have and then that requires further analyse.

I hope that clarifies.

[kopiczko]

@camilamacedo86 thanks for clarifying.

There are definitely cases where external-types are not owned. One example my company is doing is setting default http://cluster.x-k8s.io/watch-filter on CAPI types.

I'm also not sure if it feels right to have the controller logic outside the controller repository. In such scenario updating the type's repository dependency would also change the controller behaviour and this may make things more difficult to track. It also feels more rigid to me. E.g. someone may want to write a separate admission controllers for different providers.

Having said all of that a support for even the most straight forward scenario like "external == owned" would be great, but it could lead to users generating webhook for a core type and then running sed s#core/v1/Pod#external/Type#g. Which is exactly the thing I'm doing now to create a reconciler for an external type.

[camilamacedo86]

Hi @kopiczko,

Thank you for your reply.

I'm also not sure if it feels right to have the controller logic outside the controller repository

In the example, the controller logic is in the project. The CRD definition is that not. The controller will reconcile a type that is not defined by the project only that. Because of this, we call external type.

Having said all of that a support for even the most straight forward scenario like "external == owned" would be great, but it could lead to users generating webhook for a core type

We can easily identify what is a core-type or not. It is mapped in the project. See:https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/options.go#L27-L53
Note that we track that diff in the PROJECT file as well.
Because of this, that seems a not problem at all.

The only scenario that seems to be missing here to digger further is an external type such as OCP route API. How a webhook should be a scaffold for that,?

[kopiczko]

Sorry @camilamacedo86 I abuse the word "controller".

I'm also not sure if it feels right to have the controller logic outside the controller repository.

Here I meant "admission controller". So "controller logic" == "webhook logic".

The only scenario that seems to be missing here to digger further is an external type such as OCP route API. How a webhook should be a scaffold for that,?

That was my whole point. If that scenario is not supported then users may end up with generating something for a core type and replacing imports.

How would you like to scaffold a webhook for a core type? Wouldn't the same scaffolding work for non-core non-owned?

[estroz]

How would you like to scaffold a webhook for a core type? Wouldn't the same scaffolding work for non-core non-owned?

@kopiczko yes, and @camilamacedo86 that is what I meant by "All core types are external types in this context". The solution suggested in the docs will work for core types right now. For other external types, differentiation from owned types is needed first so the CLI "knows" which implementation to scaffold.

Side note: #833 talks about generally allowing low-level webhook scaffolding, which I think means creating a Handler. The work being discussed here could either build on top of a general low-level scaffold, or visa versa.

[kopiczko]

Hi @camilamacedo86 @estroz, I was pondering around a bit and have some ideas. What is the best way to discuss how this webhook scaffolding would look like? Is it fine to describe it very briefly here and then discuss the rest in a draft PR?

[Adirio]

Slack for early discussion can also be used, add me also to the conversation

[camilamacedo86]

It is important because many users constantly ask and need to do this. Therefore, because the tool does not handle this scenario it is leading to misunderstandings and wrong assumptions. Because of this, I am adding the priority label on this one.

**Summary **

• Suggested Solution (something like): Handle external API types explicitly in create api #1999 (comment)
• Note that we mapped the core types and we also add paths used to the imports in the PROJECT file. Therefore, because of this internally the logic is not the same for the end-users external (owned by other projects) and core types (k8s ones) are the same.
• Before we need to be able to properly scaffold the APIs then, only after that we can be looking for the webhooks behaviour. Webhooks uses the API info tracked in the PROJECT file to do the scaffolds and by achieving the goal of creating api properly scaffold and tracking the info we are probably sorting it out for the webhooks. And if not, it is easier to be checked/came up with a solution after we handle it for the pre-requisite which is how to have the API scaffold in the first place.

@jmrodri I think we have been not working on this one. So, unassign you from this one so that we can have the opportunity to see if someone from the community can help us out. However, if you are looking at that then, please feel free to re-assign it to yourself.

[allenhaozi]

Using the same set of tools to complete the development of webhook and controller has greatly improved the efficiency of development for developers, and the readability and maintainability of the developed code is greatly improved when everyone uses the same set of tools, because we all follow the same set of standards

In a real development scenario, we would develop a custom controller based on our scenario, but for us the more common scenario would be webhooks because there are already so many mature and good controllers in the community that there is no need to repeat development

We are the Infrastructure Department. In order to integrate the capabilities of other components of the infrastructure, we usually need to perform patch behavior on other crd resources.

For example:

• added label nodeselector boot scheduler,
• add initcontainer to do some initialization
• add a side-car for unified traffic management
• ...

so
If we can't support external webhooks as easily as a buildin crds webhooks, can we provide regular code generation capabilities example-code
this represents our recommended code structure, code directory, or webhook best practices

Examples of Using code

1. run the webhook build command

$ kubebuilder create webhook \
	--group argoproj.io \
	--version v1alpha1 \
	--kind Workflow \
	--webhook-type external  \
        --external-import "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1" \
        --defaulting 

2. inject the following code to main.go

hookServer := mgr.GetWebhookServer()
hookServer.Register("/mutate-v1alpha1-workflow", &webhook.Admission{Handler: &webhookv1alpha1.ArgoWorkflowHandler{Client: mgr.GetClient()}})

3. generate webhook file

api/v1alpha1/workflow_webhook.go

It could be under controller, it could be under api/apis, it could be under pkg
I'll take api as an example

package v1alpha1

import (
	"context"
	"encoding/json"
	"net/http"

	workflowv1alpha1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
	"sigs.k8s.io/controller-runtime/pkg/client"
	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
)

type ArgoWorkflowHandler struct {
	Client  client.Client
	decoder *admission.Decoder
}

//+kubebuilder:webhook:path=/mutate-v1alpha1-workflow,mutating=true,failurePolicy=fail,sideEffects=None,groups=argoproj.io,resources=workflows,verbs=create;update,versions=v1alpha1,name=mworkflow.argoproj.io,admissionReviewVersions=v1

// +kubebuilder:rbac:groups=workflow.argoproj.io,resources=workflows,verbs=get;list;watch;create;update;patch;delete

// ArgoWorkflowHandler
// TODO(user): Modify the Handle function to meet your needs
func (a *ArgoWorkflowHandler) Handle(ctx context.Context, req admission.Request) admission.Response {
	workflow := &workflowv1alpha1.Workflow{}

	err := a.decoder.Decode(req, workflow)
	if err != nil {
		return admission.Errored(http.StatusBadRequest, err)
	}

	// TODO(user): your logic here

	marshaledWorkflow, err := json.Marshal(workflow)
	if err != nil {
		return admission.Errored(http.StatusInternalServerError, err)
	}

	return admission.PatchResponseFromRaw(req.Object.Raw, marshaledWorkflow)
}

// ArgoWorkflowHandler InjectDecoder
func (a *ArgoWorkflowHandler) InjectDecoder(d *admission.Decoder) error {
	a.decoder = d
	return nil
}

[cornfeedhobo]

Note that we mapped the core types and we also add paths used to the imports in the PROJECT file. Therefore, because of this internally the logic is not the same for the end-users external (owned by other projects) and core types (k8s ones) are the same.

@camilamacedo86 can you elaborate on this point? I'm trying to use kubebuilder (through operator-sdk) to scaffold some webhooks for core types, but there seems to be a lot of confusion around how to do this. I've yet to find a canonical example that just works.

Edit: I hammered away on a boiled down example. From what I can tell, all scaffolding works, as well as the integration tests. It wasn't that much tweaking, but it was far from intuitive. Hopefully this repo will serve as a learning tool for this issue and for a workaround in the meantime. The commits are broken up so people can see the progression from initialization to working repo state.

[k8s-triage-robot]

This issue is labeled with priority/important-soon but has not been updated in over 90 days, and should be re-triaged.
Important-soon issues must be staffed and worked on either currently, or very soon, ideally in time for the next release.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Deprioritize it with /priority important-longterm or /priority backlog
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[cornfeedhobo]

/priority important-longterm

[mjlshen]

/assign

[lingdie]

Any update or solution?