[Security Hardened Kubernetes Cluster] Additional checks for namespaces marked for deletion #642

georgibaltiev · 2025-11-17T11:09:23Z

How to categorize this PR?

/kind enhancement

What this PR does / why we need it:
This PR enhances the already present evaluation of a Namespace' sNetworkPolicies, by checking if the namespace is marked for deletion with a .deletionTimestamp. If the namespace is marked for deletion, and has neither pods on it deployed nor any allow-all network policies, security finding results are marked as Warning instead of Failed

Which issue(s) this PR fixes:
Fixes #638

Special notes for your reviewer:

Release note:

[Rule 2000 of the Security Hardened Kubernetes Cluster ruleset] Namespaces marked for deletion without any pods will be marked as `Warning` findings instead of `Failed`

gardener-prow · 2025-11-17T11:09:28Z

@georgibaltiev: The label(s) kind/todo cannot be applied, because the repository doesn't have them.

In response to this:

How to categorize this PR?

/kind TODO

What this PR does / why we need it:
This PR enhances the already present evaluation of a Namespace' sNetworkPolicies, by checking if the namespace is marked for deletion with a .deletionTimestamp. If the namespace is marked for deletion, and has no pods on it deployed, security finding results are marked as Warning instead of Failed

Which issue(s) this PR fixes:
Fixes #638

Special notes for your reviewer:

Release note:
[Rule 2000 of the Security Hardened Kubernetes Cluster ruleset] Namespaces marked for deletion without any pods will be marked as `Warning` findings instead of `Failed`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

gardener-prow · 2025-11-17T11:09:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign dimityrmirchev for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

georgibaltiev · 2025-11-17T13:53:30Z

/hold
as I've noticed a bug with one of the unit tests

georgibaltiev · 2025-11-17T14:43:10Z

/unhold
the bug has been resolved

…ion without pods

AleksandarSavchev

Thanks! Have some minor suggestions

AleksandarSavchev · 2025-11-25T12:57:30Z

pkg/provider/managedk8s/ruleset/securityhardenedk8s/rules/2000.go

+		namespaceDeletionWithoutPodsMessage = "namespace is marked for deletion - no pods are deployed on it"
+		namespaceDeletionWithPodsMessage    = "namespace is marked for deletion - there are still pods deployed on it"


Suggested change

namespaceDeletionWithoutPodsMessage = "namespace is marked for deletion - no pods are deployed on it"

namespaceDeletionWithPodsMessage = "namespace is marked for deletion - there are still pods deployed on it"

namespaceDeletionWithoutPodsDetails = "namespace is marked for deletion with no present pods"

namespaceDeletionWithPodsDetails = "namespace is marked for deletion with present pods"

ingressTrafficNotDeniedMessage = "Ingress traffic is not denied by default."

egressTrafficNotDeniedMessage = "Egress traffic is not denied by default."

The "not denied by default" message is repeated several times, we can store it in a const.

AleksandarSavchev · 2025-11-25T13:09:07Z

pkg/provider/managedk8s/ruleset/securityhardenedk8s/rules/2000.go

+				pods, err := kubeutils.GetPods(ctx, r.Client, namespace.Name, labels.NewSelector(), 300)
+				if err != nil {
+					checkResults = append(checkResults, rule.ErroredCheckResult(err.Error(), rule.NewTarget()))
+					break
+				}


Suggested change

pods, err := kubeutils.GetPods(ctx, r.Client, namespace.Name, labels.NewSelector(), 300)

if err != nil {

checkResults = append(checkResults, rule.ErroredCheckResult(err.Error(), rule.NewTarget()))

break

}

pods, err := kubeutils.GetPods(ctx, r.Client, namespace.Name, labels.NewSelector(), 300)

if err != nil {

return rule.Result(r, rule.ErroredCheckResult(err.Error(), target)), nil

}

We should return on errored CheckResult here. I think it would only error here if there is a problem with the client. WDYT?

My assumption was based on the fact that there could be issues with the GetPods function, related to a specific namespace, for example the user not having permissions to access it, but I am not sure if those use cases are relevant, if even possible. This was my reasoning as to why I want to check all namespaces, even if one of them errors. WDYT?

Sure, the user might not have permissions for a specific namespace. On a side note the break here is not needed.

We tell the user to use an admin kubeconfig when running diki. Most likely there will be a lot of errored rules when such permissions are missing. I do not think we should cater to this case.

Okay, that makes sense.

AleksandarSavchev · 2025-11-25T13:09:18Z

pkg/provider/managedk8s/ruleset/securityhardenedk8s/rules/2000.go

