Skip to content

Bug 1870158: Use mergo to merge webhooks instead of custom apply #707

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

Merged
openshift-merge-robot merged 1 commit into from
Sep 26, 2020
Merged

Bug 1870158: Use mergo to merge webhooks instead of custom apply #707

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
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ go 1.13

require (
github.com/blang/semver v3.5.1+incompatible
github.com/evanphx/json-patch v4.9.0+incompatible
github.com/go-logr/logr v0.2.1 // indirect
github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b
github.com/google/gofuzz v1.1.0
github.com/google/uuid v1.1.1
github.com/imdario/mergo v0.3.9
github.com/onsi/ginkgo v1.12.1
github.com/onsi/gomega v1.10.1
github.com/openshift/api v0.0.0-20200901182017-7ac89ba6b971
Expand Down
123 changes: 105 additions & 18 deletions pkg/operator/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package operator

import (
"context"
"fmt"
"time"

"github.com/golang/glog"
"github.com/imdario/mergo"
"github.com/openshift/library-go/pkg/operator/events"
"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
"github.com/openshift/library-go/pkg/operator/resource/resourcehash"
Expand All @@ -13,6 +15,7 @@ import (
machinecontroller "github.com/openshift/machine-api-operator/pkg/controller/machine"
"github.com/openshift/machine-api-operator/pkg/metrics"
"github.com/openshift/machine-api-operator/pkg/util/conditions"
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -166,37 +169,121 @@ func (optr *Operator) syncWebhookConfiguration() error {
}

func (optr *Operator) syncValidatingWebhook() error {
client := optr.kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations()
ctx := context.TODO()
expected := mapiv1.NewValidatingWebhookConfiguration()
if webhook, updated, err := applyValidatingWebhookConfiguration(
optr.dynamicClient,
events.NewLoggingEventRecorder(optr.name),
expected,
expectedValidatingWebhooksConfiguration(expected.Name, optr.generations),
); err != nil {
return err
} else if updated {
setValidatingWebhooksConfigurationGeneration(&optr.generations, webhook)

current, err := client.Get(ctx, expected.Name, metav1.GetOptions{})
if apierrors.IsNotFound(err) {
// Doesn't exist yet so create a fresh configuration
if _, err := client.Create(ctx, expected, metav1.CreateOptions{}); err != nil {
return fmt.Errorf("error creating ValidatingWebhookConfiguration: %v", err)
}
return nil
} else if err != nil {
return fmt.Errorf("error getting ValidatingWebhookConfiguration: %v", err)
}

// The webhook already exists, so merge the existing fields with the desired fields
if err := mergo.Merge(expected, current); err != nil {

@Danil-Grigorev Danil-Grigorev Sep 22, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is it going to preserve custom labels we don't set on from the current resource? Are we ok with that? cc @michaelgugino

@JoelSpeed JoelSpeed Sep 23, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes it will, custom labels and annotations or even finalizers should someone wish to add one, will be preserved. I think this is desirable. We should try to be nice and allow users to modify things if they so desire, just as long as it doesn't affect the operation of our webhooks. Which in this case, I don't think it will.

return fmt.Errorf("error merging ValidatingWebhookConfiguration: %v", err)
}
// Merge webhooks separately as slices are normally overwritten by mergo and
// we need to preserve the defaults that are set within the webhooks
expected.Webhooks, err = mergeValidatingWebhooks(expected.Webhooks, current.Webhooks)
if err != nil {
return fmt.Errorf("error merging ValidatingWebhooks: %v", err)
}

if _, err := client.Update(ctx, expected, metav1.UpdateOptions{}); err != nil {
return fmt.Errorf("error updating ValidatingWebhookConfiguration: %v", err)
}

return nil
}

// mergeValidatingWebhooks merges the two sets of webhooks so that any defaulted or additional fields (eg CABundle)
// are preserved from the current set of webhooks.
// In any case where fields in the webhooks differ, expected takes precedence.
// Webhooks are merged using Name as the key, if any webhook is present in current but not expected, it is dropped.
func mergeValidatingWebhooks(expected, current []admissionregistrationv1.ValidatingWebhook) ([]admissionregistrationv1.ValidatingWebhook, error) {
currentSet := make(map[string]admissionregistrationv1.ValidatingWebhook)
for _, webhook := range current {
currentSet[webhook.Name] = webhook
}

out := []admissionregistrationv1.ValidatingWebhook{}
for _, expectedWebhook := range expected {
// If a current webhook exists with the same name, merge it with the expected one
if currentWebhook, found := currentSet[expectedWebhook.Name]; found {
if err := mergo.Merge(&expectedWebhook, currentWebhook); err != nil {
return nil, fmt.Errorf("error merging webhook %q: %v", expectedWebhook.Name, err)
}
}
out = append(out, expectedWebhook)
}

return out, nil
}

func (optr *Operator) syncMutatingWebhook() error {
client := optr.kubeClient.AdmissionregistrationV1().MutatingWebhookConfigurations()
ctx := context.TODO()
expected := mapiv1.NewMutatingWebhookConfiguration()
if webhook, updated, err := applyMutatingWebhookConfiguration(
optr.dynamicClient,
events.NewLoggingEventRecorder(optr.name),
expected,
expectedMutatingWebhooksConfiguration(expected.Name, optr.generations),
); err != nil {
return err
} else if updated {
setMutatingWebhooksConfigurationGeneration(&optr.generations, webhook)

current, err := client.Get(ctx, expected.Name, metav1.GetOptions{})
if apierrors.IsNotFound(err) {
// Doesn't exist yet so create a fresh configuration
if _, err := client.Create(ctx, expected, metav1.CreateOptions{}); err != nil {
return fmt.Errorf("error creating MutatingWebhookConfiguration: %v", err)
}
return nil
} else if err != nil {
return fmt.Errorf("error getting MutatingWebhookConfiguration: %v", err)
}

// The webhook already exists, so merge the existing fields with the desired fields
if err := mergo.Merge(expected, current); err != nil {
return fmt.Errorf("error merging MutatingWebhookConfiguration: %v", err)
}
// Merge webhooks separately as slices are normally overwritten by mergo and
// we need to preserve the defaults that are set within the webhooks
expected.Webhooks, err = mergeMutatingWebhooks(expected.Webhooks, current.Webhooks)
if err != nil {
return fmt.Errorf("error merging MutatingWebhooks: %v", err)
}

if _, err := client.Update(ctx, expected, metav1.UpdateOptions{}); err != nil {
return fmt.Errorf("error updating MutatingWebhookConfiguration: %v", err)
}

return nil
}

// mergeMutatingWebhooks merges the two sets of webhooks so that any defaulted or additional fields (eg CABundle)
// are preserved from the current set of webhooks.
// In any case where fields in the webhooks differ, expected takes precedence.
// Webhooks are merged using Name as the key, if any webhook is present in current but not expected, it is dropped.
func mergeMutatingWebhooks(expected, current []admissionregistrationv1.MutatingWebhook) ([]admissionregistrationv1.MutatingWebhook, error) {
currentSet := make(map[string]admissionregistrationv1.MutatingWebhook)
for _, webhook := range current {
currentSet[webhook.Name] = webhook
}

out := []admissionregistrationv1.MutatingWebhook{}
for _, expectedWebhook := range expected {
// If a current webhook exists with the same name, merge it with the expected one
if currentWebhook, found := currentSet[expectedWebhook.Name]; found {
if err := mergo.Merge(&expectedWebhook, currentWebhook); err != nil {
return nil, fmt.Errorf("error merging webhook %q: %v", expectedWebhook.Name, err)
}
}
out = append(out, expectedWebhook)
}

return out, nil
}

func (optr *Operator) syncBaremetalControllers(config *OperatorConfig, configName string) error {
// Stand down if cluster-bare-metal operator has claimed the metal3 deployment
// and if the baremetal clusteroperator exists
Expand Down
Loading