Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Policy: Transition Policy FQN indexing to a transaction rather than an unmonitored sideeffect #1660

Open
jrschumacher opened this issue Oct 16, 2024 · 1 comment · May be fixed by #1782
Open
Assignees

Comments

@jrschumacher
Copy link
Member

When the policy service was developed, we made the choice to have the upsert FQN function to act as a side-effect so that we could continue moving quickly. This was not a concern since the fqn reindex was relatively liteweight and could be run at any time.

// This logic is a bit complex. What we are trying to achieve is to upsert the fqn based on the
// combination of namespaceId, attributeId, and valueId. However, instead of requiring all three
// we want to support partial attribute FQNs. This means that we need to support the following
// combinations:
// 1. namespaceId
// 2. namespaceId, attributeId
// 3. namespaceId, attributeId, valueId
//
// This is a side effect -- errors will be swallowed and the fqn will be returned as an empty string
func (c *PolicyDBClient) upsertAttrFqn(ctx context.Context, opts attrFqnUpsertOptions) string {

Since we are introducing transactions with SQLC we should shift this functionality to a transaction to ensure consistency and monitoring.

Acceptance Criteria

  • transition upsertFQN function to utilize a transaction with any attribute changes
    • new namespace
    • new attribute
    • new value
    • unsafe namespace
    • unsafe attribute
    • unsafe value
@ryanulit
Copy link
Contributor

The code has changed slightly since this ticket was created. The upsertAttrFqn was removed in PR #1679 with added batching support and usage of the SQLC queries directly. An AttrFqnReindex method remains, but it is only used by a CLI method and the integration testing framework. This PR will add transaction support to the gRPC service method calls using the SQLC upsert... methods directly, as well as the CLI command using AttrFqnReindex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants