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.

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

score/disruptionbudget: output comment when namespace doesn't match #549

Merged
merged 2 commits into from
Aug 7, 2023
Merged

score/disruptionbudget: output comment when namespace doesn't match #549

merged 2 commits into from
Aug 7, 2023

Conversation

cayla
Copy link
Contributor

@cayla cayla commented Aug 3, 2023

I was troubleshooting a strange kube-score failure regarding PodDisruptionBudget.

kube-score was complaining that a deployment didn't have a PDB even though it very much did.

Going back to the code I realized the test was exiting early due to the namespace check:

if budget.Namespace() != namespace {
continue
}

Obviously, this was a bad manifest on my end (explicit is better than implicit and the tests passed as soon as I added namespace: {{ .Release.Namespace }} to my helm charts), but perhaps with a bit more output we could help other folks connect the dots faster.

RELNOTE: score/disruptionbudget: output comment when namespace doesn't match

I was troubleshooting a strange kube-score failure regarding
PodDisruptionBudget.

kube-score was complaining that a deployment didn't have a PDB even
though it very much did.

Going back to the code I realized the test was exiting early due to the
namespace check:

https://github.com/zegl/kube-score/blob/0b3f154ca3f06a13323431a7d2199a74a1869fbc/score/disruptionbudget/disruptionbudget.go#L23-L25

Obviously, this was a bad manifest on my end (explicit is better than
implicit and the tests passed as soon as I added `namespace: {{
.Release.Namespace }}` to my helm charts, but perhaps with a bit more
output we could help other folks connect the dots faster.
@zegl
Copy link
Owner

zegl commented Aug 4, 2023

Hey, thanks for the PR. This comment would fire for every PDB until a matching one is found, and can fire even if a PDB ends up getting found by later iterations of the loop.

Could you move this comment down to just before return false, nil, and only set this comment if any PDB has been ignored to mismatched namespaces?

Something like:

func hasMatching(budgets []ks.PodDisruptionBudget, namespace string, labels map[string]string) (bool, error) {
	var hasNamespaceMismatch bool
	for _, budget := range budgets {
		if budget.Namespace() != namespace {
			hasNamespaceMismatch = true
			continue
		}

		selector, err := metav1.LabelSelectorAsSelector(budget.PodDisruptionBudgetSelector())
		if err != nil {
			return false, fmt.Errorf("failed to create selector: %w", err)
		}
		if selector.Matches(internal.MapLabels(labels)) {
			return true, nil
		}
	}

	if hasNamespaceMismatch {
		score.AddComment("", "PodDisruptionBudget not found", "Cannot associate the PodDisruptionBudget to the resource because they are not explicitly in the same namespace")
	}

	return false, nil
}

In my haste yesterday, I didn't even notice the linter telling me that
`score` was undefined.  This is probably better as an error anyhow as
its similar to the selector not matching.
@cayla
Copy link
Contributor Author

cayla commented Aug 4, 2023

Thank you for your feedback. If you can't tell, I am not a proper dev, let alone a go dev. But i am trying to give back more lately even if it means I goof up now and then.

I took your advice on getting the output outside the loop. I also converted the message to a error like the selector. Yesterday I failed to notice my linter complaining about score being undefined. I was going to crack open some go documentation to learn how to do that right when I thought, this actually is more like an error anyway -- like the selector.

Anyway, hope this is helpful. If not, no worries. I appreciate your tool. When I was getting started in k8s, it was invaluable to avoiding potholes.

PS - I built locally and ran the tests wo/ failure

@zegl
Copy link
Owner

zegl commented Aug 7, 2023

Thanks, this looks great!

@zegl zegl merged commit 224b35d into zegl:master Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants