feat: implemented announce key reconciliation by NamanBalaji · Pull Request #143 · openkcm/krypton

NamanBalaji · 2026-05-26T06:17:12Z

No description provided.

fabenan-f

I like the end-to-end-approach, just a few remarks from my side

fabenan-f · 2026-05-26T16:14:30Z

+func setupOperator(ctx context.Context) (*sql.DB, *rpc.Server, *orbital.Operator) {
+	dsn := os.Getenv("AGENT_DATABASE_URL")
+	if dsn == "" {
+		log.Println("AGENT_DATABASE_URL not set, operator disabled")


I know this is only an example agent, but still I'd not allow an agent to run without an operator

fabenan-f · 2026-05-26T16:14:57Z

+	}
+}
+
+// awaitKeyExists polls the keys table until a key with the given ID and tenant exists.


We can make the tests even more integrational if we use the gRPC client for key assertions

fabenan-f · 2026-05-26T16:15:17Z

@@ -0,0 +1,42 @@
+package handler


Imho having handler.NewAnnounceKey and announcekey.NewHandler is confusing. I think we should consolidate these into a single handler package that contains both the job handler (manager side) and the request handler (operator side). This would be beneficial because:

Both handlers share common data structures

Their interconnection would be immediately visible

It would eliminate the current naming ambiguity

fabenan-f · 2026-05-26T16:15:38Z

+		key.State = model.KeyStatePreActivation
+
+		if _, err := keyStore.CreateKey(ctx, store.CreateKeyQuery{Key: key}); err != nil {
+			resp.Fail(fmt.Sprintf("store key: %v", err))


The default case should be to continue, only known terminal errors (like the one in the test case) should fail

fabenan-f · 2026-05-26T16:16:40Z

+			model.KeyStateDestroyed:      {},
+			model.KeyStateActive:         {},
+			model.KeyStateCompromised:    {},
+			model.KeyStateAnnounceFailed: {},


If we introduce a new state here, we'll deviate from the official NIST lifecycle definition. I'm uncertain whether this is advisable from either a compliance or signaling standpoint

Correct we should have a different processing state
As per some discussion we should have a field a new field called KeyProcessingState and existing KeyState should be renamed to KeyLifeCycleState

type Key struct { ID string `json:"id"` Name string `json:"name"` TenantID string `json:"tenant_id"` Kind KeyKind `json:"kind"` ParentID *string `json:"parent_id"` ManagedBy string `json:"managed_by"` Labels Labels `json:"labels"` KeyLifeCycleState KeyLifeCycleState `json:"key_lifecycle_state"` KeyProcessingState KeyProcessingState `json:"key_processing_state"` CreatedAt clock.UnixNano `json:"created_at"` UpdatedAt clock.UnixNano `json:"updated_at"` } type KeyProcessingState struct { Status string `json:"status"` JobID string `json:"job_id,omitempty"` }

Here jobID shows which JobID is having the lock on , and this will be useful when we do a large key rotation.
but maybe we can start without and extend it later

Credit to @apatsap as well

fabenan-f · 2026-05-26T16:16:56Z

+)
+
+type fakeKeyStore struct {
+	keys          map[string]*model.Key


I'd rather use the real sql store implementation (unhappy paths can still be mocked with wrapper functions)

fabenan-f · 2026-05-26T16:17:10Z

+		)
+	}
+
+	job := orbital.NewJob(announcekey.JobType, data).WithExternalID(key.ID)


Since key.ID is generated internally, a lost response could cause the client to retry, resulting in duplicate jobs performing the same action

jithinkunjachan

Nice job 👍🏽 , just few things

jithinkunjachan · 2026-05-28T09:47:18Z

+			parentID = &data.ParentID
+		}
+
+		key := model.NewKey(data.TenantID, data.Name, data.Kind, parentID, data.Target, data.Labels)


~~Not for this PR, but just a question how the tenants info will be propagated to agent DB as we have DB constraints. We might need to think about this topic later.~~

