Skip to content

Commit 934d244

Browse files
100mik100mik
authored
Enhance fallback to noop logic for apps to account for multiple namespaces (#1394)
* Enhance fallback to noop logic for apps to account for multiple namespaces Signed-off-by: Soumik Majumder <[email protected]> * Add tests for fallback to noop cases spanning multiple clusters Signed-off-by: Soumik Majumder <[email protected]> --------- Signed-off-by: Soumik Majumder <[email protected]>
1 parent 6d83c43 commit 934d244

File tree

3 files changed

+180
-18
lines changed

3 files changed

+180
-18
lines changed

‎pkg/app/app.go

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,35 @@ func (a *App) ConfigMapRefs() map[reftracker.RefKey]struct{} {
276276
return configMaps
277277
}
278278

279+
func (a *App) noopDeleteDueToTerminatingNamespaces() bool {
280+
if a.app.Status.Deploy == nil || a.app.Status.Deploy.KappDeployStatus == nil || a.app.Spec.ServiceAccountName == "" {
281+
return false
282+
}
283+
if !a.isNamespaceTerminating(a.app.Namespace) {
284+
return false
285+
}
286+
// Ensure that no cluster scoped resources are created by the app
287+
// and all affected namespaces are terminating
288+
for _, ns := range a.app.Status.Deploy.KappDeployStatus.AssociatedResources.Namespaces {
289+
if ns == "(cluster)" {
290+
return false
291+
}
292+
if !a.isNamespaceTerminating(ns) {
293+
return false
294+
}
295+
}
296+
a.log.Info("Safely performing noop delete to avoid blocking namespace deletion")
297+
return true
298+
}
299+
300+
func (a *App) isNamespaceTerminating(namespace string) bool {
301+
status, err := a.compInfo.NamespaceStatus(namespace)
302+
if err != nil {
303+
a.log.Error(err, "Error getting app namespace status", "app", a.app.Name, "namespace", a.app.Namespace)
304+
}
305+
return status.Phase == v1.NamespaceTerminating
306+
}
307+
279308
// HasImageOrImgpkgBundle is used to determine if the
280309
// App's spec contains a fetch stage for an image or
281310
// imgpkgbundle. It is mainly used to determine whether
@@ -292,11 +321,3 @@ func (a App) HasImageOrImgpkgBundle() bool {
292321
}
293322
return false
294323
}
295-
296-
func (a App) isNamespaceTerminating() bool {
297-
status, err := a.compInfo.NamespaceStatus(a.app.Namespace)
298-
if err != nil {
299-
a.log.Error(err, "Error getting app namespace status", "app", a.app.Name, "namespace", a.app.Namespace)
300-
}
301-
return status.Phase == v1.NamespaceTerminating
302-
}

