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

Add actionable failure messaging to evaluate #264

Closed
iamkelllly opened this issue Dec 1, 2021 · 4 comments · Fixed by #286
Closed

Add actionable failure messaging to evaluate #264

iamkelllly opened this issue Dec 1, 2021 · 4 comments · Fixed by #286
Labels
dev experience Targets the developer experience
Milestone

Comments

@iamkelllly
Copy link
Contributor

While the failure messaging for evaluate is descriptive with the policy declaration, system, and rule that's being violated.

Current state:

root@fa175a43c077:/fides/fidesctl# fidesctl evaluate demo_resources
...
Executing evaluations...
{
  "status": "FAIL",
  "details": [
    "Declaration (Collect data for marketing) of System (demo_marketing_system) failed Rule (Reject Direct Marketing) from Policy (demo_privacy_policy)"
  ],
  "message": null
}

Providing actionable/meaningful error and failure messaging helps an implementer clearly know next steps to take whether they

  1. "broke" the build for first-time implementers
  2. are maintaining a fides-annotated project and have now legitimately violated a policy

This issue is to add additional logging to the details node of the failure message here to understand what exactly was failing, for example:

root@fa175a43c077:/fides/fidesctl# fidesctl evaluate demo_resources
...
Executing evaluations...
{
  "status": "FAIL",
  "details": [
    "Declaration (Collect data for marketing) of System (demo_marketing_system) failed Rule (Reject Direct Marketing) from Policy (demo_privacy_policy)" because system (demo_marketing_system) contains user.provided.identifiable.contact data
  ],
  "message": null
}

There are likely edge cases where this format might not work, but would like to explore the edges of this with the demo/quickstart repo.

@iamkelllly iamkelllly added dev experience Targets the developer experience enhancement labels Dec 1, 2021
@iamkelllly iamkelllly added this to the fidesctl 1.2 milestone Dec 1, 2021
@earmenda earmenda mentioned this issue Dec 10, 2021
8 tasks
@iamkelllly
Copy link
Contributor Author

iamkelllly commented Dec 13, 2021

discussed this with @earmenda , and @ThomasLaPiana need your eyes on the below proposal.

The purpose of this issue is to provide actionable failure messaging about why the evaluation failed, however, because the evaluation failure is due to a violating combination of privacy attributes (uses, subjects, category, qualifier) at run time, it doesn't make sense to just include any less than 4 of the attributes in the failure messaging.

So we tried putting all 4 attributes into a sentence in the "details" value in the json object, and it looks really long and unreadable. As such, wanting to update the evaluate failure messaging to be something to this tune (gisty approximation, don't use this for actual requirements here 😉 )

root@fa175a43c077:/fides/fidesctl# fidesctl evaluate resources_folder
...
Executing evaluations...
{
  "status": "FAIL",
  "violations": [
  {
    "id":1
    "violating_attributes": {
      "data_category":"user.provided.identifiable.contact data",
      "data_qualifier": "aggregated.anonymized.unlinked_pseudonymized.pseudonymized.identified"
      "data_use": "advertising"
      "data_subject": "customer"
      }
    details: "Declaration (Collect data for marketing) of System (demo_marketing_system) failed Rule (Reject Direct Marketing) from Policy (demo_privacy_policy) because it contains user.provided.identifiable.contact data for data_subject type customers and data_use advertising."
    },
  {
    "id":2
    "violating_attributes":{
      "data_category":"user.provided.identifiable.contact data",
      "data_qualifier": "aggregated.anonymized.unlinked_pseudonymized.pseudonymized.identified"
      "data_use": "advertising"
      "data_subject": "customer"
      }
    details: "Declaration (Analyze customer behavior) of System (demo_analytics_system) failed Rule (Reject Direct Marketing) from Policy (demo_privacy_policy) because it contains user.provided.identifiable.contact data for data_subject type customers and data_use advertising."
    }
  ]
}

Aaaand maybe if the CLI user can include a flag to only make this for a human user using the CLI to be able to read without the json bloat (i know aws cli uses --summarize or eduardo was saying Unix uses-h / --human-readable) , we could do something like this:

root@fa175a43c077:/fides/fidesctl# fidesctl evaluate resources_folder --summarize
Evaluation failed!
> Declaration (Collect data for marketing) of System (demo_marketing_system) failed Rule (Reject Direct Marketing) from Policy (demo_privacy_policy) because it contains user.provided.identifiable.contact data for data_subject type customers and data_use advertising.
> Declaration (Analyze customer behavior) of System (demo_analytics_system) failed Rule (Reject Direct Marketing) from Policy (demo_privacy_policy) because it contains user.provided.identifiable.contact data for data_subject type customers and data_use advertising. 

sidebar: it's worth mentioning that in the future, when/if we can support storing or comparing state of the past resource commits, we would be able to definitively point at a singular violating privacy attribute (ie: data category) within a declaration. For example, last week, a system was not using user.provided.identifiable.contact data, but after this commit, it will be; fides could then point to the exact place where the data category was declared as the reason for this evaluation's failure, where maybe no other privacy attributes changed.

I don't want to blow up the scope for this issue, but wanted to make sure we just get a few eyes on this proposal before proceeding.

@PSalant726
Copy link
Contributor

Rather than include a --summarize option to condense the output, I would rather we print the condensed output by default. We can include a -v, --verbose option to print the JSON. This feels more aligned with what I've seen in other tools.

Separately, if we so desire, we can include a "fail fast" option, to only display the first violation for a given failed evaluation. It's hard for me to say off-hand if this would actually speed up the fidesctl evaluate run time, or if it would only serve to further condense the output. In general, I don't prefer to use "fail fast" options when they're available, but that doesn't mean it shouldn't be an option here (if there's agreement, demand, etc.).

@ThomasLaPiana
Copy link
Contributor

I think people using the CLI can read JSON, so I don't see the need for another CLI flag. As long as the UI version is human-readable I think we're fine.

@iamkelllly I love the idea of having the violating attributes there!

Overall my suggestions here are:

  1. make the details more verbose as described above. It might be long but it is still very readable to me. This will be useful in the UI view so we might as well store it like that.
  2. Add the violating_attributes field

@ThomasLaPiana
Copy link
Contributor

Rather than include a --summarize option to condense the output, I would rather we print the condensed output by default. We can include a -v, --verbose option to print the JSON. This feels more aligned with what I've seen in other tools.

Separately, if we so desire, we can include a "fail fast" option, to only display the first violation for a given failed evaluation. It's hard for me to say off-hand if this would actually speed up the fidesctl evaluate run time, or if it would only serve to further condense the output. In general, I don't prefer to use "fail fast" options when they're available, but that doesn't mean it shouldn't be an option here (if there's agreement, demand, etc.).

All of the rules and entities are fetched before doing the evaluation, so wouldn't make a difference from a computational standpoint. I don't really see the need for this flag right now, with that in mind

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev experience Targets the developer experience
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants