Skip to content

Commit

Permalink
Store properties for artifacts (#4345)
Browse files Browse the repository at this point in the history 
* Add isOrg to the property wrapper signature

The wrapper for artifacts will call a different github provider call
depending on whether we need to fetch an property for a user artifact or
an organizational artifact. Let's extend the wrapper with a bool to
allow that.

Related: #4339

* Add property fetching for artifacts

Adds the properties that will be fetched and stored for artifacts as
well as the wrapper fetching it.

Fixes: #4339

* Use properties for artifacts in the github webhook

Amends the webhook code to refresh artifact properties in the webhook
handler and store them for later use.

Fixes: #4339
  • Loading branch information
@jhrozek
jhrozek authored Sep 3, 2024
1 parent 095ccd8 commit 5428799
Show file tree
Hide file tree
Showing 7 changed files with 316 additions and 96 deletions.
142 changes: 63 additions & 79 deletions internal/controlplane/handlers_githubwebhooks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"mime"
"net/http"
"sort"
"strconv"
"strings"

"github.com/ThreeDotsLabs/watermill/message"
Expand All @@ -35,7 +36,6 @@ import (
"github.com/rs/zerolog"
"google.golang.org/protobuf/types/known/timestamppb"

"github.com/stacklok/minder/internal/artifacts"
"github.com/stacklok/minder/internal/config/server"
"github.com/stacklok/minder/internal/controlplane/metrics"
"github.com/stacklok/minder/internal/db"
Expand Down Expand Up @@ -118,6 +118,7 @@ type packageEvent struct {
}

type pkg struct {
ID *int64 `json:"id,omitempty"`
Name *string `json:"name,omitempty"`
PackageType *string `json:"package_type,omitempty"`
PackageVersion *packageVersion `json:"package_version,omitempty"`
Expand Down Expand Up @@ -711,19 +712,27 @@ func (s *Server) processPackageEvent(
return nil, err
}

cli, err := provifv1.As[provifv1.GitHub](provider)
pkgLookupProps, err := packageEventToProperties(event)
if err != nil {
l.Error().Err(err).Msg("error instantiating provider")
return nil, err
return nil, fmt.Errorf("error converting package event to properties: %w", err)
}

tempArtifact, err := gatherArtifact(ctx, cli, event)
pkgName, err := provider.GetEntityName(pb.Entity_ENTITY_ARTIFACTS, pkgLookupProps)
if err != nil {
return nil, fmt.Errorf("error getting package name: %w", err)
}

// we do two property lookups here: this first one will go away once we migrate artifacts to entities
// as the only reason is to have the visibility and type of the artifact available.
refreshedPkgProperties, err := s.props.RetrieveAllProperties(
ctx, provider,
repoEnt.Entity.ProjectID, repoEnt.Entity.ProviderID,
pkgLookupProps, pb.Entity_ENTITY_ARTIFACTS)
if err != nil {
return nil, fmt.Errorf("error gathering versioned artifact: %w", err)
return nil, fmt.Errorf("error retrieving properties: %w", err)
}

var artifactID uuid.UUID
pbArtifact, err := db.WithTransaction(s.store, func(tx db.ExtendQuerier) (*pb.Artifact, error) {
ei, err := db.WithTransaction(s.store, func(tx db.ExtendQuerier) (*db.EntityInstance, error) {
// TODO: remove this once we migrate artifacts to entities. We should get rid of the provider name.
dbProv, getPrErr := tx.GetProviderByID(ctx, repoEnt.Entity.ProviderID)
if getPrErr != nil {
Expand All @@ -735,9 +744,9 @@ func (s *Server) processPackageEvent(
UUID: repoEnt.Entity.ID,
Valid: true,
},
ArtifactName: tempArtifact.GetName(),
ArtifactType: tempArtifact.GetTypeLower(),
ArtifactVisibility: tempArtifact.Visibility,
ArtifactName: refreshedPkgProperties.GetProperty(ghprop.ArtifactPropertyName).GetString(),
ArtifactType: refreshedPkgProperties.GetProperty(ghprop.ArtifactPropertyType).GetString(),
ArtifactVisibility: refreshedPkgProperties.GetProperty(ghprop.ArtifactPropertyVisibility).GetString(),
ProjectID: repoEnt.Entity.ProjectID,
ProviderID: repoEnt.Entity.ProviderID,
ProviderName: dbProv.Name,
Expand All @@ -746,31 +755,10 @@ func (s *Server) processPackageEvent(
return nil, fmt.Errorf("error upserting artifact: %w", err)
}

_, pba, err := artifacts.GetArtifact(ctx, tx, repoEnt.Entity.ProjectID, dbArtifact.ID)
if err != nil {
return nil, fmt.Errorf("error getting artifact with versions: %w", err)
}
// TODO: wrap in a function
pba.Versions = tempArtifact.Versions

artifactID = dbArtifact.ID

// name is provider specific and should be based on properties.
// In github's case it's lowercase owner / artifact name
// TODO: Replace with a provider call to get
// a name based on properties.
var prefix string
if tempArtifact.GetOwner() != "" {
prefix = tempArtifact.GetOwner() + "/"
}

// At this point the package name has been checked.
artName := prefix + *event.Package.Name

_, err = tx.CreateOrEnsureEntityByID(ctx, db.CreateOrEnsureEntityByIDParams{
ent, err := tx.CreateOrEnsureEntityByID(ctx, db.CreateOrEnsureEntityByIDParams{
ID: dbArtifact.ID,
EntityType: db.EntitiesArtifact,
Name: artName,
Name: pkgName,
ProjectID: repoEnt.Entity.ProjectID,
ProviderID: repoEnt.Entity.ProviderID,
OriginatedFrom: uuid.NullUUID{
Expand All @@ -782,16 +770,46 @@ func (s *Server) processPackageEvent(
return nil, fmt.Errorf("error creating or ensuring entity: %w", err)
}

return pba, nil
return &ent, nil
})
if err != nil {
return nil, err
}

// fetch the properties
refreshedPkgProperties, err = s.props.RetrieveAllProperties(
ctx, provider,
ei.ProjectID, ei.ProviderID,
refreshedPkgProperties, pb.Entity_ENTITY_ARTIFACTS)
if err != nil {
return nil, fmt.Errorf("error retrieving properties: %w", err)
}

// refresh the version to attach it to the pb representation we send to the evaluation
cli, err := provifv1.As[provifv1.GitHub](provider)
if err != nil {
l.Error().Err(err).Msg("error instantiating provider")
return nil, err
}

version, err := gatherArtifactVersionInfo(ctx, cli, event,
refreshedPkgProperties.GetProperty(ghprop.ArtifactPropertyOwner).GetString(),
refreshedPkgProperties.GetProperty(ghprop.ArtifactPropertyName).GetString(),
)
if err != nil {
return nil, fmt.Errorf("error extracting artifact from payload: %w", err)
}

pbArtifact, err := ghprop.ArtifactV1FromProperties(refreshedPkgProperties)
if err != nil {
return nil, fmt.Errorf("error converting artifact to protobuf: %w", err)
}
pbArtifact.Versions = []*pb.ArtifactVersion{version}

eiw := entities.NewEntityInfoWrapper().
WithActionEvent(*event.Action).
WithArtifact(pbArtifact).
WithArtifactID(artifactID).
WithArtifactID(ei.ID).
WithProjectID(repoEnt.Entity.ProjectID).
WithProviderID(repoEnt.Entity.ProviderID).
WithRepositoryID(repoEnt.Entity.ID)
Expand Down Expand Up @@ -1368,11 +1386,9 @@ func handleParseError(ctx context.Context, typ string, parseErr error) *metrics.
// This routine assumes that all necessary validation is performed on
// the upper layer and accesses package and repo without checking for
// nulls.
func gatherArtifactInfo(
ctx context.Context,
client provifv1.GitHub,
func packageEventToProperties(
event *packageEvent,
) (*pb.Artifact, error) {
) (*properties.Properties, error) {
if event.Repo.GetFullName() == "" {
return nil, errors.New("invalid package: full name is nil")
}
Expand All @@ -1388,27 +1404,13 @@ func gatherArtifactInfo(
owner = event.Package.Owner.GetLogin()
}

artifact := &pb.Artifact{
Owner: owner,
Name: *event.Package.Name,
Type: *event.Package.PackageType,
Repository: event.Repo.GetFullName(),
// visibility and createdAt are not in the payload, we need to get it with a REST call
}

// we also need to fill in the visibility which is not in the payload
ghArtifact, err := client.GetPackageByName(
ctx,
artifact.Owner,
string(verifyif.ArtifactTypeContainer),
artifact.Name,
)
if err != nil {
return nil, fmt.Errorf("error extracting artifact from repo: %w", err)
}

artifact.Visibility = *ghArtifact.Visibility
return artifact, nil
return properties.NewProperties(map[string]any{
properties.PropertyUpstreamID: strconv.FormatInt(*event.Package.ID, 10),
// we need these to look up the package properties
ghprop.ArtifactPropertyOwner: owner,
ghprop.ArtifactPropertyName: *event.Package.Name,
ghprop.ArtifactPropertyType: strings.ToLower(*event.Package.PackageType),
})
}

// This routine assumes that all necessary validation is performed on
Expand Down Expand Up @@ -1462,24 +1464,6 @@ func gatherArtifactVersionInfo(
return version, nil
}

func gatherArtifact(
ctx context.Context,
cli provifv1.GitHub,
event *packageEvent,
) (*pb.Artifact, error) {
artifact, err := gatherArtifactInfo(ctx, cli, event)
if err != nil {
return nil, fmt.Errorf("error gatherinfo artifact info: %w", err)
}

version, err := gatherArtifactVersionInfo(ctx, cli, event, artifact.Owner, artifact.Name)
if err != nil {
return nil, fmt.Errorf("error extracting artifact from payload: %w", err)
}
artifact.Versions = []*pb.ArtifactVersion{version}
return artifact, nil
}

func updateArtifactVersionFromRegistry(
ctx context.Context,
client provifv1.GitHub,
Expand Down
53 changes: 41 additions & 12 deletions internal/controlplane/handlers_githubwebhooks_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,12 @@ func newPropSvcMock(opts ...func(mck propSvcMock)) propSvcMockBuilder {
}
}

func withSuccessRetrieveAllProperties(entity v1.Entity, retProps *properties.Properties) func(mck propSvcMock) {
func withSuccessRetrieveAllProperties(entity v1.Entity, retPropsMap map[string]any) func(mck propSvcMock) {
retProps, err := properties.NewProperties(retPropsMap)
if err != nil {
panic(err)
}

return func(mock propSvcMock) {
mock.EXPECT().
RetrieveAllProperties(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), entity).
Expand Down Expand Up @@ -564,7 +569,6 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() {
repositoryID := uuid.New()
projectID := uuid.New()
providerID := uuid.New()
artifactID := uuid.New()
visibility := "visibility"

tests := []struct {
Expand Down Expand Up @@ -638,6 +642,7 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() {
payload: &packageEvent{
Action: github.String("published"),
Package: &pkg{
ID: github.Int64(123),
Name: github.String("package-name"),
PackageType: github.String("package-type"),
// .package.package_version.container_metadata.tag.name
Expand All @@ -661,6 +666,23 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() {
"https://github.com/stacklok/minder",
),
},
mockPropsBld: newPropSvcMock(
withSuccessRetrieveAllProperties(v1.Entity_ENTITY_ARTIFACTS, map[string]any{
properties.PropertyUpstreamID: "123",
ghprop.ArtifactPropertyOwner: "login",
ghprop.ArtifactPropertyName: "package-name",
ghprop.ArtifactPropertyCreatedAt: "2024-05-22T07:35:16Z",
}),
withSuccessRetrieveAllProperties(v1.Entity_ENTITY_ARTIFACTS, map[string]any{
properties.PropertyUpstreamID: "123",
ghprop.ArtifactPropertyOwner: "login",
ghprop.ArtifactPropertyName: "package-name",
ghprop.ArtifactPropertyCreatedAt: "2024-05-22T07:35:16Z",
}),
),
ghMocks: []func(hubMock gf.GitHubMock){
gf.WithSuccessfulGetEntityName("login/package-name"),
},
mockRepoBld: newRepoSvcMock(
withSuccessRepoById(
models.EntityInstance{
Expand All @@ -687,11 +709,6 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() {
},
providerID,
),
df.WithSuccessfulGetArtifactByID(
db.Artifact{
ID: artifactID,
},
),
df.WithSuccessfulUpsertArtifact(
db.Artifact{
ID: uuid.New(),
Expand Down Expand Up @@ -720,6 +737,23 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() {
event: "package",
// https://pkg.go.dev/github.com/google/go-github/[email protected]/github#PackageEvent
rawPayload: []byte(rawPackageEventPublished),
mockPropsBld: newPropSvcMock(
withSuccessRetrieveAllProperties(v1.Entity_ENTITY_ARTIFACTS, map[string]any{
properties.PropertyUpstreamID: "12345",
ghprop.ArtifactPropertyOwner: "stacklok",
ghprop.ArtifactPropertyName: "demo-repo-go-debug",
ghprop.ArtifactPropertyCreatedAt: "2024-05-22T07:35:16Z",
}),
withSuccessRetrieveAllProperties(v1.Entity_ENTITY_ARTIFACTS, map[string]any{
properties.PropertyUpstreamID: "12345",
ghprop.ArtifactPropertyOwner: "stacklok",
ghprop.ArtifactPropertyName: "demo-repo-go-debug",
ghprop.ArtifactPropertyCreatedAt: "2024-05-22T07:35:16Z",
}),
),
ghMocks: []func(hubMock gf.GitHubMock){
gf.WithSuccessfulGetEntityName("stacklok/minder"),
},
mockRepoBld: newRepoSvcMock(
withSuccessRepoById(
models.EntityInstance{
Expand All @@ -746,11 +780,6 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() {
},
providerID,
),
df.WithSuccessfulGetArtifactByID(
db.Artifact{
ID: artifactID,
},
),
df.WithSuccessfulUpsertArtifact(
db.Artifact{
ID: uuid.New(),
Expand Down
4 changes: 2 additions & 2 deletions internal/providers/github/properties.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func (c *GitHub) FetchProperty(
return nil, fmt.Errorf("property %s not supported for entity %s", key, entType)
}

props, err := wrapper(ctx, c.client, getByProps)
props, err := wrapper(ctx, c.client, c.IsOrg(), getByProps)
if err != nil {
return nil, fmt.Errorf("error fetching property %s for entity %s: %w", key, entType, err)
}
Expand All @@ -66,7 +66,7 @@ func (c *GitHub) FetchAllProperties(
fetcher := c.propertyFetchers.EntityPropertyFetcher(entType)
result := make(map[string]any)
for _, wrapper := range fetcher.AllPropertyWrappers() {
props, err := wrapper(ctx, c.client, getByProps)
props, err := wrapper(ctx, c.client, c.IsOrg(), getByProps)
if err != nil {
return nil, fmt.Errorf("error fetching properties for entity %s: %w", entType, err)
}
Expand Down
Loading

0 comments on commit 5428799

Please sign in to comment.