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.

Sign up for GitHub

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

Jump to bottom

🌱 Remove reliance on controller-runtime scheme builder #9045

Merged
merged 2 commits into from
Aug 7, 2023
Merged

🌱 Remove reliance on controller-runtime scheme builder #9045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e0c9cdb
Remove reliance on controller-runtime scheme builder
JoelSpeed Jul 18, 2023
8b44611
Document schemebuilder pattern in implementers guide
JoelSpeed Aug 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/v1beta1/cluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,7 +599,7 @@ type ClusterList struct {
}

func init() {
SchemeBuilder.Register(&Cluster{}, &ClusterList{})
objectTypes = append(objectTypes, &Cluster{}, &ClusterList{})
}

// FailureDomains is a slice of FailureDomains.
Expand Down
2 changes: 1 addition & 1 deletion api/v1beta1/clusterclass_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,5 +706,5 @@ type ClusterClassList struct {
}

func init() {
SchemeBuilder.Register(&ClusterClass{}, &ClusterClassList{})
objectTypes = append(objectTypes, &ClusterClass{}, &ClusterClassList{})
}
17 changes: 13 additions & 4 deletions api/v1beta1/groupversion_info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,26 @@ limitations under the License.
package v1beta1

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"sigs.k8s.io/controller-runtime/pkg/scheme"
)

var (
// GroupVersion is group version used to register these objects.
GroupVersion = schema.GroupVersion{Group: "cluster.x-k8s.io", Version: "v1beta1"}

// SchemeBuilder is used to add go types to the GroupVersionKind scheme.
SchemeBuilder = &scheme.Builder{GroupVersion: GroupVersion}
// schemeBuilder is used to add go types to the GroupVersionKind scheme.
schemeBuilder = runtime.NewSchemeBuilder(addKnownTypes)

// AddToScheme adds the types in this group-version to the given scheme.
AddToScheme = SchemeBuilder.AddToScheme
AddToScheme = schemeBuilder.AddToScheme

objectTypes = []runtime.Object{}
)

func addKnownTypes(scheme *runtime.Scheme) error {
Copy link
Member

@sbueringer sbueringer Aug 3, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Isn't this all the same as just:

func AddToScheme(s *runtime.Scheme) {
	s.AddKnownTypes(GroupVersion, objectTypes...)
	metav1.AddToGroupVersion(s, GroupVersion)
}

Would it be simpler to just define the func?

(seems a lot easier to understand to me, but maybe I'm missing something)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern seems to be common if you look at say core/v1 or any other API package, so I was just following the usual pattern.

In those other APIs the schemebuilder is exported though, perhaps we should be leaving ours exported for consistency?

Copy link
Member

@sbueringer sbueringer Aug 3, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinion from my side (@vincepri?)

I was just wondering if we have to go through the schemeBuilder if we only export the AddToScheme func as we could do that in a simpler way.

scheme.AddKnownTypes(GroupVersion, objectTypes...)
metav1.AddToGroupVersion(scheme, GroupVersion)
return nil
}
2 changes: 1 addition & 1 deletion api/v1beta1/machine_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,5 +307,5 @@ type MachineList struct {
}

func init() {
SchemeBuilder.Register(&Machine{}, &MachineList{})
objectTypes = append(objectTypes, &Machine{}, &MachineList{})
}
2 changes: 1 addition & 1 deletion api/v1beta1/machinedeployment_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ type MachineDeploymentList struct {
}

func init() {
SchemeBuilder.Register(&MachineDeployment{}, &MachineDeploymentList{})
objectTypes = append(objectTypes, &MachineDeployment{}, &MachineDeploymentList{})
}

// GetConditions returns the set of conditions for the machinedeployment.
Expand Down
5 changes: 3 additions & 2 deletions api/v1beta1/machinedeployment_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
. "github.com/onsi/gomega"
admissionv1 "k8s.io/api/admission/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/utils/pointer"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
Expand All @@ -44,8 +45,8 @@ func TestMachineDeploymentDefault(t *testing.T) {
},
}

