diff --git a/design/vela-core/apply-workload-and-trait.md b/design/vela-core/apply-workload-and-trait.md new file mode 100644 index 000000000..86888917e --- /dev/null +++ b/design/vela-core/apply-workload-and-trait.md @@ -0,0 +1,180 @@ +# Apply workload/trait through 3-way-merge-patch + +- Owner: Yue Wang(@captainroy-hy), Jianbo Sun(@wonderflow) +- Date: 01/21/2021 +- Status: [Implemented](https://github.com/oam-dev/kubevela/pull/857) + + +## Intro + +When an ApplicationConfiguration is deployed, +vela-core will create(apply) corresponding workload/trait instances and keep them stay align with the `spec` defined in ApplicationConfiguration through periodical reconciliation in AppConfig controller. + +In each round of reconciliation, if the configurations rendered from AppConfig are changed comparing to last round reconciliation, it's required to apply all changes to the workloads or traits. +Additionally, it also allows others (anything except AppConfig controller, e.g., trait controllers) to modify workload/trait instances. + + +## Goals + +Apply should handle three kinds of modification including +- add a field +- change a field +- remove a field by omitting it + +Meanwhile, Apply should have no impact on changes made by others, namely, not eliminate or override those changes UNLESS the change is made upon fields that are rendered from AppConfig originally. + + +## Implementation + +We employed the same mechanism as `kubectl apply`, that is, computing a 3-way diff based on target object's current state, modified state, and last-appied state. +Specifically, a new annotaion, `app.oam.dev/last-applied-configuration`, is introduced to record workload/trait's last-applied state. + +Once there's a conflict on field, both changed by AppConfig and others, AppConfig's value will always override others' assignment. + + +## Impact on existing system + +Before this implementation, vela-core use `JSON-Merge` patch to apply workloads and `update` to apply traits. +That brought several defects shown in below samples. +This section introduced a comparison between how old mechanism and new applies workload/trait, also shows how new Apply overcomed the defects. + +### Apply Workloads + +The reason why abandon json-merge patch is that, it cannot remove a field through unsetting value in the patched manifest. + +#### Before + +For example, apply below deployment as a workload. json-merge patch cannot remove `minReadySeconds` field through applying a modifiied manifest with `minReadySeconds` omitted . +```yaml +# original workload manifest +apiVersion: apps/v1 +kind: Deployment +... +spec: + minReadySeconds: 60 + replicas: 3 + template: + spec: + containers: + - name: nginx + image: nginx:1.14.2 +--- +# modified workload manifest +apiVersion: apps/v1 +kind: Deployment +... +spec: + # minReadySeconds: 60 <=== unset to remove field + replicas: 3 + template: + spec: + containers: + - name: nginx + image: nginx:1.14.2 +--- +# result +apiVersion: apps/v1 +kind: Deployment +... +spec: + minReadySeconds: 60 # <=== not removed + replicas: 3 + template: + spec: + containers: + - name: nginx + image: nginx:1.14.2 +``` +#### After + +By computing a 3-way diff, we can get a patch aware of the field set in last-applied manifest is omitted in the new modified manifest, namely, users wanna remove this field. +And an annotation, `app.oam.dev/last-applied-configuration`, is used to record last-applied-state of the resource for further use in computing 3-way diff next time. + +```yaml +# result +apiVersion: apps/v1 +kind: Deployment +metadata: + annotations: # v=== record last-applied-state + app.oam.dev/last-applied-configuration: | + {"apiVersion":"apps/v1","kind":"Deployment","metadata":{"name":"nginx-deployment","labels":{"app":"nginx"}},"spec":{"replicas":3,"selector":{"matchLabels":{"app":"nginx"}},"template":{"metadata":{"labels":{"app":"nginx"}},"spec":{"containers":[{"name":"nginx","image":"nginx:1.14.2"}]}}}} +... +spec: + # minReadySeconds: 60 <=== removed successfully + replicas: 3 + template: + spec: + containers: + - name: nginx + image: nginx:1.14.2 +``` +--- + +### Apply Traits + +The reasons why abandon `update` + + - update always eliminates all fields set by others (e.g., trait controllers) + - if trait contains immutable field (e.g., k8s Service), update fails + +#### Before +For example, apply below Service as a trait. +```yaml +# original trait manifest +apiVersion: v1 +kind: Service +metadata: + name: my-service +spec: + selector: + app: myweb + ports: + - protocol: TCP + port: 80 +--- +# after applying +apiVersion: v1 +kind: Service +... +spec: + clusterIP: 172.21.7.149 # <=== immutable field + ports: + - port: 80 + protocol: TCP + targetPort: 80 + selector: + app: myweb + sessionAffinity: None + type: ClusterIP +status: + loadBalancer: {} +--- +# update with original manifest fails +# reconciling also fails for cannot applying trait +``` +Additonally, if a trait has no immutable field, update will eliminate all fields set by others. +```yaml +# original trait manifest +kind: Bar +spec: + f1: v1 +# someone add a new field to it +kind: Bar +spec: + f1: v1 + f2: v2 # <=== newly set field +# after reconciling AppConfig +kind: Bar +spec: + f1: v1 + # f2: v2 <=== removed +``` +But as described in [Goals](#goals) section, we expect to keep these changes. + +#### After + +Applying traits works in the same way as workloads. We use annotation, `app.oam.dev/last-applied-configuration`, to record last-applied manifest. + +- Because 3-way diff will ignore the fields not touched in last-applied-state, the immutable fields will not be involved into patch data. +- Because 3-way diff also will ignore the fields set by others (others are not supposed to modify the field rendered from AppConfig), the changes made by others will be retained. + diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go index 760bfe85b..1e0639a35 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go @@ -44,6 +44,7 @@ import ( "github.com/oam-dev/kubevela/pkg/oam" "github.com/oam-dev/kubevela/pkg/oam/discoverymapper" "github.com/oam-dev/kubevela/pkg/oam/util" + "github.com/oam-dev/kubevela/pkg/utils/apply" ) const ( @@ -196,11 +197,9 @@ func NewReconciler(m ctrl.Manager, dm discoverymapper.DiscoveryMapper, o ...Reco trait: ResourceRenderFn(renderTrait), }, workloads: &workloads{ - // NOTE(roywang) PatchingApplicator@v0.10.0 only use "application/merge-patch+json" type patch - patchingClient: resource.NewAPIPatchingApplicator(m.GetClient()), - updatingClient: resource.NewAPIUpdatingApplicator(m.GetClient()), - rawClient: m.GetClient(), - dm: dm, + applicator: apply.NewAPIApplicator(m.GetClient()), + rawClient: m.GetClient(), + dm: dm, }, gc: GarbageCollectorFn(eligible), log: logging.NewNopLogger(), @@ -298,7 +297,7 @@ func (r *OAMApplicationReconciler) Reconcile(req reconcile.Request) (result reco log.Debug("Successfully rendered components", "workloads", len(workloads)) r.record.Event(ac, event.Normal(reasonRenderComponents, "Successfully rendered components", "workloads", strconv.Itoa(len(workloads)))) - applyOpts := []resource.ApplyOption{resource.MustBeControllableBy(ac.GetUID())} + applyOpts := []apply.ApplyOption{apply.MustBeControllableBy(ac.GetUID())} if r.applyOnceOnly { applyOpts = append(applyOpts, applyOnceOnly()) } @@ -578,8 +577,11 @@ func (e *GenerationUnchanged) Error() string { "Please ignore this error in other logic.") } -func applyOnceOnly() resource.ApplyOption { +func applyOnceOnly() apply.ApplyOption { return func(ctx context.Context, current, desired runtime.Object) error { + if current == nil { + return nil + } // ApplyOption only works for update/patch operation and will be ignored // if the object doesn't exist before. c, _ := current.(metav1.Object) diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go index a40440e35..7cf31bf1e 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go @@ -24,7 +24,6 @@ import ( runtimev1alpha1 "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1" "github.com/crossplane/crossplane-runtime/pkg/logging" - "github.com/crossplane/crossplane-runtime/pkg/resource" "github.com/crossplane/crossplane-runtime/pkg/test" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -43,6 +42,7 @@ import ( "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2" "github.com/oam-dev/kubevela/pkg/oam/mock" + "github.com/oam-dev/kubevela/pkg/utils/apply" ) // OAMApplicationReconciler implements controller runtime Reconciler interface @@ -209,7 +209,7 @@ func TestReconciler(t *testing.T) { WithRenderer(ComponentRenderFn(func(_ context.Context, _ *v1alpha2.ApplicationConfiguration) ([]Workload, *v1alpha2.DependencyStatus, error) { return []Workload{{Workload: workload}}, &v1alpha2.DependencyStatus{}, nil })), - WithApplicator(WorkloadApplyFns{ApplyFn: func(_ context.Context, _ []v1alpha2.WorkloadStatus, _ []Workload, _ ...resource.ApplyOption) error { + WithApplicator(WorkloadApplyFns{ApplyFn: func(_ context.Context, _ []v1alpha2.WorkloadStatus, _ []Workload, _ ...apply.ApplyOption) error { return errBoom }}), }, @@ -239,7 +239,7 @@ func TestReconciler(t *testing.T) { WithRenderer(ComponentRenderFn(func(_ context.Context, _ *v1alpha2.ApplicationConfiguration) ([]Workload, *v1alpha2.DependencyStatus, error) { return []Workload{}, &v1alpha2.DependencyStatus{}, nil })), - WithApplicator(WorkloadApplyFns{ApplyFn: (func(_ context.Context, _ []v1alpha2.WorkloadStatus, _ []Workload, _ ...resource.ApplyOption) error { + WithApplicator(WorkloadApplyFns{ApplyFn: (func(_ context.Context, _ []v1alpha2.WorkloadStatus, _ []Workload, _ ...apply.ApplyOption) error { return nil })}), WithGarbageCollector(GarbageCollectorFn(func(_ string, _ []v1alpha2.WorkloadStatus, _ []Workload) []unstructured.Unstructured { @@ -302,7 +302,7 @@ func TestReconciler(t *testing.T) { WithRenderer(ComponentRenderFn(func(_ context.Context, _ *v1alpha2.ApplicationConfiguration) ([]Workload, *v1alpha2.DependencyStatus, error) { return []Workload{{ComponentName: componentName, Workload: workload}}, &depStatus, nil })), - WithApplicator(WorkloadApplyFns{ApplyFn: (func(_ context.Context, _ []v1alpha2.WorkloadStatus, _ []Workload, _ ...resource.ApplyOption) error { + WithApplicator(WorkloadApplyFns{ApplyFn: (func(_ context.Context, _ []v1alpha2.WorkloadStatus, _ []Workload, _ ...apply.ApplyOption) error { return nil })}), WithGarbageCollector(GarbageCollectorFn(func(_ string, _ []v1alpha2.WorkloadStatus, _ []Workload) []unstructured.Unstructured { @@ -337,7 +337,7 @@ func TestReconciler(t *testing.T) { WithRenderer(ComponentRenderFn(func(_ context.Context, _ *v1alpha2.ApplicationConfiguration) ([]Workload, *v1alpha2.DependencyStatus, error) { return []Workload{{ComponentName: componentName, Workload: workload}}, &v1alpha2.DependencyStatus{}, nil })), - WithApplicator(WorkloadApplyFns{ApplyFn: (func(_ context.Context, _ []v1alpha2.WorkloadStatus, _ []Workload, _ ...resource.ApplyOption) error { + WithApplicator(WorkloadApplyFns{ApplyFn: (func(_ context.Context, _ []v1alpha2.WorkloadStatus, _ []Workload, _ ...apply.ApplyOption) error { return nil })}), WithGarbageCollector(GarbageCollectorFn(func(_ string, _ []v1alpha2.WorkloadStatus, _ []Workload) []unstructured.Unstructured { @@ -410,7 +410,7 @@ func TestReconciler(t *testing.T) { WithRenderer(ComponentRenderFn(func(_ context.Context, _ *v1alpha2.ApplicationConfiguration) ([]Workload, *v1alpha2.DependencyStatus, error) { return []Workload{{ComponentName: componentName, Workload: workload}}, &v1alpha2.DependencyStatus{}, nil })), - WithApplicator(WorkloadApplyFns{ApplyFn: (func(_ context.Context, _ []v1alpha2.WorkloadStatus, _ []Workload, _ ...resource.ApplyOption) error { + WithApplicator(WorkloadApplyFns{ApplyFn: (func(_ context.Context, _ []v1alpha2.WorkloadStatus, _ []Workload, _ ...apply.ApplyOption) error { return nil })}), WithGarbageCollector(GarbageCollectorFn(func(_ string, _ []v1alpha2.WorkloadStatus, _ []Workload) []unstructured.Unstructured { @@ -454,7 +454,7 @@ func TestReconciler(t *testing.T) { WithRenderer(ComponentRenderFn(func(_ context.Context, _ *v1alpha2.ApplicationConfiguration) ([]Workload, *v1alpha2.DependencyStatus, error) { return []Workload{{ComponentName: componentName, Workload: workload}}, &v1alpha2.DependencyStatus{}, nil })), - WithApplicator(WorkloadApplyFns{ApplyFn: (func(_ context.Context, _ []v1alpha2.WorkloadStatus, _ []Workload, _ ...resource.ApplyOption) error { + WithApplicator(WorkloadApplyFns{ApplyFn: (func(_ context.Context, _ []v1alpha2.WorkloadStatus, _ []Workload, _ ...apply.ApplyOption) error { return nil })}), WithGarbageCollector(GarbageCollectorFn(func(_ string, _ []v1alpha2.WorkloadStatus, _ []Workload) []unstructured.Unstructured { @@ -527,7 +527,7 @@ func TestReconciler(t *testing.T) { WithRenderer(ComponentRenderFn(func(_ context.Context, _ *v1alpha2.ApplicationConfiguration) ([]Workload, *v1alpha2.DependencyStatus, error) { return []Workload{{ComponentName: componentName, Workload: workload}}, &v1alpha2.DependencyStatus{}, nil })), - WithApplicator(WorkloadApplyFns{ApplyFn: (func(_ context.Context, _ []v1alpha2.WorkloadStatus, _ []Workload, _ ...resource.ApplyOption) error { + WithApplicator(WorkloadApplyFns{ApplyFn: (func(_ context.Context, _ []v1alpha2.WorkloadStatus, _ []Workload, _ ...apply.ApplyOption) error { return nil })}), WithGarbageCollector(GarbageCollectorFn(func(_ string, _ []v1alpha2.WorkloadStatus, _ []Workload) []unstructured.Unstructured { diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go index 63323ac8c..854541c4e 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go @@ -22,7 +22,6 @@ import ( runtimev1alpha1 "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1" "github.com/crossplane/crossplane-runtime/pkg/fieldpath" "github.com/crossplane/crossplane-runtime/pkg/meta" - "github.com/crossplane/crossplane-runtime/pkg/resource" "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -32,6 +31,7 @@ import ( "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2" "github.com/oam-dev/kubevela/pkg/oam/discoverymapper" "github.com/oam-dev/kubevela/pkg/oam/util" + "github.com/oam-dev/kubevela/pkg/utils/apply" ) // Reconcile error strings. @@ -52,7 +52,7 @@ const ( // A WorkloadApplicator creates or updates or finalizes workloads and their traits. type WorkloadApplicator interface { // Apply a workload and its traits. - Apply(ctx context.Context, status []v1alpha2.WorkloadStatus, w []Workload, ao ...resource.ApplyOption) error + Apply(ctx context.Context, status []v1alpha2.WorkloadStatus, w []Workload, ao ...apply.ApplyOption) error // Finalize implements pre-delete hooks on workloads Finalize(ctx context.Context, ac *v1alpha2.ApplicationConfiguration) error @@ -60,12 +60,14 @@ type WorkloadApplicator interface { // A WorkloadApplyFns creates or updates or finalizes workloads and their traits. type WorkloadApplyFns struct { - ApplyFn func(ctx context.Context, status []v1alpha2.WorkloadStatus, w []Workload, ao ...resource.ApplyOption) error + ApplyFn func(ctx context.Context, status []v1alpha2.WorkloadStatus, w []Workload, ao ...apply.ApplyOption) error FinalizeFn func(ctx context.Context, ac *v1alpha2.ApplicationConfiguration) error } -// Apply a workload and its traits. -func (fn WorkloadApplyFns) Apply(ctx context.Context, status []v1alpha2.WorkloadStatus, w []Workload, ao ...resource.ApplyOption) error { +// Apply a workload and its traits. It employes the same mechanism as `kubectl apply`, that is, for each resource being applied, +// computing a three-way diff merge in client side based on its current state, modified stated and last-applied-state which is +// tracked through an specific annotaion. If the resource doesn't exist before, Apply will create it. +func (fn WorkloadApplyFns) Apply(ctx context.Context, status []v1alpha2.WorkloadStatus, w []Workload, ao ...apply.ApplyOption) error { return fn.ApplyFn(ctx, status, w, ao...) } @@ -75,24 +77,18 @@ func (fn WorkloadApplyFns) Finalize(ctx context.Context, ac *v1alpha2.Applicatio } type workloads struct { - // use patching-apply for creating/updating Workload - patchingClient resource.Applicator - // use updating-apply for creating/updating Trait - updatingClient resource.Applicator - rawClient client.Client - dm discoverymapper.DiscoveryMapper + applicator apply.Applicator + rawClient client.Client + dm discoverymapper.DiscoveryMapper } -//nolint:errorlint -func (a *workloads) Apply(ctx context.Context, status []v1alpha2.WorkloadStatus, w []Workload, ao ...resource.ApplyOption) error { +func (a *workloads) Apply(ctx context.Context, status []v1alpha2.WorkloadStatus, w []Workload, ao ...apply.ApplyOption) error { // they are all in the same namespace var namespace = w[0].Workload.GetNamespace() for _, wl := range w { if !wl.HasDep { - err := a.patchingClient.Apply(ctx, wl.Workload, ao...) - if err != nil { - // TODO(roywang) use errors.As() instead of type assertion on error - if _, ok := err.(*GenerationUnchanged); !ok { + if err := a.applicator.Apply(ctx, wl.Workload, ao...); err != nil { + if !errors.Is(err, &GenerationUnchanged{}) { // GenerationUnchanged only aborts applying current workload // but not blocks the whole reconciliation through returning an error return errors.Wrapf(err, errFmtApplyWorkload, wl.Workload.GetName()) @@ -104,9 +100,8 @@ func (a *workloads) Apply(ctx context.Context, status []v1alpha2.WorkloadStatus, continue } t := trait.Object - if err := a.updatingClient.Apply(ctx, &trait.Object, ao...); err != nil { - // TODO(roywang) use errors.As() instead of type assertion on error - if _, ok := err.(*GenerationUnchanged); !ok { + if err := a.applicator.Apply(ctx, &trait.Object, ao...); err != nil { + if !errors.Is(err, &GenerationUnchanged{}) { // GenerationUnchanged only aborts applying current trait // but not blocks the whole reconciliation through returning an error return errors.Wrapf(err, errFmtApplyTrait, t.GetAPIVersion(), t.GetKind(), t.GetName()) diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply_once_only_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply_once_only_test.go index 4ed20c127..e1ce7cbea 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply_once_only_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply_once_only_test.go @@ -175,11 +175,11 @@ var _ = Describe("Test apply (workloads/traits) once only", func() { By("Modify workload spec & Apply changed workload") cwObj.Spec.Containers[0].Image = image2 - Expect(k8sClient.Apply(ctx, &cwObj)).Should(Succeed()) + Expect(k8sClient.Patch(ctx, &cwObj, client.Merge)).Should(Succeed()) By("Modify trait spec & Apply changed trait") unstructured.SetNestedField(fooObj.Object, traitSpecValue2, "spec", "key") - Expect(k8sClient.Apply(ctx, fooObj)).Should(Succeed()) + Expect(k8sClient.Patch(ctx, fooObj, client.Merge)).Should(Succeed()) By("Get updated workload instance & Check workload spec") updateCwObj := v1alpha2.ContainerizedWorkload{} diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply_test.go index c61198096..df786cd70 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply_test.go @@ -23,7 +23,6 @@ import ( "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1" "github.com/crossplane/crossplane-runtime/pkg/fieldpath" - "github.com/crossplane/crossplane-runtime/pkg/resource" "github.com/crossplane/crossplane-runtime/pkg/test" "github.com/google/go-cmp/cmp" "github.com/pkg/errors" @@ -37,8 +36,17 @@ import ( "github.com/oam-dev/kubevela/pkg/oam/mock" "github.com/oam-dev/kubevela/pkg/oam/util" + "github.com/oam-dev/kubevela/pkg/utils/apply" ) +// ApplyFn mocks apply.Applicator for test convenience. +type ApplyFn func(context.Context, runtime.Object, ...apply.ApplyOption) error + +// Apply implements apply.Applicator +func (fn ApplyFn) Apply(ctx context.Context, o runtime.Object, ao ...apply.ApplyOption) error { + return fn(ctx, o, ao...) +} + func TestApplyWorkloads(t *testing.T) { errBoom := errors.New("boom") namespace := "ns" @@ -124,23 +132,21 @@ func TestApplyWorkloads(t *testing.T) { } cases := map[string]struct { - reason string - client resource.Applicator - updatingClient resource.Applicator - rawClient client.Client - args args - want error + reason string + applicator apply.Applicator + rawClient client.Client + args args + want error }{ "ApplyWorkloadError": { reason: "Errors applying a workload should be reflected as a status condition", - client: resource.ApplyFn(func(_ context.Context, o runtime.Object, _ ...resource.ApplyOption) error { + applicator: ApplyFn(func(_ context.Context, o runtime.Object, _ ...apply.ApplyOption) error { if w, ok := o.(*unstructured.Unstructured); ok && w.GetUID() == workload.GetUID() { return errBoom } return nil }), - updatingClient: resource.ApplyFn(func(_ context.Context, o runtime.Object, _ ...resource.ApplyOption) error { return nil }), - rawClient: nil, + rawClient: nil, args: args{ w: []Workload{{Workload: workload, Traits: []*Trait{{Object: *trait}}}}, ws: []v1alpha2.WorkloadStatus{}}, @@ -148,8 +154,7 @@ func TestApplyWorkloads(t *testing.T) { }, "ApplyTraitError": { reason: "Errors applying a trait should be reflected as a status condition", - client: resource.ApplyFn(func(_ context.Context, o runtime.Object, _ ...resource.ApplyOption) error { return nil }), - updatingClient: resource.ApplyFn(func(_ context.Context, o runtime.Object, _ ...resource.ApplyOption) error { + applicator: ApplyFn(func(_ context.Context, o runtime.Object, _ ...apply.ApplyOption) error { if t, ok := o.(*unstructured.Unstructured); ok && t.GetUID() == trait.GetUID() { return errBoom } @@ -163,8 +168,7 @@ func TestApplyWorkloads(t *testing.T) { }, "Success": { reason: "Applied workloads and traits should be returned as a set of UIDs.", - client: resource.ApplyFn(func(_ context.Context, o runtime.Object, _ ...resource.ApplyOption) error { return nil }), - updatingClient: resource.ApplyFn(func(_ context.Context, o runtime.Object, _ ...resource.ApplyOption) error { + applicator: ApplyFn(func(_ context.Context, o runtime.Object, _ ...apply.ApplyOption) error { if o.GetObjectKind().GroupVersionKind().Kind == trait.GetKind() { // check that the trait should not have a workload ref since we didn't return a special traitDefinition obj, _ := util.Object2Map(o) @@ -181,9 +185,8 @@ func TestApplyWorkloads(t *testing.T) { }, }, "SuccessWithScope": { - reason: "Applied workloads refs to scopes.", - client: resource.ApplyFn(func(_ context.Context, o runtime.Object, _ ...resource.ApplyOption) error { return nil }), - updatingClient: resource.ApplyFn(func(_ context.Context, o runtime.Object, _ ...resource.ApplyOption) error { return nil }), + reason: "Applied workloads refs to scopes.", + applicator: ApplyFn(func(_ context.Context, o runtime.Object, _ ...apply.ApplyOption) error { return nil }), rawClient: &test.MockClient{ MockGet: func(_ context.Context, key client.ObjectKey, obj runtime.Object) error { if scopeDef, ok := obj.(*v1alpha2.ScopeDefinition); ok { @@ -223,9 +226,8 @@ func TestApplyWorkloads(t *testing.T) { }, }, "SuccessWithScopeNoOp": { - reason: "Scope already has workloadRef.", - client: resource.ApplyFn(func(_ context.Context, o runtime.Object, _ ...resource.ApplyOption) error { return nil }), - updatingClient: resource.ApplyFn(func(_ context.Context, o runtime.Object, _ ...resource.ApplyOption) error { return nil }), + reason: "Scope already has workloadRef.", + applicator: ApplyFn(func(_ context.Context, o runtime.Object, _ ...apply.ApplyOption) error { return nil }), rawClient: &test.MockClient{ MockGet: func(_ context.Context, key client.ObjectKey, obj runtime.Object) error { if scopeDef, ok := obj.(*v1alpha2.ScopeDefinition); ok { @@ -265,9 +267,8 @@ func TestApplyWorkloads(t *testing.T) { }, }, "SuccessRemoving": { - reason: "Removes workload refs from scopes.", - client: resource.ApplyFn(func(_ context.Context, o runtime.Object, _ ...resource.ApplyOption) error { return nil }), - updatingClient: resource.ApplyFn(func(_ context.Context, o runtime.Object, _ ...resource.ApplyOption) error { return nil }), + reason: "Removes workload refs from scopes.", + applicator: ApplyFn(func(_ context.Context, o runtime.Object, _ ...apply.ApplyOption) error { return nil }), rawClient: &test.MockClient{ MockGet: func(_ context.Context, key client.ObjectKey, obj runtime.Object) error { if key.Name == scope.GetName() { @@ -328,8 +329,7 @@ func TestApplyWorkloads(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { mapper := mock.NewMockDiscoveryMapper() - - w := workloads{patchingClient: tc.client, updatingClient: tc.updatingClient, rawClient: tc.rawClient, dm: mapper} + w := workloads{applicator: tc.applicator, rawClient: tc.rawClient, dm: mapper} err := w.Apply(tc.args.ctx, tc.args.ws, tc.args.w) if diff := cmp.Diff(tc.want, err, test.EquateErrors()); diff != "" { @@ -415,14 +415,14 @@ func TestFinalizeWorkloadScopes(t *testing.T) { cases := []struct { caseName string - client resource.Applicator + applicator apply.Applicator rawClient client.Client wantErr error wantFinalizers []string }{ { - caseName: "Finalization successes", - client: resource.ApplyFn(func(_ context.Context, o runtime.Object, _ ...resource.ApplyOption) error { return nil }), + caseName: "Finalization successes", + applicator: ApplyFn(func(_ context.Context, o runtime.Object, _ ...apply.ApplyOption) error { return nil }), rawClient: &test.MockClient{ MockGet: func(ctx context.Context, key types.NamespacedName, obj runtime.Object) error { if key.Name == scope.GetName() { @@ -457,8 +457,8 @@ func TestFinalizeWorkloadScopes(t *testing.T) { wantFinalizers: []string{}, }, { - caseName: "Finalization fails for error", - client: resource.ApplyFn(func(_ context.Context, o runtime.Object, _ ...resource.ApplyOption) error { return nil }), + caseName: "Finalization fails for error", + applicator: ApplyFn(func(_ context.Context, o runtime.Object, _ ...apply.ApplyOption) error { return nil }), rawClient: &test.MockClient{ MockGet: func(ctx context.Context, key types.NamespacedName, obj runtime.Object) error { return errMock @@ -475,7 +475,7 @@ func TestFinalizeWorkloadScopes(t *testing.T) { t.Run(tc.caseName, func(t *testing.T) { acTest := ac mapper := mock.NewMockDiscoveryMapper() - w := workloads{patchingClient: tc.client, rawClient: tc.rawClient, dm: mapper} + w := workloads{applicator: tc.applicator, rawClient: tc.rawClient, dm: mapper} err := w.Finalize(ctx, &acTest) if diff := cmp.Diff(tc.wantErr, err, test.EquateErrors()); diff != "" { diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/update_trait_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply_trait_test.go similarity index 51% rename from pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/update_trait_test.go rename to pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply_trait_test.go index 3b6198977..d138b83a8 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/update_trait_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply_trait_test.go @@ -14,14 +14,13 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" - logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2" "github.com/oam-dev/kubevela/pkg/oam/util" ) -var _ = Describe("Test Updating-apply trait in an ApplicationConfiguration", func() { +var _ = Describe("Test apply changes to trait", func() { const ( namespace = "update-apply-trait-test" appName = "example-app" @@ -36,16 +35,12 @@ var _ = Describe("Test Updating-apply trait in an ApplicationConfiguration", fun component v1alpha2.Component traitDef v1alpha2.TraitDefinition appConfig v1alpha2.ApplicationConfiguration + fakeTratiCRD crdv1.CustomResourceDefinition appConfigKey = client.ObjectKey{ Name: appName, Namespace: namespace, } req = reconcile.Request{NamespacedName: appConfigKey} - ns = corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: namespace, - }, - } ) BeforeEach(func() { @@ -94,19 +89,12 @@ var _ = Describe("Test Updating-apply trait in an ApplicationConfiguration", fun }, } - logf.Log.Info("Start to run a test, clean up previous resources") - // delete the namespace with all its resources - Expect(k8sClient.Delete(ctx, &ns, client.PropagationPolicy(metav1.DeletePropagationForeground))). - Should(SatisfyAny(BeNil(), &util.NotFoundMatcher{})) - logf.Log.Info("make sure all the resources are removed") - Eventually( - // gomega has a bug that can't take nil as the actual input, so has to make it a func - func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: namespace}, &corev1.Namespace{}) - }, - time.Second*120, time.Millisecond*500).Should(&util.NotFoundMatcher{}) - By("Create namespace") + ns := corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespace, + }, + } Eventually( func() error { return k8sClient.Create(ctx, &ns) @@ -114,7 +102,7 @@ var _ = Describe("Test Updating-apply trait in an ApplicationConfiguration", fun time.Second*3, time.Millisecond*300).Should(SatisfyAny(BeNil(), &util.AlreadyExistMatcher{})) By("Create a CRD used as fake trait") - crd = crdv1.CustomResourceDefinition{ + fakeTratiCRD = crdv1.CustomResourceDefinition{ ObjectMeta: metav1.ObjectMeta{ Name: fakeTraitCRDName, Labels: map[string]string{"crd": namespace}, @@ -147,10 +135,10 @@ var _ = Describe("Test Updating-apply trait in an ApplicationConfiguration", fun Scope: crdv1.NamespaceScoped, }, } - Expect(k8sClient.Create(context.Background(), &crd)).Should(SatisfyAny(BeNil(), &util.AlreadyExistMatcher{})) + Expect(k8sClient.Create(context.Background(), &fakeTratiCRD)).Should(SatisfyAny(BeNil(), &util.AlreadyExistMatcher{})) By("Create Component") - Expect(k8sClient.Create(ctx, &component)).Should(Succeed()) + Expect(k8sClient.Create(ctx, &component)).Should(SatisfyAny(BeNil(), &util.AlreadyExistMatcher{})) cmpV1 := &v1alpha2.Component{} Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compName}, cmpV1)).Should(Succeed()) @@ -203,16 +191,18 @@ spec: }) AfterEach(func() { - logf.Log.Info("delete all cluster-scoped resources") - By("Delete CRD used as fake trait") - Expect(k8sClient.Delete(context.Background(), &crd)).Should(BeNil()) - By("Delete fake TraitDefinition ") - Expect(k8sClient.Delete(context.Background(), &traitDef)).Should(BeNil()) + Expect(k8sClient.DeleteAllOf(ctx, &appConfig, client.InNamespace(namespace))).Should(Succeed()) + Expect(k8sClient.DeleteAllOf(ctx, &cw, client.InNamespace(namespace))).Should(Succeed()) + Expect(k8sClient.DeleteAllOf(ctx, &component, client.InNamespace(namespace))).Should(Succeed()) + var deleteTrait unstructured.Unstructured + deleteTrait.SetAPIVersion("example.com/v1") + deleteTrait.SetKind("Bar") + Expect(k8sClient.DeleteAllOf(ctx, &deleteTrait, client.InNamespace(namespace))).Should(Succeed()) }) - When("update an ApplicationConfiguration with Traits changed", func() { - It("should updating-apply the changed trait", func() { - By("Update the ApplicationConfiguration") + When("update an ApplicationConfiguration with traits changed", func() { + It("should apply all changes(add/reset/remove fields) to the trait", func() { + By("Modify the ApplicationConfiguration") appConfigYAMLUpdated := ` apiVersion: core.oam.dev/v1alpha2 kind: ApplicationConfiguration @@ -229,12 +219,16 @@ spec: unchanged: bar valueChanged: foo added: bar` + // remove metadata.labels + // change a field (valueChanged: bar ==> foo) + // added a field + // removed a field appConfigUpdated := v1alpha2.ApplicationConfiguration{} Expect(yaml.Unmarshal([]byte(appConfigYAMLUpdated), &appConfigUpdated)).Should(BeNil()) appConfigUpdated.SetNamespace(namespace) By("Apply appConfig & check successfully") - Expect(k8sClient.Apply(ctx, &appConfigUpdated)).Should(Succeed()) + Expect(k8sClient.Patch(ctx, &appConfigUpdated, client.Merge)).Should(Succeed()) Eventually(func() int64 { if err := k8sClient.Get(ctx, appConfigKey, &appConfig); err != nil { return 0 @@ -255,7 +249,7 @@ spec: return appConfig.Status.Workloads[0].Traits[0].Reference.Name }, 3*time.Second, time.Second).ShouldNot(BeEmpty()) - By("Get updated trait object") + By("Get changed trait object") traitName := appConfig.Status.Workloads[0].Traits[0].Reference.Name var traitObj unstructured.Unstructured traitObj.SetAPIVersion("example.com/v1") @@ -280,11 +274,149 @@ spec: By("Check added field") v, _, _ = unstructured.NestedString(traitObj.UnstructuredContent(), "spec", "added") Expect(v).Should(Equal("bar")) - // if use patching trait, this field will not be removed By("Check removed field") _, found, _ = unstructured.NestedString(traitObj.UnstructuredContent(), "spec", "removed") Expect(found).Should(Equal(false)) }) }) + // others means anything except AppConfig controller + // e.g. trait controllers + When("trait instance is changed by others", func() { + // if others make changes on the fields managed by AppConfig controller + // these changes will reverted but not retained. + It("should retain changes by others, even after AppConfig spec is changed", func() { + By("Get the trait newly created") + Eventually(func() string { + if err := k8sClient.Get(ctx, appConfigKey, &appConfig); err != nil { + return "" + } + if appConfig.Status.Workloads == nil { + reconcileRetry(reconciler, req) + return "" + } + return appConfig.Status.Workloads[0].Traits[0].Reference.Name + }, 5*time.Second, time.Second).ShouldNot(BeEmpty()) + traitName := appConfig.Status.Workloads[0].Traits[0].Reference.Name + var traitObj unstructured.Unstructured + traitObj.SetAPIVersion("example.com/v1") + traitObj.SetKind("Bar") + Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: traitName}, &traitObj)).Should(Succeed()) + + By("Others change the trait") + // add a field + unstructured.SetNestedField(traitObj.Object, "bar", "spec", "added") + Expect(k8sClient.Patch(ctx, &traitObj, client.Merge)).Should(Succeed()) + + By("Check the change works") + var changedTrait unstructured.Unstructured + changedTrait.SetAPIVersion("example.com/v1") + changedTrait.SetKind("Bar") + Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: traitName}, &changedTrait)).Should(Succeed()) + v, _, _ := unstructured.NestedString(changedTrait.UnstructuredContent(), "spec", "added") + Expect(v).Should(Equal("bar")) + + By("Reconcile") + reconcileRetry(reconciler, req) + + By("Check others' change is retained") + changedTrait = unstructured.Unstructured{} + changedTrait.SetAPIVersion("example.com/v1") + changedTrait.SetKind("Bar") + Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: traitName}, &changedTrait)).Should(Succeed()) + v, _, _ = unstructured.NestedString(changedTrait.UnstructuredContent(), "spec", "added") + Expect(v).Should(Equal("bar")) + + By("Modify AppConfig without touching the field changed by others") + appConfigYAMLUpdated := ` +apiVersion: core.oam.dev/v1alpha2 +kind: ApplicationConfiguration +metadata: + name: example-app +spec: + components: + - componentName: example-comp + traits: + - trait: + apiVersion: example.com/v1 + kind: Bar + spec: + unchanged: bar + valueChanged: foo` + appConfigUpdated := v1alpha2.ApplicationConfiguration{} + Expect(yaml.Unmarshal([]byte(appConfigYAMLUpdated), &appConfigUpdated)).Should(BeNil()) + appConfigUpdated.SetNamespace(namespace) + + By("Apply appConfig & check successfully") + Expect(k8sClient.Patch(ctx, &appConfigUpdated, client.Merge)).Should(Succeed()) + Eventually(func() int64 { + if err := k8sClient.Get(ctx, appConfigKey, &appConfig); err != nil { + return 0 + } + return appConfig.GetGeneration() + }, time.Second, 300*time.Millisecond).Should(Equal(int64(2))) + By("Reconcile") + reconcileRetry(reconciler, req) + + changedTrait = unstructured.Unstructured{} + changedTrait.SetAPIVersion("example.com/v1") + changedTrait.SetKind("Bar") + Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: traitName}, &changedTrait)).Should(Succeed()) + By("Check AppConfig's change works") + // changed a field + v, _, _ = unstructured.NestedString(changedTrait.UnstructuredContent(), "spec", "valueChanged") + Expect(v).Should(Equal("foo")) + // removed a field + _, found, _ := unstructured.NestedString(changedTrait.UnstructuredContent(), "spec", "removed") + Expect(found).Should(Equal(false)) + + By("Check others' change is still retained") + v, _, _ = unstructured.NestedString(changedTrait.UnstructuredContent(), "spec", "added") + Expect(v).Should(Equal("bar")) + }) + + It("should override others' changes on fields rendered from AppConfig", func() { + By("Get the trait newly created") + Eventually(func() string { + if err := k8sClient.Get(ctx, appConfigKey, &appConfig); err != nil { + return "" + } + if appConfig.Status.Workloads == nil { + reconcileRetry(reconciler, req) + return "" + } + return appConfig.Status.Workloads[0].Traits[0].Reference.Name + }, 5*time.Second, time.Second).ShouldNot(BeEmpty()) + traitName := appConfig.Status.Workloads[0].Traits[0].Reference.Name + var traitObj unstructured.Unstructured + traitObj.SetAPIVersion("example.com/v1") + traitObj.SetKind("Bar") + Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: traitName}, &traitObj)).Should(Succeed()) + + By("Others change the field which should be rendered from AppConfig") + // unchanged: bar ==> foo + unstructured.SetNestedField(traitObj.Object, "foo", "spec", "unchanged") + Expect(k8sClient.Patch(ctx, &traitObj, client.Merge)).Should(Succeed()) + + By("Check the change works") + var changedTrait unstructured.Unstructured + changedTrait.SetAPIVersion("example.com/v1") + changedTrait.SetKind("Bar") + Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: traitName}, &changedTrait)).Should(Succeed()) + v, _, _ := unstructured.NestedString(changedTrait.UnstructuredContent(), "spec", "unchanged") + Expect(v).Should(Equal("foo")) + + By("Reconcile") + reconcileRetry(reconciler, req) + + By("Check others' change is overrided(reset)") + changedTrait = unstructured.Unstructured{} + changedTrait.SetAPIVersion("example.com/v1") + changedTrait.SetKind("Bar") + Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: traitName}, &changedTrait)).Should(Succeed()) + v, _, _ = unstructured.NestedString(changedTrait.UnstructuredContent(), "spec", "unchanged") + // unchanged: foo ==> bar + Expect(v).Should(Equal("bar")) + }) + }) }) diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/dependency_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/dependency_test.go index 627e11b4e..e989f9ab5 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/dependency_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/dependency_test.go @@ -152,6 +152,7 @@ var _ = Describe("Resource Dependency in an ApplicationConfiguration", func() { }()).Should(Equal(depStatus)) // fill value to fieldPath + Expect(k8sClient.Get(ctx, outFooKey, outFoo)).Should(Succeed()) Expect(unstructured.SetNestedField(outFoo.Object, "test", "status", "key")).Should(BeNil()) Expect(k8sClient.Status().Update(ctx, outFoo)).Should(Succeed()) Eventually(func() string { diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/suite_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/suite_test.go index 3099ffafc..ad5a1ca97 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/suite_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/suite_test.go @@ -10,7 +10,6 @@ import ( . "github.com/onsi/gomega" "github.com/crossplane/crossplane-runtime/pkg/logging" - "github.com/crossplane/crossplane-runtime/pkg/resource" crdv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -41,7 +40,7 @@ var componentHandler *ComponentHandler var mgrclose chan struct{} var testEnv *envtest.Environment var cfg *rest.Config -var k8sClient resource.ClientApplicator +var k8sClient client.Client var scheme = runtime.NewScheme() var crd crdv1.CustomResourceDefinition @@ -81,19 +80,12 @@ var _ = BeforeSuite(func(done Done) { Expect(depSchemeBuilder.AddToScheme(scheme)).Should(BeNil()) By("Setting up kubernetes client") - c, err := client.New(cfg, client.Options{Scheme: scheme}) + k8sClient, err = client.New(cfg, client.Options{Scheme: scheme}) if err != nil { logf.Log.Error(err, "failed to create a client") Fail("setup failed") } - k8sClient = resource.ClientApplicator{ - Client: c, - Applicator: resource.NewAPIUpdatingApplicator(c), - } - if err != nil { - logf.Log.Error(err, "failed to create k8sClient") - Fail("setup failed") - } + Expect(k8sClient).ShouldNot(BeNil()) By("Finished setting up test environment") By("Creating Reconciler for appconfig") diff --git a/pkg/controller/standard.oam.dev/v1alpha1/rollout/webhook.go b/pkg/controller/standard.oam.dev/v1alpha1/rollout/webhook.go index 7b44551fd..a9b3323f8 100644 --- a/pkg/controller/standard.oam.dev/v1alpha1/rollout/webhook.go +++ b/pkg/controller/standard.oam.dev/v1alpha1/rollout/webhook.go @@ -55,7 +55,7 @@ func callWebhook(webhook string, payload interface{}, timeout string) error { b, err := ioutil.ReadAll(r.Body) if err != nil { - return fmt.Errorf("error reading body: %s", err.Error()) + return fmt.Errorf("error reading body: %w", err) } if r.StatusCode > 202 { diff --git a/pkg/oam/labels.go b/pkg/oam/labels.go index 49637a8d2..1fdc5dc53 100644 --- a/pkg/oam/labels.go +++ b/pkg/oam/labels.go @@ -44,4 +44,8 @@ const ( const ( // AnnotationAppGeneration records the generation of AppConfig AnnotationAppGeneration = "app.oam.dev/generation" + + // AnnotationLastAppliedConfig records the previous configuration of a + // resource for use in a three way diff during a patching apply + AnnotationLastAppliedConfig = "app.oam.dev/last-applied-configuration" ) diff --git a/pkg/utils/apply/apply.go b/pkg/utils/apply/apply.go new file mode 100644 index 000000000..abb7d548b --- /dev/null +++ b/pkg/utils/apply/apply.go @@ -0,0 +1,162 @@ +package apply + +import ( + "context" + + "github.com/pkg/errors" + kerrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// An Object is a Kubernetes object. +type Object interface { + metav1.Object + runtime.Object +} + +// Applicator applies new state to an object or create it if not exist. +// It employes the same mechanism as `kubectl apply`, that is, for each resource being applied, +// computing a three-way diff merge in client side based on its current state, modified stated, +// and last-applied-state which is tracked through an specific annotaion. +// If the resource doesn't exist before, Apply will create it. +type Applicator interface { + Apply(context.Context, runtime.Object, ...ApplyOption) error +} + +// ApplyOption is called before applying state to the object. +// ApplyOption is still called even if the object does NOT exist. +// If the object does not exist, `existing` will be assigned as `nil`. +// nolint: golint +type ApplyOption func(ctx context.Context, existing, desired runtime.Object) error + +// NewAPIApplicator creates an Applicator that applies state to an +// object or creates the object if not exist. +func NewAPIApplicator(c client.Client) *APIApplicator { + return &APIApplicator{ + creatorFn(createOrGetExisting), + patcherFn(threeWayMergePatch), c} +} + +type creator interface { + createOrGetExisting(context.Context, client.Client, runtime.Object, ...ApplyOption) (runtime.Object, error) +} + +type creatorFn func(context.Context, client.Client, runtime.Object, ...ApplyOption) (runtime.Object, error) + +func (fn creatorFn) createOrGetExisting(ctx context.Context, c client.Client, o runtime.Object, ao ...ApplyOption) (runtime.Object, error) { + return fn(ctx, c, o, ao...) +} + +type patcher interface { + patch(c, m runtime.Object) (client.Patch, error) +} + +type patcherFn func(c, m runtime.Object) (client.Patch, error) + +func (fn patcherFn) patch(c, m runtime.Object) (client.Patch, error) { + return fn(c, m) +} + +// APIApplicator implements Applicator +type APIApplicator struct { + creator + patcher + c client.Client +} + +// Apply applies new state to an object or create it if not exist +func (a *APIApplicator) Apply(ctx context.Context, desired runtime.Object, ao ...ApplyOption) error { + existing, err := a.createOrGetExisting(ctx, a.c, desired, ao...) + if err != nil { + return err + } + if existing == nil { + return nil + } + + // the object already exists, apply new state + if err := executeApplyOptions(ctx, existing, desired, ao); err != nil { + return err + } + patch, err := a.patcher.patch(existing, desired) + if err != nil { + return errors.Wrap(err, "cannot calculate patch by computing a three way diff") + } + return errors.Wrapf(a.c.Patch(ctx, desired, patch), "cannot patch object") +} + +// createOrGetExisting will create the object if it does not exist +// or get and return the existing object +func createOrGetExisting(ctx context.Context, c client.Client, desired runtime.Object, ao ...ApplyOption) (runtime.Object, error) { + m, ok := desired.(Object) + if !ok { + return nil, errors.New("cannot access object metadata") + } + + var create = func() (runtime.Object, error) { + // execute ApplyOptions even the object doesn't exist + if err := executeApplyOptions(ctx, nil, desired, ao); err != nil { + return nil, err + } + if err := addLastAppliedConfigAnnotation(desired); err != nil { + return nil, err + } + return nil, errors.Wrap(c.Create(ctx, desired), "cannot create object") + } + + // allow to create object with only generateName + if m.GetName() == "" && m.GetGenerateName() != "" { + return create() + } + + existing := &unstructured.Unstructured{} + existing.GetObjectKind().SetGroupVersionKind(desired.GetObjectKind().GroupVersionKind()) + err := c.Get(ctx, types.NamespacedName{Name: m.GetName(), Namespace: m.GetNamespace()}, existing) + if kerrors.IsNotFound(err) { + return create() + } + if err != nil { + return nil, errors.Wrap(err, "cannot get object") + } + return existing, nil +} + +func executeApplyOptions(ctx context.Context, existing, desired runtime.Object, aos []ApplyOption) error { + // if existing is nil, it means the object is going to be created. + // ApplyOption function should handle this situation carefully by itself. + for _, fn := range aos { + if err := fn(ctx, existing, desired); err != nil { + return errors.Wrap(err, "cannot apply ApplyOption") + } + } + return nil +} + +// MustBeControllableBy requires that the existing object is controllable by an +// object with the supplied UID. An object is controllable if its controller +// reference matches the supplied UID, or it has no controller reference. An +// error will be returned if the current object cannot be controlled by the +// supplied UID. +// ACKNOWLEDGMENTS: The code was based in part on the source code of +// - github.com/crossplane/crossplane-runtime/pkg/resource/resource.go#L274 +func MustBeControllableBy(u types.UID) ApplyOption { + return func(_ context.Context, existing, _ runtime.Object) error { + if existing == nil { + return nil + } + c := metav1.GetControllerOf(existing.(metav1.Object)) + if c == nil { + return nil + } + + if c.UID != u { + return errors.Errorf("existing object is not controlled by UID %q", u) + } + return nil + } +} diff --git a/pkg/utils/apply/apply_resource_test.go b/pkg/utils/apply/apply_resource_test.go new file mode 100644 index 000000000..12949723b --- /dev/null +++ b/pkg/utils/apply/apply_resource_test.go @@ -0,0 +1,196 @@ +package apply + +import ( + "context" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/pointer" + + oamstd "github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1" + oamutil "github.com/oam-dev/kubevela/pkg/oam/util" +) + +var _ = Describe("Test apply", func() { + var ( + int32_3 = int32(3) + int32_5 = int32(5) + ctx = context.Background() + deploy *appsv1.Deployment + podspec *oamstd.PodSpecWorkload + deployKey = types.NamespacedName{ + Name: "testdeploy", + Namespace: ns, + } + podspecKey = types.NamespacedName{ + Name: "testpodspec", + Namespace: ns, + } + ) + + BeforeEach(func() { + deploy = basicTestDeployment() + podspec = basicTestPodSpecWorkload() + + Expect(k8sApplicator.Apply(ctx, deploy)).Should(Succeed()) + Expect(k8sApplicator.Apply(ctx, podspec)).Should(Succeed()) + }) + + AfterEach(func() { + Expect(rawClient.Delete(ctx, deploy)).Should(SatisfyAny(Succeed(), &oamutil.NotFoundMatcher{})) + Expect(rawClient.Delete(ctx, podspec)).Should(SatisfyAny(Succeed(), &oamutil.NotFoundMatcher{})) + }) + + Context("Test apply resources", func() { + It("Test apply core resources", func() { + deploy = basicTestDeployment() + By("Set normal & array field") + deploy.Spec.Replicas = &int32_3 + deploy.Spec.Template.Spec.Volumes = []corev1.Volume{{Name: "test"}} + Expect(k8sApplicator.Apply(ctx, deploy)).Should(Succeed()) + resultDeploy := basicTestDeployment() + Expect(rawClient.Get(ctx, deployKey, resultDeploy)).Should(Succeed()) + Expect(*resultDeploy.Spec.Replicas).Should(Equal(int32_3)) + Expect(len(resultDeploy.Spec.Template.Spec.Volumes)).Should(Equal(1)) + + deploy = basicTestDeployment() + By("Override normal & array field") + deploy.Spec.Replicas = &int32_5 + deploy.Spec.Template.Spec.Volumes = []corev1.Volume{{Name: "test"}, {Name: "test2"}} + Expect(k8sApplicator.Apply(ctx, deploy)).Should(Succeed()) + resultDeploy = basicTestDeployment() + Expect(rawClient.Get(ctx, deployKey, resultDeploy)).Should(Succeed()) + Expect(*resultDeploy.Spec.Replicas).Should(Equal(int32_5)) + Expect(len(resultDeploy.Spec.Template.Spec.Volumes)).Should(Equal(2)) + + deploy = basicTestDeployment() + By("Unset normal & array field") + deploy.Spec.Replicas = nil + deploy.Spec.Template.Spec.Volumes = nil + Expect(k8sApplicator.Apply(ctx, deploy)).Should(Succeed()) + resultDeploy = basicTestDeployment() + Expect(rawClient.Get(ctx, deployKey, resultDeploy)).Should(Succeed()) + By("Unsetted fields shoulde be removed or set to default value") + Expect(*resultDeploy.Spec.Replicas).Should(Equal(int32(1))) + Expect(len(resultDeploy.Spec.Template.Spec.Volumes)).Should(Equal(0)) + }) + + It("Test apply custom resources", func() { + // use standard.oam.dev/v1alpha1 podSpecWorkload as sample CR + podspec = basicTestPodSpecWorkload() + By("Set normal & array field") + podspec.Spec.Replicas = &int32_3 + podspec.Spec.PodSpec.Volumes = []corev1.Volume{{Name: "test"}} + Expect(k8sApplicator.Apply(ctx, podspec)).Should(Succeed()) + resultPodSpec := basicTestPodSpecWorkload() + Expect(rawClient.Get(ctx, podspecKey, resultPodSpec)).Should(Succeed()) + Expect(*resultPodSpec.Spec.Replicas).Should(Equal(int32_3)) + Expect(len(resultPodSpec.Spec.PodSpec.Volumes)).Should(Equal(1)) + + podspec = basicTestPodSpecWorkload() + By("Override normal & array field") + podspec.Spec.Replicas = &int32_5 + podspec.Spec.PodSpec.Volumes = []corev1.Volume{{Name: "test"}, {Name: "test2"}} + Expect(k8sApplicator.Apply(ctx, podspec)).Should(Succeed()) + resultPodSpec = basicTestPodSpecWorkload() + Expect(rawClient.Get(ctx, podspecKey, resultPodSpec)).Should(Succeed()) + Expect(*resultPodSpec.Spec.Replicas).Should(Equal(int32_5)) + Expect(len(resultPodSpec.Spec.PodSpec.Volumes)).Should(Equal(2)) + + podspec = basicTestPodSpecWorkload() + By("Unset normal & array field") + podspec.Spec.Replicas = nil + podspec.Spec.PodSpec.Volumes = nil + Expect(k8sApplicator.Apply(ctx, podspec)).Should(Succeed()) + resultPodSpec = basicTestPodSpecWorkload() + Expect(rawClient.Get(ctx, podspecKey, resultPodSpec)).Should(Succeed()) + By("Unsetted fields shoulde be removed") + Expect(resultPodSpec.Spec.Replicas).Should(BeNil()) + Expect(len(resultPodSpec.Spec.PodSpec.Volumes)).Should(Equal(0)) + }) + + It("Test multiple appliers", func() { + deploy = basicTestDeployment() + originalDeploy := deploy.DeepCopy() + Expect(k8sApplicator.Apply(ctx, deploy)).Should(Succeed()) + + modifiedDeploy := &appsv1.Deployment{} + modifiedDeploy.SetGroupVersionKind(deploy.GroupVersionKind()) + Expect(rawClient.Get(ctx, deployKey, modifiedDeploy)).Should(Succeed()) + By("Other applier changed the deployment") + modifiedDeploy.Spec.MinReadySeconds = 10 + modifiedDeploy.Spec.ProgressDeadlineSeconds = pointer.Int32Ptr(20) + modifiedDeploy.Spec.Template.Spec.Volumes = []corev1.Volume{{Name: "test"}} + Expect(rawClient.Update(ctx, modifiedDeploy)).Should(Succeed()) + + By("Original applier apply again") + Expect(k8sApplicator.Apply(ctx, originalDeploy)).Should(Succeed()) + resultDeploy := basicTestDeployment() + Expect(rawClient.Get(ctx, deployKey, resultDeploy)).Should(Succeed()) + + By("Check the changes from other applier are not effected") + Expect(resultDeploy.Spec.MinReadySeconds).Should(Equal(int32(10))) + Expect(*resultDeploy.Spec.ProgressDeadlineSeconds).Should(Equal(int32(20))) + Expect(len(resultDeploy.Spec.Template.Spec.Volumes)).Should(Equal(1)) + }) + }) +}) + +func basicTestDeployment() *appsv1.Deployment { + return &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + APIVersion: "apps/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "testdeploy", + Namespace: ns, + }, + Spec: appsv1.DeploymentSpec{ + // Replicas: x // normal field with default value + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "nginx", + }, + }, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ // array field + { + Name: "nginx", + Image: "nginx:1.9.4", // normal field without default value + }, + }, + }, + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "nginx"}}, + }, + }, + } +} + +func basicTestPodSpecWorkload() *oamstd.PodSpecWorkload { + return &oamstd.PodSpecWorkload{ + TypeMeta: metav1.TypeMeta{ + Kind: "PodSpecWorkload", + APIVersion: "standard.oam.dev/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "testpodspec", + Namespace: ns, + }, + Spec: oamstd.PodSpecWorkloadSpec{ + // Replicas: x (normal field) + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{ // array field + {Name: "nginx", + Image: "nginx:1.9.4"}, + }, + }, + }, + } +} diff --git a/pkg/utils/apply/apply_suite_test.go b/pkg/utils/apply/apply_suite_test.go new file mode 100644 index 000000000..77cccf0f7 --- /dev/null +++ b/pkg/utils/apply/apply_suite_test.go @@ -0,0 +1,75 @@ +package apply + +import ( + "context" + "path/filepath" + "testing" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + oamcore "github.com/oam-dev/kubevela/apis/core.oam.dev" + oamstd "github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +var mgrclose chan struct{} +var testEnv *envtest.Environment +var cfg *rest.Config +var rawClient client.Client +var k8sApplicator Applicator +var testScheme = runtime.NewScheme() +var ns = "test-apply" +var applyNS corev1.Namespace + +func TestApplicator(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Applicator Suite") +} + +var _ = BeforeSuite(func(done Done) { + By("Bootstrapping test environment") + testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{ + filepath.Join("../../..", "charts/vela-core/crds"), // this has all the required CRDs, + }, + } + var err error + cfg, err = testEnv.Start() + Expect(err).ShouldNot(HaveOccurred()) + Expect(cfg).ShouldNot(BeNil()) + + logf.SetLogger(zap.New(zap.UseDevMode(true), zap.WriteTo(GinkgoWriter))) + Expect(clientgoscheme.AddToScheme(testScheme)).Should(Succeed()) + Expect(oamcore.AddToScheme(testScheme)).Should(Succeed()) + Expect(oamstd.AddToScheme(testScheme)).Should(Succeed()) + + By("Setting up applicator") + rawClient, err = client.New(cfg, client.Options{Scheme: testScheme}) + Expect(err).ShouldNot(HaveOccurred()) + Expect(rawClient).ShouldNot(BeNil()) + k8sApplicator = NewAPIApplicator(rawClient) + + By("Create test namespace") + applyNS = corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: ns, + }, + } + Expect(rawClient.Create(context.Background(), &applyNS)).Should(Succeed()) + + close(done) +}, 300) + +var _ = AfterSuite(func() { + Expect(testEnv.Stop()).Should(Succeed()) +}) diff --git a/pkg/utils/apply/apply_test.go b/pkg/utils/apply/apply_test.go new file mode 100644 index 000000000..26e83bd8d --- /dev/null +++ b/pkg/utils/apply/apply_test.go @@ -0,0 +1,356 @@ +package apply + +import ( + "context" + "testing" + + "github.com/crossplane/crossplane-runtime/pkg/test" + "github.com/google/go-cmp/cmp" + "github.com/pkg/errors" + appsv1 "k8s.io/api/apps/v1" + kerrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var ctx = context.Background() +var errFake = errors.New("fake error") + +type testObject struct { + runtime.Object + metav1.ObjectMeta +} + +func (t *testObject) DeepCopyObject() runtime.Object { + return &testObject{ObjectMeta: *t.ObjectMeta.DeepCopy()} +} + +func (t *testObject) GetObjectKind() schema.ObjectKind { + return schema.EmptyObjectKind +} + +type testNoMetaObject struct { + runtime.Object +} + +func TestAPIApplicator(t *testing.T) { + existing := &testObject{} + existing.SetName("existing") + desired := &testObject{} + desired.SetName("desired") + // use Deployment as a registered API sample + testDeploy := &appsv1.Deployment{} + testDeploy.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "apps", + Version: "v1", + Kind: "Deployment", + }) + type args struct { + existing runtime.Object + creatorErr error + patcherErr error + desired runtime.Object + ao []ApplyOption + } + + cases := map[string]struct { + reason string + c client.Client + args args + want error + }{ + "ErrorOccursCreatOrGetExisting": { + reason: "An error should be returned if cannot create or get existing", + args: args{ + creatorErr: errFake, + }, + want: errFake, + }, + "CreateSuccessfully": { + reason: "No error should be returned if create successfully", + }, + "CannotApplyApplyOptions": { + reason: "An error should be returned if cannot apply ApplyOption", + args: args{ + existing: existing, + ao: []ApplyOption{ + func(ctx context.Context, existing, desired runtime.Object) error { + return errFake + }, + }, + }, + want: errors.Wrap(errFake, "cannot apply ApplyOption"), + }, + "CalculatePatchError": { + reason: "An error should be returned if patch failed", + args: args{ + existing: existing, + desired: desired, + patcherErr: errFake, + }, + c: &test.MockClient{MockPatch: test.NewMockPatchFn(errFake)}, + want: errors.Wrap(errFake, "cannot calculate patch by computing a three way diff"), + }, + "PatchError": { + reason: "An error should be returned if patch failed", + args: args{ + existing: existing, + desired: testDeploy, + }, + c: &test.MockClient{MockPatch: test.NewMockPatchFn(errFake)}, + want: errors.Wrap(errFake, "cannot patch object"), + }, + "PatchingApplySuccessfully": { + reason: "No error should be returned if patch successfully", + args: args{ + existing: existing, + desired: desired, + }, + c: &test.MockClient{MockPatch: test.NewMockPatchFn(nil)}, + }, + } + + for caseName, tc := range cases { + t.Run(caseName, func(t *testing.T) { + a := &APIApplicator{ + creator: creatorFn(func(_ context.Context, _ client.Client, _ runtime.Object, _ ...ApplyOption) (runtime.Object, error) { + return tc.args.existing, tc.args.creatorErr + }), + patcher: patcherFn(func(c, m runtime.Object) (client.Patch, error) { + return nil, tc.args.patcherErr + }), + c: tc.c, + } + result := a.Apply(ctx, tc.args.desired, tc.args.ao...) + if diff := cmp.Diff(tc.want, result, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\nApply(...): -want , +got \n%s\n", tc.reason, diff) + } + }) + } +} + +func TestCreator(t *testing.T) { + desired := &unstructured.Unstructured{} + desired.SetName("desired") + type args struct { + desired runtime.Object + ao []ApplyOption + } + type want struct { + existing runtime.Object + err error + } + + cases := map[string]struct { + reason string + c client.Client + args args + want want + }{ + "NotAMetadataObject": { + reason: "An error should be returned if cannot access metadata of the desired object", + args: args{ + desired: &testNoMetaObject{}, + }, + want: want{ + existing: nil, + err: errors.New("cannot access object metadata"), + }, + }, + "CannotCreateObjectWithoutName": { + reason: "An error should be returned if cannot create the object", + args: args{ + desired: &testObject{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "prefix", + }, + }, + }, + c: &test.MockClient{MockCreate: test.NewMockCreateFn(errFake)}, + want: want{ + existing: nil, + err: errors.Wrap(errFake, "cannot create object"), + }, + }, + "CannotCreate": { + reason: "An error should be returned if cannot create the object", + c: &test.MockClient{ + MockCreate: test.NewMockCreateFn(errFake), + MockGet: test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{}, ""))}, + args: args{ + desired: desired, + }, + want: want{ + existing: nil, + err: errors.Wrap(errFake, "cannot create object"), + }, + }, + "CannotGetExisting": { + reason: "An error should be returned if cannot get the object", + c: &test.MockClient{ + MockGet: test.NewMockGetFn(errFake)}, + args: args{ + desired: desired, + }, + want: want{ + existing: nil, + err: errors.Wrap(errFake, "cannot get object"), + }, + }, + "ApplyOptionErrorWhenCreatObjectWithoutName": { + reason: "An error should be returned if cannot apply ApplyOption", + args: args{ + desired: &testObject{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "prefix", + }, + }, + ao: []ApplyOption{ + func(ctx context.Context, existing, desired runtime.Object) error { + return errFake + }, + }, + }, + want: want{ + existing: nil, + err: errors.Wrap(errFake, "cannot apply ApplyOption"), + }, + }, + "ApplyOptionErrorWhenCreatObject": { + reason: "An error should be returned if cannot apply ApplyOption", + c: &test.MockClient{MockGet: test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{}, ""))}, + args: args{ + desired: desired, + ao: []ApplyOption{ + func(ctx context.Context, existing, desired runtime.Object) error { + return errFake + }, + }, + }, + want: want{ + existing: nil, + err: errors.Wrap(errFake, "cannot apply ApplyOption"), + }, + }, + "CreateWithoutNameSuccessfully": { + reason: "No error and existing should be returned if create successfully", + c: &test.MockClient{MockCreate: test.NewMockCreateFn(nil)}, + args: args{ + desired: &testObject{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "prefix", + }, + }, + }, + want: want{ + existing: nil, + err: nil, + }, + }, + "CreateSuccessfully": { + reason: "No error and existing should be returned if create successfully", + c: &test.MockClient{ + MockCreate: test.NewMockCreateFn(nil), + MockGet: test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{}, ""))}, + args: args{ + desired: desired, + }, + want: want{ + existing: nil, + err: nil, + }, + }, + "GetExistingSuccessfully": { + reason: "Existing object and no error should be returned", + c: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(obj runtime.Object) error { + o, _ := obj.(*unstructured.Unstructured) + *o = *desired + return nil + })}, + args: args{ + desired: desired, + }, + want: want{ + existing: desired, + err: nil, + }, + }, + } + + for caseName, tc := range cases { + t.Run(caseName, func(t *testing.T) { + result, err := createOrGetExisting(ctx, tc.c, tc.args.desired, tc.args.ao...) + if diff := cmp.Diff(tc.want.existing, result); diff != "" { + t.Errorf("\n%s\ncreateOrGetExisting(...): -want , +got \n%s\n", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\ncreateOrGetExisting(...): -want error, +got error\n%s\n", tc.reason, diff) + } + }) + } + +} + +func TestMustBeControllableBy(t *testing.T) { + uid := types.UID("very-unique-string") + controller := true + type args struct { + ctx context.Context + current runtime.Object + desired runtime.Object + } + + cases := map[string]struct { + reason string + u types.UID + args args + want error + }{ + "NoExistingObject": { + reason: "No error should be returned if no existing object", + }, + "Adoptable": { + reason: "A current object with no controller reference may be adopted and controlled", + u: uid, + args: args{ + current: &testObject{}, + }, + }, + "ControlledBySuppliedUID": { + reason: "A current object that is already controlled by the supplied UID is controllable", + u: uid, + args: args{ + current: &testObject{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{ + UID: uid, + Controller: &controller, + }}}}, + }, + }, + "ControlledBySomeoneElse": { + reason: "A current object that is already controlled by a different UID is not controllable", + u: uid, + args: args{ + current: &testObject{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{ + UID: types.UID("some-other-uid"), + Controller: &controller, + }}}}, + }, + want: errors.Errorf("existing object is not controlled by UID %q", uid), + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + ao := MustBeControllableBy(tc.u) + err := ao(tc.args.ctx, tc.args.current, tc.args.desired) + if diff := cmp.Diff(tc.want, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\nMustBeControllableBy(...)(...): -want error, +got error\n%s\n", tc.reason, diff) + } + }) + } +} diff --git a/pkg/utils/apply/patch.go b/pkg/utils/apply/patch.go new file mode 100644 index 000000000..935484221 --- /dev/null +++ b/pkg/utils/apply/patch.go @@ -0,0 +1,151 @@ +package apply + +import ( + "encoding/json" + + "github.com/pkg/errors" + + "github.com/oam-dev/kubevela/pkg/oam" + + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/jsonmergepatch" + "k8s.io/apimachinery/pkg/util/mergepatch" + "k8s.io/apimachinery/pkg/util/strategicpatch" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var k8sScheme = runtime.NewScheme() +var metadataAccessor = meta.NewAccessor() + +func init() { + _ = clientgoscheme.AddToScheme(k8sScheme) +} + +// threeWayMergePatch creates a patch by computing a three way diff based on +// its current state, modified state, and last-applied-state recorded in the +// annotation. +func threeWayMergePatch(currentObj, modifiedObj runtime.Object) (client.Patch, error) { + current, err := json.Marshal(currentObj) + if err != nil { + return nil, err + } + original, err := getOriginalConfiguration(currentObj) + if err != nil { + return nil, err + } + modified, err := getModifiedConfiguration(modifiedObj, true) + if err != nil { + return nil, err + } + + var patchType types.PatchType + var patchData []byte + var lookupPatchMeta strategicpatch.LookupPatchMeta + + versionedObject, err := k8sScheme.New(currentObj.GetObjectKind().GroupVersionKind()) + switch { + case runtime.IsNotRegisteredError(err): + // use JSONMergePatch for custom resources + // because StrategicMergePatch doesn't support custom resources + patchType = types.MergePatchType + preconditions := []mergepatch.PreconditionFunc{ + mergepatch.RequireKeyUnchanged("apiVersion"), + mergepatch.RequireKeyUnchanged("kind"), + mergepatch.RequireMetadataKeyUnchanged("name")} + patchData, err = jsonmergepatch.CreateThreeWayJSONMergePatch(original, modified, current, preconditions...) + if err != nil { + return nil, err + } + case err != nil: + return nil, err + default: + // use StrategicMergePatch for K8s built-in resources + patchType = types.StrategicMergePatchType + lookupPatchMeta, err = strategicpatch.NewPatchMetaFromStruct(versionedObject) + if err != nil { + return nil, err + } + patchData, err = strategicpatch.CreateThreeWayMergePatch(original, modified, current, lookupPatchMeta, true) + if err != nil { + return nil, err + } + } + return client.RawPatch(patchType, patchData), nil +} + +// addLastAppliedConfigAnnotation creates annotation recording current configuration as +// original configuration for latter use in computing a three way diff +func addLastAppliedConfigAnnotation(obj runtime.Object) error { + config, err := getModifiedConfiguration(obj, false) + if err != nil { + return err + } + annots, _ := metadataAccessor.Annotations(obj) + if annots == nil { + annots = make(map[string]string) + } + annots[oam.AnnotationLastAppliedConfig] = string(config) + return metadataAccessor.SetAnnotations(obj, annots) +} + +// getModifiedConfiguration serializes the object into byte stream. +// If `updateAnnotation` is true, it embeds the result as an annotation in the +// modified configuration. +func getModifiedConfiguration(obj runtime.Object, updateAnnotation bool) ([]byte, error) { + annots, err := metadataAccessor.Annotations(obj) + if err != nil { + return nil, errors.Wrap(err, "cannot access metadata.annotations") + } + if annots == nil { + annots = make(map[string]string) + } + + original := annots[oam.AnnotationLastAppliedConfig] + // remove the annotation to avoid recursion + delete(annots, oam.AnnotationLastAppliedConfig) + _ = metadataAccessor.SetAnnotations(obj, annots) + // do not include an empty map + if len(annots) == 0 { + _ = metadataAccessor.SetAnnotations(obj, nil) + } + + var modified []byte + modified, err = json.Marshal(obj) + if err != nil { + return nil, err + } + + if updateAnnotation { + annots[oam.AnnotationLastAppliedConfig] = string(modified) + _ = metadataAccessor.SetAnnotations(obj, annots) + modified, err = json.Marshal(obj) + if err != nil { + return nil, err + } + } + + // restore original annotations back to the object + annots[oam.AnnotationLastAppliedConfig] = original + _ = metadataAccessor.SetAnnotations(obj, annots) + return modified, nil +} + +// getOriginalConfiguration gets original configuration of the object +// form the annotation, or nil if no annotation found. +func getOriginalConfiguration(obj runtime.Object) ([]byte, error) { + annots, err := metadataAccessor.Annotations(obj) + if err != nil { + return nil, errors.Wrap(err, "cannot access metadata.annotations") + } + if annots == nil { + return nil, nil + } + original, ok := annots[oam.AnnotationLastAppliedConfig] + if !ok { + return nil, nil + } + return []byte(original), nil +} diff --git a/pkg/utils/apply/patch_test.go b/pkg/utils/apply/patch_test.go new file mode 100644 index 000000000..7b1bfcbc3 --- /dev/null +++ b/pkg/utils/apply/patch_test.go @@ -0,0 +1,78 @@ +package apply + +import ( + "fmt" + "testing" + + "github.com/crossplane/crossplane-runtime/pkg/test" + "github.com/google/go-cmp/cmp" + "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" +) + +func TestAddLastAppliedConfig(t *testing.T) { + cases := map[string]struct { + reason string + obj runtime.Object + wantObj runtime.Object + wantErr error + }{ + "ErrCannotAccessMetadata": { + reason: "An error should be returned if cannot access metadata", + obj: testNoMetaObject{}, + wantObj: testNoMetaObject{}, + wantErr: errors.Wrap(fmt.Errorf("object does not implement the Object interfaces"), "cannot access metadata.annotations"), + }, + } + + for caseName, tc := range cases { + t.Run(caseName, func(t *testing.T) { + err := addLastAppliedConfigAnnotation(tc.obj) + if diff := cmp.Diff(tc.wantObj, tc.obj); diff != "" { + t.Errorf("\n%s\ngetModifiedConfig(...): -want , +got \n%s\n", tc.reason, diff) + } + if diff := cmp.Diff(tc.wantErr, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\ngetModifiedConfig(...): -want , +got \n%s\n", tc.reason, diff) + } + }) + } +} + +func TestGetOriginalConfig(t *testing.T) { + objNoAnno := &unstructured.Unstructured{} + objNoAnno.SetAnnotations(make(map[string]string)) + + cases := map[string]struct { + reason string + obj runtime.Object + wantConfig string + wantErr error + }{ + "ErrCannotAccessMetadata": { + reason: "An error should be returned if cannot access metadata", + obj: testNoMetaObject{}, + wantErr: errors.Wrap(fmt.Errorf("object does not implement the Object interfaces"), "cannot access metadata.annotations"), + }, + "NoAnnotations": { + reason: "No error should be returned if cannot find last-applied-config annotaion", + obj: &unstructured.Unstructured{}, + }, + "LastAppliedConfigAnnotationNotFound": { + reason: "No error should be returned if cannot find last-applied-config annotaion", + obj: objNoAnno, + }, + } + + for caseName, tc := range cases { + t.Run(caseName, func(t *testing.T) { + r, err := getOriginalConfiguration(tc.obj) + if diff := cmp.Diff(tc.wantConfig, string(r)); diff != "" { + t.Errorf("\n%s\ngetModifiedConfig(...): -want , +got \n%s\n", tc.reason, diff) + } + if diff := cmp.Diff(tc.wantErr, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\ngetModifiedConfig(...): -want , +got \n%s\n", tc.reason, diff) + } + }) + } +} diff --git a/test/integration/appconfig_test.go b/test/integration/appconfig_test.go index 3b21ebb7e..e7c21b0af 100644 --- a/test/integration/appconfig_test.go +++ b/test/integration/appconfig_test.go @@ -101,12 +101,6 @@ func TestAppConfigController(t *testing.T) { Name: envVars[2], }, }, - Ports: []v1alpha2.ContainerPort{ - { - Name: "http", - Port: 8080, - }, - }, }, }), )