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

Take properties into use executor to fill selectors' properties #4377

Merged
merged 6 commits into from
Sep 5, 2024

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Sep 5, 2024

Summary

Adds a lookup of an entity with properties to the evaluation engine so
that the resulting selectorEntity is augmented with properties and
passed to the selector engine. Changes the conversion interfaces to
selector entities to only use the EntityWithProperties model.

As a result, the user can use properties in selectors such as:

- description: This should not apply copyleft repos
	entity: repository
	selector: repository.properties['github/license'] != 'GPL'

And in future allow us to extend selectors based on any other properties
we add.

Changes the ruletype test command to not use an EntityInfoWrapper as
intput, but just properties. An example of a repo entity with properties
is:

name: jakubtestorg/bad-python
is_private: false
is_fork: false
is_archived: false
upstream_id: "706108924"
github/repo_id:
	minder.internal.type: int64
	minder.internal.value: "706108924"
github/repo_name: bad-python
github/repo_owner: jakubtestorg
github/default_branch: master
foo: bar

Fixes: #4173

  • GetEntityByID only accepts an ID since it's unique - We can simplify the GetEntityByID call to not accept providerID since the call searches on the globally-unique and indexed entity ID.
  • Add GetEntityWithPropertiesByID to our entity model package - Adds a convenience function to search an entity by ID with all its properties. Also moves two utility functions form the property service here.
  • Add ToProtoStruct to properties - This utility function will allow us to use properties both in selectors that use the structupb.Struct and in the general entity protobuf message that we added recently.
  • Add an option to NewProperties to skip checking the internal prefix - This is so unit tests can set int64 properties.

Change Type

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

mostly manually. I need to really go and add some selectors.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

We can simplify the GetEntityByID call to not accept providerID since
the call searches on the globally-unique and indexed entity ID.

Related: mindersec#4173
Adds a convenience function to search an entity by ID with all its
properties. Also moves two utility functions form the property service
here.

Related: mindersec#4173
This utility function will allow us to use properties both in selectors
that use the structupb.Struct and in the general entity protobuf message
that we added recently.

Fixes: mindersec#4173
This is so unit tests can set int64 properties.

Related: mindersec#4173
Changes the ruletype test command to not use an EntityInfoWrapper as
intput, but just properties. An example of a repo entity with properties
is:
```
name: jakubtestorg/bad-python
is_private: false
is_fork: false
is_archived: false
upstream_id: "706108924"
github/repo_id:
  minder.internal.type: int64
    minder.internal.value: "706108924"
github/repo_name: bad-python
github/repo_owner: jakubtestorg
github/default_branch: master
foo: bar
```

Adds a lookup of an entity with properties to the evaluation engine so
that the resulting selectorEntity is augmented with properties and
passed to the selector engine. Changes the conversion interfaces to
selector entities to only use the EntityWithProperties model.

As a result, the user can use properties in selectors such as:
```
  - description: This should not apply copyleft repos
    entity: repository
    selector: repository.properties['github/license'] != 'GPL'
```
And in future allow us to extend selectors based on any other properties
we add.

Fixes: mindersec#4173
@@ -1,44 +0,0 @@
//
// Copyright 2024 Stacklok, Inc.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a follow-up, I'm going to remove the provider-specific interface and just move the repo,PR and artifact conversion to provider-generic code because with the addition of properties, we don't really need anything provider-specific.

@@ -61,6 +64,54 @@ func NewEntityWithPropertiesFromInstance(entity EntityInstance, props *propertie
}
}

// GetEntityWithPropertiesByID retrieves an EntityWithProperties instance by its ID.
func GetEntityWithPropertiesByID(ctx context.Context, q db.Querier, id uuid.UUID) (*EntityWithProperties, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually would have thought this would be part of the properties service. This is not a problem, I'm just figuring out what goes where. It's handy and I would immediately take it into use in my current PR I'm working on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually you are right, the properties service already has a Querier interface and we already also have the pattern of passing an ExtendQuerier to run the method in a transaction. Feel free to move the method there (or I can do it)

JAORMX
JAORMX previously approved these changes Sep 5, 2024
@coveralls
Copy link

coveralls commented Sep 5, 2024

Coverage Status

coverage: 52.746% (+0.09%) from 52.658%
when pulling 4c5e17c on jhrozek:props_selector
into 063e0ca on stacklok:main.


// WithSkipPrefixCheckTestOnly returns an option to skip checking the prefix
// This should only be used for testing purposes
func WithSkipPrefixCheckTestOnly() newPropertiesOption {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is for testing purposes only, let's move it to the test file itself. It should be fine as it's part of the same package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, i didn't realize they are the same package. I'll do that, but in a follow-up because I need this PR to build upon.

@jhrozek jhrozek merged commit 2056a62 into mindersec:main Sep 5, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hook properties fetching in executor
3 participants