Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clear last applied configuration #385

Merged
merged 9 commits into from
Apr 17, 2024
Merged

clear last applied configuration #385

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c952c18
clear last applied configuration
mfrancisc Apr 5, 2024
aacc2c6
move cleanup
mfrancisc Apr 8, 2024
0b781ea
Merge branch 'master' into fixlastappliedconfig
mfrancisc Apr 9, 2024
7ed09c1
remove resourceversion and create removeAnnotation func
mfrancisc Apr 9, 2024
ae37c06
Merge remote-tracking branch 'origin/fixlastappliedconfig' into fixla…
mfrancisc Apr 9, 2024
cf8634b
fix apply object when saveconfiguration is disabled and annotation is…
mfrancisc Apr 15, 2024
d40b79d
Merge branch 'master' into fixlastappliedconfig
MatousJobanek Apr 17, 2024
91ec671
PR comments
mfrancisc Apr 17, 2024
3004dc4
Merge remote-tracking branch 'origin/fixlastappliedconfig' into fixla…
mfrancisc Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions pkg/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@
if config.saveConfiguration {
// set current object as annotation
annotations := obj.GetAnnotations()
newConfiguration = getNewConfiguration(obj)
newConfiguration = GetNewConfiguration(obj)
if annotations == nil {
annotations = map[string]string{}
}
Expand All @@ -124,6 +124,7 @@
namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}
if err := c.Client.Get(ctx, namespacedName, existing); err != nil {
if apierrors.IsNotFound(err) {
obj.SetResourceVersion("") // reset resource version when creating to avoid error: resourceVersion should not be set on objects to be created
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know in which cases the provided object contained the resourceVersion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was happening with the new toolchaincluster_resource_controller. Unfortunately I don't have the logs anymore but creation of the objects was failing due to this ResourceVersion field being set. TBH I was confused about why this was happening, since the objects should be "new" from the templates directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

didn't you see the failure as part of the unit tests? see the comment below - it could be the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to reproduce the failure in unit tests. you mean that we should pass a deepcopy of the objects here https://github.com/codeready-toolchain/toolchain-common/pull/385/files#diff-2895f30116602d8fe6b0545c186d4f81247fa4eaab049dab174c6c91c0036e08R291 ? Or something else ..

return true, c.createObj(ctx, obj, config.owner)
}
return false, errors.Wrapf(err, "unable to get the resource '%v'", existing)
Expand Down Expand Up @@ -205,15 +206,23 @@
}
}

