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.

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

✨ enable conversion webhook by default #427

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions examples/conversion/pkg/apis/jobs/v1/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@ package v1

import (
"k8s.io/apimachinery/pkg/runtime/schema"
controllers "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/scheme"
)

var (
// SchemeGroupVersion is group version used to register these objects
SchemeGroupVersion = schema.GroupVersion{Group: "jobs.example.org", Version: "v1"}

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

// AddToScheme is required by pkg/client/...
AddToScheme = SchemeBuilder.AddToScheme
Expand Down
4 changes: 2 additions & 2 deletions examples/conversion/pkg/apis/jobs/v2/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@ package v2

import (
"k8s.io/apimachinery/pkg/runtime/schema"
controllers "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/scheme"
)

var (
// SchemeGroupVersion is group version used to register these objects
SchemeGroupVersion = schema.GroupVersion{Group: "jobs.example.org", Version: "v2"}

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

// AddToScheme is required by pkg/client/...
AddToScheme = SchemeBuilder.AddToScheme
Expand Down
7 changes: 6 additions & 1 deletion pkg/builder/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/source"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
"sigs.k8s.io/controller-runtime/pkg/webhook/conversion"
)

// Supporting mocking out functions for testing
Expand Down Expand Up @@ -285,7 +286,11 @@ func (blder *Builder) doWebhook() error {
}
}

return err
err = conversion.CheckConvertibility(blder.mgr.GetScheme(), blder.apiType)
if err != nil {
log.Error(err, "conversion check failed", "GVK", gvk)
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm concerned that this error is going to be a bit spam-y for non-conversion cases (e.g. built-ins). Maybe we should have an explicit opt-in on the builder?

Copy link
Contributor

Choose a reason for hiding this comment

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

for posterity:

  • error out if there's a partial implementation (hub w/o spokes, spokes w/o hub, partial set of spokes, multiple hubs, etc)
  • log on success at V(1).Info("conversioned enabled for kind", "kind", groupKind, "hub", hubVersion, "spokes", spokeVersions) so that people can look for that line to debug too, or check for it's absence if they've failed to implement anything.

Copy link
Member

@mengqiy mengqiy May 15, 2019

Choose a reason for hiding this comment

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

i'm concerned that this error is going to be a bit spam-y for non-conversion cases (e.g. built-ins).

If kube-apiserver sends us conversion requests for built-ins, it means there is a bug in kube-apiserver, right?

EDIT: nvm. Sunil explained to me.

}
return nil
}

func generateMutatePath(gvk schema.GroupVersionKind) string {
Expand Down
2 changes: 2 additions & 0 deletions pkg/manager/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/recorder"
"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
"sigs.k8s.io/controller-runtime/pkg/webhook"
"sigs.k8s.io/controller-runtime/pkg/webhook/conversion"
)

var log = logf.RuntimeLog.WithName("manager")
Expand Down Expand Up @@ -191,6 +192,7 @@ func (cm *controllerManager) GetWebhookServer() *webhook.Server {
Port: cm.port,
Host: cm.host,
}
cm.webhookServer.Register("/convert", &conversion.Webhook{})
if err := cm.Add(cm.webhookServer); err != nil {
panic("unable to add webhookServer to the controller manager")
}
Expand Down
81 changes: 81 additions & 0 deletions pkg/webhook/conversion/conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,87 @@ func (wh *Webhook) allocateDstObject(apiVersion, kind string) (runtime.Object, e
return obj, nil
}

// CheckConvertibility determines if given type is convertible or not. For a type
// to be convertible, the group-kind needs to have a Hub type defined and all
// non-hub types must be able to convert to/from Hub.
func CheckConvertibility(scheme *runtime.Scheme, obj runtime.Object) error {
var hubs, spokes, nonSpokes []runtime.Object

gvks, _, err := scheme.ObjectKinds(obj)
if err != nil {
return fmt.Errorf("error retriving object kinds for given object : %v", err)
}

for _, gvk := range gvks {
instance, err := scheme.New(gvk)
if err != nil {
return fmt.Errorf("failed to allocate an instance for gvk %v %v", gvk, err)
}

if isHub(instance) {
hubs = append(hubs, instance)
continue
}

if !isConvertible(instance) {
nonSpokes = append(nonSpokes, instance)
continue
}

spokes = append(spokes, instance)
}

if len(gvks) == 1 {
return nil // single version
}

if len(hubs) == 0 && len(spokes) == 0 {
// multiple version detected with no conversion implementation. This is
// true for multi-version built-in types.
return nil
}

if len(hubs) == 1 && len(nonSpokes) == 0 { // convertible
spokeVersions := []string{}
for _, sp := range spokes {
spokeVersions = append(spokeVersions, sp.GetObjectKind().GroupVersionKind().String())
}
log.V(1).Info("conversion enabled for kind", "kind",
gvks[0].GroupKind(), "hub", hubs[0], "spokes", spokeVersions)
return nil
}

return PartialImplementationError{
hubs: hubs,
nonSpokes: nonSpokes,
spokes: spokes,
}
}

// PartialImplementationError represents an error due to partial conversion
// implementation such as hub without spokes, multiple hubs or spokes without hub.
type PartialImplementationError struct {
gvk schema.GroupVersionKind
hubs []runtime.Object
nonSpokes []runtime.Object
spokes []runtime.Object
}

func (e PartialImplementationError) Error() string {
if len(e.hubs) == 0 {
return fmt.Sprintf("no hub defined for gvk %s", e.gvk)
}
if len(e.hubs) > 1 {
return fmt.Sprintf("multiple(%d) hubs defined for group-kind '%s' ",
len(e.hubs), e.gvk.GroupKind())
}
if len(e.nonSpokes) > 0 {
return fmt.Sprintf("%d inconvertible types detected for group-kind '%s'",
len(e.nonSpokes), e.gvk.GroupKind())
}
return ""
}

// isHub determines if passed-in object is a Hub or not.
func isHub(obj runtime.Object) bool {
_, yes := obj.(conversion.Hub)
Expand Down
36 changes: 36 additions & 0 deletions pkg/webhook/conversion/conversion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,3 +200,39 @@ var _ = Describe("Conversion Webhook", func() {
})

})

var _ = Describe("Convertibility Check", func() {

var scheme *runtime.Scheme

BeforeEach(func() {

scheme = kscheme.Scheme
Expect(jobsapis.AddToScheme(scheme)).To(Succeed())

})

It("should not return error for convertible types", func() {
obj := &jobsv2.ExternalJob{
TypeMeta: metav1.TypeMeta{
Kind: "ExternalJob",
APIVersion: "jobs.example.org/v2",
},
}

err := CheckConvertibility(scheme, obj)
Expect(err).NotTo(HaveOccurred())
})

It("should not return error for a built-in multi-version type", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably have a test for the error case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should. It requires adding new type definitions (with partial conversion implementation), I will do that in a follow up PR.

obj := &appsv1beta1.Deployment{
TypeMeta: metav1.TypeMeta{
Kind: "Deployment",
APIVersion: "apps/v1beta1",
},
}

err := CheckConvertibility(scheme, obj)
Expect(err).NotTo(HaveOccurred())
})
})