From a03df379a724cab1d7c8c0e8eb29d66c372ad531 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jacob=20L=C3=A4rfors?=
 <1135394+jlarfors@users.noreply.github.com>
Date: Tue, 16 Apr 2024 11:39:24 +0300
Subject: [PATCH] rename hz-internal api group to core

cleanup account reconciler based on server-side apply no-diff-no-op logic
---
 pkg/auth/rbac_test.go                        |  8 ++--
 pkg/auth/role.go                             |  2 +-
 pkg/auth/rolebinding.go                      |  2 +-
 pkg/extensions/accounts/account.go           | 47 ++++++++++----------
 pkg/extensions/accounts/member.go            |  2 +-
 pkg/extensions/accounts/user.go              |  2 +-
 pkg/extensions/accounts/user_test.go         |  1 -
 pkg/extensions/serviceaccounts/controller.go |  4 +-
 pkg/hz/portal.go                             |  2 +-
 pkg/hz/reconciler.go                         |  2 +-
 pkg/hz/testdata/account-test.yaml            |  2 +-
 pkg/server/server.go                         |  5 ---
 12 files changed, 37 insertions(+), 42 deletions(-)

diff --git a/pkg/auth/rbac_test.go b/pkg/auth/rbac_test.go
index d5c009f..a838333 100644
--- a/pkg/auth/rbac_test.go
+++ b/pkg/auth/rbac_test.go
@@ -53,7 +53,7 @@ func TestRBAC(t *testing.T) {
 				},
 				Spec: RoleBindingSpec{
 					RoleRef: RoleRef{
-						Group: "hz-internal",
+						Group: "core",
 						Kind:  "Role",
 						Name:  "role-creator",
 					},
@@ -176,7 +176,7 @@ func TestRBAC(t *testing.T) {
 				},
 				Spec: RoleBindingSpec{
 					RoleRef: RoleRef{
-						Group: "hz-internal",
+						Group: "core",
 						Kind:  "Role",
 						Name:  "role-runner",
 					},
@@ -247,7 +247,7 @@ func TestRBAC(t *testing.T) {
 				},
 				Spec: RoleBindingSpec{
 					RoleRef: RoleRef{
-						Group: "hz-internal",
+						Group: "core",
 						Kind:  "Role",
 						Name:  "role-allow-all",
 					},
@@ -266,7 +266,7 @@ func TestRBAC(t *testing.T) {
 				},
 				Spec: RoleBindingSpec{
 					RoleRef: RoleRef{
-						Group: "hz-internal",
+						Group: "core",
 						Kind:  "Role",
 						Name:  "role-deny-delete",
 					},
diff --git a/pkg/auth/role.go b/pkg/auth/role.go
index 827eb5b..69ff8d7 100644
--- a/pkg/auth/role.go
+++ b/pkg/auth/role.go
@@ -15,7 +15,7 @@ func (Role) ObjectVersion() string {
 }
 
 func (Role) ObjectGroup() string {
-	return "hz-internal"
+	return "core"
 }
 
 func (Role) ObjectKind() string {
diff --git a/pkg/auth/rolebinding.go b/pkg/auth/rolebinding.go
index 179b81d..f3e0947 100644
--- a/pkg/auth/rolebinding.go
+++ b/pkg/auth/rolebinding.go
@@ -15,7 +15,7 @@ func (RoleBinding) ObjectVersion() string {
 }
 
 func (RoleBinding) ObjectGroup() string {
-	return "hz-internal"
+	return "core"
 }
 
 func (RoleBinding) ObjectKind() string {
diff --git a/pkg/extensions/accounts/account.go b/pkg/extensions/accounts/account.go
index 2781a24..fd30f81 100644
--- a/pkg/extensions/accounts/account.go
+++ b/pkg/extensions/accounts/account.go
@@ -14,8 +14,11 @@ import (
 )
 
 const (
+	fieldManager = "ctrl-accounts"
+	Finalizer    = "core/account"
+
 	ObjectKind    = "Account"
-	ObjectGroup   = "hz-internal"
+	ObjectGroup   = "core"
 	ObjectVersion = "v1"
 )
 
@@ -38,6 +41,11 @@ func (a Account) ObjectKind() string {
 	return ObjectKind
 }
 
+// Override ObjectAccount because accounts can only exist in the root account.
+func (a Account) ObjectAccount() string {
+	return hz.RootAccount
+}
+
 type AccountSpec struct{}
 
 type AccountStatus struct {
@@ -59,7 +67,6 @@ type AccountStatus struct {
 var _ (hz.Reconciler) = (*AccountReconciler)(nil)
 
 type AccountReconciler struct {
-	hz.Client
 	Conn *nats.Conn
 
 	OpKeyPair         nkeys.KeyPair
@@ -71,7 +78,12 @@ func (r *AccountReconciler) Reconcile(
 	ctx context.Context,
 	req hz.Request,
 ) (hz.Result, error) {
-	accClient := hz.ObjectClient[Account]{Client: r.Client}
+	client := hz.NewClient(
+		r.Conn,
+		hz.WithClientInternal(true),
+		hz.WithClientManager(fieldManager),
+	)
+	accClient := hz.ObjectClient[Account]{Client: client}
 	account, err := accClient.Get(
 		ctx,
 		hz.WithGetKey(req.Key),
@@ -80,7 +92,7 @@ func (r *AccountReconciler) Reconcile(
 		return hz.Result{}, hz.IgnoreNotFound(err)
 	}
 
-	accountApply, err := hz.ExtractManagedFields(account, "ctrl-accounts")
+	accountApply, err := hz.ExtractManagedFields(account, fieldManager)
 	if err != nil {
 		return hz.Result{}, fmt.Errorf("extracting managed fields: %w", err)
 	}
@@ -103,8 +115,9 @@ func (r *AccountReconciler) Reconcile(
 		}
 		return hz.Result{}, nil
 	}
-
-	ready := true
+	// If status is non-nil, check if the account exists.
+	// If it exists, make sure it matches what it should.
+	// If it doesn't exist, re-create it.
 	existingClaims, err := jwt.DecodeAccountClaims(account.Status.JWT)
 	if err != nil {
 		return hz.Result{}, fmt.Errorf("decoding account claims: %w", err)
@@ -118,33 +131,21 @@ func (r *AccountReconciler) Reconcile(
 		if _, err := AccountClaimsUpdate(ctx, r.Conn, r.OpKeyPair, account.Status.JWT); err != nil {
 			return hz.Result{}, fmt.Errorf("updating account: %w", err)
 		}
-		ready = false
 	}
 
-	if ready && !cmp.Equal(claims, existingClaims) {
+	if !cmp.Equal(claims, existingClaims) {
 		if _, err := AccountClaimsUpdate(ctx, r.Conn, r.OpKeyPair, account.Status.JWT); err != nil {
-			return hz.Result{}, fmt.Errorf("updating account: %w", err)
-		}
-		ready = false
-	}
-
-	if !ready {
-		if account.Status.Ready {
 			accountApply.Status.Ready = false
 			if _, err := accClient.Apply(ctx, accountApply); err != nil {
 				return hz.Result{}, fmt.Errorf("updating account: %w", err)
 			}
-			return hz.Result{}, nil
-		}
-		return hz.Result{}, nil
-	}
-
-	if !account.Status.Ready {
-		accountApply.Status.Ready = true
-		if _, err := accClient.Apply(ctx, accountApply); err != nil {
 			return hz.Result{}, fmt.Errorf("updating account: %w", err)
 		}
 	}
+	accountApply.Status.Ready = true
+	if _, err := accClient.Apply(ctx, accountApply); err != nil {
+		return hz.Result{}, fmt.Errorf("updating account: %w", err)
+	}
 
 	return hz.Result{}, nil
 }
diff --git a/pkg/extensions/accounts/member.go b/pkg/extensions/accounts/member.go
index f098a2e..37f82dc 100644
--- a/pkg/extensions/accounts/member.go
+++ b/pkg/extensions/accounts/member.go
@@ -15,7 +15,7 @@ func (Member) ObjectVersion() string {
 }
 
 func (Member) ObjectGroup() string {
-	return "hz-internal"
+	return "core"
 }
 
 func (Member) ObjectKind() string {
diff --git a/pkg/extensions/accounts/user.go b/pkg/extensions/accounts/user.go
index 2b28fe8..912c5e2 100644
--- a/pkg/extensions/accounts/user.go
+++ b/pkg/extensions/accounts/user.go
@@ -25,7 +25,7 @@ func (u User) ObjectVersion() string {
 }
 
 func (u User) ObjectGroup() string {
-	return "hz-internal"
+	return "core"
 }
 
 func (u User) ObjectKind() string {
diff --git a/pkg/extensions/accounts/user_test.go b/pkg/extensions/accounts/user_test.go
index bdda409..03be196 100644
--- a/pkg/extensions/accounts/user_test.go
+++ b/pkg/extensions/accounts/user_test.go
@@ -31,7 +31,6 @@ func TestUser(t *testing.T) {
 	// In order to publish a user, the account the user references
 	// must exist in the NATS KV store.
 	recon := accounts.AccountReconciler{
-		Client:            client,
 		Conn:              ti.Conn,
 		OpKeyPair:         ti.NS.Auth.Operator.SigningKey.KeyPair,
 		RootAccountPubKey: ti.NS.Auth.RootAccount.PublicKey,
diff --git a/pkg/extensions/serviceaccounts/controller.go b/pkg/extensions/serviceaccounts/controller.go
index 3fb6264..ac3dfd0 100644
--- a/pkg/extensions/serviceaccounts/controller.go
+++ b/pkg/extensions/serviceaccounts/controller.go
@@ -114,8 +114,8 @@ func (r Reconciler) generateNATSCredentials(
 ) (string, error) {
 	accClient := hz.ObjectClient[accounts.Account]{Client: r.Client}
 	account, err := accClient.Get(ctx, hz.WithGetKey(hz.ObjectKey{
-		Name:    hz.RootAccount,
-		Account: sa.Account,
+		Account: hz.RootAccount,
+		Name:    sa.Account,
 	}))
 	if err != nil {
 		return "", fmt.Errorf("getting horizon account: %w", err)
diff --git a/pkg/hz/portal.go b/pkg/hz/portal.go
index 1606a35..1910814 100644
--- a/pkg/hz/portal.go
+++ b/pkg/hz/portal.go
@@ -31,7 +31,7 @@ func (e Portal) ObjectVersion() string {
 }
 
 func (e Portal) ObjectGroup() string {
-	return "hz-internal"
+	return "core"
 }
 
 func (e Portal) ObjectKind() string {
diff --git a/pkg/hz/reconciler.go b/pkg/hz/reconciler.go
index a600416..fd86bf7 100644
--- a/pkg/hz/reconciler.go
+++ b/pkg/hz/reconciler.go
@@ -5,7 +5,7 @@ import (
 	"time"
 )
 
-const RootAccount = "hz-root"
+const RootAccount = "root"
 
 type Reconciler interface {
 	Reconcile(context.Context, Request) (Result, error)
diff --git a/pkg/hz/testdata/account-test.yaml b/pkg/hz/testdata/account-test.yaml
index d371d38..9a465ae 100644
--- a/pkg/hz/testdata/account-test.yaml
+++ b/pkg/hz/testdata/account-test.yaml
@@ -1,4 +1,4 @@
-apiVersion: hz-internal/v1 # "core/v1"
+apiVersion: core/v1
 kind: Account
 metadata:
   name: test-account
diff --git a/pkg/server/server.go b/pkg/server/server.go
index f414c1d..0a4c1fe 100644
--- a/pkg/server/server.go
+++ b/pkg/server/server.go
@@ -266,11 +266,6 @@ func (s *Server) Start(ctx context.Context, opts ...ServerOption) error {
 	}
 	if opt.runAccountsController {
 		recon := accounts.AccountReconciler{
-			Client: hz.NewClient(
-				s.Conn,
-				hz.WithClientInternal(true),
-				hz.WithClientManager("ctlr-accounts"),
-			),
 			Conn:              s.Conn,
 			OpKeyPair:         s.NS.Auth.Operator.SigningKey.KeyPair,
 			RootAccountPubKey: s.NS.Auth.RootAccount.PublicKey,