Skip to content

Commit aba3819

Browse files
leoluzcrenshaw-dev
andauthored
feat: Implement Server-Side Diffs (#522)
* feat: Implement Server-Side Diffs Signed-off-by: Leonardo Luz Almeida <[email protected]> * trigger build Signed-off-by: Leonardo Luz Almeida <[email protected]> * chore: remove unused function Signed-off-by: Leonardo Luz Almeida <[email protected]> * make HasAnnotationOption more generic Signed-off-by: Leonardo Luz Almeida <[email protected]> * add server-side-diff printer option Signed-off-by: Leonardo Luz Almeida <[email protected]> * remove managedFields during server-side-diff Signed-off-by: Leonardo Luz Almeida <[email protected]> * add ignore mutation webhook logic Signed-off-by: Leonardo Luz Almeida <[email protected]> * fix configSet Signed-off-by: Leonardo Luz Almeida <[email protected]> * Fix comparison Signed-off-by: Leonardo Luz Almeida <[email protected]> * merge typedconfig in typedpredictedlive Signed-off-by: Leonardo Luz Almeida <[email protected]> * handle webhook diff conflicts Signed-off-by: Leonardo Luz Almeida <[email protected]> * Fix webhook normalization logic Signed-off-by: Leonardo Luz Almeida <[email protected]> * address review comments 1/2 Signed-off-by: Leonardo Luz Almeida <[email protected]> * address review comments 2/2 Signed-off-by: Leonardo Luz Almeida <[email protected]> * fix lint Signed-off-by: Leonardo Luz Almeida <[email protected]> * remove kubectl getter from cluster-cache Signed-off-by: Leonardo Luz Almeida <[email protected]> * fix query param verifier instantiation Signed-off-by: Michael Crenshaw <[email protected]> * Add server-side-diff unit tests Signed-off-by: Leonardo Luz Almeida <[email protected]> --------- Signed-off-by: Leonardo Luz Almeida <[email protected]> Signed-off-by: Michael Crenshaw <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]>
1 parent c0c2dd1 commit aba3819

14 files changed

+548
-19
lines changed

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ require (
2222
k8s.io/kube-openapi v0.0.0-20230501164219-8b0f38b5fd1f
2323
k8s.io/kubectl v0.26.4
2424
k8s.io/kubernetes v1.26.4
25-
sigs.k8s.io/structured-merge-diff/v4 v4.2.3
25+
sigs.k8s.io/structured-merge-diff/v4 v4.4.1
2626
sigs.k8s.io/yaml v1.3.0
2727
)
2828

go.sum

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -670,7 +670,7 @@ sigs.k8s.io/kustomize/api v0.12.1 h1:7YM7gW3kYBwtKvoY216ZzY+8hM+lV53LUayghNRJ0vM
670670
sigs.k8s.io/kustomize/api v0.12.1/go.mod h1:y3JUhimkZkR6sbLNwfJHxvo1TCLwuwm14sCYnkH6S1s=
671671
sigs.k8s.io/kustomize/kyaml v0.13.9 h1:Qz53EAaFFANyNgyOEJbT/yoIHygK40/ZcvU3rgry2Tk=
672672
sigs.k8s.io/kustomize/kyaml v0.13.9/go.mod h1:QsRbD0/KcU+wdk0/L0fIp2KLnohkVzs6fQ85/nOXac4=
673-
sigs.k8s.io/structured-merge-diff/v4 v4.2.3 h1:PRbqxJClWWYMNV1dhaG4NsibJbArud9kFxnAMREiWFE=
674-
sigs.k8s.io/structured-merge-diff/v4 v4.2.3/go.mod h1:qjx8mGObPmV2aSZepjQjbmb2ihdVs8cGKBraizNC69E=
673+
sigs.k8s.io/structured-merge-diff/v4 v4.4.1 h1:150L+0vs/8DA78h1u02ooW1/fFq/Lwr+sGiqlzvrtq4=
674+
sigs.k8s.io/structured-merge-diff/v4 v4.4.1/go.mod h1:N8hJocpFajUSSeSJ9bOZ77VzejKZaXsTtZo4/u7Io08=
675675
sigs.k8s.io/yaml v1.3.0 h1:a2VclLzOGrwOHDiV8EfBGhvjHvP46CtW5j6POvhYGGo=
676676
sigs.k8s.io/yaml v1.3.0/go.mod h1:GeOyir5tyXNByN85N/dRIT9es5UQNerPYEKK56eTBm8=

