feat: add send to slack support

[patricktalmeida]

As discussed in #73, this is a separate PR to add send to Slack support.

[yonahd]

As a general direction here we should pass a client set to the functions instead of a kubeconfig.
That way we can check if we are in cluster before and either create the clientset from the service account or from the kubeconfig

[patricktalmeida]

As a general direction here we should pass a client set to the functions instead of a kubeconfig. That way we can check if we are in cluster before and either create the clientset from the service account or from the kubeconfig

@yonahd I didn't quite understand your idea. I created 2 separate handlers for InCluster and KubeConfig configurations.
I create the clientset from those handlers and I let GetKubeClient handle which one to use. And kubeconfig file is only sent to KubeConfigClient which needs it to create the client.

Could you explain your idea when looking into those 3 functions? KubeConfigClient, InClusterConfigClient and GetKubeClient

[yonahd]

As a general direction here we should pass a client set to the functions instead of a kubeconfig. That way we can check if we are in cluster before and either create the clientset from the service account or from the kubeconfig

@yonahd I didn't quite understand your idea. I created 2 separate handlers for InCluster and KubeConfig configurations. I create the clientset from those handlers and I let GetKubeClient handle which one to use. And kubeconfig file is only sent to KubeConfigClient which needs it to create the client.

Could you explain your idea when looking into those 3 functions? KubeConfigClient, InClusterConfigClient and GetKubeClient

I don't want to limit any of the functions to only be in cluster or only out of cluster. That's why I want to create the separation of kubeconfig vs service account earlier and regardless pass a clientset to the functions.

[patricktalmeida]

As a general direction here we should pass a client set to the functions instead of a kubeconfig. That way we can check if we are in cluster before and either create the clientset from the service account or from the kubeconfig

@yonahd I didn't quite understand your idea. I created 2 separate handlers for InCluster and KubeConfig configurations. I create the clientset from those handlers and I let GetKubeClient handle which one to use. And kubeconfig file is only sent to KubeConfigClient which needs it to create the client.
Could you explain your idea when looking into those 3 functions? KubeConfigClient, InClusterConfigClient and GetKubeClient

I don't want to limit any of the functions to only be in cluster or only out of cluster. That's why I want to create the separation of kubeconfig vs service account earlier and regardless pass a clientset to the functions.

I think I got your idea. But still thinking on how to control that. I mean, when it should authenticate with kubeconfig or when to use rest.InClusterConfig().
What do you think of sending a flag when it should authenticate from inside the cluster, e.g. kor all --in-cluster-auth?
With that it could be could be hard coded inside the helm template for CronJob and also in the future for Deployments. e.g. args: ["{{ .Values.cronJob.korCommand }} --in-cluster-auth --slack-channel {{ .Values.cronJob.slackChannel }} --slack-auth-token {{ .Values.cronJob.slackAuthToken }}"]

[yonahd]

We can try something like this

func GetKubeClient(kubeconfig string) *kubernetes.Clientset {
	if _, err := os.Stat("/var/run/secrets/kubernetes.io/serviceaccount/token"); err == nil {
		config, err := rest.InClusterConfig()
		if err != nil {
			fmt.Fprintf(os.Stderr, "Failed to load kubeconfig: %v\n", err)
			os.Exit(1)
		}

		clientset, err := kubernetes.NewForConfig(config)
		if err != nil {
			fmt.Fprintf(os.Stderr, "Failed to create Kubernetes client: %v\n", err)
			os.Exit(1)
		}
		return clientset
	}
	if kubeconfig == "" {
		if configEnv := os.Getenv("KUBECONFIG"); configEnv != "" {
			kubeconfig = configEnv
		} else {
			kubeconfig = GetKubeConfigPath()
		}
	}
	config, err := clientcmd.BuildConfigFromFlags("", kubeconfig)
	if err != nil {
		fmt.Fprintf(os.Stderr, "Failed to load kubeconfig: %v\n", err)
		os.Exit(1)
	}

	clientset, err := kubernetes.NewForConfig(config)
	if err != nil {
		fmt.Fprintf(os.Stderr, "Failed to create Kubernetes client: %v\n", err)
		os.Exit(1)
	}
	return clientset
}

and in the command we can do e.g.

var configmapCmd = &cobra.Command{
	Use:     "configmap",
	Aliases: []string{"cm"},
	Short:   "Gets unused configmaps",
	Args:    cobra.ExactArgs(0),
	Run: func(cmd *cobra.Command, args []string) {
		clientset := kor.GetKubeClient(kubeconfig)
		if outputFormat == "json" || outputFormat == "yaml" {
			if response, err := kor.GetUnusedConfigmapsStructured(includeExcludeLists, clientset, outputFormat); err != nil {
				fmt.Println(err)
			} else {
				fmt.Println(response)
			}
		} else {
			kor.GetUnusedConfigmaps(includeExcludeLists, clientset)
		}

	},
}

[patricktalmeida]

We can try something like this

func GetKubeClient(kubeconfig string) *kubernetes.Clientset {
	if _, err := os.Stat("/var/run/secrets/kubernetes.io/serviceaccount/token"); err == nil {
		config, err := rest.InClusterConfig()
		if err != nil {
			fmt.Fprintf(os.Stderr, "Failed to load kubeconfig: %v\n", err)
			os.Exit(1)
		}

		clientset, err := kubernetes.NewForConfig(config)
		if err != nil {
			fmt.Fprintf(os.Stderr, "Failed to create Kubernetes client: %v\n", err)
			os.Exit(1)
		}
		return clientset
	}
	if kubeconfig == "" {
		if configEnv := os.Getenv("KUBECONFIG"); configEnv != "" {
			kubeconfig = configEnv
		} else {
			kubeconfig = GetKubeConfigPath()
		}
	}
	config, err := clientcmd.BuildConfigFromFlags("", kubeconfig)
	if err != nil {
		fmt.Fprintf(os.Stderr, "Failed to load kubeconfig: %v\n", err)
		os.Exit(1)
	}

	clientset, err := kubernetes.NewForConfig(config)
	if err != nil {
		fmt.Fprintf(os.Stderr, "Failed to create Kubernetes client: %v\n", err)
		os.Exit(1)
	}
	return clientset
}

