fix: delete misconfigured pods in error state. by hessjcg · Pull Request #338 · GoogleCloudPlatform/cloud-sql-proxy-operator

hessjcg · 2023-05-08T14:54:38Z

The pod cannot be configured to reliably instrument every single pod creation event due to limitations in the K8s API.
Thus, the operator needs to listen for pod create and update events, to check if the pod is configured correctly. If the
pod is not configured correctly, and the pod is in an error or waiting state, the operator will delete the pod so that it can
be replaced. The replacement pod will be correctly configured by the webhook as it is created.

Fixes #337

hessjcg · 2023-05-08T16:30:03Z

 // handleCreatePodRequest Finds relevant AuthProxyWorkload resources and updates the pod
 // with matching resources, returning a non-nil pod when the pod was updated.
 func (a *PodAdmissionWebhook) handleCreatePodRequest(ctx context.Context, p corev1.Pod) (*corev1.Pod, error) {
+	l := logf.FromContext(ctx)


This change moves code from the handleCreatePodRequest() function into a new function called findMatchingProxies() so that it can be reused between the webhook and the new Pod event listener.

hessjcg · 2023-05-08T16:32:30Z

 // listOwners returns the list of this object's owners and its extended owners.
 // Warning: this is a recursive function
-func (a *PodAdmissionWebhook) listOwners(ctx context.Context, object client.Object) ([]workload.Workload, error) {
+func listOwners(ctx context.Context, c client.Client, object client.Object) ([]workload.Workload, error) {


This refactors listOwners() from a member of PodAdmissionWebhook to a plain function so that it can be used both in the webhook and the pod event listener.

hessjcg · 2023-05-08T16:32:55Z

 	}
 	return owners, nil
 }
+


This is the new pod event listener implementation.

hessjcg · 2023-05-08T16:33:54Z

 		setupLog.Error(err, "unable to create workload admission webhook controller")
 		return err
 	}
+	err = registerPodInformer(mgr, u)


Register the PodEventHandler when the operator starts up.

jackwotherspoon · 2023-05-08T16:59:05Z

@hessjcg Is it possible to add tests for this to make sure PodEventHandler is WAI? We should aim to have tests for all feat PRs.

enocom · 2023-05-08T19:18:48Z

Does this fix #337?

hessjcg · 2023-05-08T22:22:00Z

@enocom, Yes this fixes #337.
@jackwotherspoon, I added some unit tests in pod_controller_test.go

…configured incorrectly.

… PodEventHandler.

enocom · 2023-05-09T15:48:52Z

Updated the title to reflect what we're doing here.

enocom · 2023-05-09T15:54:22Z

+}
+
+// OnAdd is called by the informer when a Pod is added.
+func (h *PodEventHandler) OnAdd(obj interface{}) {


Is this and the following methods part of an interface that Kubernetes uses?

the OnAdd, OnChange, OnDelete methods all implement cache.ResourceEventHandler. I've updated the method comment.

hessjcg

Updated in response to @enocom

hessjcg · 2023-05-09T20:20:58Z

+}
+
+// OnAdd is called by the informer when a Pod is added.
+func (h *PodEventHandler) OnAdd(obj interface{}) {


the OnAdd, OnChange, OnDelete methods all implement cache.ResourceEventHandler. I've updated the method comment.

enocom

LGTM. I'd like @pwschuurman to approve this one just to make sure we have the logic and nuances right here.

enocom

LGTM. I'd like @pwschuurman to approve this one just to make sure we have the logic and nuances right here.

pwschuurman · 2023-05-12T16:30:20Z

/lgtm
/approve

hessjcg · 2023-05-12T21:56:22Z

@enocom This needs an "Approve" review from you before I can merge it, since @pwschuurman is not a code owner.

jackwotherspoon · 2023-05-15T17:42:01Z

+		return nil, fmt.Errorf("there is an AuthProxyWorkloadConfiguration error reconciling this workload %v", wlConfigErr)
+	}
+
+	return wl.Pod, nil // updated


Is this comment needed here? Seems slightly vague, maybe change to // update pod? if that is what it is intended to say

jackwotherspoon · 2023-05-15T17:43:11Z

