Implement profile update #1566

JAORMX · 2023-11-07T12:40:50Z

This implements the profile update method in the minder server.

This was done last as it's kinda complex... as you can tell from the PR.

We have to verify that the new profile is valid, that certain values like the
project, name and provider don't change. Then we update the rules, and clean up
the unused ones. And keep the instantiations in check.

This, however, will be a very nice usability improvement.

JAORMX · 2023-11-08T13:24:34Z

Manual testing - toggling alerts on/off

Created a simple profile with only one rule:

version: v1
type: profile
name: acme-github-profile
context:
  provider: github
alert: "off"
remediate: "off"
repository:
  - type: secret_scanning
    def:
      enabled: true

enrolled two repositories.

Then, I updated the profile to enable alerts

version: v1
type: profile
name: acme-github-profile
context:
  provider: github
alert: "on"
remediate: "off"
repository:
  - type: secret_scanning
    def:
      enabled: true

I applied the changes with the following:

minder profile update -f <path/to/profile>

I can see the profile was updated:

minder profile list -p github -o yaml
profiles:
    - alert: "on"
      context:
        project: jaormx
        provider: github
      id: d3bc751e-9d39-47e2-9bc9-208d77f1d39b
      name: acme-github-profile
      remediate: "off"
      repository:
        - def:
            enabled: true
          type: secret_scanning

Toggling the alerts on and off subsequently works as expected.

JAORMX · 2023-11-08T13:28:52Z

Manual testing - adding rules

I added a rule to the aforementioned profile:

version: v1
type: profile
name: acme-github-profile
context:
  provider: github
alert: "on"
remediate: "off"
repository:
  - type: secret_scanning
    def:
      enabled: true
  - type: secret_push_protection
    def:
      enabled: true

The update was done as follows:

minder profile update -f examples/rules-and-profiles/profiles/github/profile.yaml                                                                                                                                                                                 (base) 
+--------------------------------------+---------------------+----------+------------+------------------------+-------------+-----------------+
|                  ID                  |        NAME         | PROVIDER |   ENTITY   |          RULE          | RULE PARAMS | RULE DEFINITION |
+--------------------------------------+---------------------+----------+------------+------------------------+-------------+-----------------+
| d3bc751e-9d39-47e2-9bc9-208d77f1d39b | acme-github-profile | github   | repository | secret_scanning        |             | enabled: true   |
|                                      |                     |          |            |                        |             |                 |
+                                      +                     +          +            +------------------------+-------------+-----------------+
|                                      |                     |          |            | secret_push_protection |             | enabled: true   |
|                                      |                     |          |            |                        |             |                 |
+--------------------------------------+---------------------+----------+------------+------------------------+-------------+-----------------+

the output of the command already suggests that the rule was added. But let's verify the status:

 minder profile_status list -p github --profile acme-github-profile --detailed                                                                                                                                                                                     (base) 
+--------------------------------------+---------------------+----------------+----------------------+
|                  ID                  |        NAME         | OVERALL STATUS |     LAST UPDATED     |
+--------------------------------------+---------------------+----------------+----------------------+
| d3bc751e-9d39-47e2-9bc9-208d77f1d39b | acme-github-profile | ✅ Success     | 2023-11-08T13:27:42Z |
+--------------------------------------+---------------------+----------------+----------------------+
+--------------------------------------+------------------------+------------+------------+--------------------+-----------------------------------------------------+----------+
|               RULE ID                |       RULE NAME        |   ENTITY   |   STATUS   | REMEDIATION STATUS |                     ENTITY INFO                     | GUIDANCE |
+--------------------------------------+------------------------+------------+------------+--------------------+-----------------------------------------------------+----------+
| aeabada4-1eb5-4a73-a7bd-19131f0b15bb | secret_scanning        | repository | ✅ Success |                    | provider: github                                    | 👍       |
|                                      |                        |            |            |                    | repo_name: auditevent                               |          |
|                                      |                        |            |            |                    | repo_owner: JAORMX                                  |          |
|                                      |                        |            |            |                    | repository_id: 4f2e4a92-e85c-4e04-9831-f66125d4838c |          |
|                                      |                        |            |            |                    |                                                     |          |
+--------------------------------------+------------------------+------------+------------+--------------------+-----------------------------------------------------+----------+
| 4f744d8b-02fc-4bd0-b5c9-d853481b186e | secret_push_protection | repository | ✅ Success |                    | provider: github                                    | 👍       |
|                                      |                        |            |            |                    | repo_name: auditevent                               |          |
|                                      |                        |            |            |                    | repo_owner: JAORMX                                  |          |
|                                      |                        |            |            |                    | repository_id: 4f2e4a92-e85c-4e04-9831-f66125d4838c |          |
|                                      |                        |            |            |                    |                                                     |          |
+--------------------------------------+------------------------+------------+------------+--------------------+-----------------------------------------------------+----------+
| aeabada4-1eb5-4a73-a7bd-19131f0b15bb | secret_scanning        | repository | ✅ Success |                    | provider: github                                    | 👍       |
|                                      |                        |            |            |                    | repo_name: audito-maldito                           |          |
|                                      |                        |            |            |                    | repo_owner: JAORMX                                  |          |
|                                      |                        |            |            |                    | repository_id: 99f82173-ecde-4764-a5ad-0391cb386837 |          |
|                                      |                        |            |            |                    |                                                     |          |
+--------------------------------------+------------------------+------------+------------+--------------------+-----------------------------------------------------+----------+
| 4f744d8b-02fc-4bd0-b5c9-d853481b186e | secret_push_protection | repository | ✅ Success |                    | provider: github                                    | 👍       |
|                                      |                        |            |            |                    | repo_name: audito-maldito                           |          |
|                                      |                        |            |            |                    | repo_owner: JAORMX                                  |          |
|                                      |                        |            |            |                    | repository_id: 99f82173-ecde-4764-a5ad-0391cb386837 |          |
|                                      |                        |            |            |                    |                                                     |          |
+--------------------------------------+------------------------+------------+------------+--------------------+-----------------------------------------------------+----------+

