Add bundle lifecycle status tracking (build, push) by ashutosh-narkar · Pull Request #315 · open-policy-agent/opa-control-plane

ashutosh-narkar · 2026-04-01T15:35:58Z

This change adds support for tracking the status of a bundle
through the build and push phases. The sync phase
is not tracked as we need a bundle revision to keep
track of a tenant+bundle+revision combo.

srenatus

Couple of early comments! 👍

srenatus · 2026-04-02T10:18:36Z

pkg/service/service_test.go

+		return svc.Run(ctx)
+	})
+
+	time.Sleep(time.Millisecond * 500)


Can you please use testing/synctest? It should let us run the same test without an actual sleep. (Same as other test.)

I'd appreciate some help. I'm getting the error panic: deadlock: main bubble goroutine has exited but blocked goroutines remain [recovered, repanicked] when I use synctest.Test and synctest.Wait. I'm probably doing something wrong here.

Ah, let's leave it as-is then. We can clean this up later.

srenatus · 2026-04-02T10:19:18Z

pkg/service/service_test.go

+		WithLogger(log)
+
+	var g errgroup.Group
+	ctx, cancel := context.WithCancel(context.Background())


[nit] Let's be consistent wth ctx := t.Context(). Also we can then use this below with GetLatestBundleStatus.

srenatus · 2026-04-02T10:20:39Z

pkg/service/worker.go

