-
Notifications
You must be signed in to change notification settings - Fork 54
Implement RegisterEntity generic API endpoint #5959
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
Changes from 4 commits
bced660
7c13f96
4d73f07
2ba6e8f
53e1baa
a65a799
e53b022
fd72c6c
751c7b7
45ef1d0
6c8248b
f279976
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,11 +11,14 @@ import ( | |
|
|
||
| "github.com/google/uuid" | ||
| "google.golang.org/grpc/codes" | ||
| "google.golang.org/protobuf/proto" | ||
|
|
||
| "github.com/mindersec/minder/internal/engine/engcontext" | ||
| "github.com/mindersec/minder/internal/entities/models" | ||
| "github.com/mindersec/minder/internal/logger" | ||
| "github.com/mindersec/minder/internal/util" | ||
| pb "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1" | ||
| "github.com/mindersec/minder/pkg/entities/properties" | ||
| ) | ||
|
|
||
| // ListEntities returns a list of entity instances for a given project and provider | ||
|
|
@@ -181,3 +184,110 @@ func (s *Server) DeleteEntityById( | |
| Id: in.GetId(), | ||
| }, nil | ||
| } | ||
|
|
||
| // RegisterEntity creates a new entity instance | ||
| func (s *Server) RegisterEntity( | ||
| ctx context.Context, | ||
| in *pb.RegisterEntityRequest, | ||
| ) (*pb.RegisterEntityResponse, error) { | ||
| // 1. Extract context information | ||
| entityCtx := engcontext.EntityFromContext(ctx) | ||
| projectID := entityCtx.Project.ID | ||
| providerName := entityCtx.Provider.Name | ||
|
|
||
| logger.BusinessRecord(ctx).Provider = providerName | ||
| logger.BusinessRecord(ctx).Project = projectID | ||
|
|
||
| // 2. Validate entity type | ||
| if in.GetEntityType() == pb.Entity_ENTITY_UNSPECIFIED { | ||
| return nil, util.UserVisibleError(codes.InvalidArgument, | ||
| "entity_type must be specified") | ||
| } | ||
|
|
||
| // 3. Parse identifying properties | ||
| identifyingProps, err := parseIdentifyingProperties(in) | ||
| if err != nil { | ||
| return nil, util.UserVisibleError(codes.InvalidArgument, | ||
| "invalid identifying_properties: %v", err) | ||
| } | ||
|
|
||
| // 4. Get provider from database | ||
| provider, err := s.providerStore.GetByName(ctx, projectID, providerName) | ||
| if err != nil { | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return nil, util.UserVisibleError(codes.NotFound, "provider not found") | ||
| } | ||
| return nil, util.UserVisibleError(codes.Internal, "cannot get provider: %v", err) | ||
| } | ||
|
|
||
| // 5. Create entity using EntityCreator service | ||
| ewp, err := s.entityCreator.CreateEntity(ctx, provider, projectID, | ||
| in.GetEntityType(), identifyingProps, nil) // Use default options | ||
| if err != nil { | ||
| // If the error is already a UserVisibleError, pass it through directly. | ||
| // This allows providers and EntityCreator to add user-visible errors | ||
| // without needing to update this allow-list. | ||
| var userErr *util.NiceStatus | ||
| if errors.As(err, &userErr) { | ||
| return nil, err | ||
| } | ||
| return nil, util.UserVisibleError(codes.Internal, | ||
| "unable to register entity: %v", err) | ||
|
Comment on lines
+227
to
+235
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI, I found this pattern elsewhere, and I plan to fix it so that we can write this as: And then the interceptor wrapper will use There's nothing to do here at the moment, though.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On second thought, laundering all errors as user-visible internal errors is probably wrong: This is both gobbledygook from a normal user's point of view, and risks leaking internal details in other cases. I'm also realizing that I didn't need to specify a provider in my call, which seems surprising? |
||
| } | ||
|
|
||
| // 6. Convert to EntityInstance protobuf | ||
| entityInstance := entityInstanceToProto(ewp, providerName) | ||
|
|
||
| // 7. Return response | ||
| return &pb.RegisterEntityResponse{ | ||
| Entity: entityInstance, | ||
| }, nil | ||
| } | ||
|
|
||
| // parseIdentifyingProperties converts proto properties to Properties object | ||
| func parseIdentifyingProperties(req *pb.RegisterEntityRequest) (*properties.Properties, error) { | ||
| identifyingProps := req.GetIdentifyingProperties() | ||
| if len(identifyingProps) == 0 { | ||
| return nil, errors.New("identifying_properties is required") | ||
| } | ||
|
|
||
| // Validate total size to prevent resource exhaustion | ||
| // We sum the proto.Size of each value since map itself isn't a proto message | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is fine, but there are a couple ways to simplify at the cost of precision:
|
||
| const maxProtoSize = 32 * 1024 // 32KB should be plenty for identifying properties | ||
| var totalSize int | ||
| for _, v := range identifyingProps { | ||
| totalSize += proto.Size(v) | ||
| } | ||
| if totalSize > maxProtoSize { | ||
| return nil, fmt.Errorf("identifying_properties too large: %d bytes, max %d bytes", | ||
| totalSize, maxProtoSize) | ||
| } | ||
|
|
||
| // Convert map[string]*structpb.Value to map[string]any | ||
| propsMap := make(map[string]any, len(identifyingProps)) | ||
| for key, value := range identifyingProps { | ||
| // Validate property keys are reasonable length | ||
| if len(key) > 200 { | ||
| return nil, fmt.Errorf("property key too long: %d characters", len(key)) | ||
| } | ||
| if value != nil { | ||
| propsMap[key] = value.AsInterface() | ||
| } | ||
| } | ||
|
|
||
| return properties.NewProperties(propsMap), nil | ||
| } | ||
|
|
||
| // entityInstanceToProto converts EntityWithProperties to EntityInstance protobuf | ||
| func entityInstanceToProto(ewp *models.EntityWithProperties, providerName string) *pb.EntityInstance { | ||
| return &pb.EntityInstance{ | ||
| Id: ewp.Entity.ID.String(), | ||
| Context: &pb.ContextV2{ | ||
| ProjectId: ewp.Entity.ProjectID.String(), | ||
| Provider: providerName, | ||
| }, | ||
| Type: ewp.Entity.Type, | ||
| Name: ewp.Entity.Name, | ||
| // Properties are intentionally omitted - use GetEntityById to fetch them | ||
JAORMX marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this path with
niloptions needs to work, can we remove the need for options fromCreateEntityaltogether?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Options are needed for originated entities (artifacts, pull requests) that need to pass the parent entity ID via
OriginatingEntityID. The nil check provides sensible defaults for the common case (direct RegisterEntity calls), but internal code can override when needed (e.g., AddOriginatingEntityStrategy passes the parent repo ID).