-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for server side apply #392
base: develop
Are you sure you want to change the base?
Changes from 44 commits
fd0f790
176ebb5
20bd0a6
27e77f8
6884b1e
7de82bd
1f77748
5ff0cd5
a1b7202
e7b4682
d17a09e
868d362
6a42593
3c0ba71
1e81fce
df4a333
307bbe9
8eb9d07
c11badb
5a5d997
5b6b056
d2a434b
7745d81
9a6b41f
8488060
de9dca5
a135136
f203cec
977df6f
c97d804
e8bcfcb
1033a23
632638e
6af080b
c01292c
c6d868e
527553b
c98601d
4301a35
2c4cb43
6fd31bd
c9be433
8a952e0
3a9a9fc
389316e
96fb26c
9795527
f94e5b3
9c5d0ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,12 +4,13 @@ | |
package clusterapply | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"k8s.io/apimachinery/pkg/types" | ||
"time" | ||
|
||
ctldiff "github.com/k14s/kapp/pkg/kapp/diff" | ||
ctlres "github.com/k14s/kapp/pkg/kapp/resources" | ||
"github.com/k14s/kapp/pkg/kapp/util" | ||
"k8s.io/apimachinery/pkg/api/errors" | ||
) | ||
|
||
|
@@ -26,7 +27,10 @@ const ( | |
) | ||
|
||
type AddOrUpdateChangeOpts struct { | ||
DefaultUpdateStrategy string | ||
DefaultUpdateStrategy string | ||
ServerSideApply bool | ||
ServerSideForceConflict bool | ||
FieldManagerName string | ||
} | ||
|
||
type AddOrUpdateChange struct { | ||
|
@@ -131,7 +135,7 @@ func (c AddOrUpdateChange) tryToResolveUpdateConflict( | |
changeSet := c.changeSetFactory.New([]ctlres.Resource{latestExistingRes}, | ||
[]ctlres.Resource{c.change.AppliedResource()}) | ||
|
||
recalcChanges, err := changeSet.Calculate() | ||
recalcChanges, err := changeSet.Calculate(context.TODO()) | ||
if err != nil { | ||
return err | ||
} | ||
|
@@ -172,7 +176,7 @@ func (c AddOrUpdateChange) tryToUpdateAfterCreateConflict() error { | |
changeSet := c.changeSetFactory.New([]ctlres.Resource{latestExistingRes}, | ||
[]ctlres.Resource{c.change.AppliedResource()}) | ||
|
||
recalcChanges, err := changeSet.Calculate() | ||
recalcChanges, err := changeSet.Calculate(context.TODO()) | ||
if err != nil { | ||
return err | ||
} | ||
|
@@ -218,36 +222,14 @@ func (c AddOrUpdateChange) recordAppliedResource(savedRes ctlres.Resource) error | |
return fmt.Errorf("Calculating change after the save: %s", err) | ||
} | ||
|
||
// first time, try using memory copy | ||
latestResWithHistory := &savedResWithHistory | ||
|
||
return util.Retry(time.Second, time.Minute, func() (bool, error) { | ||
// subsequent times try to retrieve latest copy, | ||
// for example, ServiceAccount seems to change immediately | ||
if latestResWithHistory == nil { | ||
res, err := c.identifiedResources.Get(savedRes) | ||
if err != nil { | ||
return false, err | ||
} | ||
|
||
resWithHistory := c.changeFactory.NewResourceWithHistory(res) | ||
latestResWithHistory = &resWithHistory | ||
} | ||
|
||
// Record last applied change on the latest version of a resource | ||
latestResWithHistoryUpdated, err := latestResWithHistory.RecordLastAppliedResource(applyChange) | ||
if err != nil { | ||
return true, fmt.Errorf("Recording last applied resource: %s", err) | ||
} | ||
|
||
_, err = c.identifiedResources.Update(latestResWithHistoryUpdated) | ||
if err != nil { | ||
latestResWithHistory = nil // Get again | ||
return false, fmt.Errorf("Saving record of last applied resource: %s", err) | ||
} | ||
// Record last applied change on the latest version of a resource | ||
recordHistoryPatch, err := savedResWithHistory.RecordLastAppliedResource(applyChange) | ||
if err != nil { | ||
return fmt.Errorf("Recording last applied resource: %s", err) | ||
} | ||
|
||
return true, nil | ||
}) | ||
_, err = c.identifiedResources.Patch(savedRes, types.MergePatchType, recordHistoryPatch, ctlres.PatchOpts{DryRun: false}) | ||
return err | ||
} | ||
|
||
type AddPlainStrategy struct { | ||
|
@@ -263,6 +245,26 @@ func (c AddPlainStrategy) Apply() error { | |
return err | ||
} | ||
|
||
// Create is recorded in the metadata.fieldManagers as | ||
// Update operation creating distinct field manager from Apply operation, | ||
// which means that these fields wont be updateable using SSA. | ||
// To fix it, we change operation to be "Apply" | ||
// See https://github.com/kubernetes/kubernetes/issues/107417 for details | ||
if c.aou.opts.ServerSideApply { | ||
createdRes, err = c.aou.identifiedResources.Patch(createdRes, types.JSONPatchType, []byte(` | ||
[ | ||
{ "op": "test", "path": "/metadata/managedFields/0/manager", "value": "`+c.aou.opts.FieldManagerName+`" }, | ||
{ "op": "replace", "path": "/metadata/managedFields/0/operation", "value": "Apply" } | ||
] | ||
`), ctlres.PatchOpts{DryRun: false}) | ||
if err != nil { | ||
// TODO: potentially patch can fail if '"op": "test"' fails, which can happen if another | ||
// controller changes managedFields. We | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment got cut off... |
||
return err | ||
} | ||
|
||
} | ||
|
||
return c.aou.recordAppliedResource(createdRes) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is a gap of time between create succeeding and patch succeeding where other changes may be applied to the resource. recordAppliedResource on the result of patch which means that we would capture such changes as "correlated" to initial submission. im not sure if this matters at all when ssa is enabled, but definitely seems suspicious if we keep it. at the least this deserves a comment, explaining why its ok for ssa. |
||
} | ||
|
||
|
@@ -275,13 +277,27 @@ func (c AddOrFallbackOnUpdateStrategy) Op() ClusterChangeApplyStrategyOp { | |
return createStrategyFallbackOnUpdateAnnValue | ||
} | ||
|
||
func (c AddOrFallbackOnUpdateStrategy) Apply() error { | ||
createdRes, err := c.aou.identifiedResources.Create(c.newRes) | ||
if err != nil { | ||
if errors.IsAlreadyExists(err) { | ||
return c.aou.tryToUpdateAfterCreateConflict() | ||
func (c AddOrFallbackOnUpdateStrategy) Apply() (err error) { | ||
var createdRes ctlres.Resource | ||
if c.aou.opts.ServerSideApply { | ||
resBytes, err := c.newRes.AsYAMLBytes() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
// Apply patch is like upsert, combining create + update, no need to fallback on error | ||
createdRes, err = c.aou.identifiedResources.Patch(c.newRes, types.ApplyPatchType, resBytes, ctlres.PatchOpts{DryRun: false}) | ||
if err != nil { | ||
return err | ||
} | ||
} else { | ||
createdRes, err = c.aou.identifiedResources.Create(c.newRes) | ||
if err != nil { | ||
if errors.IsAlreadyExists(err) { | ||
return c.aou.tryToUpdateAfterCreateConflict() | ||
} | ||
return err | ||
} | ||
return err | ||
} | ||
|
||
return c.aou.recordAppliedResource(createdRes) | ||
|
@@ -295,7 +311,17 @@ type UpdatePlainStrategy struct { | |
func (c UpdatePlainStrategy) Op() ClusterChangeApplyStrategyOp { return updateStrategyPlainAnnValue } | ||
|
||
func (c UpdatePlainStrategy) Apply() error { | ||
updatedRes, err := c.aou.identifiedResources.Update(c.newRes) | ||
var updatedRes ctlres.Resource | ||
var err error | ||
|
||
if c.aou.opts.ServerSideApply { | ||
updatedRes, err = ctlres.WithIdentityAnnotation(c.newRes, func(r ctlres.Resource) (ctlres.Resource, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems that we have two types of Patch operations: one where we are working with a resource, and one where we are working with some "minimal/raw" patch. instead of exposing WithIdentityAnnotation outside of IdentifiedResources resources class, lets keep existing Patch method to work with Resource class (instead of []byte) and change it so that it can add annotation on the fly. lets also add PatchRaw which does take []byte. this should result in keeping WithIdentityAnnotation as private. |
||
resBytes, _ := r.AsYAMLBytes() | ||
redbaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return c.aou.identifiedResources.Patch(r, types.ApplyPatchType, resBytes, ctlres.PatchOpts{DryRun: false}) | ||
}) | ||
} else { | ||
updatedRes, err = c.aou.identifiedResources.Update(c.newRes) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we be tryToResolveUpdateConflict doing when SSA is on? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right. |
||
} | ||
if err != nil { | ||
if errors.IsConflict(err) { | ||
return c.aou.tryToResolveUpdateConflict(err, func(err error) error { return err }) | ||
|
@@ -323,8 +349,20 @@ func (c UpdateOrFallbackOnReplaceStrategy) Apply() error { | |
return err | ||
} | ||
|
||
updatedRes, err := c.aou.identifiedResources.Update(c.newRes) | ||
var updatedRes ctlres.Resource | ||
var err error | ||
|
||
if c.aou.opts.ServerSideApply { | ||
updatedRes, err = ctlres.WithIdentityAnnotation(c.newRes, func(r ctlres.Resource) (ctlres.Resource, error) { | ||
resBytes, _ := r.AsYAMLBytes() | ||
return c.aou.identifiedResources.Patch(r, types.ApplyPatchType, resBytes, ctlres.PatchOpts{DryRun: false}) | ||
}) | ||
} else { | ||
updatedRes, err = c.aou.identifiedResources.Update(c.newRes) | ||
} | ||
|
||
if err != nil { | ||
//TODO: find out if SSA conflicts worth retrying | ||
if errors.IsConflict(err) { | ||
return c.aou.tryToResolveUpdateConflict(err, replaceIfIsInvalidErrFunc) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess this is safe-ish to do because metadata.annotations will always be present because kapp adds identity annotation.
(it's worth noting that "patch" verb is required in permissions in addition to update.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DryRun: false
is not necessaryThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if
metadata.annotations
is not present MergePatch will create it. It would be a problem if we used JsonPatch.