and in the command we can do e.g.

var configmapCmd = &cobra.Command{
	Use:     "configmap",
	Aliases: []string{"cm"},
	Short:   "Gets unused configmaps",
	Args:    cobra.ExactArgs(0),
	Run: func(cmd *cobra.Command, args []string) {
		clientset := kor.GetKubeClient(kubeconfig)
		if outputFormat == "json" || outputFormat == "yaml" {
			if response, err := kor.GetUnusedConfigmapsStructured(includeExcludeLists, clientset, outputFormat); err != nil {
				fmt.Println(err)
			} else {
				fmt.Println(response)
			}
		} else {
			kor.GetUnusedConfigmaps(includeExcludeLists, clientset)
		}

	},
}

Agreed and done. Let me know if you need to change something.

[yonahd]

Perfect!
Don't want to inconvenience you too much, but can we separate the slack part to a different MR?
This MR would close #75
And I'll open a different ticket for Slack messages(I think we can combine some of the functions).
In addition We can have the slack reports also out of the cluster

[patricktalmeida]

Perfect! Don't want to inconvenience you too much, but can we separate the slack part to a different MR? This MR would close #75 And I'll open a different ticket for Slack messages(I think we can combine some of the functions). In addition We can have the slack reports also out of the cluster

Sure. I can split it in two.
What do you mean by having slack report out of the cluster? I mean it already works out of the cluster also. If you run it from your local machine kor all --slack-channel <your-channel> --slack-auth-token <your-token> --include-namespaces default,some-ns it's gonna work just fine.

[yonahd]

Perfect! Don't want to inconvenience you too much, but can we separate the slack part to a different MR? This MR would close #75 And I'll open a different ticket for Slack messages(I think we can combine some of the functions). In addition We can have the slack reports also out of the cluster

Sure. I can split it in two. What do you mean by having slack report out of the cluster? I mean it already works out of the cluster also. If you run it from your local machine kor all --slack-channel <your-channel> --slack-auth-token <your-token> --include-namespaces default,some-ns it's gonna work just fine.

My mistake.
Now though it can be a condition in the regular function.

[codecov-commenter]

Codecov Report

Attention: 242 lines in your changes are missing coverage. Please review.

Comparison is base (f3235b0) 49.36% compared to head (12cd7e8) 43.58%.
Report is 1 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
- Coverage   49.36%   43.58%   -5.79%     
==========================================
  Files          15       16       +1     
  Lines        1493     1714     +221     
==========================================
+ Hits          737      747      +10     
- Misses        657      865     +208     
- Partials       99      102       +3     

Files	Coverage Δ
pkg/kor/confimgmaps.go	56.48% <0.00%> (-5.18%)	⬇️
pkg/kor/deployments.go	36.06% <0.00%> (-7.94%)	⬇️
pkg/kor/ingresses.go	50.48% <0.00%> (-6.04%)	⬇️
pkg/kor/pdbs.go	38.46% <0.00%> (-6.32%)	⬇️
pkg/kor/roles.go	44.18% <0.00%> (-6.49%)	⬇️
pkg/kor/secrets.go	60.00% <0.00%> (-5.33%)	⬇️
pkg/kor/serviceaccounts.go	52.59% <0.00%> (-4.67%)	⬇️
pkg/kor/services.go	36.06% <0.00%> (-7.94%)	⬇️
pkg/kor/hpas.go	40.19% <0.00%> (-4.86%)	⬇️
pkg/kor/multi.go	0.00% <0.00%> (ø)
... and 4 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[yonahd]

I'd like to condense the slack functions to be part of the GetUnusedHpas function.
Can we make the slack parameters optional and based on the users sent parameters decide if we are sending to slack(and whether it's a file or webhook)?

[patricktalmeida]

I'd like to condense the slack functions to be part of the GetUnusedHpas function. Can we make the slack parameters optional and based on the users sent parameters decide if we are sending to slack(and whether it's a file or webhook)?

I'd like to condense the slack functions to be part of the GetUnusedHpas function. Can we make the slack parameters optional and based on the users sent parameters decide if we are sending to slack(and whether it's a file or webhook)?

alright, give me some time to take a look at that.

[yonahd]

@patricktalmeida Is this ready for review?

[patricktalmeida]

@patricktalmeida Is this ready for review?

Yes it is.

[yonahd]

Overall looks good.
If you can add a screenshot from slack to images(otherwise I'll do it)
Also there is the Readme updates which you can optionally do

[yonahd]

FYI I also opened this.
#91
We might have to make a small refactor in the code to support it in Helm properly

[patricktalmeida]

FYI I also opened this. #91 We might have to make a small refactor in the code to support it in Helm properly

I'm gonna look into that.

[patricktalmeida]

Overall looks good. If you can add a screenshot from slack to images(otherwise I'll do it) Also there is the Readme updates which you can optionally do

Well, I believe it would be better if you added it. The cluster I'm testing on has sensitive data.

[yonahd]

LGTM