jithinkunjachan · 2026-05-28T10:40:44Z

+			model.KeyStateDestroyed:      {},
+			model.KeyStateActive:         {},
+			model.KeyStateCompromised:    {},
+			model.KeyStateAnnounceFailed: {},


Correct we should have a different processing state
As per some discussion we should have a field a new field called KeyProcessingState and existing KeyState should be renamed to KeyLifeCycleState

type Key struct { ID string `json:"id"` Name string `json:"name"` TenantID string `json:"tenant_id"` Kind KeyKind `json:"kind"` ParentID *string `json:"parent_id"` ManagedBy string `json:"managed_by"` Labels Labels `json:"labels"` KeyLifeCycleState KeyLifeCycleState `json:"key_lifecycle_state"` KeyProcessingState KeyProcessingState `json:"key_processing_state"` CreatedAt clock.UnixNano `json:"created_at"` UpdatedAt clock.UnixNano `json:"updated_at"` } type KeyProcessingState struct { Status string `json:"status"` JobID string `json:"job_id,omitempty"` }

Here jobID shows which JobID is having the lock on , and this will be useful when we do a large key rotation.
but maybe we can start without and extend it later

Credit to @apatsap as well

Signed-off-by: Naman Balaji <namanb487@gmail.com>

apatsap · 2026-06-02T10:06:12Z

+	defer agentDB.Close()
+
+	go func() {
+		if err := operator.ListenAndRespond(ctx); err != nil {


can we add a log.Info before ListenAndRespond

apatsap · 2026-06-02T10:09:42Z

can we put this in /examples/

apatsap · 2026-06-02T10:10:19Z

can we put this in /examples/

apatsap · 2026-06-02T10:14:21Z

+		return orbital.CancelJobConfirmer(fmt.Sprintf("invalid job data: %v", err)), nil
+	}
+
+	_, err := h.keyStore.GetKeyByID(ctx, data.KeyID, data.TenantID)


we need to check that the key state allows the target state using keylifecycle.ValidateTransition

imo keylifecycle.ValidateTransition is more for checking state transitions, but in this case we just create a key and for the whole announce operation it will stay in a preactive state so there's effectively no transition taking place here.

apatsap · 2026-06-02T10:16:40Z

+// TaskData is the payload exchanged between the root job handler and the
+// agent task handler. It is JSON-encoded into the orbital Job/Task data
+// field.
+type TaskData struct {


For now its alright. But lets add a comment here that when we add the next handler that certain parts of the data (e.g. Target, Labels, TenantID) might need to be refactored into something like common.TaskInfo

apatsap · 2026-06-02T11:08:34Z

+		key.ID = data.KeyID
+		key.LifeCycleState = model.KeyLifeCyclePreActivation
+
+		err := keyStore.CreateKey(ctx, key)


we need to make sure parentID key exists and can be used to from a lifecycle perspective to announce this key

Will be done as part of a subsequent PR

apatsap · 2026-06-02T11:32:12Z

+		if data.ParentID != "" {
+			parentID = &data.ParentID
+		}
+


we need to check that data.TenantID exists

Will be done as part of a subsequent PR

apatsap · 2026-06-02T11:36:21Z

@@ -37,16 +61,97 @@ func (s *KeyService) AnnounceKey(ctx context.Context, req *AnnounceKeyRequest) (
 		req.GetLabels(),
 	)



We need to check

tenantID exists

Parent exists within the same tenant.

If Parent doesn't exist we need to make sure key is root jkey

If Parent exists we need to check that parent:

is in an allowed key lifecycle state

that the keyKind of the new key can be attached to the parent key by the hierarchy definition

For now just allowing active state and new key being generated from it's direct parent.

apatsap · 2026-06-02T11:42:32Z

 	)

-	if err := s.keyStore.CreateKey(ctx, key); err != nil {
+	key, err := s.upsertKey(ctx, newKey)


for the double commit to work we should create the job first and then check in the jobConfirm func whether the key exists and can be activated

apatsap · 2026-06-02T11:43:20Z

+	}
+
+	// If a job is already linked, the caller is retrying — return as-is.
+	if key.KeyProcessingState.JobID != "" {


What if the previous job failed (i.e. it has a job id) and this call's intention is to retry the job

We look up the key by name and if we find a key in failed state we create a new job, we use oldJobID concatenated with the keyID as an externalID. This is effectively our retry job and by using oldJobId we kind of dedup it.

Signed-off-by: Naman Balaji <namanb487@gmail.com>

fabenan-f

The uniqueness constraint on key.Name can detect concurrent jobs when used as an external ID. This might let us remove some of the conditional logic we currently have in the double commit. But the current approach should work as well

fabenan-f · 2026-06-04T15:16:15Z

+	if err != nil {
+		if errors.Is(err, store.ErrKeyNotFound) {
+			// ConfirmJob is idempotent and orbital will eventually
+			// time out the job if the key never lands.


Jobs do not time out in the confirming phase but we can think of something in the future

fabenan-f · 2026-06-04T15:19:28Z

+		return nil, vErr
+	}
+
+	existing, lookupErr := s.keyStore.GetKeyByName(ctx, store.GetKeyByNameQuery{


We could be more optimistic and try to create a key first and handle a potential existing key error later, but nothing that needs to be done now

jithinkunjachan

Nicely done ✅

push-tags-from-workflow Bot added dependencies Pull requests that update a dependency file tests feature labels May 26, 2026

NamanBalaji force-pushed the feat/announce-key-agent branch from f46c40c to 6b83fc5 Compare May 26, 2026 06:19

NamanBalaji self-assigned this May 26, 2026

fabenan-f reviewed May 26, 2026

View reviewed changes

jithinkunjachan reviewed May 28, 2026

View reviewed changes

NamanBalaji force-pushed the feat/announce-key-agent branch from 6b83fc5 to 7ebc5cd Compare June 2, 2026 07:51

NamanBalaji added 2 commits June 2, 2026 09:53

feat: implemented announce key reconciliation

614177c

Signed-off-by: Naman Balaji <namanb487@gmail.com>

fix review comments

89d7a65

Signed-off-by: Naman Balaji <namanb487@gmail.com>

NamanBalaji force-pushed the feat/announce-key-agent branch from 7ebc5cd to 89d7a65 Compare June 2, 2026 07:54

fix key announce retries

ee5569d

Signed-off-by: Naman Balaji <namanb487@gmail.com>

NamanBalaji requested review from fabenan-f and jithinkunjachan June 2, 2026 08:27

apatsap requested changes Jun 2, 2026

View reviewed changes

addressed review comments

9d68fe0

Signed-off-by: Naman Balaji <namanb487@gmail.com>

NamanBalaji force-pushed the feat/announce-key-agent branch from 0917ea9 to 9d68fe0 Compare June 4, 2026 09:01

NamanBalaji requested a review from apatsap June 4, 2026 09:11

apatsap approved these changes Jun 4, 2026

View reviewed changes

fabenan-f approved these changes Jun 4, 2026

View reviewed changes

jithinkunjachan approved these changes Jun 5, 2026

View reviewed changes

NamanBalaji merged commit 45b4343 into main Jun 5, 2026
5 checks passed

NamanBalaji deleted the feat/announce-key-agent branch June 5, 2026 07:56

		@@ -37,16 +61,97 @@ func (s KeyService) AnnounceKey(ctx context.Context, req AnnounceKeyRequest) (
		req.GetLabels(),
		)

Conversation

NamanBalaji commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabenan-f left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jithinkunjachan left a comment

Choose a reason for hiding this comment

Uh oh!

jithinkunjachan May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

fabenan-f left a comment

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!

jithinkunjachan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

NamanBalaji commented May 26, 2026 •

edited

Loading

jithinkunjachan May 28, 2026 •

edited

Loading