[Discussion] V3 breaking changes

[Adirio]

Now that we are working on v3, it may be time to go ahead and do some of this changes that we can't do on an stable version (such as v2). This issue is meant to be a discussion to see which of them shows enough interest by the developers/community in order to implement them.

1. Split CRD and controller scaffolding into two different subcommands. In v1 and v2, kubebuilder create api crteates both the CRD and the controller. Do we still think this is the desired behavior? Should we have a separate subcommand for the controller and eliminate the --resource and --controller flags? Should these two subcommands replace the current one or should we have three (create resource for the resource only, create controller for the controller only, create api for both of them)?
2. Add external resource flag to controller creating subcommand. When this flag is passed, kubebuilder won't search for the corresponding resource and will also take it into account at the tests, and when it is not passed the controller will validate that the appropiate resource is created, either by this same call, by a previous call, or even by hand.

If you think there are other related topics we should discuss, comment and I will add them here.

/cc @droot @DirectXMan12 @mengqiy @pwittrock @camilamacedo86 @estroz @joelanford

[camilamacedo86]

IMO:

Over split the command create api

• Pros: keep concepts such as cohesion and single responsibility in the commands/plugins (maintainability)
• Cons: GKV would be required in both which means that the user would need to inform them twice

So, shows for me that we might be prevailing keep it ease to maintaining instead of user experience. In this way, I am afraid that I am more inclined to vote against since I think the UX is more important here.

Over new flag to work with external types

I totally agree that we need to address this scenario. WDYT about we raise a new specific issue for this RFE? Also, instead of not scaffold the files I think we ought to educate and facilitate to the users how to work with for the external type which could be something such as we have a new plugin for that since we will not create an api but we will add one preexistent into the project.

Example: kubebuiler add api --module=github.com/openshift/api --kind=route --version=v1

• Run go get -u go get -u github.com/openshift/api // run go get -u {{module}}
• Add the schema into the main.go:

import (
	...
	routev1 "github.com/openshift/api/route/v1"  // see {{kindversion}} {{module}} 
	...
)

...

func init() {
...
	   // Add external type  	
           utilruntime.Must(routev1.AddToScheme(scheme))

• Add in the external type in the suite_test.go as well.

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

[estroz]

1. 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.
2. 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/v1@v0.18.2 \
   	--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.

[camilamacedo86]

Hi @estroz and @Adirio,

+1000 for @estroz suggestions 🥇 :

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

I liked the following as well to be scaffolded in the go.mod

--external-import=github.com/openshift/api/route/v1@v0.18.2 \

I just would like to clarifies that I do not think that we should add that to the create api command because we are not creating an api. IHMO it would be a new plugin/subcommand such as add api. Then, in this way, I think a less verbose flag would be fine but better meaning as suggested by @estroz shows great 👍

[Adirio]

We should probably define the API from a design point of view, chosing what are commands, what are arguments and what are flags, and from the arguments and flags, which are required, which have a default value, and which will ask the user for input if he didnt specify it.

[camilamacedo86]

Shows that we could create an EP for the add api option. WDYT?

[Adirio]

Any template for the EP?

[estroz]

@Adirio this doc probably.

[camilamacedo86]

I'd suggest you try to add further details and maybe follow up a little bit the template https://github.com/operator-framework/enhancements/blob/master/enhancements/template.md might be helpful for you achieve it.

[Adirio]

Discussion related to this has taken place in another Issue.

@estroz (#1826 (comment)) suggested re-designing the project configuration file in order to provide a better structure as the current one has received several patches over time and is a bit unclear.

version: 3-alpha
resources:
- api: # api contains all important information from `create api`
    group: my.domain
    version: v1
    kind: CronJob
    crdVersion: v1
    resourceTypes: true # If 'api/v1/cronjob_types.go' was created
    controller: true # If 'controllers/cronjob_controller.go' was created
  webhook: # webhook contains all important information from `create webhook`
    webhookConfigVersion: v1
    types:
    - validating # If `webhook.Validator` was implemented
    - mutating # If `webhook.Defaulter` was implemented
    - conversion # If `--conversion` was set

The above design groups fields by create subcommand, which is logical given the expected kubebuilder command flow of init -> create api -> create webhook.

I also suggested a new design (#1826 (comment))

version: 3-alpha
domain: my-domain.com
crdVersion: v1beta1
webhookVersion: v1beta1
apis:
- resource:
    group: Ships
    # version omitted means v1
    kind: Boat
    # plural omitted means Boats
    # domain omitted means my-domain.com (see line 2)
    # path omitted means it is not an external type
  definition:
    # version omitted means v1beta1 (see line 3)
    namespaced: true
  # controller omitted means false
  webhooks:
    # version omitted means v1beta1 (see line 4)
    types:
    - validating
- resource:
    group: Animals
    version: v2
    kind: Sheep
    plural: Flock
    domain: my-animals.com
    path: "github.com/owner/repo/apis/animals"
  controller: true

@camilamacedo86 and @joelanford said that they would push this re-design to v4.

[camilamacedo86]

Following my concerns with the proposed design:

• Regards crdVersion and webhookVersion: Currently, in the business layer we do not allow diff versions. However, the model should allow it since in the future we might be able to improve this behaviour. More info: ⚠️ (go/v3-alpha) default to v1 CRDs and webhooks #1644 (comment)
• Regards apis: controllers, for example, are not API's so I do not see it fitting well.

So, before we discuss the changes I think that would be important we list what are the problems with the current project file design that we are trying to solve that justify these changes. WDYT? Could we list that here?

[Adirio]

We have this scheduled for thursday, but just some quick notes:

• Regards crdVersion and webhookVersion: Currently, in the business layer we do not allow diff versions. However, the model should allow it since in the future we might be able to improve this behaviour. More info: #1644 (comment)

As you said, we need to allow it. The model I provided sets a default version for each but the individual resources can modify it. This way we reduce verbosity when all or most of the resources use one version.

• Regards apis: controllers, for example, are not API's so I do not see it fitting well.

I do agree that API is not a good term, and I asked for suggestions for how to call a CRD + controllers + webhook. But, currently, we are calling API to CRD + controller (kubebuilder create api ...), so technically controllers are API in kubebuilder terms.

[camilamacedo86]

Hi @Adirio,

As you said, we need to allow it. The model I provided sets a default version for each but the individual resources can modify it. > This way we reduce verbosity when all or most of the resources use one version.

The suggestion made does not allow we have diff versions of api for crds and webhooks in the same project. So, we cannot:

version: 3-alpha
domain: my-domain.com
crdVersion: v1beta1
webhookVersion: v1beta1

By following the thread #1644 (comment) you will find all reasons and use cases. See that it is :

1 GKV can have a 1 CRD which can have 1 crdVersion

The model needs to be legitim to what they represent (DDD concept) what will be or not an allowed because of the business rules should not change it. In this way, I do not agree that it would reduce verbosity. It would not allow the tool to do the scaffolds as it can be done and introduce limitations that can result in breaking changes in the future.

I do agree that API is not a good term, and I asked for suggestions for how to call a CRD + controllers + webhook. But, currently, we are calling API to CRD + controller (kubebuilder create api ...), so technically controllers are API in kubebuilder terms.

The command create api creates an API and also allows users creates the controller for the api created. It does not create webhoks. webhooks are scaffolds with the command create webhooks. And then, in IMO the commands UX should not define the model as it shows goes against the DDD principles.

[Adirio]

@camilamacedo86 The provided model does support different versions. See how in apis[].definition there is a comment saying that we are omiting the version so the project default is used. If we set a version like so we would be able to specify one for that specific resource:

version: 3-alpha
domain: my-domain.com
crdVersion: v1beta1
webhookVersion: v1beta1
apis:
- resource:
    group: Ships
    kind: Boat
  definition:
    version: v1            # THIS IS WHERE YOU CAN SET IT PER RESOURCE
  webhooks:
    version: v1            # THIS IS WHERE YOU CAN SET IT PER RESOURCE

I added comments in places where some fields could be added to override the defaults and specify which the defaults would be.

About the API point:
As I already said: I don't like the term API. I find it missleading and inaccurate. If you go to the linked comment above, you will see in the image how I ask if there is a better name to represent the group of a resource, its controller and webhooks.
But kubebuilder uses the term API to group a resource and its controller. And I don't think there will be a change in the foreseeable future in this aspect.

I updated the diagram to swap the slice of strings for the pointer to a struct for webhook types and a couple of other cosmetic changes:

---

An alternative would be making Resource the lowest level class:

Which would result in the following project config file:

version: 3-alpha
domain: my-domain.com
crdVersion: v1beta1
webhookVersion: v1beta1
resources:
- group: Ships
  # version omitted means v1
  kind: Boat
  # plural omitted means Boats
  # domain omitted means my-domain.com (see line 2)
  # path omitted means it is not an external type
  definition:
    # external ommited means false
    # version omitted means v1beta1 (see line 3)
    namespaced: true
  # controller omitted means false
  webhooks:
    # version omitted means v1beta1 (see line 4)
    types:
      validating: true
- group: Animals
  version: v2
  kind: Sheep
  plural: Flock
  domain: my-animals.com
  path: "github.com/owner/repo/apis/animals"
  definition:
    external: true
  controller: true

[camilamacedo86]

Hi @Adirio,

Following my considerations regards the suggestions made.

Regards:

version: 3-alpha
domain: my-domain.com
crdVersion: v1beta1
webhookVersion: v1beta1

• 1 Project can have many APIs
• Each API can be scaffolded with diff k8s API versions which are represented by the attribute conversion

And then, the same is possible for webhook. So, we cannot add the crdVersion and the webhookVersion whcih represents the K8S api version ( v1 or betav1 currently) on the root since it would mean that we can only have one apiVersion for each type of resource per project. Also, I do not like the above assumptions over default string values. IMO version omitted means v1beta1 (see line 3) is very confusing and make harder it keep maintained besides it shows to be putting the view and business layer in the persistent one. Also, it was exhaustively discussed in the PR #1644 so, I would consider not move forward with this one.

Regards:

resources:
- group: Ships
  # version omitted means v1

It makes no sense for me since the GVK can assume any possible string value. Has no default value for the version of the GKV.
PS.: I am afraid that I do not agree with the complexities added over assuming default string values on the persistent layer as well.

Regards:

# path omitted means it is not an external type
....
  definition:
    # external ommited means false 

I am ok with we add attrs related to the external types 👍 if they are required, but it should be part of the scope and code implementation of the EP to add helpers to allow users works with. Try to define it before we have the feature shows put the cart before the horse (Anti-Pattern Premature Optimization). WDYT about we have an issue to only discuss this subject? Does it show to be the item 2 here whcih shows that we all love your idea to add this feature?

Regards

  definition:
    # external ommited means false
    # version omitted means v1beta1 (see line 3)
    namespaced: true

This attr is ONLY relevant when we scaffold API"s for the project. I agree that we could track that but then, it shows part of the api structure domain of responsibility. The definition does not represent an object used by the project which also shows hurts the DDD concept. I am ok with we add namespaced: true 👍 but as an attr of the api which would like such as which matches with shows goes more in the @estroz suggestions #1826 direction in as well:

resources:
- api: 
      namespaced: true
      crdVersion: v1
  controller: true

Regards:

webhooks:
    # version omitted means v1beta1 (see line 4)
    types:
    - validating

In the PR #1696 we have now:

webhooks: // respresents the webhooks
    apiVersion: v1
    defaulting: true
    validation: true

I am not in favour of the suggestion because:

• Note that each type in the future could be a new object/structure to store its one attrs/specs.
• I cannot see any advantage in marshal/unmarshal strings instead of bools. Work with strings here will increase unnecessarily the complexity and reduces maintainability and performance. And then, I am struggling to find out a use case (I am as , I would like to , so that ) that justify it. I hope that it makes sense.

[Adirio]

Hi @camilamacedo86

Regards:

version: 3-alpha
domain: my-domain.com
crdVersion: v1beta1
webhookVersion: v1beta1

• 1 Project can have many APIs
• Each API can be scaffolded with diff k8s API versions which are represented by the attribute conversion

And then, the same is possible for webhook. So, we cannot add the crdVersion and the webhookVersion whcih represents the K8S api version ( v1 or betav1 currently) on the root since it would mean that we can only have one apiVersion for each type of resource per project. Also, I do not like the above assumptions over default string values. IMO version omitted means v1beta1 (see line 3) is very confusing and make harder it keep maintained besides it shows to be putting the view and business layer in the persistent one. Also, it was exhaustively discussed in the PR #1644 so, I would consider not move forward with this one.

These fields can be also specified per resource. They are just defining a project-wide default. This is just an optimization (and could be added later as it would be non-breaking).

Imagine that we are working with an old kuebernetes version a we want all our crd versions to be v1beta1. Instead of having to specify it in each resource we could specify it here. However, if you specify it in each resource it would still be ok. It is just a project-wide default that reduces the length of the generated project config files. Technically, domain is already being used as a project wide default. So yes, each resource can have its own version, or all of them can have a shared one here and avoid writting it one time per resource. This way, only those resources that use a different version to the project wide default need to specify it.

Regards:

resources:
- group: Ships
  # version omitted means v1

It makes no sense for me since the GVK can assume any possible string value. Has no default value for the version of the GKV.
PS.: I am afraid that I do not agree with the complexities added over assuming default string values on the persistent layer as well.

Again, defaulting to "v1" is just a way of reducing the number of lines of the project configuration file. Most of the times this is the version used. It is just an optimization (and is also non-breaking so could be implemented later).

Regards:

# path omitted means it is not an external type
....
  definition:
    # external ommited means false 

I am ok with we add attrs related to the external types 👍 if they are required, but it should be part of the scope and code implementation of the EP to add helpers to allow users works with. Try to define it before we have the feature shows put the cart before the horse (Anti-Pattern Premature Optimization). WDYT about we have an issue to only discuss this subject? Does it show to be the item 2 here whcih shows that we all love your idea to add this feature?

All the discussion is because we are adding fields that we are not using yet but plan to use in a future. Why do you think adding this is a premature optimization but adding webhooks (for example) isn't? They are not being used, they are not required yet. If I'm not mistaken, this was one of the points that @estroz wanted to make yesterday at the triage meeting to remove this part from the blockers for v3.0.0. Correct me if I'm wrong Eric.

Regards

  definition:
    # external ommited means false
    # version omitted means v1beta1 (see line 3)
    namespaced: true

This attr is ONLY relevant when we scaffold API"s for the project. I agree that we could track that but then, it shows part of the api structure domain of responsibility. The definition does not represent an object used by the project which also shows hurts the DDD concept. I am ok with we add namespaced: true 👍 but as an attr of the api which would like such as which matches with shows goes more in the @estroz suggestions #1826 direction in as well:

resources:
- api: 
      namespaced: true
      crdVersion: v1
  controller: true

Okey, just replace Definition by API in the previous figures and definition by api in the yamls.

Regards:

webhooks:
    # version omitted means v1beta1 (see line 4)
    types:
    - validating

In the PR #1696 we have now:

webhooks: // respresents the webhooks
    apiVersion: v1
    defaulting: true
    validation: true

I am not in favour of the suggestion because:

• Note that each type in the future could be a new object/structure to store its one attrs/specs.
• I cannot see any advantage in marshal/unmarshal strings instead of bools. Work with strings here will increase unnecessarily the complexity and reduces maintainability and performance. And then, I am struggling to find out a use case (I am as , I would like to , so that ) that justify it. I hope that it makes sense.

Sorry that was a typo, I meant:

webhooks:
  # version omitted means v1beta1 (see line 4)
  types:
    validating: true