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

feat: add Validating/Mutating webhook analyzer #548

Merged
merged 5 commits into from
Jul 12, 2023

Conversation

rakshitgondwal
Copy link
Member

@rakshitgondwal rakshitgondwal commented Jul 7, 2023

Closes #533

📑 Description

Changes made:

  • Added an analyzer for Validating Webhook that checks if the pod that webhook points to is not active.
  • Added an analyzer for Mutating Webhook that checks if the pod that webhook points to is not active.

✅ Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

Additional Information

Screenshot from 2023-07-07 17-47-44

Signed-off-by: Rakshit Gondwal <rakshitgondwal3@gmail.com>
Signed-off-by: Rakshit Gondwal <rakshitgondwal3@gmail.com>
@rakshitgondwal rakshitgondwal marked this pull request as ready for review July 7, 2023 12:18
@rakshitgondwal rakshitgondwal requested review from a team as code owners July 7, 2023 12:18
@arbreezy
Copy link
Member

@rakshitgondwal would be nice to share what openai's response is as well based on the error messages you feed it

@rakshitgondwal
Copy link
Member Author

@rakshitgondwal would be nice to share what openai's response is as well based on the error messages you feed it

Screenshot from 2023-07-10 22-15-31

@arbreezy
Copy link
Member

@rakshitgondwal would be nice to share what openai's response is as well based on the error messages you feed it

Screenshot from 2023-07-10 22-15-31

I also left a few comments, thanks for your contribution !

@rakshitgondwal
Copy link
Member Author

I also left a few comments, thanks for your contribution !

Sorry, but I can't see your comments, can you please re check?

pkg/analyzer/mutating-webhook.go Outdated Show resolved Hide resolved
pkg/analyzer/validating-webhook.go Outdated Show resolved Hide resolved
pkg/analyzer/validating-webhook.go Show resolved Hide resolved
pkg/analyzer/mutating-webhook.go Show resolved Hide resolved
pkg/analyzer/validating-webhook.go Show resolved Hide resolved
pkg/analyzer/mutating-webhook.go Show resolved Hide resolved
Signed-off-by: Rakshit Gondwal <rakshitgondwal3@gmail.com>
Signed-off-by: Rakshit Gondwal <rakshitgondwal3@gmail.com>
@arbreezy arbreezy self-requested a review July 12, 2023 12:30
@AlexsJones AlexsJones merged commit 750a10d into k8sgpt-ai:main Jul 12, 2023
@rakshitgondwal
Copy link
Member Author

Thank you for the merge!

@AlexsJones
Copy link
Member

Hi @rakshitgondwal this was my fault for merging entirely.

Looking at the code can you explain what it's doing?
Because to my eyes it looks like you are looping through each webhook, then looping through every pod in the cluster and checking running status. This means it's looping multiple times

Keep me honest here @arbreezy

@rakshitgondwal
Copy link
Member Author

Oh yes @AlexsJones, even I am at fault here for misunderstanding the issue. I didn't think about this thoroughly once the PR was approved. Should have made a Draft PR first and then should've asked for feedback on the logic.

But, after giving this a second thought, I had one question that should the analyzer be pointing to a service or a pod? Because in the WebhookConfiguration we mention the service that the webhook points to so how come would we be able to point to a pod? Correct me if I'm wrong here, and yes I'd like to fix this if you allow me.

@AlexsJones
Copy link
Member

image

As per here, use the service-name the webhook is bound too and check if the service has pods that are active ( at least 1 )
that should be the real logic

@rakshitgondwal
Copy link
Member Author

I was trying out this logic, but it is not working. I added a LabelSelector while getting the pods to be service.name, then looped on the pods and check if they are running. Can you help me out with this? @AlexsJones

svc := webhook.ClientConfig.Service
pods, err := a.Client.GetClient().CoreV1().Pods(a.Namespace).List(context.Background(), v1.ListOptions{LabelSelector: svc.Name})
	if err != nil {
		return nil, err
	}
	activePods := 0
	for _, pod := range pods.Items {
		if pod.Status.Phase == "Running" {
			activePods++
		}
	}
	if activePods == 0 {
		doc := apiDoc.GetApiDocV2("spec.webhook")
		failures = append(failures, common.Failure{

@AlexsJones
Copy link
Member

I can take a look at implementing it a version of this and seeing whats wrong later today

@AlexsJones
Copy link
Member

#553

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Feature]: Mutating/Validating web hook analyser
3 participants