+					checkResults = append(checkResults, rule.ErroredCheckResult(err.Error(), rule.NewTarget()))
+					break
+				}
+				if len(pods) > 0 {


Suggested change

if len(pods) > 0 {

if len(pods) > 0 {

AleksandarSavchev · 2025-11-25T13:09:59Z

pkg/provider/managedk8s/ruleset/securityhardenedk8s/rules/2000.go

 				checkResults = append(checkResults, rule.FailedCheckResult("All Ingress traffic is allowed by default.", allowsAllIngressTarget))
-			default:
+			case namespace.DeletionTimestamp == nil:
 				checkResults = append(checkResults, rule.FailedCheckResult("Ingress traffic is not denied by default.", target))


Suggested change

checkResults = append(checkResults, rule.FailedCheckResult("Ingress traffic is not denied by default.", target))

checkResults = append(checkResults, rule.FailedCheckResult(ingressTrafficNotDeniedMessage, target))

other applicable places.

AleksandarSavchev · 2025-11-25T13:10:28Z

pkg/provider/managedk8s/ruleset/securityhardenedk8s/rules/2000.go

+				if err != nil {
+					checkResults = append(checkResults, rule.ErroredCheckResult(err.Error(), rule.NewTarget()))
+					break


same as above

AleksandarSavchev · 2025-11-25T13:12:43Z

pkg/provider/managedk8s/ruleset/securityhardenedk8s/rules/2000_test.go

+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "deny-all",
+						Namespace: "plain-namespace",
+					},


Suggested change

ObjectMeta: metav1.ObjectMeta{

Name: "deny-all",

Namespace: "plain-namespace",

},

ObjectMeta: metav1.ObjectMeta{

Name: "foo",

Namespace: "plain-namespace",

},

I see that most of the entries use the same name & namespace. We can have the ObjectMeta be set in the test func and only pass the Spec here.

AleksandarSavchev · 2025-11-25T13:54:40Z

pkg/provider/managedk8s/ruleset/securityhardenedk8s/rules/2000_test.go

+					},
+				},
+			),
+			Entry("should warn fail an allow-all network policy is configured for the ingress traffic and there are no pods present",


Suggested change

Entry("should warn fail an allow-all network policy is configured for the ingress traffic and there are no pods present",

Entry("should fail when an allow-all network policy is configured for the ingress traffic and there are no pods present",

AleksandarSavchev · 2025-11-25T13:54:58Z

pkg/provider/managedk8s/ruleset/securityhardenedk8s/rules/2000_test.go

+					},
+				},
+			),
+			Entry("should warn fail an allow-all network policy is configured for the egress traffic and there are no pods present",


Suggested change

Entry("should warn fail an allow-all network policy is configured for the egress traffic and there are no pods present",

Entry("should fail when an allow-all network policy is configured for the egress traffic and there are no pods present",

AleksandarSavchev · 2025-11-25T13:57:14Z

pkg/provider/managedk8s/ruleset/securityhardenedk8s/rules/2000_test.go

+				&networkingv1.NetworkPolicy{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "deny-all",
+						Namespace: "plain-namespace",
+					},
+					Spec: networkingv1.NetworkPolicySpec{
+						PolicyTypes: []networkingv1.PolicyType{
+							"Ingress",
+							"Egress",
+						},
+					},
 				},


Can also have only the spec passed here.

georgibaltiev requested a review from a team as a code owner November 17, 2025 11:09

gardener-prow bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 17, 2025

gardener-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 17, 2025

gardener-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 17, 2025

georgibaltiev marked this pull request as draft November 17, 2025 15:22

gardener-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 17, 2025

georgibaltiev added 2 commits November 21, 2025 15:57

add: modifications for rule 2000 to warn only for namespaces in delet…

801c75c

…ion without pods

refactor: unit test cases

5256e60

georgibaltiev force-pushed the enh/rule-2000-shkc-warning-checkresults branch from 4dc35e6 to 5256e60 Compare November 21, 2025 13:59

georgibaltiev marked this pull request as ready for review November 21, 2025 13:59

gardener-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 21, 2025

AleksandarSavchev requested changes Nov 25, 2025

View reviewed changes

georgibaltiev added 4 commits November 27, 2025 12:30

fix: target details messages

a5e3fef

fix: cosmetic changes

f68e0a3

fix: shorten DescribeTable parameters

f7b904e

fix: errored check results when GetPods fails

4dc9cda

		namespaceDeletionWithoutPodsMessage = "namespace is marked for deletion - no pods are deployed on it"
		namespaceDeletionWithPodsMessage = "namespace is marked for deletion - there are still pods deployed on it"

-		namespaceDeletionWithoutPodsMessage = "namespace is marked for deletion - no pods are deployed on it"
-		namespaceDeletionWithPodsMessage    = "namespace is marked for deletion - there are still pods deployed on it"
+		namespaceDeletionWithoutPodsDetails = "namespace is marked for deletion with no present pods"
+		namespaceDeletionWithPodsDetails    = "namespace is marked for deletion with present pods"
+		ingressTrafficNotDeniedMessage = "Ingress traffic is not denied by default."
+		egressTrafficNotDeniedMessage = "Egress traffic is not denied by default."

	checkResults = append(checkResults, rule.FailedCheckResult("Ingress traffic is not denied by default.", target))
	checkResults = append(checkResults, rule.FailedCheckResult(ingressTrafficNotDeniedMessage, target))

	Entry("should warn fail an allow-all network policy is configured for the ingress traffic and there are no pods present",
	Entry("should fail when an allow-all network policy is configured for the ingress traffic and there are no pods present",

[Security Hardened Kubernetes Cluster] Additional checks for namespaces marked for deletion #642

Are you sure you want to change the base?

[Security Hardened Kubernetes Cluster] Additional checks for namespaces marked for deletion #642

Uh oh!

Conversation

georgibaltiev commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gardener-prow bot commented Nov 17, 2025

Uh oh!

gardener-prow bot commented Nov 17, 2025

Uh oh!

georgibaltiev commented Nov 17, 2025

Uh oh!

georgibaltiev commented Nov 17, 2025

Uh oh!

AleksandarSavchev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AleksandarSavchev Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

georgibaltiev commented Nov 17, 2025 •

edited

Loading

AleksandarSavchev Nov 25, 2025 •

edited

Loading