+	return worker
+}
+
+func (worker *BundleWorker) WithPrincipal(principal string) *BundleWorker {


Why does the bundle worker need a principal..? 🤔 I think it would make sense if UpsertBundleStatus didn't require authz. It's not exposed through any http handler.

Makes sense. Removed authz for UpsertBundleStatus

srenatus · 2026-04-02T10:24:17Z

internal/database/database.go

+// For sqlite, postgres, and cockroach, it behaves identically to upsertReturning with returning=true.
+func (d *Database) upsertReturningLastInsertID(ctx context.Context, tx *sql.Tx, tenant, table string, columns []string, primaryKey []string, values ...any) (int64, error) {
+	valueArgs := d.args(len(columns))
+	if tenant != "" { // not a relation-only table


This is taken from the generic upsert helper, I think? We don't need to deal with it like that here, I suppose.

removed upsertReturningLastInsertID and rolled this into upsertReturning

srenatus · 2026-04-02T10:25:50Z

internal/database/database.go

+// For MySQL, it adds `id = LAST_INSERT_ID(id)` to the ON DUPLICATE KEY UPDATE clause so that
+// LastInsertId() returns the correct ID for both inserts and updates.


Would it perhaps make sense to adjust upsertReturning? Sounds like this twist would be applicable across the tables.

removed upsertReturningLastInsertID and rolled this into upsertReturning

srenatus

Had a quick look, it looks good, some nitpicks only!

srenatus · 2026-04-02T19:57:16Z

internal/authz/authz_test.rego

 	"secrets.view",
 	"tokens.view",
 	"sources.data.read",
+	"bundles_statuses.view",


Given that we have sources.data.read, wouldn't bundles.status.view fit in slightly better?

Updated to bundles.status.view

srenatus · 2026-04-02T19:59:37Z

internal/database/database.go

 }

-func (d *Database) upsertReturning(ctx context.Context, returning bool, tx *sql.Tx, tenant, table string, columns []string, primaryKey []string, values ...any) (int, error) {
+func (d *Database) upsertReturning(ctx context.Context, returning, returningLastInsertID bool, tx *sql.Tx, tenant, table string, columns []string, primaryKey []string, values ...any) (int, error) {


Hm! I'm surprised there's a need to differentiate "returning" and "returningLastInsertID". I took the former to mean the same when it was introduced. When do we only want one of them?

IIUC the special handling is only needed for MySQL. In our case, we want the ID for both inserts and updates. Since the other databases support the RETURNING clause, they return the correct ID. MySQL will return 0 or something for the update case. Hence this is a workaround to get the correct ID on update.

srenatus · 2026-04-02T20:00:49Z

pkg/builder/builder.go

 	return b
 }

+func (b *Builder) Revision() string {


Is this always safe to call? I think b.revision might only get populated at bundle build time...? 🤔

We're only tracking build and upload state as we need the revision to be non-empty.

We can't track sync state due to this.

srenatus · 2026-04-02T20:02:20Z

internal/server/server_test.go

+				assert.Empty(t, resp.Result)
+			})
+
+			t.Run("unauthorized", func(t *testing.T) {


Should we check some cross-tenant request, perhaps?

Added a test case for this.

johanfylling

LGTM 👍

johanfylling · 2026-04-07T08:56:24Z

internal/server/server.go

 	setup("PUT", "/v1/bundles/{bundle}", s.v1BundlesPut)
 	setup("DELETE", "/v1/bundles/{bundle}", s.v1BundlesDelete)
+	setup("GET", "/v1/bundles/{bundle}/status/latest", s.v1BundleStatusLatestGet)
+	setup("GET", "/v1/bundles/{bundle}/status", s.v1BundleStatusList)


Will there be a follow up docs update for these new endpoints, or is that still too early?

Once we merge this, I will create a PR to update the docs.

johanfylling · 2026-04-07T09:05:49Z

internal/server/server_test.go

+			}
+
+			const ownerKey = "test-owner-key"
+			if err := db.UpsertToken(ctx, "internal", "default", &config.Token{


[nit] to have less "magic" strings to track down

Suggested change

if err := db.UpsertToken(ctx, "internal", "default", &config.Token{

if err := db.UpsertToken(ctx, principal.Id, principal.Tenant, &config.Token{

Same in other places.

johanfylling · 2026-04-07T09:11:10Z

internal/server/server_test.go

+
+			if _, err := db.UpsertBundleStatus(ctx, "default", "bundle1", "rev1", "build", "in_progress", ""); err != nil {
+				t.Fatal(err)
+			}


Are there missing assertions following this status update, or this is here to show that we only get the immediately following status update when querying? Maybe there is value in asserting both statuses(?)

We are testing that we get the last inserted status for a tenant+bundle combo. So we expect to get only one status which we assert later in the test.

johanfylling · 2026-04-07T09:22:25Z

internal/server/server.go

 	setup("PUT", "/v1/bundles/{bundle}", s.v1BundlesPut)
 	setup("DELETE", "/v1/bundles/{bundle}", s.v1BundlesDelete)
+	setup("GET", "/v1/bundles/{bundle}/status/latest", s.v1BundleStatusLatestGet)
+	setup("GET", "/v1/bundles/{bundle}/status", s.v1BundleStatusList)


Given that we're returning a list of statuses I suppose this could also be .../statuses if we want to be sticklers for naming consistency; but I'm guessing we're seeing this as a general "status" endpoint for the bundle, and the actual return type is irrelevant(?)
(also, status kinda reads better 😄)

also, status kinda reads better

👍

johanfylling · 2026-04-07T09:27:43Z

internal/server/server.go

+		return
+	}
+
+	resp := types.BundleStatusGetResponseV1{Result: status}


If there are no status records, we're gonna return an empty dictionary ({})? Maybe we should add a test for this to not have future regressions on this behavior.

Scratch that, I was misled by the func doc 😄. But we should still assert this behavior in testing.

This behavior is consistent with v1BundlesGet. We have a test for this in internal/database/bundle_status_test.go (see TestGetLatestBundleStatus)

johanfylling · 2026-04-07T10:38:09Z

internal/database/bundle_status.go

+		if err == nil {
+			_, err = tx.ExecContext(ctx,
+				fmt.Sprintf(
+					`DELETE FROM bundles_statuses WHERE bundle_id = %s AND id < %s`,


I forget, are bundle ID:s unique server-wide, across tenants? If not, might we be deleting records for another tenant with here?

I think so. This is the same pattern used for deleting other resources.

johanfylling · 2026-04-07T10:43:08Z

pkg/service/worker.go

+		if b.Revision() != "" {
+			_, err := w.database.UpsertBundleStatus(ctx, w.tenant, w.bundleConfig.Name, b.Revision(), BuildPhaseBuild.String(), BuildStateBuildFailed.String(), err.Error())
+			if err != nil {
+				w.log.Warnf("failed to track bundle build state %q: %v", w.bundleConfig.Name, err)


Ok, so we don't abort on status error 👍.

johanfylling · 2026-04-07T10:48:03Z

pkg/service/worker.go

 			w.log.Warnf("failed to upload bundle %q: %v", w.bundleConfig.Name, err)
+
+			if b.Revision() != "" {
+				_, err := w.database.UpsertBundleStatus(ctx, w.tenant, w.bundleConfig.Name, b.Revision(), BuildPhasePush.String(), BuildStatePushFailed.String(), err.Error())


We only need to report on the ultimate success/failure of each phase, not that they're in progress?

Only success/failure atm

johanfylling · 2026-04-07T10:50:04Z

pkg/service/worker.go

+		}
+	}
+
 	if w.storage != nil {


It's up to the client to know that storage has been configured, and therefore when to expect a PUSH phase?
Would there be value in tracking each phase separately, or that would just be unwarranted complexity?

It's up to the client to know that storage has been configured, and therefore when to expect a PUSH phase?

Yes. So clients can plugin their own storage implementation.

Would there be value in tracking each phase separately, or that would just be unwarranted complexity?

We currently track build and push phases. One phase for a tenant+bundle+revision combo.

johanfylling · 2026-04-07T10:57:25Z

pkg/service/service_test.go

+		t.Fatalf("expected bundle status %v but got %v", service.BuildStatePushFailed.String(), status.Status)
+	}
+}
+


Should we also add a test case for when there is no storage, and we only expect BUILD-phase status?

The service will create a storage if the user does not provide one.

This change adds support for tracking the status of a bundle through the build and push phases. The sync phase is not tracked as we need a bundle revision to keep track of a tenant+bundle+revision combo. Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

ashutosh-narkar requested a review from srenatus April 2, 2026 00:33

srenatus reviewed Apr 2, 2026

View reviewed changes

ashutosh-narkar force-pushed the add-bundle-state branch from ac4bc49 to 9681af7 Compare April 2, 2026 17:48

ashutosh-narkar requested a review from johanfylling April 2, 2026 19:36

ashutosh-narkar marked this pull request as ready for review April 2, 2026 19:42

srenatus reviewed Apr 2, 2026

View reviewed changes

ashutosh-narkar changed the title ~~Add bundle lifecycle status tracking (sync, build, push)~~ Add bundle lifecycle status tracking (build, push) Apr 2, 2026

johanfylling approved these changes Apr 7, 2026

View reviewed changes

ashutosh-narkar force-pushed the add-bundle-state branch from 64420c7 to 39bb353 Compare April 7, 2026 22:52

ashutosh-narkar merged commit 7e550cf into open-policy-agent:main Apr 7, 2026
9 checks passed

ashutosh-narkar mentioned this pull request Apr 8, 2026

docs: Add spec for OCP bundle status tracking API open-policy-agent/opa#8502

Merged

		// For MySQL, it adds `id = LAST_INSERT_ID(id)` to the ON DUPLICATE KEY UPDATE clause so that
		// LastInsertId() returns the correct ID for both inserts and updates.

	if err := db.UpsertToken(ctx, "internal", "default", &config.Token{
	if err := db.UpsertToken(ctx, principal.Id, principal.Tenant, &config.Token{

Conversation

ashutosh-narkar commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srenatus 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!

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!

srenatus 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!

Choose a reason for hiding this comment

Uh oh!

johanfylling 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!

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

ashutosh-narkar commented Apr 1, 2026 •

edited

Loading