feat(namespaces): add empty namespace detection and removal #249
Conversation
|
fixes #92 This PR is still WIP, as it lacks parallel processing of the namespaces as well as unit tests, could you please review if in principle it is what you'd be happy to accept ? |
|
Hi @isindir, Take a look at https://github.com/yonahd/kor/blob/main/CONTRIBUTING.md#repository-structure. |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
==========================================
- Coverage 43.72% 43.54% -0.19%
==========================================
Files 63 65 +2
Lines 4064 4219 +155
==========================================
+ Hits 1777 1837 +60
- Misses 2039 2134 +95
Partials 248 248 ☔ View full report in Codecov by Sentry. |
This is definitely a good direction(this is not a simple one). |
Helm chart I think needs extra changes:
I'm looking to work on unit tests if time allows these weekends. |
isindir
force-pushed
the
unused_namespaces
branch
3 times, most recently
from
07bc31c to
7246d82
Compare
Correct, currently the deployment is only used to export metrics, hence read-only RBAC, but we can consider going that way.
Interesting, especially when scanning unused resources in high-scaled clusters that might take more time than the interval set. We can continue with progress on both comments in separated issues/discussions (or in our Discord channel) as they are out-of-scope for this PR, but should be dippen into. I'd like to futher discuss them. Referring to https://github.com/yonahd/kor/blob/main/CONTRIBUTING.md#repository-structure was made to make sure you modify all required files, as adding any new unused resource support should address them. |
|
@doronkg ,
Thank you, I think I covered most of the files except helm chart and some unit tests, atm I'm trying to figure out if I can use fake clients to reliably test a feature I'm implementing. I deliberately left chart untouched as the change already looks very big. btw, for chart - it will be better to have fine-tuned RBAC - possibility to enable only those read/write/per namespace permissions per feature (resource). |
isindir
force-pushed
the
unused_namespaces
branch
from
ac738c6 to
5b15ce6
Compare
|
@yonahd @luisdavim @doronkg, I finally found time and adding some more testing to the namespace removal - but I was not following if json configs with resources to skip from namespace evaluation for different k8s distros is fully implemented or not. Could you please suggest and review if my testing in |
The resource exceptions are merged fully. |
|
Let me know when this is ready for review. This is a massive pr and will take quite a while to review |
isindir
force-pushed
the
unused_namespaces
branch
from
b6dd857 to
02a3f7e
Compare
|
@yonahd I feel that I'll not be able to do 100% unit test coverage for my new code, please review this PR. Thanks. |
isindir
force-pushed
the
unused_namespaces
branch
from
d5d757e to
1f091da
Compare
|
@yonahd - I rebased the PR to the latest, would you have time to review and may be merge it ? We could negotiate next course of actions if needed from my side on a call, please reach me out on keybase , same uid as here. |
Thanks for the PR |
Hi, sorry for the late response. |
isindir
force-pushed
the
unused_namespaces
branch
from
4d854e2 to
fecedb6
Compare
isindir
force-pushed
the
unused_namespaces
branch
from
fecedb6 to
c38cac2
Compare
|
@doronkg would you have some time to review it now ? |
Hey, I’m currently traveling, so reviewing everything at once might be a bit challenging. To make this manageable, let’s break it down into smaller chunks and review each component step by step. So far, I’ve gone through all the additions (except for the tests—we’ll tackle those later) and reviewed the previous comments. To help me better understand the PR, could you please clarify the following: Also, please update the PR description with the format found under |
|
Hi @doronkg , thank you for going through the review, I'll try to answer your questions, and I'm happy to do some rework if needed.
General logic is defines filter functions and run all of these with the specified parameters for a given namespace from here: these functions are:
once we know subjects to inspect - next check is - if namespace contains any non-default resources and resources which user specified to be ignored, which is done here: and namespaced resource filters are applied here to identify which namespaces can be considered as empty:
I guess this is the function mentioned above:
These are not automatic via IDE, but it feels that long strings may read better once refactored: - fmt.Printf("Do you want to delete %s %s in namespace %s? (Y/N): ", resourceType, resource.Name, namespace)
+ fmt.Printf(
+ "Do you want to delete %s %s%s? (Y/N): ",
+ resourceType,
+ resource.Name,
+ namespacedMessageSuffix(namespace),
+ )but I can undo these changes.
I'll try to do this tonight and will send via new comment. |
|
I have a couple of examples now out of the box where it prints unwanted information: and |
|
|
Thank you, I have forked your branch to review it more thoroughly and run some tests.
We'll begin with Filters, that includes changes mostly in:
From my impression, all filter-related additions could be reverted or refactored, I'll explain. IncludeNamespacesFilter(), ExcludeNamespacesFilter() & ApplyFilters()As mentioned in #249 (comment), this logic is already implemented under // Namespaces returns the namespaces, only called once
func (o *Options) Namespaces(clientset kubernetes.Interface) []string {
o.once.Do(func() {
namespaces := make([]string, 0)
namespacesMap := make(map[string]bool)
if len(o.IncludeNamespaces) > 0 && len(o.ExcludeNamespaces) > 0 {
fmt.Fprintf(os.Stderr, "Exclude namespaces can't be used together with include namespaces. Ignoring --exclude-namespaces (-e) flag\n")
o.ExcludeNamespaces = nil
}
includeNamespaces := o.IncludeNamespaces
excludeNamespaces := o.ExcludeNamespaces
...Therefore, both Let's see an example for that with DaemonSet implementation under func GetUnusedDaemonSets(filterOpts *filters.Options, clientset kubernetes.Interface, outputFormat string, opts common.Opts) (string, error) {
resources := make(map[string]map[string][]ResourceInfo)
for _, namespace := range filterOpts.Namespaces(clientset) {
diff, err := processNamespaceDaemonSets(clientset, namespace, filterOpts)
if err != nil {
fmt.Fprintf(os.Stderr, "Failed to process namespace %s: %v\n", namespace, err)
continue
}
...In order to process the cluster DaemonSets, we call func processNamespaceDaemonSets(clientset kubernetes.Interface, namespace string, filterOpts *filters.Options) ([]ResourceInfo, error) {
daemonSetsList, err := clientset.AppsV1().DaemonSets(namespace).List(context.TODO(), metav1.ListOptions{LabelSelector: filterOpts.IncludeLabels})
if err != nil {
return nil, err
}
...
for _, daemonSet := range daemonSetsList.Items {
if pass, _ := filter.SetObject(&daemonSet).Run(filterOpts); pass {
continue
}
if daemonSet.Labels["kor/used"] == "false" {
reason := "Marked with unused label"
daemonSetsWithoutReplicas = append(daemonSetsWithoutReplicas, ResourceInfo{Name: daemonSet.Name, Reason: reason})
continue
}
...Afterwards, we iterate all the found DaemonSets in each namespace, and apply resource-specific filters such as labels and age with
Moving forward, please refactor SystemNamespaceFilter()This filter is of course needed, but its not in the appropriate format, for the following reasons:
The method we use to overcome these requirements is using Exceptions, let's return to DaemonSets to better understand it. {
"exceptionDaemonSets": [
{
"Namespace": "kube-system",
"ResourceName": "aws-node"
},
...
And then, we easily filter these specific resources out in //go:embed exceptions/daemonsets/daemonsets.json
var daemonsetsConfig []byte
func processNamespaceDaemonSets(clientset kubernetes.Interface, namespace string, filterOpts *filters.Options) ([]ResourceInfo, error) {
...
for _, daemonSet := range daemonSetsList.Items {
if pass, _ := filter.SetObject(&daemonSet).Run(filterOpts); pass {
continue
}
exceptionFound, err := isResourceException(daemonSet.Name, daemonSet.Namespace, config.ExceptionDaemonSets)
if err != nil {
return nil, err
}
if exceptionFound {
continue
}
...
Same here, please refactor In summary
That's it for now, feel free to ask me any questions if something is unclear or if I missed something. Lastly, please update the PR description with the format found under |
isindir
force-pushed
the
unused_namespaces
branch
from
c38cac2 to
95a0140
Compare
|
@doronkg , please check now - I think my refactoring for the first batch is what you asked me to do:
I've also added one |
|
The refactor looks good, can you add a commit to discard the additions of I'll get started on the following review. |
|
@doronkg , removed - somehow through git manipulations made these changes back and did not double check the final result, sorry |
There was a problem hiding this comment.
This review covers Unused Namespaces Evaluation, I commented on the following files:
- pkg/kor/namespaces.go
- pkg/kor/namespaces_test.go
- pkg/kor/exceptions/namespaces/namespaces.json
As for pkg/kor/namespacesMoreTests_test.go -
The direction with the different discovery scenarios is very nice and provides solid test coverage, but, it really differs from the method we widely use in our tests, leveraging fake client and creating custom resources for the test suites.
I left no comments here for now, @yonahd I'd like your take on this before moving forward with this test structure.
| gv := strings.Split(apiResourceList.GroupVersion, "/") | ||
| gvr, err := getGVR(apiResource.Name, gv) |
There was a problem hiding this comment.
Moving the splitting action into getGVR() would have a better flow.
func getGVR(name string, groupVersion string) (*schema.GroupVersionResource, error) {
splitGV = strings.Split(groupVersion, "/")
switch NumberOfGVPartsFound := len(splitGV); NumberOfGVPartsFound {
...| return true, err | ||
| } | ||
|
|
||
| unstructuredList, err := dynamicClient.Resource(*gvr).Namespace(namespace).List(ctx, metav1.ListOptions{}) |
There was a problem hiding this comment.
Both unstructuredList & unstructuredObj are not indicative parameter names.
pkg/kor/namespaces_test.go
Outdated
There was a problem hiding this comment.
- Fix function tests if needed following
namespaces.goreview. - Remove tests for removed functions.
- Refactor function naming convention to match the rest of the codebase -
TestFunctionName()(for example,Test_namespaces_IgnoreResourceTypes() -> TestIgnoreResourceTypes). - Missing tests for main functions:
processNamespaces()&GetUnusedNamespaces()(seeTestGetUnusedStorageClassesStructured()for example). Create test namespaces and use*fake.ClientSetto evaluate them.
NOTE: Highly recommended to review other resources' tests before working on this.
… Identifier, as it contains Name and cannot be called Name
isindir
force-pushed
the
unused_namespaces
branch
from
26a9236 to
a6433c0
Compare
What this PR does / why we need it?
This PR finds empty and non-default kubernetes namespaces, and if instructed removes these from the cluster.
PR Checklist
GitHub Issue
Closes [#92]
Notes for your reviewers