pkg/diff/diff.go

Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package diff
66

77
import (
88
"bytes"
9+
"context"
910
"encoding/json"
1011
"errors"
1112
"fmt"
@@ -83,6 +84,14 @@ func Diff(config, live *unstructured.Unstructured, opts ...Option) (*DiffResult,
8384
Normalize(live, opts...)
8485
}
8586

87+
if o.serverSideDiff {
88+
r, err := ServerSideDiff(config, live, opts...)
89+
if err != nil {
90+
return nil, fmt.Errorf("error calculating server side diff: %w", err)
91+
}
92+
return r, nil
93+
}
94+
8695
// TODO The two variables bellow are necessary because there is a cyclic
8796
// dependency with the kube package that blocks the usage of constants
8897
// from common package. common package needs to be refactored and exclude
@@ -120,6 +129,165 @@ func Diff(config, live *unstructured.Unstructured, opts ...Option) (*DiffResult,
120129
return TwoWayDiff(config, live)
121130
}
122131

132+
// ServerSideDiff will execute a k8s server-side apply in dry-run mode with the
133+
// given config. The result will be compared with given live resource to determine
134+
// diff. If config or live are nil it means resource creation or deletion. In this
135+
// no call will be made to kube-api and a simple diff will be returned.
136+
func ServerSideDiff(config, live *unstructured.Unstructured, opts ...Option) (*DiffResult, error) {
137+
if live != nil && config != nil {
138+
result, err := serverSideDiff(config, live, opts...)
139+
if err != nil {
140+
return nil, fmt.Errorf("serverSideDiff error: %w", err)
141+
}
142+
return result, nil
143+
}
144+
// Currently, during resource creation a shallow diff (non ServerSide apply
145+
// based) will be returned. The reasons are:
146+
// - Saves 1 additional call to KubeAPI
147+
// - Much lighter/faster diff
148+
// - This is the existing behaviour users are already used to
149+
// - No direct benefit to the user
150+
result, err := handleResourceCreateOrDeleteDiff(config, live)
151+
if err != nil {
152+
return nil, fmt.Errorf("error handling resource creation or deletion: %w", err)
153+
}
154+
return result, nil
155+
}
156+
157+
// ServerSideDiff will execute a k8s server-side apply in dry-run mode with the
158+
// given config. The result will be compared with given live resource to determine
159+
// diff. Modifications done by mutation webhooks are removed from the diff by default.
160+
// This behaviour can be customized with Option.WithIgnoreMutationWebhook.
161+
func serverSideDiff(config, live *unstructured.Unstructured, opts ...Option) (*DiffResult, error) {
162+
o := applyOptions(opts)
163+
if o.serverSideDryRunner == nil {
164+
return nil, fmt.Errorf("serverSideDryRunner is null")
165+
}
166+
predictedLiveStr, err := o.serverSideDryRunner.Run(context.Background(), config, o.manager)
167+
if err != nil {
168+
return nil, fmt.Errorf("error running server side apply in dryrun mode: %w", err)
169+
}
170+
predictedLive, err := jsonStrToUnstructured(predictedLiveStr)
171+
if err != nil {
172+
return nil, fmt.Errorf("error converting json string to unstructured: %w", err)
173+
}
174+
175+
if o.ignoreMutationWebhook {
176+
predictedLive, err = removeWebhookMutation(predictedLive, live, o.gvkParser, o.manager)
177+
if err != nil {
178+
return nil, fmt.Errorf("error removing non config mutations: %w", err)
179+
}
180+
}
181+
182+
Normalize(predictedLive, opts...)
183+
unstructured.RemoveNestedField(predictedLive.Object, "metadata", "managedFields")
184+
185+
predictedLiveBytes, err := json.Marshal(predictedLive)
186+
if err != nil {
187+
return nil, fmt.Errorf("error marshaling predicted live resource: %w", err)
188+
}
189+
190+
unstructured.RemoveNestedField(live.Object, "metadata", "managedFields")
191+
liveBytes, err := json.Marshal(live)
192+
if err != nil {
193+
return nil, fmt.Errorf("error marshaling live resource: %w", err)
194+
}
195+
return buildDiffResult(predictedLiveBytes, liveBytes), nil
196+
}
197+
198+
// removeWebhookMutation will compare the predictedLive with live to identify
199+
// changes done by mutation webhooks. Webhook mutations are identified by finding
200+
// changes in predictedLive fields not associated with any manager in the
201+
// managedFields. All fields under this condition will be reverted with their state
202+
// from live. If the given predictedLive does not have the managedFields, an error
203+
// will be returned.
204+
func removeWebhookMutation(predictedLive, live *unstructured.Unstructured, gvkParser *managedfields.GvkParser, manager string) (*unstructured.Unstructured, error) {
205+
plManagedFields := predictedLive.GetManagedFields()
206+
if len(plManagedFields) == 0 {
207+
return nil, fmt.Errorf("predictedLive for resource %s/%s must have the managedFields", predictedLive.GetKind(), predictedLive.GetName())
208+
}
209+
gvk := predictedLive.GetObjectKind().GroupVersionKind()
210+
pt := gvkParser.Type(gvk)
211+
typedPredictedLive, err := pt.FromUnstructured(predictedLive.Object)
212+
if err != nil {
213+
return nil, fmt.Errorf("error converting predicted live state from unstructured to %s: %w", gvk, err)
214+
}
215+
216+
typedLive, err := pt.FromUnstructured(live.Object)
217+
if err != nil {
218+
return nil, fmt.Errorf("error converting live state from unstructured to %s: %w", gvk, err)
219+
}
220+
221+
// Compare the predicted live with the live resource
222+
comparison, err := typedLive.Compare(typedPredictedLive)
223+
if err != nil {
224+
return nil, fmt.Errorf("error comparing predicted resource to live resource: %w", err)
225+
}
226+
227+
// Loop over all existing managers in predicted live resource to identify
228+
// fields mutated (in predicted live) not owned by any manager.
229+
for _, mfEntry := range plManagedFields {
230+
mfs := &fieldpath.Set{}
231+
err := mfs.FromJSON(bytes.NewReader(mfEntry.FieldsV1.Raw))
232+
if err != nil {
233+
return nil, fmt.Errorf("error building managedFields set: %s", err)
234+
}
235+
if comparison.Added != nil && !comparison.Added.Empty() {
236+
// exclude the added fields owned by this manager from the comparison
237+
comparison.Added = comparison.Added.Difference(mfs)
238+
}
239+
if comparison.Modified != nil && !comparison.Modified.Empty() {
240+
// exclude the modified fields owned by this manager from the comparison
241+
comparison.Modified = comparison.Modified.Difference(mfs)
242+
}
243+
if comparison.Removed != nil && !comparison.Removed.Empty() {
244+
// exclude the removed fields owned by this manager from the comparison
245+
comparison.Removed = comparison.Removed.Difference(mfs)
246+
}
247+
}
248+
// At this point, comparison holds all mutations that aren't owned by any
249+
// of the existing managers.
250+
251+
if comparison.Added != nil && !comparison.Added.Empty() {
252+
// remove added fields that aren't owned by any manager
253+
typedPredictedLive = typedPredictedLive.RemoveItems(comparison.Added)
254+
}
255+
256+
if comparison.Modified != nil && !comparison.Modified.Empty() {
257+
liveModValues := typedLive.ExtractItems(comparison.Modified)
258+
// revert modified fields not owned by any manager
259+
typedPredictedLive, err = typedPredictedLive.Merge(liveModValues)
260+
if err != nil {
261+
return nil, fmt.Errorf("error reverting webhook modified fields in predicted live resource: %s", err)
262+
}
263+
}
264+
265+
if comparison.Removed != nil && !comparison.Removed.Empty() {
266+
liveRmValues := typedLive.ExtractItems(comparison.Removed)
267+
// revert removed fields not owned by any manager
268+
typedPredictedLive, err = typedPredictedLive.Merge(liveRmValues)
269+
if err != nil {
270+
return nil, fmt.Errorf("error reverting webhook removed fields in predicted live resource: %s", err)
271+
}
272+
}
273+
274+
plu := typedPredictedLive.AsValue().Unstructured()
275+
pl, ok := plu.(map[string]interface{})
276+
if !ok {
277+
return nil, fmt.Errorf("error converting live typedValue: expected map got %T", plu)
278+
}
279+
return &unstructured.Unstructured{Object: pl}, nil
280+
}
281+
282+
func jsonStrToUnstructured(jsonString string) (*unstructured.Unstructured, error) {
283+
res := make(map[string]interface{})
284+
err := json.Unmarshal([]byte(jsonString), &res)
285+
if err != nil {
286+
return nil, fmt.Errorf("unmarshal error: %s", err)
287+
}
288+
return &unstructured.Unstructured{Object: res}, nil
289+
}
290+
123291
// StructuredMergeDiff will calculate the diff using the structured-merge-diff
124292
// k8s library (https://github.com/kubernetes-sigs/structured-merge-diff).
125293
func StructuredMergeDiff(config, live *unstructured.Unstructured, gvkParser *managedfields.GvkParser, manager string) (*DiffResult, error) {

pkg/diff/diff_options.go

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,13 @@
11
package diff
22

33
import (
4+
"context"
5+
46
"github.com/go-logr/logr"
7+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
58
"k8s.io/apimachinery/pkg/util/managedfields"
69
"k8s.io/klog/v2/klogr"
10+
cmdutil "k8s.io/kubectl/pkg/cmd/util"
711
)
812

913
type Option func(*options)
@@ -17,11 +21,15 @@ type options struct {
1721
structuredMergeDiff bool
1822
gvkParser *managedfields.GvkParser
1923
manager string
24+
serverSideDiff bool
25+
serverSideDryRunner ServerSideDryRunner
26+
ignoreMutationWebhook bool
2027
}
2128

2229
func applyOptions(opts []Option) options {
2330
o := options{
2431
ignoreAggregatedRoles: false,
32+
ignoreMutationWebhook: true,
2533
normalizer: GetNoopNormalizer(),
2634
log: klogr.New(),
2735
}
@@ -31,6 +39,36 @@ func applyOptions(opts []Option) options {
3139
return o
3240
}
3341

42+
type KubeApplier interface {
43+
ApplyResource(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, force, validate, serverSideApply bool, manager string, serverSideDiff bool) (string, error)
44+
}
45+
46+
// ServerSideDryRunner defines the contract to run a server-side apply in
47+
// dryrun mode.
48+
type ServerSideDryRunner interface {
49+
Run(ctx context.Context, obj *unstructured.Unstructured, manager string) (string, error)
50+
}
51+
52+
// K8sServerSideDryRunner is the Kubernetes implementation of ServerSideDryRunner.
53+
type K8sServerSideDryRunner struct {
54+
dryrunApplier KubeApplier
55+
}
56+
57+
// NewK8sServerSideDryRunner will instantiate a new K8sServerSideDryRunner with
58+
// the given kubeApplier.
59+
func NewK8sServerSideDryRunner(kubeApplier KubeApplier) *K8sServerSideDryRunner {
60+
return &K8sServerSideDryRunner{
61+
dryrunApplier: kubeApplier,
62+
}
63+
}
64+
65+
// ServerSideApplyDryRun will invoke a kubernetes server-side apply with the given
66+
// obj and the given manager in dryrun mode. Will return the predicted live state
67+
// json as string.
68+
func (kdr *K8sServerSideDryRunner) Run(ctx context.Context, obj *unstructured.Unstructured, manager string) (string, error) {
69+
return kdr.dryrunApplier.ApplyResource(ctx, obj, cmdutil.DryRunServer, false, false, true, manager, true)
70+
}
71+
3472
func IgnoreAggregatedRoles(ignore bool) Option {
3573
return func(o *options) {
3674
o.ignoreAggregatedRoles = ignore
@@ -66,3 +104,21 @@ func WithManager(manager string) Option {
66104
o.manager = manager
67105
}
68106
}
107+
108+
func WithServerSideDiff(ssd bool) Option {
109+
return func(o *options) {
110+
o.serverSideDiff = ssd
111+
}
112+
}
113+
114+
func WithIgnoreMutationWebhook(mw bool) Option {
115+
return func(o *options) {
116+
o.ignoreMutationWebhook = mw
117+
}
118+
}
119+
120+
func WithServerSideDryRunner(ssadr ServerSideDryRunner) Option {
121+
return func(o *options) {
122+
o.serverSideDryRunner = ssadr
123+
}
124+
}

pkg/diff/diff_test.go

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package diff
22

33
import (
4+
"context"
45
"encoding/json"
56
"fmt"
67
"os"
@@ -9,9 +10,11 @@ import (
910
"strings"
1011
"testing"
1112

13+
"github.com/argoproj/gitops-engine/pkg/diff/mocks"
1214
"github.com/argoproj/gitops-engine/pkg/diff/testdata"
1315
openapi_v2 "github.com/google/gnostic/openapiv2"
1416
"github.com/stretchr/testify/assert"
17+
"github.com/stretchr/testify/mock"
1518
"github.com/stretchr/testify/require"
1619
"google.golang.org/protobuf/proto"
1720
appsv1 "k8s.io/api/apps/v1"
@@ -887,6 +890,76 @@ func TestStructuredMergeDiff(t *testing.T) {
887890
})
888891
}
889892

893+
func TestServerSideDiff(t *testing.T) {
894+
buildOpts := func(predictedLive string) []Option {
895+
gvkParser := buildGVKParser(t)
896+
manager := "argocd-controller"
897+
dryRunner := mocks.NewServerSideDryRunner(t)
898+
899+
dryRunner.On("Run", mock.Anything, mock.AnythingOfType("*unstructured.Unstructured"), manager).
900+
Return(func(ctx context.Context, obj *unstructured.Unstructured, manager string) (string, error) {
901+
return predictedLive, nil
902+
})
903+
opts := []Option{
904+
WithGVKParser(gvkParser),
905+
WithManager(manager),
906+
WithServerSideDryRunner(dryRunner),
907+
}
908+
909+
return opts
910+
}
911+
912+
t.Run("will ignore modifications done by mutation webhook by default", func(t *testing.T) {
913+
// given
914+
t.Parallel()
915+
liveState := StrToUnstructured(testdata.ServiceLiveYAMLSSD)
916+
desiredState := StrToUnstructured(testdata.ServiceConfigYAMLSSD)
917+
opts := buildOpts(testdata.ServicePredictedLiveJSONSSD)
918+
919+
// when
920+
result, err := serverSideDiff(desiredState, liveState, opts...)
921+
922+
// then
923+
require.NoError(t, err)
924+
assert.NotNil(t, result)
925+
assert.True(t, result.Modified)
926+
predictedSVC := YamlToSvc(t, result.PredictedLive)
927+
liveSVC := YamlToSvc(t, result.NormalizedLive)
928+
require.NotNil(t, predictedSVC.Spec.InternalTrafficPolicy)
929+
require.NotNil(t, liveSVC.Spec.InternalTrafficPolicy)
930+
assert.Equal(t, "Cluster", string(*predictedSVC.Spec.InternalTrafficPolicy))
931+
assert.Equal(t, "Cluster", string(*liveSVC.Spec.InternalTrafficPolicy))
932+
assert.Empty(t, predictedSVC.Annotations[AnnotationLastAppliedConfig])
933+
assert.Empty(t, liveSVC.Annotations[AnnotationLastAppliedConfig])
934+
assert.Empty(t, predictedSVC.Labels["event"])
935+
})
936+
t.Run("will include mutation webhook modifications", func(t *testing.T) {
937+
// given
938+
t.Parallel()
939+
liveState := StrToUnstructured(testdata.ServiceLiveYAMLSSD)
940+
desiredState := StrToUnstructured(testdata.ServiceConfigYAMLSSD)
941+
opts := buildOpts(testdata.ServicePredictedLiveJSONSSD)
942+
opts = append(opts, WithIgnoreMutationWebhook(false))
943+
944+
// when
945+
result, err := serverSideDiff(desiredState, liveState, opts...)
946+
947+
// then
948+
require.NoError(t, err)
949+
assert.NotNil(t, result)
950+
assert.True(t, result.Modified)
951+
predictedSVC := YamlToSvc(t, result.PredictedLive)
952+
liveSVC := YamlToSvc(t, result.NormalizedLive)
953+
require.NotNil(t, predictedSVC.Spec.InternalTrafficPolicy)
954+
require.NotNil(t, liveSVC.Spec.InternalTrafficPolicy)
955+
assert.Equal(t, "Cluster", string(*predictedSVC.Spec.InternalTrafficPolicy))
956+
assert.Equal(t, "Cluster", string(*liveSVC.Spec.InternalTrafficPolicy))
957+
assert.Empty(t, predictedSVC.Annotations[AnnotationLastAppliedConfig])
958+
assert.Empty(t, liveSVC.Annotations[AnnotationLastAppliedConfig])
959+
assert.NotEmpty(t, predictedSVC.Labels["event"])
960+
})
961+
}
962+
890963
func createSecret(data map[string]string) *unstructured.Unstructured {
891964
secret := corev1.Secret{TypeMeta: metav1.TypeMeta{Kind: "Secret"}}
892965
if data != nil {

0 commit comments

Comments
 (0)