As we can see, it adds the new rule as expected.

Also note that the usage of these rules was persisted in the database:

id                                  |entity_profile_id                   |rule_type_id                        |created_at             |
------------------------------------+------------------------------------+------------------------------------+-----------------------+
a4786352-96bb-41f8-b040-4bbd25f0d410|f74d3451-8989-4928-82a1-4912bc996d67|aeabada4-1eb5-4a73-a7bd-19131f0b15bb|2023-11-08 13:19:53.100|
9b2376c0-e2d8-4365-baed-76ce77a78dab|f74d3451-8989-4928-82a1-4912bc996d67|4f744d8b-02fc-4bd0-b5c9-d853481b186e|2023-11-08 13:27:42.030|

As a final test, I added the dependabot_configured rule to the list, which also happened successfully.

JAORMX · 2023-11-08T13:48:52Z

Manual testing - removing rules

With the aforementioned modified profile, I decided to remove the rule I just added. Which seemed to happen successfully:

minder profile update -f examples/rules-and-profiles/profiles/github/profile.yaml                                                                                                                                                                                 (base) 
+--------------------------------------+---------------------+----------+------------+-----------------------+-------------+---------------------------+
|                  ID                  |        NAME         | PROVIDER |   ENTITY   |         RULE          | RULE PARAMS |      RULE DEFINITION      |
+--------------------------------------+---------------------+----------+------------+-----------------------+-------------+---------------------------+
| d3bc751e-9d39-47e2-9bc9-208d77f1d39b | acme-github-profile | github   | repository | secret_scanning       |             | enabled: true             |
|                                      |                     |          |            |                       |             |                           |
+                                      +                     +          +            +-----------------------+-------------+---------------------------+
|                                      |                     |          |            | dependabot_configured |             | apply_if_file: go.mod     |
|                                      |                     |          |            |                       |             | package_ecosystem: gomod  |
|                                      |                     |          |            |                       |             | schedule_interval: weekly |
|                                      |                     |          |            |                       |             |                           |
+--------------------------------------+---------------------+----------+------------+-----------------------+-------------+---------------------------+

Upon further inspection, it seems that we don't remove the rule evaluations from the status.

This has to be fixed

We also don't yet remove the usage from the entity_profile_rules table.

This also has to be fixed

Further work is required. I'll keep this up-to-date.

JAORMX · 2023-11-08T14:49:24Z

The latest PR ensures that we can also remove instantiation of the rules. This works and reflects in the database.

What's missing is cleaning up rule evaluations.

JAORMX · 2023-11-08T15:30:55Z

The latest commit fixes the last issues I saw.

JAORMX · 2023-11-08T15:31:57Z

Found another issue, wait up:

{"Timestamp":1699457492465494829,"message":"2023/11/08 15:31:32 error updating profile for entity ENTITY_ARTIFACTS: sql: no rows in result set"}
{"level":"error","Resource":{"service":"minder.v1.ProfileService","method":"UpdateProfile"},"Attributes":{"http.code":"Unknown","http.content-type":["application/grpc"],"http.duration":"22.449842ms","http.user_agent":["minder-cli/v0.0.14+ref.2ddb9373 grpc-go/1.59.0"],"exception.message":"sql: no rows in result set"},"Timestamp":1699457492465941251}

JAORMX · 2023-11-08T17:05:47Z

Alright! This is a summary of what was missing:

We didn't have a unique index that would allow us to ensure we only have a single entity+profile entity_profile row... I know it sounds weird but we didn't. I added that, and now any updates to unexistent entity+profile pairs are handled as an upsert. So, this works now and ensures data integrity too!

I had to use a CONCURRENTLY for the index creation to ensure this works well on an upgrade in the live environment.

JAORMX · 2023-11-08T17:07:05Z

I rebased and added missing licenses

jhrozek

Overall seems to work well, some nits and questions inline

database/query/profiles.sql

jhrozek · 2023-11-09T12:13:07Z

internal/controlplane/handlers_profile.go

-	profiles, err := s.store.GetProfileByProjectAndID(ctx, db.GetProfileByProjectAndIDParams{
+	prof, err := getProfilePBFromDB(ctx, parsedProfileID, entityCtx, s.store)
+	if err != nil {
+		if errors.Is(err, sql.ErrNoRows) || strings.Contains(err.Error(), "not found") {


when do we get the second case? Wouldn't it be better to catch that issue by Postgres error code if that error comes directly from Postgres?

the "not found" is not a postgres error. I just didn't create a specialized error for this 😕

internal/controlplane/handlers_profile.go

database/query/profiles.sql

JAORMX · 2023-11-09T14:48:15Z

@rdimitrov and I are gonna go through this tomorrow

rdimitrov

Holding off this until tomorrow since there are a few issues left to fix 👍

This implements the profile update method in the minder server. This was done last as it's kinda complex... as you can tell from the PR. We have to verify that the new profile is valid, that certain values like the project, name and provider don't change. Then we update the rules, and clean up the unused ones. And keep the instantiations in check. This, however, will be a very nice usability improvement. This also adds a profile update command

JAORMX · 2023-11-10T14:48:55Z

done! Now this is up with the latest changes.

rdimitrov

Alright, I've given it a go again today trying various scenarios but I did not found anything worrying so far 🚀

JAORMX changed the title ~~Implement profile update~~ WIP: Implement profile update Nov 7, 2023

JAORMX force-pushed the profile-update branch 3 times, most recently from 0bbc9a1 to 1dde87f Compare November 7, 2023 13:39

JAORMX changed the title ~~WIP: Implement profile update~~ Implement profile update Nov 7, 2023

JAORMX force-pushed the profile-update branch from 1dde87f to ba7bb62 Compare November 7, 2023 13:46

JAORMX requested review from rdimitrov and jhrozek November 7, 2023 14:24

JAORMX force-pushed the profile-update branch from ba7bb62 to f81c523 Compare November 8, 2023 14:48

JAORMX force-pushed the profile-update branch from 69143d1 to 3d287c1 Compare November 8, 2023 17:06

jhrozek reviewed Nov 9, 2023

View reviewed changes

JAORMX force-pushed the profile-update branch from 3d287c1 to 3783904 Compare November 9, 2023 12:59

JAORMX requested a review from jhrozek November 9, 2023 13:05

JAORMX force-pushed the profile-update branch 2 times, most recently from 6a88d39 to e481648 Compare November 9, 2023 13:09

jhrozek previously approved these changes Nov 9, 2023

View reviewed changes

JAORMX changed the title ~~Implement profile update~~ WIP: Implement profile update Nov 9, 2023

rdimitrov requested changes Nov 9, 2023

View reviewed changes

JAORMX dismissed jhrozek’s stale review via bf9cc6c November 10, 2023 13:13

JAORMX force-pushed the profile-update branch from e481648 to bf9cc6c Compare November 10, 2023 13:13

JAORMX force-pushed the profile-update branch from bf9cc6c to 28a9c1c Compare November 10, 2023 14:42

JAORMX changed the title ~~WIP: Implement profile update~~ Implement profile update Nov 10, 2023

JAORMX requested review from jhrozek and rdimitrov November 13, 2023 07:41

rdimitrov approved these changes Nov 13, 2023

View reviewed changes

jhrozek approved these changes Nov 13, 2023

View reviewed changes

JAORMX merged commit c5b0767 into main Nov 13, 2023
15 checks passed

JAORMX deleted the profile-update branch November 13, 2023 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement profile update #1566

Implement profile update #1566

JAORMX commented Nov 7, 2023

JAORMX commented Nov 8, 2023 •

edited

Loading

JAORMX commented Nov 8, 2023 •

edited

Loading

JAORMX commented Nov 8, 2023

JAORMX commented Nov 8, 2023

JAORMX commented Nov 8, 2023

JAORMX commented Nov 8, 2023

JAORMX commented Nov 8, 2023

JAORMX commented Nov 8, 2023

jhrozek left a comment

jhrozek Nov 9, 2023

JAORMX Nov 9, 2023

JAORMX commented Nov 9, 2023

rdimitrov left a comment

JAORMX commented Nov 10, 2023

rdimitrov left a comment

Implement profile update #1566

Implement profile update #1566

Conversation

JAORMX commented Nov 7, 2023

JAORMX commented Nov 8, 2023 • edited Loading

Manual testing - toggling alerts on/off

JAORMX commented Nov 8, 2023 • edited Loading

Manual testing - adding rules

JAORMX commented Nov 8, 2023

Manual testing - removing rules

JAORMX commented Nov 8, 2023

JAORMX commented Nov 8, 2023

JAORMX commented Nov 8, 2023

JAORMX commented Nov 8, 2023

JAORMX commented Nov 8, 2023

jhrozek left a comment

Choose a reason for hiding this comment

jhrozek Nov 9, 2023

Choose a reason for hiding this comment

JAORMX Nov 9, 2023

Choose a reason for hiding this comment

JAORMX commented Nov 9, 2023

rdimitrov left a comment

Choose a reason for hiding this comment

JAORMX commented Nov 10, 2023

rdimitrov left a comment

Choose a reason for hiding this comment

JAORMX commented Nov 8, 2023 •

edited

Loading

JAORMX commented Nov 8, 2023 •

edited

Loading