Skip to content

Commit

Permalink
fix: Handle nil health check in post-delete hooks (argoproj#18270) (a…
Browse files Browse the repository at this point in the history 
…rgoproj#18767)

* fix: Handle nil health check in post-delete hooks

Signed-off-by: Yonatan Sasson <yonatanxd7@gmail.com>

* test: Validate deletion of RBAC resources for post-delete hook

Signed-off-by: Yonatan Sasson <yonatanxd7@gmail.com>

* Update appcontroller_test.go

Signed-off-by: Yonatan Sasson <107778824+Yuni-sa@users.noreply.github.com>

---------

Signed-off-by: Yonatan Sasson <yonatanxd7@gmail.com>
Signed-off-by: Yonatan Sasson <107778824+Yuni-sa@users.noreply.github.com>
  • Loading branch information
@Yuni-sa
Yuni-sa committed Jun 26, 2024
1 parent 6f84afc commit 999b75f
Show file tree
Hide file tree
Showing 2 changed files with 162 additions and 24 deletions.
169 changes: 145 additions & 24 deletions controller/appcontroller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,8 +374,8 @@ data:

var fakePostDeleteHook = `
{
"apiVersion": "v1",
"kind": "Pod",
"apiVersion": "batch/v1",
"kind": "Job",
"metadata": {
"name": "post-delete-hook",
"namespace": "default",
Expand All @@ -388,22 +388,93 @@ var fakePostDeleteHook = `
}
},
"spec": {
"containers": [
{
"name": "post-delete-hook",
"image": "busybox",
"restartPolicy": "Never",
"command": [
"/bin/sh",
"-c",
"sleep 5 && echo hello from the post-delete-hook pod"
]
"template": {
"metadata": {
"name": "post-delete-hook"
},
"spec": {
"containers": [
{
"name": "post-delete-hook",
"image": "busybox",
"command": [
"/bin/sh",
"-c",
"sleep 5 && echo hello from the post-delete-hook job"
]
}
],
"restartPolicy": "Never"
}
]
}
}
}
`

var fakeServiceAccount = `
{
"apiVersion": "v1",
"kind": "ServiceAccount",
"metadata": {
"name": "hook-serviceaccount",
"namespace": "default",
"annotations": {
"argocd.argoproj.io/hook": "PostDelete",
"argocd.argoproj.io/hook-delete-policy": "BeforeHookCreation,HookSucceeded"
}
}
}
`

var fakeRole = `
{
"apiVersion": "rbac.authorization.k8s.io/v1",
"kind": "Role",
"metadata": {
"name": "hook-role",
"namespace": "default",
"annotations": {
"argocd.argoproj.io/hook": "PostDelete",
"argocd.argoproj.io/hook-delete-policy": "BeforeHookCreation,HookSucceeded"
}
},
"rules": [
{
"apiGroups": [""],
"resources": ["secrets"],
"verbs": ["get", "delete", "list"]
}
]
}
`

var fakeRoleBinding = `
{
"apiVersion": "rbac.authorization.k8s.io/v1",
"kind": "RoleBinding",
"metadata": {
"name": "hook-rolebinding",
"namespace": "default",
"annotations": {
"argocd.argoproj.io/hook": "PostDelete",
"argocd.argoproj.io/hook-delete-policy": "BeforeHookCreation,HookSucceeded"
}
},
"roleRef": {
"apiGroup": "rbac.authorization.k8s.io",
"kind": "Role",
"name": "hook-role"
},
"subjects": [
{
"kind": "ServiceAccount",
"name": "hook-serviceaccount",
"namespace": "default"
}
]
}
`

func newFakeApp() *v1alpha1.Application {
return createFakeApp(fakeApp)
}
Expand Down Expand Up @@ -439,12 +510,39 @@ func newFakeCM() map[string]interface{} {
}

func newFakePostDeleteHook() map[string]interface{} {
var cm map[string]interface{}
err := yaml.Unmarshal([]byte(fakePostDeleteHook), &cm)
var hook map[string]interface{}
err := yaml.Unmarshal([]byte(fakePostDeleteHook), &hook)
if err != nil {
panic(err)
}
return cm
return hook
}

func newFakeRoleBinding() map[string]interface{} {
var roleBinding map[string]interface{}
err := yaml.Unmarshal([]byte(fakeRoleBinding), &roleBinding)
if err != nil {
panic(err)
}
return roleBinding
}

func newFakeRole() map[string]interface{} {
var role map[string]interface{}
err := yaml.Unmarshal([]byte(fakeRole), &role)
if err != nil {
panic(err)
}
return role
}

func newFakeServiceAccount() map[string]interface{} {
var serviceAccount map[string]interface{}
err := yaml.Unmarshal([]byte(fakeServiceAccount), &serviceAccount)
if err != nil {
panic(err)
}
return serviceAccount
}

func TestAutoSync(t *testing.T) {
Expand Down Expand Up @@ -721,7 +819,7 @@ func TestFinalizeAppDeletion(t *testing.T) {

// Ensure any stray resources irregularly labeled with instance label of app are not deleted upon deleting,
// when app project restriction is in place
t.Run("ProjectRestrictionEnforced", func(*testing.T) {
t.Run("ProjectRestrictionEnforced", func(t *testing.T) {
restrictedProj := v1alpha1.AppProject{
ObjectMeta: metav1.ObjectMeta{
Name: "restricted",
Expand Down Expand Up @@ -882,7 +980,13 @@ func TestFinalizeAppDeletion(t *testing.T) {
app.SetPostDeleteFinalizer()
app.Spec.Destination.Namespace = test.FakeArgoCDNamespace
liveHook := &unstructured.Unstructured{Object: newFakePostDeleteHook()}
require.NoError(t, unstructured.SetNestedField(liveHook.Object, "Succeeded", "status", "phase"))
conditions := []interface{}{
map[string]interface{}{
"type": "Complete",
"status": "True",
},
}
require.NoError(t, unstructured.SetNestedField(liveHook.Object, conditions, "status", "conditions"))
ctrl := newFakeController(&fakeData{
manifestResponses: []*apiclient.ManifestResponse{{
Manifests: []string{fakePostDeleteHook},
Expand Down Expand Up @@ -916,15 +1020,27 @@ func TestFinalizeAppDeletion(t *testing.T) {
app := newFakeApp()
app.SetPostDeleteFinalizer("cleanup")
app.Spec.Destination.Namespace = test.FakeArgoCDNamespace
liveRoleBinding := &unstructured.Unstructured{Object: newFakeRoleBinding()}
liveRole := &unstructured.Unstructured{Object: newFakeRole()}
liveServiceAccount := &unstructured.Unstructured{Object: newFakeServiceAccount()}
liveHook := &unstructured.Unstructured{Object: newFakePostDeleteHook()}
require.NoError(t, unstructured.SetNestedField(liveHook.Object, "Succeeded", "status", "phase"))
conditions := []interface{}{
map[string]interface{}{
"type": "Complete",
"status": "True",
},
}
require.NoError(t, unstructured.SetNestedField(liveHook.Object, conditions, "status", "conditions"))
ctrl := newFakeController(&fakeData{
manifestResponses: []*apiclient.ManifestResponse{{
Manifests: []string{fakePostDeleteHook},
Manifests: []string{fakeRoleBinding, fakeRole, fakeServiceAccount, fakePostDeleteHook},
}},
apps: []runtime.Object{app, &defaultProj},
managedLiveObjs: map[kube.ResourceKey]*unstructured.Unstructured{
kube.GetResourceKey(liveHook): liveHook,
kube.GetResourceKey(liveRoleBinding): liveRoleBinding,
kube.GetResourceKey(liveRole): liveRole,
kube.GetResourceKey(liveServiceAccount): liveServiceAccount,
kube.GetResourceKey(liveHook): liveHook,
},
}, nil)

Expand All @@ -943,9 +1059,14 @@ func TestFinalizeAppDeletion(t *testing.T) {
return []*v1alpha1.Cluster{}, nil
})
require.NoError(t, err)
// post-delete hook is deleted
require.Len(t, ctrl.kubectl.(*MockKubectl).DeletedResources, 1)
require.Equal(t, "post-delete-hook", ctrl.kubectl.(*MockKubectl).DeletedResources[0].Name)
// post-delete hooks are deleted
require.Len(t, ctrl.kubectl.(*MockKubectl).DeletedResources, 4)
deletedResources := []string{}
for _, res := range ctrl.kubectl.(*MockKubectl).DeletedResources {
deletedResources = append(deletedResources, res.Name)
}
expectedNames := []string{"hook-rolebinding", "hook-role", "hook-serviceaccount", "post-delete-hook"}
require.ElementsMatch(t, expectedNames, deletedResources, "Deleted resources should match expected names")
// finalizer is not removed
assert.False(t, patched)
})
Expand Down
17 changes: 17 additions & 0 deletions controller/hook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,18 @@ func (ctrl *ApplicationController) executePostDeleteHooks(app *v1alpha1.Applicat
if err != nil {
return false, err
}
if hookHealth == nil {
logCtx.WithFields(log.Fields{
"group": obj.GroupVersionKind().Group,
"version": obj.GroupVersionKind().Version,
"kind": obj.GetKind(),
"name": obj.GetName(),
"namespace": obj.GetNamespace(),
}).Info("No health check defined for resource, considering it healthy")
hookHealth = &health.HealthStatus{
Status: health.HealthStatusHealthy,
}
}
if hookHealth.Status == health.HealthStatusProgressing {
progressingHooksCnt++
}
Expand Down Expand Up @@ -128,6 +140,11 @@ func (ctrl *ApplicationController) cleanupPostDeleteHooks(liveObjs map[kube.Reso
if err != nil {
return false, err
}
if hookHealth == nil {
hookHealth = &health.HealthStatus{
Status: health.HealthStatusHealthy,
}
}
if health.IsWorse(aggregatedHealth, hookHealth.Status) {
aggregatedHealth = hookHealth.Status
}
Expand Down

0 comments on commit 999b75f

Please sign in to comment.