func getNewConfiguration(newResource runtime.Object) string {
newJSON, err := marshalObjectContent(newResource)
func GetNewConfiguration(newResource client.Object) string {
// reset the previous config to avoid recursive embedding of the object
copyResource := removeAnnotation(newResource, LastAppliedConfigurationAnnotationKey)
newJSON, err := marshalObjectContent(copyResource)
if err != nil {
log.Error(err, "unable to marshal the object", "object", newResource)
return fmt.Sprintf("%v", newResource)
log.Error(err, "unable to marshal the object", "object", copyResource)
return fmt.Sprintf("%v", copyResource)

Check warning on line 215 in pkg/client/client.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/client.go#L214-L215 

Added lines #L214 - L215 were not covered by tests
}
return string(newJSON)
}

func removeAnnotation(newResource client.Object, annotationKey string) client.Object {
copyResource := newResource.DeepCopyObject().(client.Object)
delete(copyResource.GetAnnotations(), annotationKey)
return copyResource
}

func marshalObjectContent(newResource runtime.Object) ([]byte, error) {
if newRes, ok := newResource.(runtime.Unstructured); ok {
return json.Marshal(newRes.UnstructuredContent())
Expand Down Expand Up @@ -278,7 +287,7 @@
for _, unstructuredObj := range unstructuredObjects {
log.Info("applying object", "object_namespace", unstructuredObj.GetNamespace(), "object_name", unstructuredObj.GetObjectKind().GroupVersionKind().Kind+"/"+unstructuredObj.GetName())
MergeLabels(unstructuredObj, newLabels)
_, err := applyClient.ApplyObject(ctx, unstructuredObj)
_, err := applyClient.ApplyObject(ctx, unstructuredObj, SaveConfiguration(false))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately for Unstructured objects it is still not working, so I'm keeping the saveConfiguration functionality disabled. I've tried multiple things:

  • removing the LastAppliedConfigurationAnnotationKey annotation from both new object before creating it
  • removing theLastAppliedConfigurationAnnotationKey annotation from the existing object before merging the annotations with the new object
  • removing the special handling of Unstructured objects in marshalObjectContent:
if newRes, ok := newResource.(runtime.Unstructured); ok {
		return json.Marshal(newRes.UnstructuredContent())
}

but nothing seems to work. The toolchain.dev.openshift.com/last-applied-configuration annotation is still being recursively embedded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also wondering if we really need this functionality at all. We could

a. trigger the Update of the object anyway , that should do anything if there are no changes on the object.
b. rely on kubectl.kubernetes.io/last-applied-configuration , how is this different from our custom annotation ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good points @mfrancisc. Not sure why we started using our annotation instead. Maybe @MatousJobanek remembers why.

Copy link
Contributor

Choose a reason for hiding this comment

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

it was an attempt to get closer to what kubectl does, to optimize the number of updates we do from our controllers and to have some control over what was updated and what not.
But I really doubt that it works as we hoped that it could work, so I'm for dropping the annotation 👍 to simplify the logic. But let's do it in a separate PR.

if err != nil {
return err
}
Expand Down
37 changes: 37 additions & 0 deletions pkg/client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,43 @@ func TestApplySingle(t *testing.T) {
assert.Equal(t, "second-value", configMap.Data["first-param"])
})
})

t.Run("updates of ServiceAccount", func(t *testing.T) {

t.Run("should update service account with last applied configuration", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

since the "last-appplied-configuration" is used and is set to the same value, then there the SA shouldn't be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point! I've changed the name of the test in 91ec671. Thank you!

// given
// there's an existing SA with secret refs
existingSA := newSA()
secretRefs := []corev1.ObjectReference{
{
Name: "secret",
Namespace: existingSA.Namespace,
},
}
existingSA.Secrets = secretRefs
existingLastAppliedAnnotation := map[string]string{
client.LastAppliedConfigurationAnnotationKey: client.GetNewConfiguration(existingSA),
}
existingSA.SetAnnotations(existingLastAppliedAnnotation) // let's set the last applied annotation
cl, cli := newClient(t)
_, err := cl.ApplyRuntimeObject(context.TODO(), existingSA)
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that we should pass a deep-copy of the object, otherwise, the generation as well as resourceVersion values would be set and would influence the late execution when the object that is passed to the function again:

Suggested change
_, err := cl.ApplyRuntimeObject(context.TODO(), existingSA)
_, err := cl.ApplyRuntimeObject(context.TODO(), existingSA.DeepCopyObject())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 91ec671

require.NoError(t, err)

// when
// we update with existing annotations
newSA := existingSA.DeepCopy()
newSA.SetAnnotations(existingLastAppliedAnnotation) // let's set the last applied annotation
_, err = cl.ApplyRuntimeObject(context.TODO(), newSA) // then

// then
require.NoError(t, err)
var actualSa corev1.ServiceAccount
err = cli.Get(context.TODO(), types.NamespacedName{Name: "appstudio-user-sa", Namespace: "john-dev"}, &actualSa) // assert sa was created
require.NoError(t, err)
assert.Equal(t, secretRefs, actualSa.Secrets) // secret refs are still there
assert.Equal(t, existingLastAppliedAnnotation[client.LastAppliedConfigurationAnnotationKey], actualSa.Annotations[client.LastAppliedConfigurationAnnotationKey]) // the last apply configuration should match the previous object
})
})
}

func toUnstructured(obj runtime.Object) (*unstructured.Unstructured, error) {
Expand Down
Loading