scheme, err := SchemeBuilder.Build()
g.Expect(err).ToNot(HaveOccurred())
scheme := runtime.NewScheme()
g.Expect(AddToScheme(scheme)).To(Succeed())
defaulter := MachineDeploymentDefaulter(scheme)

t.Run("for MachineDeployment", defaultValidateTestCustomDefaulter(md, defaulter))
Expand Down
2 changes: 1 addition & 1 deletion api/v1beta1/machinehealthcheck_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,5 +169,5 @@ type MachineHealthCheckList struct {
}

func init() {
SchemeBuilder.Register(&MachineHealthCheck{}, &MachineHealthCheckList{})
objectTypes = append(objectTypes, &MachineHealthCheck{}, &MachineHealthCheckList{})
}
2 changes: 1 addition & 1 deletion api/v1beta1/machineset_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,5 +240,5 @@ type MachineSetList struct {
}

func init() {
SchemeBuilder.Register(&MachineSet{}, &MachineSetList{})
objectTypes = append(objectTypes, &MachineSet{}, &MachineSetList{})
}
65 changes: 65 additions & 0 deletions docs/book/src/developer/providers/implementers-guide/create_api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,68 @@ As the deleted comments request, run `make manager manifests` to regenerate some
git add .
git commit -m "Added cluster types"
```

# Registering APIs in the scheme

To enable clients to encode and decode your API, your types must be able to be registered within a [scheme].

[scheme]: https://pkg.go.dev/k8s.io/apimachinery/pkg/runtime#Scheme

By default, Kubebuilder will provide you with a scheme builder like:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have an issue in Kubebuilder to try steer away from the current pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet, I'll make one


```go
import "sigs.k8s.io/controller-runtime/pkg/scheme"

var (
// SchemeBuilder is used to add go types to the GroupVersionKind scheme
SchemeBuilder = &scheme.Builder{GroupVersion: GroupVersion}

// AddToScheme adds the types in this group-version to the given scheme.
AddToScheme = SchemeBuilder.AddToScheme
)
```

and scheme registration that looks like:

```go
func init() {
SchemeBuilder.Register(&Captain{}, &CaptainList{})
}
```

This pattern introduces a dependency on controller-runtime to your API types, which is discouraged for
API packages as it makes it more difficult for consumers of your API to import your API types.
In general, you should minimise the imports within the API folder of your package to allow your API types
to be imported cleanly into other projects.

To mitigate this, use the following schemebuilder pattern:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @furkatgofurov7 In case we might want to include this as an optional action item in the v1.6 release

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vincepri For the provider migration doc, right? Sounds like a good idea!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct yeah

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vincepri @sbueringer +1 to add it to the provider migration doc.

Opened #9146 to cover that, PTAL and let me know what you think, thanks!


```go
import "k8s.io/apimachinery/pkg/runtime"

var (
// schemeBuilder is used to add go types to the GroupVersionKind scheme.
schemeBuilder = runtime.NewSchemeBuilder(addKnownTypes)

// AddToScheme adds the types in this group-version to the given scheme.
AddToScheme = schemeBuilder.AddToScheme

objectTypes = []runtime.Object{}
)

func addKnownTypes(scheme *runtime.Scheme) error {
scheme.AddKnownTypes(GroupVersion, objectTypes...)
metav1.AddToGroupVersion(scheme, GroupVersion)
return nil
}
```

and register types as below:

```go
func init() {
objectTypes = append(objectTypes, &Captain{}, &CaptainList{})
}
```

This pattern reduces the number of dependencies being introduced into the API package within your project.
5 changes: 3 additions & 2 deletions internal/test/builder/builders.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/intstr"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
Expand Down Expand Up @@ -1268,8 +1269,8 @@ func (m *MachineDeploymentBuilder) Build() *clusterv1.MachineDeployment {
}
}
if m.defaulter {
scheme, err := clusterv1.SchemeBuilder.Build()
if err != nil {
scheme := runtime.NewScheme()
if err := clusterv1.AddToScheme(scheme); err != nil {
panic(err)
}
ctx := admission.NewContextWithRequest(context.Background(), admission.Request{
Expand Down