Skip to content

Reconciliation of MutatingWebhookConfiguration values didnt happen #682

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

Closed
Danil-Grigorev wants to merge 4 commits into from
Closed

Reconciliation of MutatingWebhookConfiguration values didnt happen #682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
56c279c
Refactor applyUnstructured and add/refactor tests
Sep 8, 2020
0326bca
Enforce values for MachineWebhooks, which should be strictly set by o…
Sep 23, 2020
7f73f90
Apply webhook after generation change
Sep 24, 2020
455cee3
Make webhook apply typed and add tests
Sep 24, 2020
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
16 changes: 14 additions & 2 deletions pkg/apis/machine/v1beta1/machine_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,11 @@ const (
var (
// webhookFailurePolicy is ignore so we don't want to block machine lifecycle on the webhook operational aspects.
// This would be particularly problematic for chicken egg issues when bootstrapping a cluster.
webhookFailurePolicy = admissionregistrationv1.Ignore
webhookSideEffects = admissionregistrationv1.SideEffectClassNone
webhookFailurePolicy = admissionregistrationv1.Ignore
webhookSideEffects = admissionregistrationv1.SideEffectClassNone
webhookMatchPolicy = admissionregistrationv1.Equivalent
webhookScope = admissionregistrationv1.NamespacedScope
webhookReinvocationPolicy = admissionregistrationv1.IfNeededReinvocationPolicy
Copy link
Contributor

@JoelSpeed JoelSpeed Sep 23, 2020

Choose a reason for hiding this comment

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

Can you highlight which of these are defaults? With a comment, eg is IfNeeded the default reinvocation policy? If not, why have we chose it

Copy link
Author

@Danil-Grigorev Danil-Grigorev Sep 24, 2020

Choose a reason for hiding this comment

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

It is not a default one, but I set it explicitly as someone could try to negate our changes from mutation with his own webhook. This way our webhook will be invoked for each additional call.

)

func getInfra() (*osconfigv1.Infrastructure, error) {
Expand Down Expand Up @@ -301,6 +304,7 @@ func MachineValidatingWebhook() admissionregistrationv1.ValidatingWebhook {
APIGroups: []string{machine.GroupName},
APIVersions: []string{SchemeGroupVersion.Version},
Resources: []string{"machines"},
Scope: &webhookScope,
},
Operations: []admissionregistrationv1.OperationType{
admissionregistrationv1.Create,
Expand All @@ -324,6 +328,7 @@ func MachineSetValidatingWebhook() admissionregistrationv1.ValidatingWebhook {
Name: "validation.machineset.machine.openshift.io",
FailurePolicy: &webhookFailurePolicy,
SideEffects: &webhookSideEffects,
MatchPolicy: &webhookMatchPolicy,
ClientConfig: admissionregistrationv1.WebhookClientConfig{
Service: &machinesetServiceReference,
},
Expand All @@ -333,6 +338,7 @@ func MachineSetValidatingWebhook() admissionregistrationv1.ValidatingWebhook {
APIGroups: []string{machine.GroupName},
APIVersions: []string{SchemeGroupVersion.Version},
Resources: []string{"machinesets"},
Scope: &webhookScope,
},
Operations: []admissionregistrationv1.OperationType{
admissionregistrationv1.Create,
Expand Down Expand Up @@ -377,6 +383,8 @@ func MachineMutatingWebhook() admissionregistrationv1.MutatingWebhook {
Name: "default.machine.machine.openshift.io",
FailurePolicy: &webhookFailurePolicy,
SideEffects: &webhookSideEffects,
MatchPolicy: &webhookMatchPolicy,
ReinvocationPolicy: &webhookReinvocationPolicy,
ClientConfig: admissionregistrationv1.WebhookClientConfig{
Service: &machineServiceReference,
},
Expand All @@ -386,6 +394,7 @@ func MachineMutatingWebhook() admissionregistrationv1.MutatingWebhook {
APIGroups: []string{machine.GroupName},
APIVersions: []string{SchemeGroupVersion.Version},
Resources: []string{"machines"},
Scope: &webhookScope,
},
Operations: []admissionregistrationv1.OperationType{
admissionregistrationv1.Create,
Expand All @@ -408,6 +417,8 @@ func MachineSetMutatingWebhook() admissionregistrationv1.MutatingWebhook {
Name: "default.machineset.machine.openshift.io",
FailurePolicy: &webhookFailurePolicy,
SideEffects: &webhookSideEffects,
MatchPolicy: &webhookMatchPolicy,
ReinvocationPolicy: &webhookReinvocationPolicy,
ClientConfig: admissionregistrationv1.WebhookClientConfig{
Service: &machineSetServiceReference,
},
Expand All @@ -417,6 +428,7 @@ func MachineSetMutatingWebhook() admissionregistrationv1.MutatingWebhook {
APIGroups: []string{machine.GroupName},
APIVersions: []string{SchemeGroupVersion.Version},
Resources: []string{"machinesets"},
Scope: &webhookScope,
},
Operations: []admissionregistrationv1.OperationType{
admissionregistrationv1.Create,
Expand Down
2 changes: 1 addition & 1 deletion pkg/operator/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ func isMachineWebhook(obj interface{}) bool {
return mutatingWebhook.Name == "machine-api"
}

validatingWebhook, ok := obj.(*admissionregistrationv1.MutatingWebhookConfiguration)
validatingWebhook, ok := obj.(*admissionregistrationv1.ValidatingWebhookConfiguration)
if ok {
return validatingWebhook.Name == "machine-api"
}
Expand Down
42 changes: 42 additions & 0 deletions pkg/operator/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package operator

import (
"path/filepath"
"testing"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/envtest"
"sigs.k8s.io/controller-runtime/pkg/envtest/printer"
)

var (
cfg *rest.Config
testEnv *envtest.Environment
)

func TestMachinesetController(t *testing.T) {
RegisterFailHandler(Fail)
Copy link
Contributor

@JoelSpeed JoelSpeed Sep 24, 2020

Choose a reason for hiding this comment

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

If we're using klog at all within this code, let's set the output

Suggested change
RegisterFailHandler(Fail)
RegisterFailHandler(Fail)
klog.SetOutput(GinkgoWriter)


RunSpecsWithDefaultAndCustomReporters(t,
"Operator Suite",
[]Reporter{printer.NewlineReporter{}})
}

var _ = BeforeSuite(func() {
By("bootstrapping test environment")
testEnv = &envtest.Environment{
CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "install")},
}

var err error
cfg, err = testEnv.Start()
Expect(err).ToNot(HaveOccurred())
Expect(cfg).ToNot(BeNil())
})

var _ = AfterSuite(func() {
By("tearing down the test environment")
Expect(testEnv.Stop()).To(Succeed())
})
4 changes: 2 additions & 2 deletions pkg/operator/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ func (optr *Operator) syncWebhookConfiguration() error {
func (optr *Operator) syncValidatingWebhook() error {
expected := mapiv1.NewValidatingWebhookConfiguration()
if webhook, updated, err := applyValidatingWebhookConfiguration(
optr.dynamicClient,
optr.kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations(),
events.NewLoggingEventRecorder(optr.name),
expected,
expectedValidatingWebhooksConfiguration(expected.Name, optr.generations),
Expand All @@ -184,7 +184,7 @@ func (optr *Operator) syncValidatingWebhook() error {
func (optr *Operator) syncMutatingWebhook() error {
expected := mapiv1.NewMutatingWebhookConfiguration()
if webhook, updated, err := applyMutatingWebhookConfiguration(
optr.dynamicClient,
optr.kubeClient.AdmissionregistrationV1().MutatingWebhookConfigurations(),
events.NewLoggingEventRecorder(optr.name),
expected,
expectedMutatingWebhooksConfiguration(expected.Name, optr.generations),
Expand Down
147 changes: 146 additions & 1 deletion pkg/operator/sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package operator

import (
"context"
"errors"
"fmt"
"testing"
"time"
Expand Down Expand Up @@ -203,6 +204,18 @@ func TestSyncValidatingWebhooks(t *testing.T) {
},
shouldSync: true,
},
{
testCase: "It should update webhookConfiguration if its some webhook has a change in a field populated by defaults",
exisingWebhook: func() *unstructured.Unstructured {
webhook := defaultConfiguration.DeepCopy()
// Set a non default policy
policy := admissionregistrationv1.Exact
webhook.Webhooks[0].MatchPolicy = &policy
exisingWebhook, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(webhook)
return &unstructured.Unstructured{Object: exisingWebhook}
},
shouldSync: true,
},
{
testCase: "It shoud update webhookConfiguration if some webhooks are removed from the list",
exisingWebhook: func() *unstructured.Unstructured {
Expand Down Expand Up @@ -380,7 +393,7 @@ func testSyncWebhookConfiguration(
clientIn := fakedynamic.NewSimpleDynamicClient(scheme.Scheme, []runtime.Object{}...)
optr.dynamicClient = clientIn

serverError := fmt.Errorf("Server error")
serverError := errors.New("Server error")
addReactor := func(operation string) {
reactor := func(action clientTesting.Action) (bool, runtime.Object, error) {
return true, &admissionregistrationv1.ValidatingWebhookConfiguration{}, serverError
Expand Down Expand Up @@ -466,6 +479,138 @@ func testSyncWebhookConfiguration(
}
}

// func TestApplyUnstructured(t *testing.T) {
// testResource := admissionregistrationv1.SchemeGroupVersion.WithResource("mutatingwebhookconfigurations")

// cases := []struct {
// name string
// path string
// existing runtime.Object
// object func() *unstructured.Unstructured
// expectModified bool
// serverError string
// err error
// }{
// {
// name: "Successfully create an object",
// path: "webhooks",
// object: func() *unstructured.Unstructured {
// obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(mapiv1.NewMutatingWebhookConfiguration())
// if err != nil {
// t.Fatalf("Failed to covnert resource to unstructured: %v", err)
// }
// return &unstructured.Unstructured{Object: obj}
// },
// expectModified: true,
// },
// {
// name: "Successfully update an object",
// path: "webhooks",
// object: func() *unstructured.Unstructured {
// resource := mapiv1.NewMutatingWebhookConfiguration()
// resource.Webhooks = append(resource.Webhooks, resource.Webhooks[0])
// obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource)
// if err != nil {
// t.Fatalf("Failed to covnert resource to unstructured: %v", err)
// }
// return &unstructured.Unstructured{Object: obj}
// },
// existing: mapiv1.NewMutatingWebhookConfiguration(),
// expectModified: true,
// },
// {
// name: "Fail to apply a nil object",
// object: func() *unstructured.Unstructured { return nil },
// err: errors.New("Unexpected nil instead of an object"),
// },
// {
// name: "Fail to apply an empty object",
// path: "webhooks",
// object: func() *unstructured.Unstructured { return &unstructured.Unstructured{} },
// err: errors.New("Object does not contain the specified path: webhooks"),
// },
// {
// name: "Fail to create an object in case of a server error on 'get' requests",
// path: "webhooks",
// object: func() *unstructured.Unstructured {
// obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(mapiv1.NewMutatingWebhookConfiguration())
// if err != nil {
// t.Fatalf("Failed to covnert resource to unstructured: %v", err)
// }
// return &unstructured.Unstructured{Object: obj}
// },
// serverError: "get",
// err: errors.New("Custom server error"),
// },
// {
// name: "Fail to create an object in case of a server error on 'create' requests",
// path: "webhooks",
// object: func() *unstructured.Unstructured {
// obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(mapiv1.NewMutatingWebhookConfiguration())
// if err != nil {
// t.Fatalf("Failed to covnert resource to unstructured: %v", err)
// }
// return &unstructured.Unstructured{Object: obj}
// },
// serverError: "create",
// err: errors.New("Custom server error"),
// },
// {
// name: "Fail to update an object in case of a server error on 'create' requests",
// path: "webhooks",
// object: func() *unstructured.Unstructured {
// resource := mapiv1.NewMutatingWebhookConfiguration()
// resource.Webhooks = append(resource.Webhooks, resource.Webhooks[0])
// obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource)
// if err != nil {
// t.Fatalf("Failed to covnert resource to unstructured: %v", err)
// }
// return &unstructured.Unstructured{Object: obj}
// },
// existing: mapiv1.NewMutatingWebhookConfiguration(),
// serverError: "update",
// err: errors.New("Custom server error"),
// },
// }

// for _, test := range cases {
// t.Run(test.name, func(t *testing.T) {
// var objects []runtime.Object
// if test.existing != nil {
// objects = append(objects, test.existing)
// }
// clientIn := fakedynamic.NewSimpleDynamicClient(scheme.Scheme, objects...)
// client := clientIn.Resource(testResource)
// if test.serverError != "" {
// reactor := func(action clientTesting.Action) (bool, runtime.Object, error) {
// return true, nil, test.err
// }
// clientIn.PrependReactor(test.serverError, "*", reactor)
// }

// applied, modified, err := applyUnstructured(client, test.path, eventstesting.NewTestingEventRecorder(t), test.object(), 0)
// if test.err != nil {
// if err == nil || test.err.Error() != err.Error() {
// t.Fatalf("Expected error to be equal %v, got %v", test.err, err)
// }
// return
// }

// if test.expectModified != modified {
// t.Errorf("Expected modified to be %v, got %v", test.expectModified, modified)
// return
// }

// expectedChange := test.err == nil
// if expectedChange && equality.Semantic.DeepEqual(applied, test.object) {
// t.Error("Expected object to be updated")
// } else if !expectedChange && !equality.Semantic.DeepEqual(applied, test.object) {
// t.Error("Expected object to not be updated")
// }
// })
// }
// }

func Test_ensureDependecyAnnotations(t *testing.T) {
cases := []struct {
name string
Expand Down
Loading