+	updater *workload.Updater
+}
+
+// newDeletePodController constructs an podDeleteController


Suggested change

// newDeletePodController constructs an podDeleteController

// newDeletePodController constructs a podDeleteController

jackwotherspoon · 2023-05-15T17:44:36Z

+		return nil
+	}
+
+	// Check if this pod is in error and missing proxy containers


add detail here? is in error the proper wording? in error state perhaps?

Suggested change

// Check if this pod is in error and missing proxy containers

// Check if this pod is in error state and missing proxy containers

jackwotherspoon · 2023-05-15T17:46:00Z

+	// Check if this pod is in error and missing proxy containers
+	wlConfigErr := r.updater.CheckWorkloadContainers(wl, proxies)
+
+	// If this pod is in error, delete it. Simply logging an error is sufficient.


same here should we be saying in error?

hessjcg commented May 8, 2023

View reviewed changes

hessjcg marked this pull request as ready for review May 8, 2023 16:40

hessjcg requested a review from a team as a code owner May 8, 2023 16:40

hessjcg requested review from enocom and jackwotherspoon May 8, 2023 16:40

hessjcg added 5 commits May 8, 2023 17:30

feat: Listen for pod changes and delete pods in error state that are …

69b3884

…configured incorrectly.

test: add unit tests for deleting misconfigured pods.

df572a5

chore: remove extra logging.

01a7604

test: add unit test for event handler.

333eccb

chore: separate responsibility for implementing manager.Runnable from…

8a33ea6

… PodEventHandler.

hessjcg force-pushed the gh-337-pod-config-sync branch from 01a864b to 8a33ea6 Compare May 8, 2023 23:30

enocom changed the title ~~feat: Listen for pod changes and delete misconfigured pods in error state.~~ fix: delete misconfigured pods in error state. May 9, 2023

enocom reviewed May 9, 2023

View reviewed changes

chore: code review comments.

da7d837

hessjcg requested a review from pwschuurman May 10, 2023 19:58

hessjcg commented May 10, 2023

View reviewed changes

hessjcg requested a review from enocom May 10, 2023 19:59

enocom reviewed May 10, 2023

View reviewed changes

pwschuurman reviewed May 12, 2023

View reviewed changes

Comment thread internal/controller/pod_controller.go Outdated

pwschuurman reviewed May 12, 2023

View reviewed changes

Comment thread internal/controller/pod_controller.go Outdated

pwschuurman reviewed May 12, 2023

View reviewed changes

Comment thread internal/controller/pod_controller.go Outdated

pwschuurman approved these changes May 12, 2023

View reviewed changes

hessjcg removed the request for review from jackwotherspoon May 12, 2023 21:55

fix: rework the pod event handler into a pod reconciler.

a8577bf

hessjcg added the tests: run Run all the tests for this PR label May 15, 2023

github-actions Bot removed the tests: run Run all the tests for this PR label May 15, 2023

jackwotherspoon reviewed May 15, 2023

View reviewed changes

chore: Respond to @jackwotherspoon code review comments.

a1352e5

hessjcg requested a review from jackwotherspoon May 15, 2023 17:51

jackwotherspoon approved these changes May 15, 2023

View reviewed changes

hessjcg merged commit 4a02aa7 into main May 15, 2023

hessjcg deleted the gh-337-pod-config-sync branch May 15, 2023 18:16

release-please Bot mentioned this pull request May 15, 2023

chore(main): release 1.0.0 #344

Merged

	// newDeletePodController constructs an podDeleteController
	// newDeletePodController constructs a podDeleteController

	// Check if this pod is in error and missing proxy containers
	// Check if this pod is in error state and missing proxy containers

Conversation

hessjcg commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

jackwotherspoon commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enocom commented May 8, 2023

Uh oh!

hessjcg commented May 8, 2023

Uh oh!

enocom commented May 9, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hessjcg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

enocom left a comment

Choose a reason for hiding this comment

Uh oh!

enocom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pwschuurman commented May 12, 2023

Uh oh!

hessjcg commented May 12, 2023

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hessjcg commented May 8, 2023 •

edited

Loading

jackwotherspoon commented May 8, 2023 •

edited

Loading