Add Validating/MutatingWebhookConfiguration structured Apply by Danil-Grigorev · Pull Request #836 · openshift/library-go

Danil-Grigorev · 2020-07-14T09:09:12Z

This PR adds Apply method in the library for ValidatingWebhookConfigurations and MutatingWebhookConfigurations, to efficiently update existing resources, and preserve the functionality of service-ca-bundle inject-cabundle annotations.

The functionality relies solely on generation changes done by the server. To track generation changes and prevent unnecessary polling on resource updates, the helper generation caching is introduced. It should be used similarly to:

func (optr *Operator) syncValidatingWebhook() error {
	expectedGeneration := resourcemerge.ExpectedValidatingWebhooksConfiguration(mapiv1.NewValidatingWebhookConfiguration().Name, optr.generations)
	validatingWebhook, updated, err := resourceapply.ApplyValidatingWebhookConfiguration(optr.kubeClient.AdmissionregistrationV1(),
		events.NewLoggingEventRecorder(optr.name),
		mapiv1.NewValidatingWebhookConfiguration(), expectedGeneration)
	if err != nil {
		return err
	}
	if updated {
		resourcemerge.SetValidatingWebhooksConfigurationGeneration(&optr.generations, validatingWebhook)
	}

	return nil
}

This PR is tested with e2e coverage in MAO with openshift/machine-api-operator#642

Fixes #953

Danil-Grigorev · 2020-07-14T09:30:55Z

/retest

mfojtik · 2020-07-14T09:44:03Z

pkg/operator/resource/resourceapply/admissionregistration.go

+		}
+	}
+
+	// Explcitily specify type for requiredUnstr to get object meta accessor


sttts · 2020-07-14T09:52:14Z

pkg/operator/resource/resourceapply/unstructured.go

