search

kubeshop / vscode-monokle

An extension for Visual Studio Code to validate your Kubernetes configuration
https://marketplace.visualstudio.com/items?itemName=kubeshop.monokle
MIT License
6 stars 0 forks source link

Introduce annotation based suppressions #87

Closed f1ames closed 7 months ago

f1ames commented 7 months ago

This PR introduces annotation based suppressions. Fixes https://github.com/kubeshop/monokle-saas/issues/2309.

Changes

Fixes

How it works

monokle 20240124-1

Remarks

  1. As you can see on the gif above, suppressed violations are marked as Suppressed in violations panel only. There is still highlight for them and are shown in problems panel. This is quite confusing and there is dedicated issue to fix this - https://github.com/kubeshop/vscode-monokle/issues/78.
  2. Sometimes one suppression would fix multiple violation (for same rule, like recommended labels). I think it doesn't make sense to put separate action for each single violation just for the rule itself (as shown above).
  3. I used an annotation format like monokle.io/suppress.ruleId: suppress because using boolean true violates schema resulting in K8S001 violation 😅
  4. The above and quick fix labels to discuss. We can change wording there if we find better alternatives.
  5. I saw that in the cloud we select new changes applied to resource (for autofixes). From what I tested with qucikfixes from different VSC plugins, the selection stays untouched so I went with the same approach to not change it.
  6. Left one @TODO in the code on purpose. It's not needed for this PR but will be needed for others.

Checklist

WitoDelnat commented 7 months ago

Awesome work!

For the annotation key, I believe we should rely more on the human-readable form (monokle.io/suppress.kbp.no-latest-image) over the rule identifier form (monokle.io/suppress.KBP007). When you have multiple suppressions, it's hard to understand what the latter is now actually suppressing.

For the annotation value, it's indeed a petty limitation that you cannot use true due to the JSON Schema as it makes more sense. I see Traefik uses "true" which might be a possibility. For full context, the original intention was that if you provide a string, it's to be added as the SARIF suppression's justification (e.g. "won't fix" or "false positive"). I think for a quick action it's to complicated to give a selection / input for justifications.

f1ames commented 7 months ago

Thanks for feedback @WitoDelnat. Changed to use full rule name for annotations e55dd7f and also added telemetry c0c9490.

f1ames commented 7 months ago

For the annotation value, it's indeed a petty limitation that you cannot use true due to the JSON Schema as it makes more sense. I see Traefik uses "true" which might be a possibility. For full context, the original intention was that if you provide a string, it's to be added as the SARIF suppression's justification (e.g. "won't fix" or "false positive"). I think for a quick action it's to complicated to give a selection / input for justifications.

Yeah, was considering "true" but it looks like some kind of workaround. Maybe good way to tackle this would be mentioning in the docs/readme that one can change annotation value as it acts as justification comment?

WitoDelnat commented 7 months ago

Maybe good way to tackle this would be mentioning in the docs/readme that one can change annotation value as it acts as justification comment

That's definitely a good place for it! Feel free to do that.