From 2622be5450b4f79c834f49ad042c2d744e05ac2e Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Thu, 10 Oct 2024 10:12:53 +0300 Subject: [PATCH] Use single and unique entity ID to deal with entity info wrapper (#4704) * Use single and unique entity ID to deal with entity info wrapper This drops our reliance on a per-entity-type ID and instead just uses a single entity ID in the entity info wrapper. The intention is to simplify the code and make adding new entities simpler. Signed-off-by: Juan Antonio Osorio Fix entity ID serialized in wrong place and data race issue Signed-off-by: Juan Antonio Osorio Use local error variable for EEA test Signed-off-by: Juan Antonio Osorio Add more guardrails Signed-off-by: Juan Antonio Osorio fix more unit tests Signed-off-by: Juan Antonio Osorio remove unused function Signed-off-by: Juan Antonio Osorio fix tests Signed-off-by: Juan Antonio Osorio * Use entity ID instead of repo ID in message meta in gh handler test Signed-off-by: Juan Antonio Osorio --------- Signed-off-by: Juan Antonio Osorio --- database/mock/store.go | 2 +- .../controlplane/handlers_githubwebhooks.go | 2 +- .../handlers_githubwebhooks_test.go | 16 +- internal/db/store.go | 11 +- internal/eea/eea.go | 9 +- internal/eea/eea_test.go | 8 +- internal/engine/entities/entity_event.go | 329 +++++++----------- internal/engine/entities/entity_event_test.go | 211 +++++++---- internal/engine/eval_status.go | 41 +-- internal/engine/executor.go | 13 - internal/engine/executor_test.go | 2 +- internal/engine/handler.go | 1 + internal/engine/handler_test.go | 2 +- internal/engine/interfaces/interface.go | 14 +- internal/entities/handlers/handler_test.go | 8 +- .../strategies/message/entity_info_wrapper.go | 14 +- internal/entities/utils.go | 46 --- internal/logger/telemetry_store_watermill.go | 34 +- .../logger/telemetry_store_watermill_test.go | 2 +- 19 files changed, 302 insertions(+), 463 deletions(-) delete mode 100644 internal/entities/utils.go diff --git a/database/mock/store.go b/database/mock/store.go index 428a237235..7c18e82a03 100644 --- a/database/mock/store.go +++ b/database/mock/store.go @@ -1525,7 +1525,7 @@ func (mr *MockStoreMockRecorder) GetRepositoryByRepoName(arg0, arg1 any) *gomock } // GetRuleEvaluationByProfileIdAndRuleType mocks base method. -func (m *MockStore) GetRuleEvaluationByProfileIdAndRuleType(arg0 context.Context, arg1 uuid.UUID, arg2 sql.NullString, arg3 uuid.NullUUID, arg4 sql.NullString) (*db.ListRuleEvaluationsByProfileIdRow, error) { +func (m *MockStore) GetRuleEvaluationByProfileIdAndRuleType(arg0 context.Context, arg1 uuid.UUID, arg2 sql.NullString, arg3 uuid.UUID, arg4 sql.NullString) (*db.ListRuleEvaluationsByProfileIdRow, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetRuleEvaluationByProfileIdAndRuleType", arg0, arg1, arg2, arg3, arg4) ret0, _ := ret[0].(*db.ListRuleEvaluationsByProfileIdRow) diff --git a/internal/controlplane/handlers_githubwebhooks.go b/internal/controlplane/handlers_githubwebhooks.go index f63e0fafa7..978d6c9e6e 100644 --- a/internal/controlplane/handlers_githubwebhooks.go +++ b/internal/controlplane/handlers_githubwebhooks.go @@ -799,7 +799,7 @@ func (s *Server) processRelevantRepositoryEvent( WithProjectID(repoEntity.Entity.ProjectID). WithProviderID(repoEntity.Entity.ProviderID). WithRepository(pbRepo). - WithRepositoryID(repoEntity.Entity.ID) + WithID(repoEntity.Entity.ID) return &processingResult{ topic: events.TopicQueueEntityEvaluate, diff --git a/internal/controlplane/handlers_githubwebhooks_test.go b/internal/controlplane/handlers_githubwebhooks_test.go index 87f3fb9fef..afd80e00a5 100644 --- a/internal/controlplane/handlers_githubwebhooks_test.go +++ b/internal/controlplane/handlers_githubwebhooks_test.go @@ -1404,7 +1404,7 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() { require.Equal(t, "https://api.github.com/", received.Metadata["source"]) require.Equal(t, providerID.String(), received.Metadata["provider_id"]) require.Equal(t, projectID.String(), received.Metadata[entities.ProjectIDEventKey]) - require.Equal(t, repositoryID.String(), received.Metadata["repository_id"]) + require.Equal(t, repositoryID.String(), received.Metadata[entities.EntityIDEventKey]) }, }, { @@ -1455,7 +1455,7 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() { require.Equal(t, "https://api.github.com/", received.Metadata["source"]) require.Equal(t, providerID.String(), received.Metadata["provider_id"]) require.Equal(t, projectID.String(), received.Metadata[entities.ProjectIDEventKey]) - require.Equal(t, repositoryID.String(), received.Metadata["repository_id"]) + require.Equal(t, repositoryID.String(), received.Metadata[entities.EntityIDEventKey]) }, }, { @@ -1602,7 +1602,7 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() { require.Equal(t, "https://api.github.com/", received.Metadata["source"]) require.Equal(t, providerID.String(), received.Metadata["provider_id"]) require.Equal(t, projectID.String(), received.Metadata[entities.ProjectIDEventKey]) - require.Equal(t, repositoryID.String(), received.Metadata["repository_id"]) + require.Equal(t, repositoryID.String(), received.Metadata[entities.EntityIDEventKey]) }, }, { @@ -1653,7 +1653,7 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() { require.Equal(t, "https://api.github.com/", received.Metadata["source"]) require.Equal(t, providerID.String(), received.Metadata["provider_id"]) require.Equal(t, projectID.String(), received.Metadata[entities.ProjectIDEventKey]) - require.Equal(t, repositoryID.String(), received.Metadata["repository_id"]) + require.Equal(t, repositoryID.String(), received.Metadata[entities.EntityIDEventKey]) }, }, { @@ -1704,7 +1704,7 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() { require.Equal(t, "https://api.github.com/", received.Metadata["source"]) require.Equal(t, providerID.String(), received.Metadata["provider_id"]) require.Equal(t, projectID.String(), received.Metadata[entities.ProjectIDEventKey]) - require.Equal(t, repositoryID.String(), received.Metadata["repository_id"]) + require.Equal(t, repositoryID.String(), received.Metadata[entities.EntityIDEventKey]) }, }, { @@ -1755,7 +1755,7 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() { require.Equal(t, "https://api.github.com/", received.Metadata["source"]) require.Equal(t, providerID.String(), received.Metadata["provider_id"]) require.Equal(t, projectID.String(), received.Metadata[entities.ProjectIDEventKey]) - require.Equal(t, repositoryID.String(), received.Metadata["repository_id"]) + require.Equal(t, repositoryID.String(), received.Metadata[entities.EntityIDEventKey]) }, }, { @@ -1854,7 +1854,7 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() { require.Equal(t, "https://api.github.com/", received.Metadata["source"]) require.Equal(t, providerID.String(), received.Metadata["provider_id"]) require.Equal(t, projectID.String(), received.Metadata[entities.ProjectIDEventKey]) - require.Equal(t, repositoryID.String(), received.Metadata["repository_id"]) + require.Equal(t, repositoryID.String(), received.Metadata[entities.EntityIDEventKey]) }, }, { @@ -1944,7 +1944,7 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() { require.Equal(t, "https://api.github.com/", received.Metadata["source"]) require.Equal(t, providerID.String(), received.Metadata["provider_id"]) require.Equal(t, projectID.String(), received.Metadata[entities.ProjectIDEventKey]) - require.Equal(t, repositoryID.String(), received.Metadata["repository_id"]) + require.Equal(t, repositoryID.String(), received.Metadata[entities.EntityIDEventKey]) }, }, diff --git a/internal/db/store.go b/internal/db/store.go index 809c2d007c..5a9d6073be 100644 --- a/internal/db/store.go +++ b/internal/db/store.go @@ -33,7 +33,7 @@ type GetTypedEntitiesOptions struct { type ExtendQuerier interface { Querier GetRuleEvaluationByProfileIdAndRuleType(ctx context.Context, profileID uuid.UUID, - ruleName sql.NullString, entityID uuid.NullUUID, ruleTypeName sql.NullString) (*ListRuleEvaluationsByProfileIdRow, error) + ruleName sql.NullString, entityID uuid.UUID, ruleTypeName sql.NullString) (*ListRuleEvaluationsByProfileIdRow, error) UpsertPropertyValueV1(ctx context.Context, params UpsertPropertyValueV1Params) (Property, error) GetPropertyValueV1(ctx context.Context, entityID uuid.UUID, key string) (PropertyValueV1, error) GetAllPropertyValuesV1(ctx context.Context, entityID uuid.UUID) ([]PropertyValueV1, error) @@ -119,12 +119,15 @@ func (q *Queries) GetRuleEvaluationByProfileIdAndRuleType( ctx context.Context, profileID uuid.UUID, ruleName sql.NullString, - entityID uuid.NullUUID, + entityID uuid.UUID, ruleTypeName sql.NullString, ) (*ListRuleEvaluationsByProfileIdRow, error) { params := ListRuleEvaluationsByProfileIdParams{ - ProfileID: profileID, - EntityID: entityID, + ProfileID: profileID, + EntityID: uuid.NullUUID{ + UUID: entityID, + Valid: true, + }, RuleName: ruleName, RuleTypeName: ruleTypeName, } diff --git a/internal/eea/eea.go b/internal/eea/eea.go index 63e7f60a1e..496d087384 100644 --- a/internal/eea/eea.go +++ b/internal/eea/eea.go @@ -299,7 +299,7 @@ func (e *EEA) buildRepositoryInfoWrapper( return entities.NewEntityInfoWrapper(). WithRepository(r). - WithRepositoryID(repoID). + WithID(repoID). WithProjectID(projID). WithProviderID(ent.Entity.ProviderID), nil } @@ -331,7 +331,7 @@ func (e *EEA) buildArtifactInfoWrapper( eiw := entities.NewEntityInfoWrapper(). WithProjectID(projID). WithArtifact(a). - WithArtifactID(artID). + WithID(artID). WithProviderID(ent.Entity.ProviderID) return eiw, nil } @@ -350,8 +350,6 @@ func (e *EEA) buildPullRequestInfoWrapper( return nil, fmt.Errorf("entity %s does not belong to project %s", prID, projID) } - repoID := ent.Entity.OriginatedFrom - rawPR, err := e.entityFetcher.EntityWithPropertiesAsProto(ctx, ent, e.provMan) if err != nil { return nil, fmt.Errorf("error converting entity to protobuf: %w", err) @@ -363,9 +361,8 @@ func (e *EEA) buildPullRequestInfoWrapper( } return entities.NewEntityInfoWrapper(). - WithRepositoryID(repoID). WithProjectID(projID). WithPullRequest(pr). - WithPullRequestID(prID). + WithID(prID). WithProviderID(ent.Entity.ProviderID), nil } diff --git a/internal/eea/eea_test.go b/internal/eea/eea_test.go index be8d47ce59..a51394c422 100644 --- a/internal/eea/eea_test.go +++ b/internal/eea/eea_test.go @@ -109,9 +109,11 @@ func TestAggregator(t *testing.T) { inf := entities.NewEntityInfoWrapper(). WithRepository(&minderv1.Repository{}). - WithRepositoryID(repoID). + WithID(repoID). WithProjectID(projectID). WithProviderID(providerID) + msg, err := inf.BuildMessage() + require.NoError(t, err, "expected no error when building message") <-evt.Running() @@ -121,9 +123,7 @@ func TestAggregator(t *testing.T) { wg.Add(1) go func() { defer wg.Done() - msg, err := inf.BuildMessage() - require.NoError(t, err, "expected no error when building message") - err = evt.Publish(rateLimitedMessageTopic, msg.Copy()) + err := evt.Publish(rateLimitedMessageTopic, msg.Copy()) require.NoError(t, err, "expected no error when publishing message") }() } diff --git a/internal/engine/entities/entity_event.go b/internal/engine/entities/entity_event.go index ee3bba303c..3c637cac3a 100644 --- a/internal/engine/entities/entity_event.go +++ b/internal/engine/entities/entity_event.go @@ -19,6 +19,7 @@ import ( "github.com/ThreeDotsLabs/watermill/message" "github.com/google/uuid" + "github.com/rs/zerolog" "google.golang.org/protobuf/encoding/protojson" "google.golang.org/protobuf/reflect/protoreflect" @@ -37,18 +38,11 @@ import ( // It also assumes the following metadata keys are present: // // - EntityTypeEventKey - entity_type -// - ProjectIDEventKey - project_id -// - RepositoryIDEventKey - repository_id -// - ArtifactIDEventKey - artifact_id (only for versioned artifacts) -// -// Entity type is used to determine the type of the protobuf message -// and the entity type in the database. It may be one of the following: -// -// - RepositoryEventEntityType - repository -// - VersionedArtifactEventEntityType - versioned_artifact +// - EntityIDEventKey - entity_id type EntityInfoWrapper struct { ProviderID uuid.UUID ProjectID uuid.UUID + EntityID uuid.UUID Entity protoreflect.ProtoMessage Type minderv1.Entity OwnershipData map[string]string @@ -56,36 +50,23 @@ type EntityInfoWrapper struct { ActionEvent string } -const ( - // RepositoryEventEntityType is the entity type for repositories - RepositoryEventEntityType = "repository" - // VersionedArtifactEventEntityType is the entity type for versioned artifacts - VersionedArtifactEventEntityType = "versioned_artifact" - // PullRequestEventEntityType is the entity type for pull requests - PullRequestEventEntityType = "pull_request" -) - const ( // EntityTypeEventKey is the key for the entity type EntityTypeEventKey = "entity_type" + // EntityIDEventKey is the key for the entity ID + // Note that we'll be migrating to this key + // and deprecating the other entity ID keys + EntityIDEventKey = "entity_id" // ProviderIDEventKey is the key for the provider ID ProviderIDEventKey = "provider_id" // ProjectIDEventKey is the key for the project ID ProjectIDEventKey = "project_id" - // RepositoryIDEventKey is the key for the repository ID - RepositoryIDEventKey = "repository_id" - // ArtifactIDEventKey is the key for the artifact ID - ArtifactIDEventKey = "artifact_id" - // PullRequestIDEventKey is the key for the pull request ID - PullRequestIDEventKey = "pull_request_id" - // ReleaseIDEventKey is the key for the pull request ID - ReleaseIDEventKey = "release_id" - // PipelineRunIDEventKey is the key for a pipeline run - PipelineRunIDEventKey = "pipeline_run_id" - // TaskRunIDEventKey is the key for a task run - TaskRunIDEventKey = "task_run_id" - // BuildIDEventKey is the key for a build - BuildIDEventKey = "build_run_id" + // repositoryIDEventKey is the key for the repository ID + repositoryIDEventKey = "repository_id" + // artifactIDEventKey is the key for the artifact ID + artifactIDEventKey = "artifact_id" + // pullRequestIDEventKey is the key for the pull request ID + pullRequestIDEventKey = "pull_request_id" // ExecutionIDKey is the key for the execution ID. This is set when acquiring a lock. ExecutionIDKey = "execution_id" ) @@ -137,34 +118,10 @@ func (eiw *EntityInfoWrapper) WithPullRequest(p *minderv1.PullRequest) *EntityIn return eiw } -// WithRelease sets a Release as the entity of the wrapper -func (eiw *EntityInfoWrapper) WithRelease(r *minderv1.Release) *EntityInfoWrapper { - eiw.Type = minderv1.Entity_ENTITY_RELEASE - eiw.Entity = r - - return eiw -} - -// WithPipelineRun sets a PipelineRun as the entity of the wrapper -func (eiw *EntityInfoWrapper) WithPipelineRun(plr *minderv1.PipelineRun) *EntityInfoWrapper { - eiw.Type = minderv1.Entity_ENTITY_PIPELINE_RUN - eiw.Entity = plr - - return eiw -} - -// WithTaskRun sets a TaskRun as the entity of the wrapper -func (eiw *EntityInfoWrapper) WithTaskRun(tr *minderv1.TaskRun) *EntityInfoWrapper { - eiw.Type = minderv1.Entity_ENTITY_TASK_RUN - eiw.Entity = tr - - return eiw -} - -// WithBuild sets a Build as the entity of the wrapper -func (eiw *EntityInfoWrapper) WithBuild(tr *minderv1.Build) *EntityInfoWrapper { - eiw.Type = minderv1.Entity_ENTITY_TASK_RUN - eiw.Entity = tr +// WithEntityInstance sets the entity to an entity instance +func (eiw *EntityInfoWrapper) WithEntityInstance(etyp minderv1.Entity, ei *minderv1.EntityInstance) *EntityInfoWrapper { + eiw.Type = etyp + eiw.Entity = ei return eiw } @@ -176,51 +133,9 @@ func (eiw *EntityInfoWrapper) WithProjectID(id uuid.UUID) *EntityInfoWrapper { return eiw } -// WithRepositoryID sets the repository ID -func (eiw *EntityInfoWrapper) WithRepositoryID(id uuid.UUID) *EntityInfoWrapper { - eiw.withID(RepositoryIDEventKey, id.String()) - - return eiw -} - -// WithArtifactID sets the artifact ID -func (eiw *EntityInfoWrapper) WithArtifactID(id uuid.UUID) *EntityInfoWrapper { - eiw.withID(ArtifactIDEventKey, id.String()) - - return eiw -} - -// WithPullRequestID sets the pull request ID -func (eiw *EntityInfoWrapper) WithPullRequestID(id uuid.UUID) *EntityInfoWrapper { - eiw.withID(PullRequestIDEventKey, id.String()) - - return eiw -} - -// WithReleaseID sets the release ID -func (eiw *EntityInfoWrapper) WithReleaseID(id uuid.UUID) *EntityInfoWrapper { - eiw.withID(ReleaseIDEventKey, id.String()) - - return eiw -} - -// WithPipelineRunID sets the pipeline run ID -func (eiw *EntityInfoWrapper) WithPipelineRunID(id uuid.UUID) *EntityInfoWrapper { - eiw.withID(PipelineRunIDEventKey, id.String()) - - return eiw -} - -// WithTaskRunID sets the pipeline run ID -func (eiw *EntityInfoWrapper) WithTaskRunID(id uuid.UUID) *EntityInfoWrapper { - eiw.withID(TaskRunIDEventKey, id.String()) - - return eiw -} - -// WithBuildID sets the pipeline run ID -func (eiw *EntityInfoWrapper) WithBuildID(id uuid.UUID) *EntityInfoWrapper { - eiw.withID(BuildIDEventKey, id.String()) +// WithID sets the ID for an entity type +func (eiw *EntityInfoWrapper) WithID(id uuid.UUID) *EntityInfoWrapper { + eiw.EntityID = id return eiw } @@ -254,6 +169,12 @@ func (eiw *EntityInfoWrapper) AsPullRequest() { eiw.Entity = &minderv1.PullRequest{} } +// AsEntityInstance sets the entity type to an entity instance +func (eiw *EntityInfoWrapper) AsEntityInstance(entityType minderv1.Entity) { + eiw.Type = entityType + eiw.Entity = &minderv1.EntityInstance{} +} + // BuildMessage builds a message.Message from the information func (eiw *EntityInfoWrapper) BuildMessage() (*message.Message, error) { id, err := uuid.NewUUID() @@ -285,10 +206,7 @@ func (eiw *EntityInfoWrapper) Publish(evt events.Publisher) error { // ToMessage sets the information to a message.Message func (eiw *EntityInfoWrapper) ToMessage(msg *message.Message) error { - typ, err := pbEntityTypeToString(eiw.Type) - if err != nil { - return err - } + typ := eiw.Type.ToString() if eiw.ProjectID == uuid.Nil { return fmt.Errorf("project ID is required") @@ -298,16 +216,30 @@ func (eiw *EntityInfoWrapper) ToMessage(msg *message.Message) error { return fmt.Errorf("provider ID is required") } + if eiw.EntityID != uuid.Nil { + msg.Metadata.Set(EntityIDEventKey, eiw.EntityID.String()) + } + if eiw.ExecutionID != nil { msg.Metadata.Set(ExecutionIDKey, eiw.ExecutionID.String()) } + if eiw.Type == minderv1.Entity_ENTITY_UNSPECIFIED { + return fmt.Errorf("entity type is required") + } + + if eiw.Entity == nil { + return fmt.Errorf("no entity set") + } + msg.Metadata.Set(ProviderIDEventKey, eiw.ProviderID.String()) msg.Metadata.Set(EntityTypeEventKey, typ) msg.Metadata.Set(ProjectIDEventKey, eiw.ProjectID.String()) for k, v := range eiw.OwnershipData { msg.Metadata.Set(k, v) } + + var err error msg.Payload, err = protojson.Marshal(eiw.Entity) if err != nil { return fmt.Errorf("error marshalling repository: %w", err) @@ -316,42 +248,17 @@ func (eiw *EntityInfoWrapper) ToMessage(msg *message.Message) error { return nil } -// GetEntityDBIDs returns the repository, artifact and pull request IDs -// from the ownership data -func (eiw *EntityInfoWrapper) GetEntityDBIDs() (repoID uuid.NullUUID, artifactID uuid.NullUUID, pullRequestID uuid.NullUUID) { - strRepoID, ok := eiw.OwnershipData[RepositoryIDEventKey] - if ok { - repoID = uuid.NullUUID{ - UUID: uuid.MustParse(strRepoID), - Valid: true, - } - } - - strArtifactID, ok := eiw.OwnershipData[ArtifactIDEventKey] - if ok { - artifactID = uuid.NullUUID{ - UUID: uuid.MustParse(strArtifactID), - Valid: true, - } - } - - strPullRequestID, ok := eiw.OwnershipData[PullRequestIDEventKey] - if ok { - pullRequestID = uuid.NullUUID{ - UUID: uuid.MustParse(strPullRequestID), - Valid: true, - } - } - - return repoID, artifactID, pullRequestID -} - // GetID returns the entity ID. func (eiw *EntityInfoWrapper) GetID() (uuid.UUID, error) { if eiw == nil { return uuid.Nil, fmt.Errorf("no entity info wrapper") } + if eiw.EntityID != uuid.Nil { + return eiw.EntityID, nil + } + + // Fall back to the ownership data id, ok := eiw.getIDForEntityType(eiw.Type) if ok { return id, nil @@ -360,6 +267,8 @@ func (eiw *EntityInfoWrapper) GetID() (uuid.UUID, error) { return uuid.Nil, fmt.Errorf("no entity ID found") } +// This will be deprecated in the future in favor of relying on the entity ID key. +// For now, this is just a fallback. func (eiw *EntityInfoWrapper) getIDForEntityType(t minderv1.Entity) (uuid.UUID, bool) { key, err := getEntityMetadataKey(t) if err != nil { @@ -404,15 +313,30 @@ func (eiw *EntityInfoWrapper) withProviderIDFromMessage(msg *message.Message) er } func (eiw *EntityInfoWrapper) withRepositoryIDFromMessage(msg *message.Message) error { - return eiw.withIDFromMessage(msg, RepositoryIDEventKey) + return eiw.withIDFromMessage(msg, repositoryIDEventKey) } func (eiw *EntityInfoWrapper) withArtifactIDFromMessage(msg *message.Message) error { - return eiw.withIDFromMessage(msg, ArtifactIDEventKey) + return eiw.withIDFromMessage(msg, artifactIDEventKey) } func (eiw *EntityInfoWrapper) withPullRequestIDFromMessage(msg *message.Message) error { - return eiw.withIDFromMessage(msg, PullRequestIDEventKey) + return eiw.withIDFromMessage(msg, pullRequestIDEventKey) +} + +func (eiw *EntityInfoWrapper) withEntityInstanceIDFromMessage(msg *message.Message) error { + rawEntityID := msg.Metadata.Get(EntityIDEventKey) + if rawEntityID == "" { + return fmt.Errorf("%s not found in metadata", EntityIDEventKey) + } + + entityID, err := uuid.Parse(rawEntityID) + if err != nil { + return fmt.Errorf("malformed entity id %s", rawEntityID) + } + + eiw.EntityID = entityID + return nil } // WithExecutionIDFromMessage sets the execution ID from the message @@ -441,61 +365,25 @@ func (eiw *EntityInfoWrapper) withIDFromMessage(msg *message.Message, key string return nil } -func (eiw *EntityInfoWrapper) withID(key string, id string) { - eiw.OwnershipData[key] = id -} - func (eiw *EntityInfoWrapper) unmarshalEntity(msg *message.Message) error { return protojson.Unmarshal(msg.Payload, eiw.Entity) } -func pbEntityTypeToString(t minderv1.Entity) (string, error) { - switch t { - case minderv1.Entity_ENTITY_REPOSITORIES: - return RepositoryEventEntityType, nil - case minderv1.Entity_ENTITY_ARTIFACTS: - return VersionedArtifactEventEntityType, nil - case minderv1.Entity_ENTITY_PULL_REQUESTS: - return PullRequestEventEntityType, nil - case minderv1.Entity_ENTITY_RELEASE: - return "", fmt.Errorf("releases not yet supported") - case minderv1.Entity_ENTITY_PIPELINE_RUN: - return "", fmt.Errorf("pipeline runs not yet supported") - case minderv1.Entity_ENTITY_TASK_RUN: - return "", fmt.Errorf("task runs not yet supported") - case minderv1.Entity_ENTITY_BUILD: - return "", fmt.Errorf("builds not yet supported") - case minderv1.Entity_ENTITY_BUILD_ENVIRONMENTS: - return "", fmt.Errorf("build environments not yet supported") - case minderv1.Entity_ENTITY_UNSPECIFIED: - return "", fmt.Errorf("entity type unspecified") - default: - return "", fmt.Errorf("unknown entity type: %s", t.String()) - } -} - +// This is only used to get a specific entity ID from the metadata +// This will be deprecated in the future in favor of relying on the entity ID key func getEntityMetadataKey(t minderv1.Entity) (string, error) { + //nolint:exhaustive // We want to fail if it's not one of the explicit types switch t { case minderv1.Entity_ENTITY_REPOSITORIES: - return RepositoryIDEventKey, nil + return repositoryIDEventKey, nil case minderv1.Entity_ENTITY_ARTIFACTS: - return ArtifactIDEventKey, nil + return artifactIDEventKey, nil case minderv1.Entity_ENTITY_PULL_REQUESTS: - return PullRequestIDEventKey, nil - case minderv1.Entity_ENTITY_RELEASE: - return ReleaseIDEventKey, nil - case minderv1.Entity_ENTITY_PIPELINE_RUN: - return PipelineRunIDEventKey, nil - case minderv1.Entity_ENTITY_TASK_RUN: - return TaskRunIDEventKey, nil - case minderv1.Entity_ENTITY_BUILD: - return BuildIDEventKey, nil - case minderv1.Entity_ENTITY_BUILD_ENVIRONMENTS: - return "", fmt.Errorf("build environments not yet supported") + return pullRequestIDEventKey, nil case minderv1.Entity_ENTITY_UNSPECIFIED: return "", fmt.Errorf("entity type unspecified") default: - return "", fmt.Errorf("unknown entity type: %s", t.String()) + return "", fmt.Errorf("unknown or unsupported entity type: %s", t.String()) } } @@ -509,6 +397,8 @@ func getIDFromMessage(msg *message.Message, key string) (string, error) { } // ParseEntityEvent parses a message.Message and returns an EntityInfoWrapper +// +//nolint:gocyclo // This will be simplified once we rely solely on the entity ID key func ParseEntityEvent(msg *message.Message) (*EntityInfoWrapper, error) { out := &EntityInfoWrapper{ OwnershipData: make(map[string]string), @@ -522,32 +412,60 @@ func ParseEntityEvent(msg *message.Message) (*EntityInfoWrapper, error) { return nil, err } + if err := out.withEntityInstanceIDFromMessage(msg); err != nil { + // We don't fail, but instead log the error and continue + // We'll fall back to the other entity ID keys. + zerolog.Ctx(msg.Context()).Debug(). + Str("message_id", msg.UUID). + Msg("message does not contain entity ID") + } + // We don't always have repo ID (e.g. for artifacts) typ := msg.Metadata.Get(EntityTypeEventKey) - switch typ { - case RepositoryEventEntityType: + strtyp := minderv1.EntityFromString(typ) + + //nolint:exhaustive // We have a default case + switch strtyp { + case minderv1.Entity_ENTITY_REPOSITORIES: out.AsRepository() - if err := out.withRepositoryIDFromMessage(msg); err != nil { - return nil, err + if out.EntityID == uuid.Nil { + if err := out.withRepositoryIDFromMessage(msg); err != nil { + return nil, err + } } - case VersionedArtifactEventEntityType: + case minderv1.Entity_ENTITY_ARTIFACTS: out.AsArtifact() - if err := out.withArtifactIDFromMessage(msg); err != nil { - return nil, err + if out.EntityID == uuid.Nil { + if err := out.withArtifactIDFromMessage(msg); err != nil { + return nil, err + } + //nolint:gosec // The repo is not always present + out.withRepositoryIDFromMessage(msg) } - //nolint:gosec // The repo is not always present - out.withRepositoryIDFromMessage(msg) - case PullRequestEventEntityType: + case minderv1.Entity_ENTITY_PULL_REQUESTS: out.AsPullRequest() - if err := out.withPullRequestIDFromMessage(msg); err != nil { - return nil, err + if out.EntityID == uuid.Nil { + if err := out.withPullRequestIDFromMessage(msg); err != nil { + return nil, err + } + if err := out.withRepositoryIDFromMessage(msg); err != nil { + return nil, err + } } - if err := out.withRepositoryIDFromMessage(msg); err != nil { + case minderv1.Entity_ENTITY_UNSPECIFIED: + return nil, fmt.Errorf("entity type unspecified") + default: + // We can't fall back in this case. + if out.EntityID == uuid.Nil { + return nil, fmt.Errorf("entity ID not found") + } + + // Any other entity type + out.AsEntityInstance(strtyp) + if err := out.withEntityInstanceIDFromMessage(msg); err != nil { return nil, err } - default: - return nil, fmt.Errorf("unknown entity type: %s", typ) } if err := out.unmarshalEntity(msg); err != nil { @@ -556,14 +474,3 @@ func ParseEntityEvent(msg *message.Message) (*EntityInfoWrapper, error) { return out, nil } - -// WithID sets the ID for an entity type -func (eiw *EntityInfoWrapper) WithID(entType minderv1.Entity, id uuid.UUID) *EntityInfoWrapper { - key, err := getEntityMetadataKey(entType) - if err != nil { - return nil - } - - eiw.withID(key, id.String()) - return eiw -} diff --git a/internal/engine/entities/entity_event_test.go b/internal/engine/entities/entity_event_test.go index c82e45b362..973e00d226 100644 --- a/internal/engine/entities/entity_event_test.go +++ b/internal/engine/entities/entity_event_test.go @@ -33,6 +33,7 @@ func Test_parseEntityEvent(t *testing.T) { projectID := uuid.New() providerID := uuid.New() repoID := uuid.NewString() + prID := uuid.NewString() artifactID := uuid.NewString() type args struct { @@ -40,6 +41,7 @@ func Test_parseEntityEvent(t *testing.T) { entType string projectID uuid.UUID providerID uuid.UUID + entityID string ownership map[string]string } tests := []struct { @@ -49,16 +51,16 @@ func Test_parseEntityEvent(t *testing.T) { wantErr bool }{ { - name: "repository event", + name: "legacy repository event", args: args{ ent: &pb.Repository{ Name: "test", RepoId: 123, }, - entType: RepositoryEventEntityType, + entType: pb.Entity_ENTITY_REPOSITORIES.ToString(), projectID: projectID, providerID: providerID, - ownership: map[string]string{RepositoryIDEventKey: repoID}, + ownership: map[string]string{repositoryIDEventKey: repoID}, }, want: &EntityInfoWrapper{ ProjectID: projectID, @@ -69,12 +71,12 @@ func Test_parseEntityEvent(t *testing.T) { ProviderID: providerID, Type: pb.Entity_ENTITY_REPOSITORIES, OwnershipData: map[string]string{ - RepositoryIDEventKey: repoID, + repositoryIDEventKey: repoID, }, }, }, { - name: "versioned artifact event", + name: "legacy versioned artifact event", args: args{ ent: &pb.Artifact{ ArtifactPk: artifactID, @@ -84,12 +86,12 @@ func Test_parseEntityEvent(t *testing.T) { }, }, }, - entType: VersionedArtifactEventEntityType, + entType: pb.Entity_ENTITY_ARTIFACTS.ToString(), projectID: projectID, providerID: providerID, ownership: map[string]string{ - RepositoryIDEventKey: repoID, - ArtifactIDEventKey: artifactID, + repositoryIDEventKey: repoID, + artifactIDEventKey: artifactID, }, }, want: &EntityInfoWrapper{ @@ -105,13 +107,13 @@ func Test_parseEntityEvent(t *testing.T) { ProviderID: providerID, Type: pb.Entity_ENTITY_ARTIFACTS, OwnershipData: map[string]string{ - RepositoryIDEventKey: repoID, - ArtifactIDEventKey: artifactID, + repositoryIDEventKey: repoID, + artifactIDEventKey: artifactID, }, }, }, { - name: "pull_request event", + name: "legacy pull_request event", args: args{ ent: &pb.PullRequest{ Url: "https://api.github.com/repos/jakubtestorg/bad-npm/pulls/3", @@ -120,12 +122,12 @@ func Test_parseEntityEvent(t *testing.T) { RepoOwner: "jakubtestorg", RepoName: "bad-npm", }, - entType: PullRequestEventEntityType, + entType: pb.Entity_ENTITY_PULL_REQUESTS.ToString(), projectID: projectID, providerID: providerID, ownership: map[string]string{ - PullRequestIDEventKey: "3", - RepositoryIDEventKey: repoID, + pullRequestIDEventKey: prID, + repositoryIDEventKey: repoID, }, }, want: &EntityInfoWrapper{ @@ -140,9 +142,95 @@ func Test_parseEntityEvent(t *testing.T) { ProviderID: providerID, Type: pb.Entity_ENTITY_PULL_REQUESTS, OwnershipData: map[string]string{ - PullRequestIDEventKey: "3", - RepositoryIDEventKey: repoID, + pullRequestIDEventKey: prID, + repositoryIDEventKey: repoID, + }, + }, + }, + { + name: "repository event with entity ID", + args: args{ + ent: &pb.Repository{ + Name: "test", + RepoId: 123, + }, + entType: pb.Entity_ENTITY_REPOSITORIES.ToString(), + projectID: projectID, + providerID: providerID, + entityID: repoID, + }, + want: &EntityInfoWrapper{ + ProjectID: projectID, + Entity: &pb.Repository{ + Name: "test", + RepoId: 123, + }, + ProviderID: providerID, + Type: pb.Entity_ENTITY_REPOSITORIES, + EntityID: uuid.MustParse(repoID), + OwnershipData: map[string]string{}, + }, + }, + { + name: "artifact event with entity ID", + args: args{ + ent: &pb.Artifact{ + ArtifactPk: artifactID, + Versions: []*pb.ArtifactVersion{ + { + VersionId: 789, + }, + }, + }, + entType: pb.Entity_ENTITY_ARTIFACTS.ToString(), + projectID: projectID, + providerID: providerID, + entityID: artifactID, + }, + want: &EntityInfoWrapper{ + ProjectID: projectID, + Entity: &pb.Artifact{ + ArtifactPk: artifactID, + Versions: []*pb.ArtifactVersion{ + { + VersionId: 789, + }, + }, + }, + ProviderID: providerID, + Type: pb.Entity_ENTITY_ARTIFACTS, + EntityID: uuid.MustParse(artifactID), + OwnershipData: map[string]string{}, + }, + }, + { + name: "pull_request event with entity ID", + args: args{ + ent: &pb.PullRequest{ + Url: "https://api.github.com/repos/jakubtestorg/bad-npm/pulls/3", + CommitSha: "bd9958a63c9b95ccc2bc0cf1eef65a87529aed16", + Number: 3, + RepoOwner: "jakubtestorg", + RepoName: "bad-npm", }, + entType: pb.Entity_ENTITY_PULL_REQUESTS.ToString(), + projectID: projectID, + providerID: providerID, + entityID: prID, + }, + want: &EntityInfoWrapper{ + ProjectID: projectID, + Entity: &pb.PullRequest{ + Url: "https://api.github.com/repos/jakubtestorg/bad-npm/pulls/3", + CommitSha: "bd9958a63c9b95ccc2bc0cf1eef65a87529aed16", + Number: 3, + RepoOwner: "jakubtestorg", + RepoName: "bad-npm", + }, + ProviderID: providerID, + Type: pb.Entity_ENTITY_PULL_REQUESTS, + EntityID: uuid.MustParse(prID), + OwnershipData: map[string]string{}, }, }, } @@ -158,12 +246,19 @@ func Test_parseEntityEvent(t *testing.T) { msg := message.NewMessage("", marshalledEntity) msg.Metadata.Set(ProjectIDEventKey, tt.args.projectID.String()) msg.Metadata.Set(EntityTypeEventKey, tt.args.entType) - msg.Metadata.Set(RepositoryIDEventKey, tt.args.ownership["repository_id"]) msg.Metadata.Set(ProviderIDEventKey, tt.args.providerID.String()) - if tt.args.entType == VersionedArtifactEventEntityType { - msg.Metadata.Set(ArtifactIDEventKey, tt.args.ownership["artifact_id"]) - } else if tt.args.entType == PullRequestEventEntityType { - msg.Metadata.Set(PullRequestIDEventKey, tt.args.ownership["pull_request_id"]) + + if len(tt.args.entityID) > 0 { + msg.Metadata.Set(EntityIDEventKey, tt.args.entityID) + } + + if len(tt.args.ownership) > 0 { + msg.Metadata.Set(repositoryIDEventKey, tt.args.ownership["repository_id"]) + if tt.args.entType == pb.Entity_ENTITY_ARTIFACTS.ToString() { + msg.Metadata.Set(artifactIDEventKey, tt.args.ownership["artifact_id"]) + } else if tt.args.entType == pb.Entity_ENTITY_PULL_REQUESTS.ToString() { + msg.Metadata.Set(pullRequestIDEventKey, tt.args.ownership["pull_request_id"]) + } } got, err := ParseEntityEvent(msg) @@ -207,12 +302,12 @@ func TestEntityInfoWrapper_ToMessage(t *testing.T) { WithRepository(&pb.Repository{ Owner: "test", RepoId: 123, - }).WithRepositoryID(repoID), + }).WithID(repoID), expected: map[string]string{ - ProviderIDEventKey: providerID.String(), - EntityTypeEventKey: RepositoryEventEntityType, - ProjectIDEventKey: projectID.String(), - RepositoryIDEventKey: repoID.String(), + ProviderIDEventKey: providerID.String(), + EntityTypeEventKey: pb.Entity_ENTITY_REPOSITORIES.ToString(), + ProjectIDEventKey: projectID.String(), + EntityIDEventKey: repoID.String(), }, }, { @@ -224,12 +319,12 @@ func TestEntityInfoWrapper_ToMessage(t *testing.T) { Owner: "test", RepoId: 123, }). - WithID(pb.Entity_ENTITY_REPOSITORIES, repoID), + WithID(repoID), expected: map[string]string{ - ProviderIDEventKey: providerID.String(), - EntityTypeEventKey: RepositoryEventEntityType, - ProjectIDEventKey: projectID.String(), - RepositoryIDEventKey: repoID.String(), + ProviderIDEventKey: providerID.String(), + EntityTypeEventKey: pb.Entity_ENTITY_REPOSITORIES.ToString(), + ProjectIDEventKey: projectID.String(), + EntityIDEventKey: repoID.String(), }, }, { @@ -244,35 +339,13 @@ func TestEntityInfoWrapper_ToMessage(t *testing.T) { VersionId: 101112, }, }, - }).WithRepositoryID(repoID). - WithArtifactID(artifactID), - expected: map[string]string{ - ProviderIDEventKey: providerID.String(), - EntityTypeEventKey: VersionedArtifactEventEntityType, - ProjectIDEventKey: projectID.String(), - RepositoryIDEventKey: repoID.String(), - ArtifactIDEventKey: artifactID.String(), - }, - }, - { - name: "artifact using WithID and WithProtoMessage", - eiw: NewEntityInfoWrapper(). - WithProviderID(providerID). - WithProjectID(projectID). - WithProtoMessage(pb.Entity_ENTITY_ARTIFACTS, &pb.Artifact{ - ArtifactPk: artifactID.String(), - Versions: []*pb.ArtifactVersion{ - { - VersionId: 101112, - }, - }, }). - WithID(pb.Entity_ENTITY_ARTIFACTS, artifactID), + WithID(artifactID), expected: map[string]string{ ProviderIDEventKey: providerID.String(), - EntityTypeEventKey: VersionedArtifactEventEntityType, + EntityTypeEventKey: pb.Entity_ENTITY_ARTIFACTS.ToString(), ProjectIDEventKey: projectID.String(), - ArtifactIDEventKey: artifactID.String(), + EntityIDEventKey: artifactID.String(), }, }, { @@ -287,12 +360,12 @@ func TestEntityInfoWrapper_ToMessage(t *testing.T) { RepoOwner: "jakubtestorg", RepoName: "bad-npm", }). - WithID(pb.Entity_ENTITY_PULL_REQUESTS, pullRequestID), + WithID(pullRequestID), expected: map[string]string{ - ProviderIDEventKey: providerID.String(), - EntityTypeEventKey: PullRequestEventEntityType, - ProjectIDEventKey: projectID.String(), - PullRequestIDEventKey: pullRequestID.String(), + ProviderIDEventKey: providerID.String(), + EntityTypeEventKey: pb.Entity_ENTITY_PULL_REQUESTS.ToString(), + ProjectIDEventKey: projectID.String(), + EntityIDEventKey: pullRequestID.String(), }, }, { @@ -307,12 +380,12 @@ func TestEntityInfoWrapper_ToMessage(t *testing.T) { RepoOwner: "jakubtestorg", RepoName: "bad-npm", }). - WithPullRequestID(pullRequestID), + WithID(pullRequestID), expected: map[string]string{ - ProviderIDEventKey: providerID.String(), - EntityTypeEventKey: PullRequestEventEntityType, - ProjectIDEventKey: projectID.String(), - PullRequestIDEventKey: pullRequestID.String(), + ProviderIDEventKey: providerID.String(), + EntityTypeEventKey: pb.Entity_ENTITY_PULL_REQUESTS.ToString(), + ProjectIDEventKey: projectID.String(), + EntityIDEventKey: pullRequestID.String(), }, }, } @@ -339,7 +412,7 @@ func TestEntityInfoWrapper_FailsWithoutProjectID(t *testing.T) { WithRepository(&pb.Repository{ Owner: "test", RepoId: 123, - }).WithRepositoryID(uuid.New()) + }).WithID(uuid.New()) msg, err := eiw.BuildMessage() t.Logf("OZZ: %+v", msg) @@ -354,7 +427,7 @@ func TestEntityInfoWrapper_FailsWithoutProvider(t *testing.T) { WithRepository(&pb.Repository{ Owner: "test", RepoId: 123, - }).WithRepositoryID(uuid.New()) + }).WithID(uuid.New()) _, err := eiw.BuildMessage() require.Error(t, err, "expected error") @@ -366,7 +439,7 @@ func TestEntityInfoWrapper_FailsWithoutRepository(t *testing.T) { eiw := NewEntityInfoWrapper(). WithProviderID(uuid.New()). WithProjectID(uuid.New()). - WithRepositoryID(uuid.New()) + WithID(uuid.New()) _, err := eiw.BuildMessage() require.Error(t, err, "expected error") diff --git a/internal/engine/eval_status.go b/internal/engine/eval_status.go index 500e0cd88b..2dcfb5879c 100644 --- a/internal/engine/eval_status.go +++ b/internal/engine/eval_status.go @@ -21,14 +21,12 @@ import ( "errors" "fmt" - "github.com/google/uuid" "github.com/rs/zerolog" "github.com/stacklok/minder/internal/db" "github.com/stacklok/minder/internal/engine/entities" evalerrors "github.com/stacklok/minder/internal/engine/errors" engif "github.com/stacklok/minder/internal/engine/interfaces" - ent "github.com/stacklok/minder/internal/entities" "github.com/stacklok/minder/internal/profiles/models" ) @@ -38,35 +36,18 @@ func (e *executor) createEvalStatusParams( profile *models.ProfileAggregate, rule *models.RuleInstance, ) (*engif.EvalStatusParams, error) { - repoID, artID, prID := inf.GetEntityDBIDs() eID, err := inf.GetID() if err != nil { return nil, fmt.Errorf("Error getting ID from entity info wrapper") } params := &engif.EvalStatusParams{ - Rule: rule, - Profile: profile, - EntityType: entities.EntityTypeToDB(inf.Type), - EntityID: eID, - RepoID: repoID, - ArtifactID: artID, - PullRequestID: prID, - ProjectID: inf.ProjectID, - ExecutionID: *inf.ExecutionID, // Execution ID is required in the executor. - } - - entityID := uuid.NullUUID{} - switch params.EntityType { - case db.EntitiesArtifact: - entityID = params.ArtifactID - case db.EntitiesRepository: - entityID = params.RepoID - case db.EntitiesPullRequest: - entityID = params.PullRequestID - case db.EntitiesBuildEnvironment, db.EntitiesRelease, db.EntitiesPipelineRun, - db.EntitiesTaskRun, db.EntitiesBuild: - return nil, fmt.Errorf("entity type not yet supported") + Rule: rule, + Profile: profile, + EntityType: entities.EntityTypeToDB(inf.Type), + EntityID: eID, + ProjectID: inf.ProjectID, + ExecutionID: *inf.ExecutionID, // Execution ID is required in the executor. } // TODO: once we replace the existing profile state types with the new @@ -90,7 +71,7 @@ func (e *executor) createEvalStatusParams( evalStatus, err := e.querier.GetRuleEvaluationByProfileIdAndRuleType(ctx, params.Profile.ID, ruleName, - entityID, + eID, nullableRuleTypeName, ) if err != nil { @@ -129,12 +110,8 @@ func (e *executor) createOrUpdateEvalStatus( return nil } - entityID, entityType, err := ent.EntityFromIDs(params.RepoID.UUID, params.ArtifactID.UUID, params.PullRequestID.UUID) - if err != nil { - return err - } status := evalerrors.ErrorAsEvalStatus(params.GetEvalErr()) - e.metrics.CountEvalStatus(ctx, status, entityType) + e.metrics.CountEvalStatus(ctx, status, params.EntityType) remediationStatus := evalerrors.ErrorAsRemediationStatus(params.GetActionsErr().RemediateErr) e.metrics.CountRemediationStatus(ctx, remediationStatus) @@ -156,7 +133,7 @@ func (e *executor) createOrUpdateEvalStatus( params.Rule.ID, params.Profile.ID, params.EntityType, - entityID, + params.EntityID, params.GetEvalErr(), chkpjs, ) diff --git a/internal/engine/executor.go b/internal/engine/executor.go index a64b0e2c5d..04fdd937f5 100644 --- a/internal/engine/executor.go +++ b/internal/engine/executor.go @@ -282,7 +282,6 @@ func (e *executor) releaseLockAndFlush( ctx context.Context, inf *entities.EntityInfoWrapper, ) { - repoID, artID, prID := inf.GetEntityDBIDs() eID, err := inf.GetID() if err != nil { zerolog.Ctx(ctx).Error().Err(err).Msg("error getting entity id") @@ -294,18 +293,6 @@ func (e *executor) releaseLockAndFlush( Str("execution_id", inf.ExecutionID.String()). Str("entity_id", eID.String()) - // TODO: change these to entity_id - if repoID.Valid { - logger = logger.Str("repo_id", repoID.UUID.String()) - } - - if artID.Valid { - logger = logger.Str("artifact_id", artID.UUID.String()) - } - if prID.Valid { - logger = logger.Str("pull_request_id", prID.UUID.String()) - } - if err := e.querier.ReleaseLock(ctx, db.ReleaseLockParams{ EntityInstanceID: eID, LockedBy: *inf.ExecutionID, diff --git a/internal/engine/executor_test.go b/internal/engine/executor_test.go index 81889d883f..5fcb3b6b25 100644 --- a/internal/engine/executor_test.go +++ b/internal/engine/executor_test.go @@ -356,7 +356,7 @@ default allow = true`, Name: "test", RepoId: 123, CloneUrl: "github.com/foo/bar.git", - }).WithRepositoryID(repositoryID). + }).WithID(repositoryID). WithExecutionID(executionID) ts := &logger.TelemetryStore{ diff --git a/internal/engine/handler.go b/internal/engine/handler.go index 3e699e388e..fc97bbc5cf 100644 --- a/internal/engine/handler.go +++ b/internal/engine/handler.go @@ -167,6 +167,7 @@ func (e *ExecutorEventHandler) HandleEntityEvent(msg *message.Message) error { Str("project", inf.ProjectID.String()). Str("provider_id", inf.ProviderID.String()). Str("entity", inf.Type.String()). + Str("entity_id", inf.EntityID.String()). Err(err).Msg("got error while evaluating entity event") } diff --git a/internal/engine/handler_test.go b/internal/engine/handler_test.go index cc267987a2..e5fde8321b 100644 --- a/internal/engine/handler_test.go +++ b/internal/engine/handler_test.go @@ -78,7 +78,7 @@ func TestExecutorEventHandler_handleEntityEvent(t *testing.T) { Name: "test", RepoId: 123, CloneUrl: "github.com/foo/bar.git", - }).WithRepositoryID(repositoryID). + }).WithID(repositoryID). WithExecutionID(executionID) executor := mockengine.NewMockExecutor(ctrl) diff --git a/internal/engine/interfaces/interface.go b/internal/engine/interfaces/interface.go index 392e7b5450..c2df0c283a 100644 --- a/internal/engine/interfaces/interface.go +++ b/internal/engine/interfaces/interface.go @@ -108,9 +108,6 @@ type EvalStatusParams struct { Result *Result Profile *models.ProfileAggregate Rule *models.RuleInstance - RepoID uuid.NullUUID - ArtifactID uuid.NullUUID - PullRequestID uuid.NullUUID ProjectID uuid.UUID ReleaseID uuid.UUID PipelineRunID uuid.UUID @@ -217,17 +214,8 @@ func (e *EvalStatusParams) DecorateLogger(l zerolog.Logger) zerolog.Logger { Str("rule_name", e.GetRule().Name). Str("execution_id", e.ExecutionID.String()). Str("rule_type_id", e.Rule.RuleTypeID.String()). + Str("entity_id", e.EntityID.String()). Logger() - if e.RepoID.Valid { - outl = outl.With().Str("repository_id", e.RepoID.UUID.String()).Logger() - } - - if e.ArtifactID.Valid { - outl = outl.With().Str("artifact_id", e.ArtifactID.UUID.String()).Logger() - } - if e.PullRequestID.Valid { - outl = outl.With().Str("pull_request_id", e.PullRequestID.UUID.String()).Logger() - } return outl } diff --git a/internal/entities/handlers/handler_test.go b/internal/entities/handlers/handler_test.go index 52b3f79fa0..2b50555922 100644 --- a/internal/entities/handlers/handler_test.go +++ b/internal/entities/handlers/handler_test.go @@ -525,7 +525,7 @@ func TestRefreshEntityAndDoHandler_HandleRefreshEntityAndEval(t *testing.T) { df.WithSuccessfulUpsertPullRequestWithParams( db.PullRequest{ID: pullRequestID}, db.EntityInstance{ - ID: uuid.UUID{}, + ID: pullRequestID, EntityType: db.EntitiesPullRequest, Name: "", ProjectID: projectID, @@ -551,12 +551,6 @@ func TestRefreshEntityAndDoHandler_HandleRefreshEntityAndEval(t *testing.T) { }, }, ), - df.WithSuccessfullGetEntityByID( - repoID, - db.EntityInstance{ - ID: repoID, - EntityType: db.EntitiesRepository, - }), ), providerSetup: newProviderMock( withSuccessfulGetEntityName(pullName), diff --git a/internal/entities/handlers/strategies/message/entity_info_wrapper.go b/internal/entities/handlers/strategies/message/entity_info_wrapper.go index 35852d60c2..1ceee4dd55 100644 --- a/internal/entities/handlers/strategies/message/entity_info_wrapper.go +++ b/internal/entities/handlers/strategies/message/entity_info_wrapper.go @@ -68,19 +68,7 @@ func (c *toEntityInfoWrapper) CreateMessage( WithProjectID(ewp.Entity.ProjectID). WithProviderID(ewp.Entity.ProviderID). WithProtoMessage(ewp.Entity.Type, pbEnt). - WithID(ewp.Entity.Type, ewp.Entity.ID) - - // in case the entity originated from another entity, add that information as well. - // the property service does not provide this information (should it?) so we need to fetch it from the store. - // for now we could have hardcoded the entity type as everything originates from a repository, - // but this is more flexible. - if ewp.Entity.OriginatedFrom != uuid.Nil { - dbEnt, err := c.store.GetEntityByID(ctx, ewp.Entity.OriginatedFrom) - if err != nil { - return nil, fmt.Errorf("error getting originating entity: %w", err) - } - eiw.WithID(entities.EntityTypeFromDB(dbEnt.EntityType), dbEnt.ID) - } + WithID(ewp.Entity.ID) err = eiw.ToMessage(m) if err != nil { diff --git a/internal/entities/utils.go b/internal/entities/utils.go deleted file mode 100644 index 6d5e30a311..0000000000 --- a/internal/entities/utils.go +++ /dev/null @@ -1,46 +0,0 @@ -// Copyright 2024 Stacklok, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// Package entities contains logic relating to entity management -package entities - -import ( - "errors" - - "github.com/google/uuid" - - "github.com/stacklok/minder/internal/db" -) - -// EntityFromIDs takes the IDs of the three known entity types and -// returns a single ID along with the type of the entity. -// This assumes that exactly one of the IDs is not equal to uuid.Nil -func EntityFromIDs( - repositoryID uuid.UUID, - artifactID uuid.UUID, - pullRequestID uuid.UUID, -) (uuid.UUID, db.Entities, error) { - // Note that the repo ID is often passed around with PRs. - // As a result, we test PRs and artifacts first, then repos. - if pullRequestID != uuid.Nil { - return pullRequestID, db.EntitiesPullRequest, nil - } - if artifactID != uuid.Nil { - return artifactID, db.EntitiesArtifact, nil - } - if repositoryID != uuid.Nil { - return repositoryID, db.EntitiesRepository, nil - } - return uuid.Nil, "", errors.New("all entity IDs are nil") -} diff --git a/internal/logger/telemetry_store_watermill.go b/internal/logger/telemetry_store_watermill.go index c9c6ff43d4..651ad07273 100644 --- a/internal/logger/telemetry_store_watermill.go +++ b/internal/logger/telemetry_store_watermill.go @@ -18,7 +18,6 @@ import ( "fmt" "github.com/ThreeDotsLabs/watermill/message" - "github.com/google/uuid" "github.com/rs/zerolog" "github.com/stacklok/minder/internal/engine/entities" @@ -75,11 +74,9 @@ func newTelemetryStoreFromEntity(inf *entities.EntityInfoWrapper) (*TelemetrySto // Create a new telemetry store ts := &TelemetryStore{} - // Get the entity UUID - this is the entity we are processing - ent, err := getEntityID(inf) + ent, err := inf.GetID() if err != nil { - // Return an error but also return the telemetry store so we don't fail the event - return ts, fmt.Errorf("error getting entity ID: %w", err) + return ts, fmt.Errorf("error getting ID from entity info wrapper: %w", err) } // Set the provider and project ID @@ -104,30 +101,3 @@ func newTelemetryStoreFromEntity(inf *entities.EntityInfoWrapper) (*TelemetrySto return ts, nil } - -// getEntityID returns the entity ID from the entity info wrapper based on its type. -func getEntityID(inf *entities.EntityInfoWrapper) (uuid.UUID, error) { - repoID, artID, prID := inf.GetEntityDBIDs() - - var ent uuid.UUID - - // In the case of this middleware, we receive entities - // to process by the executor. - switch inf.Type { - case minderv1.Entity_ENTITY_UNSPECIFIED: - return uuid.Nil, fmt.Errorf("unspecified entity type") - case minderv1.Entity_ENTITY_BUILD_ENVIRONMENTS: - return uuid.Nil, fmt.Errorf("build environments not supported") - case minderv1.Entity_ENTITY_REPOSITORIES: - ent = repoID.UUID - case minderv1.Entity_ENTITY_ARTIFACTS: - ent = artID.UUID - case minderv1.Entity_ENTITY_PULL_REQUESTS: - ent = prID.UUID - case minderv1.Entity_ENTITY_RELEASE, minderv1.Entity_ENTITY_PIPELINE_RUN, - minderv1.Entity_ENTITY_TASK_RUN, minderv1.Entity_ENTITY_BUILD: - // Noop, see https://github.com/stacklok/minder/issues/3838 - } - - return ent, nil -} diff --git a/internal/logger/telemetry_store_watermill_test.go b/internal/logger/telemetry_store_watermill_test.go index eff5001c83..c6c5149325 100644 --- a/internal/logger/telemetry_store_watermill_test.go +++ b/internal/logger/telemetry_store_watermill_test.go @@ -72,7 +72,7 @@ func TestTelemetryStoreWMMiddlewareLogsRepositoryInfo(t *testing.T) { Name: "test", RepoId: 123, CloneUrl: "github.com/foo/bar.git", - }).WithRepositoryID(repositoryID) + }).WithID(repositoryID) msg, err := eiw.BuildMessage() require.NoError(t, err, "expected no error")