+// usntructured will be merged with the existing usntructured and an update performed if the
+// usntructured spec and metadata differ from the previously required spec and metadata.
+// For further detail, check the top-level comment.
+func applyUnstructured(resourceClient dynamic.ResourceInterface, path string, recorder events.Recorder,


doesn't this ask for hot loops? Apply in a generic way does not really make sense IMO.

@sttts So you think this needs an option to forceApply, same as it is for Deployments and DaemonSets? Or it should repeat the attempt couple of times before failing?

@Danil-Grigorev Can you please build this into the MAO and try it on a real cluster to see how it behaves?

I don't think it should hot loop, it's based on the ApplyDeployment logic, so if that doesn't hot loop I think this shouldn't? If the metadata and spec that we input doesn't change, then the spec hash annotation shouldn't change and we should short circuit on line 42 before we attempt to update again. @sttts Is there something in this in particular that makes you think it would hot loop?

This func looks generic. But it is wrong in most cases. It does not consider things like list-type markers (compare https://github.com/kubernetes/enhancements/blob/6a1ac61fbfd64d49e13fae3a8d773dd4ebda53c8/keps/sig-api-machinery/0006-apply.md). It might be fine for one specific resource. But having this helper here encourages simple, but wrong code.

Once this PR passes e2e tests, we would have the proof that hot loops do not occur in current implementation: openshift/machine-api-operator#642

@Danil-Grigorev please check #836 (comment). This is a blocker for the PR.

sttts · 2020-07-15T10:07:55Z

pkg/operator/resource/resourceapply/unstructured.go

+
+// This is based on resourcemerge.EnsureObjectMeta but uses the metav1.Object interface instead
+// TODO: Update this to use resourcemerge.EnsureObjectMeta or update resourcemerge.EnsureObjectMeta to use the interface
+func ensureObjectMeta(existing, required metav1.Object) (bool, error) {


what do we do for other resources? I looks wrong to have this here in a generic way, but not use it for other resources as well.

Other resources use

library-go/pkg/operator/resource/resourcemerge/object_merger.go

Lines 10 to 17 in 100bf3f

// EnsureObjectMeta writes namespace, name, labels, and annotations. Don't set other things here.

// TODO finalizer support maybe?

func EnsureObjectMeta(modified *bool, existing *metav1.ObjectMeta, required metav1.ObjectMeta) {

SetStringIfSet(modified, &existing.Namespace, required.Namespace)

SetStringIfSet(modified, &existing.Name, required.Name)

MergeMap(modified, &existing.Labels, required.Labels)

MergeMap(modified, &existing.Annotations, required.Annotations)

}

which does the same thing but doesn't use the metav1.Object interface, even though it could.

We could update the public implementation so we only have one version but that would be a breaking change to the method since it accepts a *metav1.ObjectMeta and a metav1.ObjectMeta, the latter not satisfying the interface

sttts · 2020-07-15T15:03:21Z

Why is this using unstructured after all an not a typed client as we do for all other resources?

/hold

JoelSpeed · 2020-07-16T11:06:26Z

Why is this using unstructured after all an not a typed client as we do for all other resources?

For background on why we are proposing this change:
We have, during the 4.6 cycle, introduced webhooks into the Machine API project. Currently, these are being applied by CVO, with a create only annotation so that the CA Bundle can be injected without conflicting with the CVO.

We would like to be able to reconfigure the MWC/VWC resources in the future so looked at how we could do this. For now, we looked at bringing the management of the MWC/VWC into the MAO with our other resources. Other resources being the Deployment and DaemonSet we manage. They use the library-go resourceapply.ApplyDeployment and resourceapply.ApplyDaemonset helpers to apply so that we can be a bit more clever about the changes.

We then thought it would be good to have the same for MWC and VWC. However, the logic for ApplyDeployment and ApplyDaemonset is pretty much duplicated, the algorithm is the same bar some initial setup and the types that are fed in.
We thought it might be good to make a reusable implementation of the apply algorithm so that we can more easily test the logic, and to make it reusable, it has to be unstructured. The idea being that all of the ApplyMyThing methods could do their initial set up (eg copying the CABundle across if needed for MWC/VWC), then use the shared, underlying Apply algorithm that is unstructured.

The algorithm we have in applyUnstructured matches that in ApplyDeployment and ApplyDaemonset, we hash the spec that we are given, put that in an annotation, check that the metadata hasn't changed (ie the spec hash hasn't changed), check that the generation hasn't been changed since we last updated, and then only update if one of these things has changed., overwriting the entire spec with the given input. We could replace the internal logic of ApplyDeployment and ApplyDaemonset in theory with this implementation without breaking anything.

By copying the CABundles specifically in the MWC/VWC implementation, we can get around the problem that an external party needs to modify an otherwise managed object, but because we do that before the spec hash is calculated, we are safe for future updates.

There is prior art for using unstructured in this package as ApplyServiceMonitor uses unstructured, though they have a slightly different implementation that merges the spec here, perhaps we should look to use that instead? https://github.com/openshift/library-go/blob/master/pkg/operator/resource/resourceapply/monitoring.go#L24-L48

While I agree that ideally we would use server side apply, I didn't think that was ready for general consumption yet? This logic replicates what we have already with other resources that have a spec, if the spec has changed, we overwrite it completely. CVO does this today for many resources already, and this just mimics that, so I don't think we are making things worse really?

Long term, it is my belief that CVO should be able to manage MWC and VWC resources and handle the update to the CABundle (at the moment it uses a generic apply which just replaces the spec verabatim if there's a difference), if there is a better way to approach this problem and solve the issue for the wider Openshift community then I'm happy to help implement it

JoelSpeed · 2020-07-16T11:15:21Z

FWIW, I've manually tested this code which is being used in openshift/machine-api-operator#642 this morning and it is working pretty much as expected, there was one minor discrepancy but that was within the MAO repo and I believe it has now been fixed. I did not observe any hot loops, as I modified the resource spec it got put back and kept the CABundle as desired.

JoelSpeed · 2020-07-16T11:46:23Z

Can't really agree with that, although I see advantages for typed approach. But just a note - current implementation requires to provide a path to resource spec equivalent, which is used to calculate hash. This should be as generic as it could be, with this line in place - https://github.com/openshift/library-go/pull/836/files#diff-61be38f5023c7c6da801d664793070d5R25

I was not on the ball this morning, ignore that previous comment, it was completely wrong 🤦 Too much context switching right now

Danil-Grigorev · 2020-07-16T12:05:18Z

/retest

sttts · 2020-07-16T12:16:28Z

@JoelSpeed I understand your motiviation to add webhook support here. And I agree it makes sense. But for this very resourceapply package it is harmful to have a generic implementation. All generic implementations are wrong if defaulting happens in the apiserver. This even applies to specific implementations if upstream adds defaults and we forget to update our apply funcs. Hence, we have to be super careful with the logic here for each single resource type, otherwise we end up in hot loops.

tl/dr: Not having too many tempting generic helpers is a feature of this package, not code smell.

Maybe we can have some common logic like for ObjectMeta if that makes sense. Am not against that.

JoelSpeed · 2020-07-16T12:56:39Z

All generic implementations are wrong if defaulting happens in the apiserver. This even applies to specific implementations if upstream adds defaults and we forget to update our apply funcs. Hence, we have to be super careful with the logic here for each single resource type, otherwise we end up in hot loops.

Ok, this makes sense. How would you suggest we approach this then? Should we rewrite this to be structured and specific for the MWC/VWC, would specific implementations for these be acceptable for this package? This would then allow us to add CVO support for MWC/VWC, do you think that would be accepted?

Or should we just keep our own implementation and manage the webhooks from MAO long term?

Maybe we can have some common logic like for ObjectMeta if that makes sense. Am not against that.

How do you mean for that? I don't follow.

sttts · 2020-07-16T13:43:37Z

Maybe we can have some common logic like for ObjectMeta if that makes sense. Am not against that.
How do you mean for that? I don't follow.

You had some logic about ObjectMeta which looked generic. If it is and matches what we do everywhere, that could make sense. But this should go to another independent PR.

sttts · 2020-07-16T13:49:28Z

Should we rewrite this to be structured and specific for the MWC/VWC, would specific implementations for these be acceptable for this package?

Yes. Similarly as for the other types. Be careful with the defaults that kube applies. You can find the defaulters here:

https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/admissionregistration/v1/defaults.go

You have to manually replicate that logic. And yes, this is super tedious and error-prone, and for many of the supported types here we did not do that in a complete way, i.e. we lack applying defaults risking hotloops. That's why we must be so careful in this package. Manual apply requires this kind of deep knowledge. This is the reason for server side apply upstream that we can eventually use some day.

JoelSpeed · 2020-07-16T14:21:52Z

Ok, thanks for the guidance @sttts! I will work with @Danil-Grigorev over the next few days to re-approach this

JoelSpeed

Looks good. I am slightly concerned that we aren't accounting for the defaulting as suggested by @sttts, I think if we take the defaulting functions linked below and copy them here, we can run our required object through that before we start comparing it, then we shouldn't end up conflicting and hot looping with the defaulted fields in existing object.

https://github.com/kubernetes/kubernetes/blob/a3e3b355fa30244d0641aec4ce11cb742f2464f2/pkg/apis/admissionregistration/v1/defaults.go#L30-L78

We should look around this package to see how other implementations are handling this

JoelSpeed · 2020-07-17T10:00:42Z

pkg/operator/resource/resourcemerge/admissionregistration.go

+	if !equality.Semantic.DeepEqual(existing.Webhooks, required.Webhooks) {
+		*modified = true
+		existing.Webhooks = required.Webhooks
+	}


Is there any defaulting within the Webhooks that we need to account for here? We could end up in a hot loop if there's server side defaulting that we are missing

JoelSpeed · 2020-07-17T10:00:56Z

pkg/operator/resource/resourcemerge/admissionregistration.go

+	if !equality.Semantic.DeepEqual(existing.Webhooks, required.Webhooks) {
+		*modified = true
+		existing.Webhooks = required.Webhooks
+	}


Is there any defaulting within the Webhooks that we need to account for here? We could end up in a hot loop if there's server side defaulting that we are missing

Having to call defaulting in here is the reason for possible hot loops. If we pull server implementation for webhooks, someone would have to suffer a burden of maintaining it in sync with upstream, or otherwise, any defaulting implementation change would be a 100% cause for hot looping. The location @sttts pointed at, is an internal kubenetes/kubernetes package. If it is not exposed at https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/api/admissionregistration/v1, I don't think it is wise to use it and especially, copying that. I'm not sure that the strict rules for kubernetes api changes are applied there, so the probability of described situation is higher. Let's leave it to the server-side apply, as a future improvement.

Right now I could agree that if you immediately apply the updated resource, returned from ApplyMutatingWebhookConfiguration or ApplyValidatingWebhookConfigurationonce again, it would not be filtered by the logic, and Update call will still happen. Even if the resource is technically the same, from the algorithmic perspective you provided a different input, and the change should occur.

I'm adding the missing comment from resourceapply/apps.go from where the implementation was inspired, so the explanation will be preserved:

library-go/pkg/operator/resource/resourceapply/apps.go

Lines 21 to 51 in 3b4f06f

// The Apply<type> methods in this file ensure that a resource is created or updated to match

// the form provided by the caller.

//

// If the resource does not yet exist, it will be created.

//

// If the resource exists, the metadata of the required resource will be merged with the

// existing resource and an update will be performed if the spec and metadata differ between

// the required and existing resources. To be reliable, the input of the required spec from

// the operator should be stable. It does not need to set all fields, since some fields are

// defaulted server-side. Detection of spec drift from intent by other actors is determined

// by generation, not by spec comparison.

//

// To ensure an update in response to state external to the resource spec, the caller should

// set an annotation representing that external state e.g.

//

// `myoperator.openshift.io/config-resource-version: <resourceVersion>`

//

// An update will be performed if:

//

// - The required resource metadata differs from that of the existing resource.

// - The difference will be detected by comparing the name, namespace, labels and

// annotations of the 2 resources.

//

// - The generation expected by the operator differs from generation of the existing

// resource.

// - This is the likely result of an actor other than the operator updating a resource

// managed by the operator.

//

// - The spec of the required resource differs from the spec of the existing resource.

// - The difference will be detected via metadata comparison since the hash of the

// resource's spec will be set as an annotation prior to comparison.

While I agree with that comment for the deployment implementation, this does not match with the logic that you have in this implementation.

We are checking the Webhooks for equality here, we aren't solely relying on the spec hash. There is no way that the two sets of webhooks will be equal unless the input has already been defaulted, so this will currently cause a hot loop.

We have two options, we copy the logic from the K8s package (definitely do not import it), or we remove the lines I've highlighted here and solely rely on the spec hash annotation for detecting differences.

someone would have to suffer a burden of maintaining it in sync with upstream, or otherwise, any defaulting implementation change would be a 100% cause for hot looping.

This has already been discussed, this is what Stefan was saying about in his previous comments. We have to be careful when adding to this package because if we don't get the defaulting right, we will hot loop. He's already said it's annoying because we have to keep them up to date. See #836 (comment)

Now I see it, thanks. Updated.

jsafrane

I tested ApplyValidatingWebhookConfiguration and it works as expected. I got one Generation bumped by caBundle injector, but after that things are pretty stable.

BTW, most (all?) my comments apply interchangeably both to ApplyMutatingWebhookConfiguration and ApplyValidatingWebhookConfiguration

pkg/operator/resource/resourceapply/admissions.go

jsafrane · 2020-12-01T17:52:26Z

pkg/operator/resource/resourceapply/admissions.go

+		return nil, false, fmt.Errorf("Unexpected nil instead of an object")
+	}
+	required := requiredOriginal.DeepCopy()
+


Please add specHashAnnotation to the object, it will force update the webhook config when a new operator starts with a new version of the config.

See how ApplyDeployment does it. Hash webhooks instead of spec, of course.

This is one thing which I don't think we should do. That mix of hashing and generation is what was causing the necessity of using server-side apply. If you really require to see the full populated spec of the resource, as the server returns it, you need to copy defaulting methods from the server. Otherwise, you have to merge the new spec with the previous one (which we could avoid). In any case, this brings chaos in the approach and make it actually difficult to maintain the state with other operators doing changes on the webhooks with the mentioned annotation.

If you'd like to force redeployment of the resource, the expected generation should be set to 0. This is also replicated on operator startup, as the actual cache for the generation will be empty.

I'll add a test case that will provide an example.

Hash and generation serve different purposes.

Generation (in existing) signalizes that something outside of the operator has modified the WebhookConfiguration and the operator should overwrite these changes.

Hash of required detects changes in required - e.g. when a new operator with say new webhook[].timeout starts, it should update existing WebhookConfiguration. It does not hash existing with defaulted values and caBundle and whatnot! The hash should be very very stable, unless the operator really needs a different required than it provided previously. We do the same for Deployments / DaemonSets and they don't mess up the generation at all.

There are corner cases. Let's say we just created a new webhookConfiguration and didn't use some of the default values which are populated by the server, instead we set the same on the resource ourselves for "better visibility". Then after some time, we remove those - from the server perspective those resources are absolutely equal, so it should not do anything. But it has to apply a new version just because of the hash.

IMO that's expected and OK. Hash is not a perfect solution, it may lead to unnecessary updates. But the update will be just one (to get the hash into annotation), not an endless loop of updates.

This just seems like an unnecessary complication. Generation covers this use-case too, and we won't have to copy fields with changed certificates from https://github.com/openshift/service-ca-operator to calculate the hash correctly on each reconcile. The code is less error-prone IMO when we rely on a single source of truth, and generation provides a stable one.

No, Generation does not cover this use case at all. And these are not a corner case either.

Say, you have operator v4.7 that deploys ValidatingWebhookConfiguration with 1 webhook. It gets applied and gets Generation: 2 and the operator gets cr.Status.Generations[..].LastGeneration: 2.

In operator v4.8, you add a new webhook, so the array has now 2 items.

cr.Status has still Generations[..].LastGeneration: 2

ValidatingWebhookConfiguration has Generation: 2, which matches the expected one.

-> The current code won't update the object at all! There will be still only one webhook[]. It will introduce hard to debug issues (trust me, been there...). Ad that's why we added hash of spec to Deployment+DaemonSet and we should add them to *WebhookConfiguration too.

In this case, this is managed on a higher level. There is surely at least one restart for the operator, so the leader for handling this state will change. The approach is different from the typical one, but it is still the same from a functional perspective. We just store this "hash" approximation in the controller cache, which makes more sense, as the operator's responsibility is to apply his version of the resource at least once, but not more than that. In the case of the stored hash on the resource, it is worse - the new operator may not ever put his version of resource even on startup if someone would sneak in the correct spec hash, but pass a different spec content while the operator was down/upgrading etc..

as the operator's responsibility is to apply his version of the resource at least once,

Why an operator must apply it at least once? Apply*WebhookConfig can see that everything is fine (using this hash) and nothing is needed.

Anyway, I can apply the has in the operator, but I'd bet some users of Apply* will get it wrong.

pkg/operator/resource/resourceapply/admissions.go

jsafrane · 2020-12-01T18:00:20Z

pkg/operator/resource/resourceapply/generic.go

 				result.Error = fmt.Errorf("missing kubeClient")
 			}
-			result.Result, result.Changed, result.Error = ApplyValidatingWebhookConfiguration(clients.kubeClient.AdmissionregistrationV1(), recorder, t)
+			result.Result, result.Changed, result.Error = ApplyValidatingWebhookConfiguration(clients.kubeClient.AdmissionregistrationV1(), recorder, t, 0)


Since they need expectedGeneration now, should we remove them from ApplyDirectly? It will keep overwriting the existing object and that's not the behavior we want to support.

This behavior is equal to other ApplyDirectly methods

library-go/pkg/operator/resource/resourceapply/storage.go

Line 90 in c8d0c55

func ApplyCSIDriver(client storageclientv1.CSIDriversGetter, recorder events.Recorder, required *storagev1.CSIDriver) (*storagev1.CSIDriver, bool, error) {

I added a test case which shows how it will work.

CSIDriver does not have Generation and can't be (easily) compared, direct apply is the only way. Anyway, I am not going to use ApplyDirectly / StaticController, so feel free to ignore this comment.

sttts · 2020-12-02T08:46:02Z

pkg/operator/resource/resourceapply/admissions.go

+	// and ignore clientConfig.caBundle changes on "inject-cabundle" label
+	if required.GetAnnotations() != nil && required.GetAnnotations()[genericCABundleInjectorAnnotation] != "" {
+		copyMutatingWebhookCABundle(existing, required)
+	}


can't we check in requiredOriginal whether the CA is set and keep the one on the server if that's not the case? I don't like a dependency on service-ca here. It can be other actors setting the CA.

I changed it and then checked the operator behavior... It forces the caBundle, so will revert the change: https://github.com/openshift/service-ca-operator/blob/master/pkg/controller/cabundleinjector/mutatingwebhook.go#L48

sttts · 2020-12-03T11:45:11Z

pkg/operator/resource/resourceapply/admissions.go

+// and an update performed if the validatingwebhookconfiguration spec and metadata differ from
+// the previously required spec and metadata based on generation change.
+//
+// expectedGeneration set to 0 disables generation tracking, forcing redeployment logic.


what is "forcing redeployment logic" ?

This signature breaks every other user of the func. Do we want that? ApplyValidatingWebhookConfigurationWithGeneration would avoid that.

Let's break consumers IMO. According to @jsafrane there is no other consumer so.

This is used in this PR in ApplyDirectly. Maybe it should not be here in the first place, I'm not sure about a use-case either. But this comment is nothing new, just highlighting the side-effect of the chosen algorithm on Deployment/DaemonSet apply logic too. For some reason, they use forceRollout, but it is not needed -

library-go/pkg/operator/resource/resourceapply/apps.go

Line 107 in c8d0c55

forceRollout bool) (*appsv1.Deployment, bool, error) {

sttts · 2020-12-03T11:46:22Z

pkg/operator/resource/resourceapply/admissions.go

+	toWrite.Webhooks = required.Webhooks

-	actual, err := client.MutatingWebhookConfigurations().Update(context.TODO(), required, metav1.UpdateOptions{})
+	klog.V(4).Infof("ValidatingWebhookConfiguration %q changes: %v", required.GetNamespace()+"/"+required.GetName(), JSONPatchNoError(existing, toWrite))


there was a reason for if klog.V(4).Enabled() {

It should be the same, according to docs:

// Thus, one may write either // if glog.V(2).Enabled() { klog.Info("log this") } // or // klog.V(2).Info("log this") // The second form is shorter but the first is cheaper if logging is off because it does // not evaluate its arguments.

sttts · 2020-12-03T11:46:52Z

pkg/operator/resource/resourceapply/admissions.go

+}
+
+// copyValidatingWebhookCABundle populates webhooks[].clientConfig.caBundle fields from existing resource
+func copyValidatingWebhookCABundle(existing, required *admissionregistrationv1.ValidatingWebhookConfiguration) {


rename params to from, to

sttts · 2020-12-03T11:53:19Z

pkg/operator/resource/resourceapply/admissions.go

+	toWrite := existingCopy // shallow copy so the code reads easier
+	// Providing upgrade compatibility with service-ca-bundle operator
+	// and ignore clientConfig.caBundle changes on "inject-cabundle" label
+	if required.GetAnnotations() != nil && required.GetAnnotations()[genericCABundleInjectorAnnotation] != "" {


I had a previous comment about this which seem to be gone: why don't we check for a CA cert in required, and keep the existing one if there isn't one enforced. I don't like this dependency on service-ca here. There might be different actors creating CA bundles.

Sorry, I hoped it was unresolved. If we do that and the operator just does its job, then we will get to a hot loop situation with such an approach - https://github.com/openshift/library-go/pull/836/files#r533587142 as it enforces its certificate over the one present.

Matching existing implementation for apps.Deployment, the generation management is extended with support for MutatingWebhookConfigurations and ValidatingWebhookConfiguration - Implementation relies on usage of copyMutatingWebhookCABundle and copyValidatingWebhookCABundle for compatibility with service-ca-operator and "inject-cabundle" annotation - ApplyValidatingWebhooksConfiguration and ApplyMutatingWebhooksConfiguration tracks generation changes between resources, reducing the number of unwanted updates, and returns updated: true only when the Update call increased resource generation - meaning the change was registered.

Danil-Grigorev · 2020-12-03T12:40:20Z

@sttts @jsafrane All updated, only squashed the commits, renamed admissions.go to admissionregistration.go, and added a small fix to an older comment of yours. PTAL and thanks for all your help reviewing this!

Danil-Grigorev · 2020-12-03T14:22:53Z

/retest

sttts · 2020-12-04T13:11:03Z

pkg/operator/resource/resourceapply/admissionregistration.go

+		existingMutatingWebhooks[webhook.Name] = webhook
+	}
+
+	webhooks := make([]admissionregistrationv1.MutatingWebhook, len(to.Webhooks))


why a new map? Just change the existing one.

for i, wh := range to.Webhooks { if existing, ok := fromMap[wh.Name]; ok && wh.ClientConfig.CABundle == nil { to.Webhooks[i].ClientConfig.CABundle = existing.ClientConfig.CABundle } }

sttts · 2020-12-04T13:17:12Z

Up to #836 (comment) lgtm.

@jsafrane can you do a final review? I will approve when lgtm'ed.

…nly if is not set

jsafrane · 2020-12-07T10:55:45Z

/lgtm

sttts · 2020-12-07T14:47:32Z

/approve

openshift-ci-robot · 2020-12-07T14:47:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Danil-Grigorev, jsafrane, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot requested review from deads2k and sttts July 14, 2020 09:09

mfojtik reviewed Jul 14, 2020

View reviewed changes

sttts reviewed Jul 14, 2020

View reviewed changes

Danil-Grigorev force-pushed the mwc-mvc-apply branch from 0efbe2b to 7e2669b Compare July 14, 2020 10:38

Danil-Grigorev requested review from mfojtik and sttts July 14, 2020 11:11

Danil-Grigorev force-pushed the mwc-mvc-apply branch 2 times, most recently from cc20589 to a9808ab Compare July 15, 2020 08:19

sttts reviewed Jul 15, 2020

View reviewed changes

Danil-Grigorev mentioned this pull request Jul 15, 2020

[OCPCLOUD-1024]: Refactor webhook syncronization to work with library-go implementation openshift/machine-api-operator#642

Merged

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2020

Danil-Grigorev force-pushed the mwc-mvc-apply branch 2 times, most recently from 17ea56f to a9808ab Compare July 16, 2020 10:32

Danil-Grigorev force-pushed the mwc-mvc-apply branch from a9808ab to 0f3ca2b Compare July 16, 2020 19:07

Danil-Grigorev changed the title ~~Add Validating/MutatingWebhookConfiguration Apply based on Unstructured~~ Add Validating/MutatingWebhookConfiguration structured Apply Jul 16, 2020

Danil-Grigorev requested review from JoelSpeed and sttts July 16, 2020 19:08

JoelSpeed reviewed Jul 17, 2020

View reviewed changes

Danil-Grigorev requested review from JoelSpeed and sttts December 1, 2020 14:51

Danil-Grigorev mentioned this pull request Dec 1, 2020

Reconciliation of MutatingWebhookConfiguration values didnt happen openshift/machine-api-operator#682

Closed

jsafrane reviewed Dec 1, 2020

View reviewed changes

sttts reviewed Dec 2, 2020

View reviewed changes

Danil-Grigorev requested review from jsafrane and sttts December 2, 2020 12:28

Danil-Grigorev force-pushed the mwc-mvc-apply branch from 03f39c4 to e94d5be Compare December 2, 2020 14:07

sttts reviewed Dec 3, 2020

View reviewed changes

Danil-Grigorev force-pushed the mwc-mvc-apply branch from e94d5be to 606abe9 Compare December 3, 2020 12:22

Danil-Grigorev requested a review from sttts December 3, 2020 12:24

Danil-Grigorev force-pushed the mwc-mvc-apply branch from 606abe9 to 352bc2d Compare December 3, 2020 12:30

sttts reviewed Dec 4, 2020

View reviewed changes

Remove dependency on inject-ca-bundle annotation, and copy caBundle o…

13d8844

…nly if is not set

Danil-Grigorev force-pushed the mwc-mvc-apply branch from a9ca3b2 to 13d8844 Compare December 4, 2020 16:01

Danil-Grigorev requested a review from sttts December 7, 2020 10:49

openshift-ci-robot assigned jsafrane Dec 7, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 7, 2020

openshift-merge-robot merged commit 4745c8a into openshift:master Dec 7, 2020

	// EnsureObjectMeta writes namespace, name, labels, and annotations. Don't set other things here.
	// TODO finalizer support maybe?
	func EnsureObjectMeta(modified bool, existing metav1.ObjectMeta, required metav1.ObjectMeta) {
	SetStringIfSet(modified, &existing.Namespace, required.Namespace)
	SetStringIfSet(modified, &existing.Name, required.Name)
	MergeMap(modified, &existing.Labels, required.Labels)
	MergeMap(modified, &existing.Annotations, required.Annotations)
	}

	// The Apply<type> methods in this file ensure that a resource is created or updated to match
	// the form provided by the caller.
	//
	// If the resource does not yet exist, it will be created.
	//
	// If the resource exists, the metadata of the required resource will be merged with the
	// existing resource and an update will be performed if the spec and metadata differ between
	// the required and existing resources. To be reliable, the input of the required spec from
	// the operator should be stable. It does not need to set all fields, since some fields are
	// defaulted server-side. Detection of spec drift from intent by other actors is determined
	// by generation, not by spec comparison.
	//
	// To ensure an update in response to state external to the resource spec, the caller should
	// set an annotation representing that external state e.g.
	//
	// `myoperator.openshift.io/config-resource-version: <resourceVersion>`
	//
	// An update will be performed if:
	//
	// - The required resource metadata differs from that of the existing resource.
	// - The difference will be detected by comparing the name, namespace, labels and
	// annotations of the 2 resources.
	//
	// - The generation expected by the operator differs from generation of the existing
	// resource.
	// - This is the likely result of an actor other than the operator updating a resource
	// managed by the operator.
	//
	// - The spec of the required resource differs from the spec of the existing resource.
	// - The difference will be detected via metadata comparison since the hash of the
	// resource's spec will be set as an annotation prior to comparison.

Comments

Conversation

Danil-Grigorev commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Danil-Grigorev commented Jul 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttts commented Jul 15, 2020

Uh oh!

JoelSpeed commented Jul 16, 2020

Uh oh!

JoelSpeed commented Jul 16, 2020

Uh oh!

JoelSpeed commented Jul 16, 2020

Uh oh!

Danil-Grigorev commented Jul 16, 2020

Uh oh!

sttts commented Jul 16, 2020

Uh oh!

JoelSpeed commented Jul 16, 2020

Uh oh!

sttts commented Jul 16, 2020

Uh oh!

sttts commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoelSpeed commented Jul 16, 2020

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsafrane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsafrane Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Danil-Grigorev commented Jul 14, 2020 •

edited

Loading

Danil-Grigorev Jul 14, 2020 •

edited

Loading

sttts commented Jul 16, 2020 •

edited

Loading

jsafrane Dec 2, 2020 •

edited

Loading

jsafrane Dec 3, 2020 •

edited

Loading