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

🐛 Envtest should enable conversion only for convertible GVKs #1532

Merged
merged 1 commit into from
May 19, 2021
Merged

🐛 Envtest should enable conversion only for convertible GVKs #1532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
105 changes: 101 additions & 4 deletions pkg/envtest/crd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,31 @@ import (
"k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/wait"
k8syaml "k8s.io/apimachinery/pkg/util/yaml"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"k8s.io/client-go/util/retry"
"k8s.io/utils/pointer"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/webhook/conversion"
"sigs.k8s.io/yaml"
)

// CRDInstallOptions are the options for installing CRDs
type CRDInstallOptions struct {
// Scheme is used to determine if conversion webhooks should be enabled
// for a particular CRD / object.
//
// Conversion webhooks are going to be enabled if an object in the scheme
// implements Hub and Spoke conversions.
//
// If nil, scheme.Scheme is used.
Scheme *runtime.Scheme

// Paths is a list of paths to the directories or files containing CRDs
Paths []string

Expand Down Expand Up @@ -86,7 +98,7 @@ func InstallCRDs(config *rest.Config, options CRDInstallOptions) ([]client.Objec
return nil, err
}

if err := modifyConversionWebhooks(options.CRDs, options.WebhookOptions); err != nil {
if err := modifyConversionWebhooks(options.CRDs, options.Scheme, options.WebhookOptions); err != nil {
return nil, err
}

Expand Down Expand Up @@ -118,6 +130,9 @@ func readCRDFiles(options *CRDInstallOptions) error {

// defaultCRDOptions sets the default values for CRDs
func defaultCRDOptions(o *CRDInstallOptions) {
if o.Scheme == nil {
o.Scheme = scheme.Scheme
}
if o.MaxTime == 0 {
o.MaxTime = defaultMaxWait
}
Expand Down Expand Up @@ -356,11 +371,30 @@ func renderCRDs(options *CRDInstallOptions) ([]client.Object, error) {
return res, nil
}

func modifyConversionWebhooks(crds []client.Object, webhookOptions WebhookInstallOptions) error {
// modifyConversionWebhooks takes all the registered CustomResourceDefinitions and applies modifications
// to conditionally enable webhooks if the type is registered within the scheme.
//
// The complexity of this function is high mostly due to all the edge cases that we need to handle:
// CRDv1beta1, CRDv1, and their unstructured counterpart.
//
// We should be able to simplify this code once we drop support for v1beta1 and standardize around the typed CRDv1 object.
func modifyConversionWebhooks(crds []client.Object, scheme *runtime.Scheme, webhookOptions WebhookInstallOptions) error { //nolint:gocyclo
if len(webhookOptions.LocalServingCAData) == 0 {
return nil
}

// Determine all registered convertible types.
convertibles := map[schema.GroupKind]struct{}{}
for gvk := range scheme.AllKnownTypes() {
obj, err := scheme.New(gvk)
if err != nil {
return err
}
if ok, err := conversion.IsConvertible(scheme, obj); ok && err == nil {
convertibles[gvk.GroupKind()] = struct{}{}
}
}

// generate host port.
hostPort, err := webhookOptions.generateHostPort()
if err != nil {
Expand All @@ -371,10 +405,19 @@ func modifyConversionWebhooks(crds []client.Object, webhookOptions WebhookInstal
for _, crd := range crds {
switch c := crd.(type) {
case *apiextensionsv1beta1.CustomResourceDefinition:
// Continue if we're preserving unknown fields.
//
// preserveUnknownFields defaults to true if `nil` in v1beta1.
if c.Spec.PreserveUnknownFields == nil || *c.Spec.PreserveUnknownFields {
continue
}
// Continue if the GroupKind isn't registered as being convertible.
if _, ok := convertibles[schema.GroupKind{
Group: c.Spec.Group,
Kind: c.Spec.Names.Kind,
}]; !ok {
continue
}
c.Spec.Conversion.Strategy = apiextensionsv1beta1.WebhookConverter
c.Spec.Conversion.WebhookClientConfig.Service = nil
c.Spec.Conversion.WebhookClientConfig = &apiextensionsv1beta1.WebhookClientConfig{
Expand All @@ -383,9 +426,17 @@ func modifyConversionWebhooks(crds []client.Object, webhookOptions WebhookInstal
CABundle: webhookOptions.LocalServingCAData,
}
case *apiextensionsv1.CustomResourceDefinition:
// Continue if we're preserving unknown fields.
if c.Spec.PreserveUnknownFields {
continue
}
// Continue if the GroupKind isn't registered as being convertible.
if _, ok := convertibles[schema.GroupKind{
Group: c.Spec.Group,
Kind: c.Spec.Names.Kind,
}]; !ok {
continue
}
c.Spec.Conversion.Strategy = apiextensionsv1.WebhookConverter
c.Spec.Conversion.Webhook.ClientConfig.Service = nil
c.Spec.Conversion.Webhook.ClientConfig = &apiextensionsv1.WebhookClientConfig{
Expand All @@ -401,8 +452,32 @@ func modifyConversionWebhooks(crds []client.Object, webhookOptions WebhookInstal

switch c.GroupVersionKind().Version {
case "v1beta1":
// Continue if we're preserving unknown fields.
//
// preserveUnknownFields defaults to true if `nil` in v1beta1.
if preserve, found, _ := unstructured.NestedBool(c.Object, "spec", "preserveUnknownFields"); preserve || !found {
if preserve, found, err := unstructured.NestedBool(c.Object, "spec", "preserveUnknownFields"); preserve || !found {
continue
} else if err != nil {
return err
}

// Continue if the GroupKind isn't registered as being convertible.
group, found, err := unstructured.NestedString(c.Object, "spec", "group")
if !found {
continue
} else if err != nil {
return err
}
kind, found, err := unstructured.NestedString(c.Object, "spec", "names", "kind")
if !found {
continue
} else if err != nil {
return err
}
if _, ok := convertibles[schema.GroupKind{
Group: group,
Kind: kind,
}]; !ok {
continue
}

Expand All @@ -428,7 +503,29 @@ func modifyConversionWebhooks(crds []client.Object, webhookOptions WebhookInstal
return err
}
case "v1":
if preserve, _, _ := unstructured.NestedBool(c.Object, "spec", "preserveUnknownFields"); preserve {
if preserve, _, err := unstructured.NestedBool(c.Object, "spec", "preserveUnknownFields"); preserve {
continue
} else if err != nil {
return err
}

// Continue if the GroupKind isn't registered as being convertible.
group, found, err := unstructured.NestedString(c.Object, "spec", "group")
if !found {
continue
} else if err != nil {
return err
}
kind, found, err := unstructured.NestedString(c.Object, "spec", "names", "kind")
if !found {
continue
} else if err != nil {
return err
}
if _, ok := convertibles[schema.GroupKind{
Group: group,
Kind: kind,
}]; !ok {
continue
}

Expand Down
16 changes: 16 additions & 0 deletions pkg/envtest/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (
"strings"
"time"

"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/config"
Expand Down Expand Up @@ -95,6 +97,15 @@ type Environment struct {
// ControlPlane is the ControlPlane including the apiserver and etcd
ControlPlane integration.ControlPlane

// Scheme is used to determine if conversion webhooks should be enabled
// for a particular CRD / object.
//
// Conversion webhooks are going to be enabled if an object in the scheme
// implements Hub and Spoke conversions.
//
// If nil, scheme.Scheme is used.
Scheme *runtime.Scheme

// Config can be used to talk to the apiserver. It's automatically
// populated if not set using the standard controller-runtime config
// loading.
Expand Down Expand Up @@ -263,6 +274,11 @@ func (te *Environment) Start() (*rest.Config, error) {
}
}

// Set the default scheme if nil.
if te.Scheme == nil {
te.Scheme = scheme.Scheme
}

// Call PrepWithoutInstalling to setup certificates first
// and have them available to patch CRD conversion webhook as well.
if err := te.WebhookInstallOptions.PrepWithoutInstalling(); err != nil {
Expand Down