feat: Add reconcile controller logic issue #37 #39

hessjcg · 2022-10-04T02:44:01Z

Adds the logic to reconcile changes to AuthProxyWorkload resources to workloads.

controllers/authproxyworkload_controller.go

test/helpers/testcases.go

test/integration/integration_test.go

controllers/authproxyworkload_controller.go

…ted to getting workloads.

hessjcg

Updates from this review have been pushed.

controllers/authproxyworkload_controller.go

controllers/list_workloads.go

test/helpers/testcases.go

test/integration/integration_test.go

test/helpers/testcases.go

hessjcg

Resolved the comments I just fixed as @enocom intended.

controllers/authproxyworkload_controller.go

enocom · 2022-10-07T15:44:08Z

controllers/authproxyworkload_controller.go

+func (c *recentlyDeletedCache) get(k types.NamespacedName) bool {
+	c.lock.RLock()
+	defer c.lock.RUnlock()
+	if c.values == nil {


Reading out of a nil map is totally fine. It's setting into a nil map that will panic.

So this could just be:

deleted, ok := c.values[k] if !ok { return false } return deleted

enocom · 2022-10-07T15:49:37Z

controllers/authproxyworkload_controller.go

 // AuthProxyWorkloadReconciler reconciles a AuthProxyWorkload object
 type AuthProxyWorkloadReconciler struct {
 	client.Client
-	Scheme *runtime.Scheme
+	Scheme          *runtime.Scheme
+	recentlyDeleted recentlyDeletedCache


Since you're defining methods on recentlyDeletedCache as pointer receivers, this should match and be a pointer, if only to avoid the ambiguity of whether your sync.Mutex gets copied (It doesn't, but still it's not immediately obvious).

It's probably worth adding an initializer so callers don't have to understand these details:

r := controllers.NewAuthProxyReconciler(mgr)

enocom · 2022-10-07T16:10:12Z

controllers/authproxyworkload_controller.go

+func replaceCondition(conds []*metav1.Condition, newC *metav1.Condition) []*metav1.Condition {
+	for i := range conds {
+		c := conds[i]
+		if c.Type == newC.Type {


nit: indent the error case

if c.Type !== newC.Type { continue } // .. happy path here

enocom · 2022-10-07T16:12:58Z

controllers/authproxyworkload_controller_test.go

+}
+
+func TestReconcileDeleted(t *testing.T) {
+	wantRequeue := false


var wantRequeue bool

Also, let's move this closer to where it's used -- which might remove the need to even declare it as a variable.

I've inlined all of these test case parameters in this and other methods.

enocom · 2022-10-07T16:15:34Z

controllers/authproxyworkload_controller_test.go

+	c := cb.WithObjects(p).Build()
+	r, req, ctx := reconciler(p, c)
+
+	c.Delete(ctx, p)


Let's check the error here.

enocom · 2022-10-07T16:19:36Z

controllers/authproxyworkload_controller_test.go

+}
+
+func TestReconcileState21ByName(t *testing.T) {
+	wantRequeue := false


Let's move these right above where they're used.

Ditto below. Keeping the declaration right next to the usage helps readability.

All these are parameters to the assertion. I've embedded them below.

enocom · 2022-10-07T16:22:53Z

controllers/authproxyworkload_controller_test.go

+	os.Exit(result)
+}
+
+func TestReconcileState11(t *testing.T) {


Many of the tests below look very similar. Could we refactor them into table based tests where we consolidate only what's different into test cases?

test/integration/integration_test.go

enocom · 2022-10-07T16:24:18Z

test/integration/integration_test.go

@@ -194,5 +202,6 @@ func TestModifiesExistingDeployment(t *testing.T) {
 		ConnectionString: "region:project:inst",
 		ProxyImageURL:    "proxy-image:latest",
 	}
-	helpers.TestModifiesExistingDeployment(tctx, true)
+	testRemove := helpers.TestModifiesExistingDeployment(tctx)
+	testRemove()


Should this be deferred?

No, its actually a part of the testcase.

So testRemove should only run if the test before it didn't fail?

The TestModifiesExistingDeployment tescase does these steps:

create a Deployment

create an AuthProxyWorkload

check that the Deployment was updated to have the proxy sidecar container
(and optionally)

delete the AuthProxyWorkload

check that the Deployment is updated to not have the proxy sidecar container

I want to do steps 4 and 5 in the integration test, but skip tests 4 and 5 in the E2E test because after the test is over, I usually go inspect the state of the k8s cluster to see if the proxy sidecars are working right.

Originally the test helper was helpers.TestModifiesExistingDeployment(tctx, true) which would run the deployment, but you suggested that it should return a "cleanup function" instead. However, it's not really a cleanup function, It is extra steps in the test case.

I'm not totally convinced this was the right thing to do. Now that it returns a function that will do the last 2 steps of the test, it's more confusing.

I see that this helper is only used in one place. Maybe we shouldn't have a helper until we use it in a few places? Perhaps that's the problem -- we're not ready to make a shareable test helper function?

hessjcg

I fixed the comments.

controllers/authproxyworkload_controller_test.go

hessjcg · 2022-10-07T19:07:17Z

controllers/authproxyworkload_controller_test.go

+}
+
+func TestReconcileDeleted(t *testing.T) {
+	wantRequeue := false


I've inlined all of these test case parameters in this and other methods.

hessjcg · 2022-10-07T19:07:44Z

controllers/authproxyworkload_controller_test.go

+}
+
+func TestReconcileState21ByName(t *testing.T) {
+	wantRequeue := false


All these are parameters to the assertion. I've embedded them below.

hessjcg · 2022-10-07T19:09:21Z

controllers/authproxyworkload_controller.go

+func (c *recentlyDeletedCache) get(k types.NamespacedName) bool {
+	c.lock.RLock()
+	defer c.lock.RUnlock()
+	if c.values == nil {


hessjcg · 2022-10-07T19:18:15Z

controllers/authproxyworkload_controller.go

 // AuthProxyWorkloadReconciler reconciles a AuthProxyWorkload object
 type AuthProxyWorkloadReconciler struct {
 	client.Client
-	Scheme *runtime.Scheme
+	Scheme          *runtime.Scheme
+	recentlyDeleted recentlyDeletedCache


hessjcg · 2022-10-07T19:21:45Z

controllers/authproxyworkload_controller.go

+func replaceCondition(conds []*metav1.Condition, newC *metav1.Condition) []*metav1.Condition {
+	for i := range conds {
+		c := conds[i]
+		if c.Type == newC.Type {


hessjcg added 2 commits October 3, 2022 20:13

feat: add controller logic, webhooks, and integration tests.

0ec573d

doc: improve documentation on event reconcile functions.

b1a71db

hessjcg requested a review from a team October 4, 2022 02:44

blunderbuss-gcf bot assigned enocom Oct 4, 2022

hessjcg assigned shubha-rajan Oct 4, 2022

chore: remove sample yaml, it doesn't belong in this PR.

99735b1

enocom requested changes Oct 4, 2022

View reviewed changes

enocom requested a review from a team October 4, 2022 22:08

hessjcg added 4 commits October 4, 2022 17:30

chore: consolidate all the switches by Kind name and reduce code rela…

6e3f0db

…ted to getting workloads.

chore: add godoc to new exported functions.

f5029d7

chore: update code review comments and add new controller unit tests.

9ca02e0

chore: fix tests related to code review changes.

e732aeb

hessjcg commented Oct 5, 2022

View reviewed changes

hessjcg requested review from enocom and shubha-rajan October 6, 2022 00:50

hessjcg assigned hessjcg and unassigned enocom and shubha-rajan Oct 6, 2022

chore: one more tiny change for the review comments.

ec583a4

hessjcg commented Oct 6, 2022

View reviewed changes

controllers/authproxyworkload_controller.go Outdated Show resolved Hide resolved

enocom requested changes Oct 7, 2022

View reviewed changes

hessjcg commented Oct 7, 2022

View reviewed changes

chore: more code comments.

6abf816

enocom approved these changes Oct 7, 2022

View reviewed changes

hessjcg merged commit 0be2c0d into main Oct 7, 2022

hessjcg deleted the controller-logic branch October 7, 2022 19:51

release-please bot mentioned this pull request Oct 7, 2022

chore(main): release 1.0.0 #22

Closed

hessjcg mentioned this pull request Oct 10, 2022

feat: Finish implementing operator reconcile loop #37

Closed

This was referenced Oct 12, 2022

chore(main): release 0.0.1 #71

Merged

chore(main): release 0.0.1 #73

Closed

release-please bot mentioned this pull request Oct 18, 2022

chore(main): release 0.0.1 #85

Closed

This was referenced Dec 7, 2022

chore(main): release 0.0.2 #131

Closed

chore(main): release 0.0.3 #134

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add reconcile controller logic issue #37 #39

feat: Add reconcile controller logic issue #37 #39

hessjcg commented Oct 4, 2022

hessjcg left a comment

hessjcg left a comment

enocom Oct 7, 2022

hessjcg Oct 7, 2022

enocom Oct 7, 2022

enocom Oct 7, 2022

hessjcg Oct 7, 2022

enocom Oct 7, 2022 •

edited

Loading

hessjcg Oct 7, 2022

enocom Oct 7, 2022

enocom Oct 7, 2022

hessjcg Oct 7, 2022

enocom Oct 7, 2022

hessjcg Oct 7, 2022

enocom Oct 7, 2022

enocom Oct 7, 2022

hessjcg Oct 7, 2022

enocom Oct 7, 2022

enocom Oct 7, 2022

hessjcg Oct 7, 2022

enocom Oct 7, 2022

hessjcg Oct 7, 2022

enocom Oct 7, 2022

hessjcg left a comment

hessjcg Oct 7, 2022

hessjcg Oct 7, 2022

hessjcg Oct 7, 2022

hessjcg Oct 7, 2022

hessjcg Oct 7, 2022

feat: Add reconcile controller logic issue #37 #39

feat: Add reconcile controller logic issue #37 #39

Conversation

hessjcg commented Oct 4, 2022

hessjcg left a comment

Choose a reason for hiding this comment

hessjcg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enocom Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hessjcg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enocom Oct 7, 2022 •

edited

Loading