‎pkg/app/app_deploy.go

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -57,15 +57,7 @@ func (a *App) delete(changedFunc func(exec.CmdRunResult)) exec.CmdRunResult {
5757

5858
var result exec.CmdRunResult
5959

60-
appResourcesInSameNs := a.app.Status.Deploy != nil && a.app.Status.Deploy.KappDeployStatus != nil &&
61-
len(a.app.Status.Deploy.KappDeployStatus.AssociatedResources.Namespaces) == 1 &&
62-
a.app.Status.Deploy.KappDeployStatus.AssociatedResources.Namespaces[0] == a.app.Namespace
63-
64-
// Use noopDelete if the namespace is terminating and app resources are in same namespace because
65-
// the app resources will be automatically deleted including the kapp ServiceAccount
66-
noopDelete := a.isNamespaceTerminating() && appResourcesInSameNs && a.app.Spec.Cluster == nil
67-
68-
if !a.app.Spec.NoopDelete && !noopDelete {
60+
if !a.app.Spec.NoopDelete && !a.noopDeleteDueToTerminatingNamespaces() {
6961
for _, dep := range a.app.Spec.Deploy {
7062
switch {
7163
case dep.Kapp != nil:

‎test/e2e/kappcontroller/namespace_deletion_test.go

Lines changed: 150 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,156 @@ spec:
6868
})
6969
}
7070

71-
func Test_NamespaceDelete_AppWithResourcesInDifferentNamespaces(t *testing.T) {
71+
func Test_NamespaceDelete_AppWithResourcesInDifferentTerminatingNamespaces(t *testing.T) {
72+
env := e2e.BuildEnv(t)
73+
logger := e2e.Logger{}
74+
kapp := e2e.Kapp{t, env.Namespace, logger}
75+
kubectl := e2e.Kubectl{t, env.Namespace, logger}
76+
77+
nsName1 := "ns-delete-1"
78+
nsName2 := "ns-delete-2"
79+
nsApp := "testnamespaces"
80+
name := "resources-in-different-namespaces"
81+
82+
namespaceTemplate := `---
83+
apiVersion: v1
84+
kind: Namespace
85+
metadata:
86+
name: %v`
87+
namespaceYAML := fmt.Sprintf(namespaceTemplate, nsName1) + "\n" + fmt.Sprintf(namespaceTemplate, nsName2)
88+
89+
appYaml := fmt.Sprintf(`---
90+
apiVersion: kappctrl.k14s.io/v1alpha1
91+
kind: App
92+
metadata:
93+
name: %s
94+
spec:
95+
serviceAccountName: kappctrl-e2e-ns-sa
96+
fetch:
97+
- inline:
98+
paths:
99+
file.yml: |
100+
apiVersion: v1
101+
kind: ConfigMap
102+
metadata:
103+
name: configmap
104+
namespace: %s
105+
data:
106+
key: value
107+
---
108+
apiVersion: v1
109+
kind: ConfigMap
110+
metadata:
111+
name: configmap
112+
namespace: %s
113+
data:
114+
key: value
115+
template:
116+
- ytt: {}
117+
deploy:
118+
- kapp: {}`, name, nsName1, nsName2) + e2e.ServiceAccounts{nsName1}.ForClusterYAML()
119+
120+
cleanUp := func() {
121+
kapp.Run([]string{"delete", "-a", name})
122+
}
123+
124+
cleanUpTestNamespace := func() {
125+
kapp.Run([]string{"delete", "-a", name})
126+
kapp.Run([]string{"delete", "-a", nsApp})
127+
}
128+
129+
cleanUp()
130+
defer cleanUp()
131+
defer cleanUpTestNamespace()
132+
133+
logger.Section("create namespace and deploy App", func() {
134+
kapp.RunWithOpts([]string{"deploy", "-a", nsApp, "-f", "-"}, e2e.RunOpts{StdinReader: strings.NewReader(namespaceYAML)})
135+
kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", name, "--into-ns", nsName1}, e2e.RunOpts{StdinReader: strings.NewReader(appYaml)})
136+
})
137+
138+
logger.Section("delete namespace", func() {
139+
// delete SA first to reduce flakiness, sometimes SA deletion happens after app is deleted
140+
kubectl.RunWithOpts([]string{"delete", "serviceaccount", "kappctrl-e2e-ns-sa", "-n", nsName1},
141+
e2e.RunOpts{NoNamespace: true})
142+
kubectl.Run([]string{"delete", "ns", nsName1, nsName2, "--timeout=1m"})
143+
})
144+
}
145+
146+
func Test_NamespaceDelete_AppWithClusterScopedResources(t *testing.T) {
147+
env := e2e.BuildEnv(t)
148+
logger := e2e.Logger{}
149+
kapp := e2e.Kapp{t, env.Namespace, logger}
150+
kubectl := e2e.Kubectl{t, env.Namespace, logger}
151+
152+
nsName := "ns-delete-1"
153+
name := "app-with-cluster-scoped-resource"
154+
inAppNsName := "delete-test-ns"
155+
156+
namespaceYAML := fmt.Sprintf(`---
157+
apiVersion: v1
158+
kind: Namespace
159+
metadata:
160+
name: %v`, nsName)
161+
162+
appYaml := fmt.Sprintf(`---
163+
apiVersion: kappctrl.k14s.io/v1alpha1
164+
kind: App
165+
metadata:
166+
name: %s
167+
spec:
168+
serviceAccountName: kappctrl-e2e-ns-sa
169+
fetch:
170+
- inline:
171+
paths:
172+
file.yml: |
173+
apiVersion: v1
174+
kind: Namespace
175+
metadata:
176+
name: %s
177+
---
178+
apiVersion: v1
179+
kind: ConfigMap
180+
metadata:
181+
name: configmap
182+
namespace: %s
183+
data:
184+
key: value
185+
template:
186+
- ytt: {}
187+
deploy:
188+
- kapp: {}`, name, inAppNsName, nsName) + e2e.ServiceAccounts{nsName}.ForClusterYAML()
189+
190+
cleanUp := func() {
191+
kapp.Run([]string{"delete", "-a", name})
192+
}
193+
194+
cleanUpTestNamespace := func() {
195+
kubectl.Run([]string{"delete", "ns", inAppNsName})
196+
kubectl.RunWithOpts([]string{"patch", "App", name, "--type=json", "--patch", `[{ "op": "replace", "path": "/spec/noopDelete", "value": true}]`,
197+
"-n", nsName}, e2e.RunOpts{NoNamespace: true})
198+
kapp.Run([]string{"delete", "-a", nsName})
199+
}
200+
201+
cleanUp()
202+
defer cleanUp()
203+
defer cleanUpTestNamespace()
204+
205+
logger.Section("create namespace and deploy App", func() {
206+
kapp.RunWithOpts([]string{"deploy", "-a", nsName, "-f", "-"}, e2e.RunOpts{StdinReader: strings.NewReader(namespaceYAML)})
207+
kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", name, "--into-ns", nsName}, e2e.RunOpts{StdinReader: strings.NewReader(appYaml)})
208+
})
209+
210+
logger.Section("delete namespace", func() {
211+
// delete SA first to reduce flakiness, sometimes SA deletion happens after app is deleted
212+
kubectl.RunWithOpts([]string{"delete", "serviceaccount", "kappctrl-e2e-ns-sa", "-n", nsName},
213+
e2e.RunOpts{NoNamespace: true})
214+
_, err := kubectl.RunWithOpts([]string{"delete", "ns", nsName, "--timeout=30s"},
215+
e2e.RunOpts{AllowError: true})
216+
assert.Error(t, err, "Expected to get time out error, but did not")
217+
})
218+
}
219+
220+
func Test_NamespaceDelete_AppWithWithOneNonTerminatingAffectedNamespace(t *testing.T) {
72221
env := e2e.BuildEnv(t)
73222
logger := e2e.Logger{}
74223
kapp := e2e.Kapp{t, env.Namespace, logger}

0 commit comments

Comments
 (0)