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

fix(validate): support returning empty resources object/slice #704

Merged
merged 18 commits into from
Nov 6, 2024

Conversation

brandtkeller
Copy link
Member

@brandtkeller brandtkeller commented Oct 4, 2024

BLUF

Domains are responsible for the collection of data that will be processed by a provider. This PR makes a stance that domains should:

  • Always perform the full collection of data possible(do not exit on error before completion)
  • Return an empty object when an error occurs

This sets a standard where Lula always attempts to retrieve the full extent of evidence possible. When an error occurs - reviewers/auditors should want to have a full view of what worked and what did not with regards to domain resource collection.

Description

Looking to setup the changes required to test impact to our current validations on allowing domains to return a map item that contains an empty payload. Most applicable to our k8s domain but still relevant elsewhere.

Update: We will need to rewrite some of our existing policies to account for an edge case where no resources were collected. This is more of an edge case in my opinion as the stability of the current validations has been good.

Context primarily captured in the issue below - but the primary goal is that providers should be gating whether the data allows for policy adherence and not a domain. A domain should be objective in that - if there was no errors during the collection of the data - it should continue to progress.

Removed a return statement in QueryCluster and instead aggregate the errors into a single string - Given the ability to continue writing resources if one of the resource items fails - this would allow for the validation to still collect evidence for later review - I believe this is a fundamental part of how Lula should operate - always being in a position to provide as much evidence as possible to enable less guessing in the event a validation or environment needs correction.

Related Issue

Fixes #589

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@brandtkeller brandtkeller self-assigned this Oct 4, 2024
@brandtkeller brandtkeller marked this pull request as ready for review October 29, 2024 14:42
@brandtkeller brandtkeller requested a review from a team as a code owner October 29, 2024 14:42
mildwonkey
mildwonkey previously approved these changes Oct 29, 2024
Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

I left some comments, but none of them have to block merging this as-is from my POV.

src/pkg/domains/kubernetes/resource.go Outdated Show resolved Hide resolved
src/pkg/domains/kubernetes/resource.go Outdated Show resolved Hide resolved
@brandtkeller brandtkeller marked this pull request as ready for review November 2, 2024 17:24
mildwonkey
mildwonkey previously approved these changes Nov 5, 2024
Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

I have some feedback but I don't think it's anything that has to block merging this as-is. I'm a bit concerned that changing this behavior didn't necessitate test changes; we should consider adding some tests that validate the partial response on error.

@meganwolf0
Copy link
Collaborator

I think this looks good, possibly agree with Kristin's comment above. Also wondering if you did any current validation tests and measured the outcomes against this branch v main (just to make sure partial resource return isn't flipping satisfaction on any existing validations, particularly those in UDS Core)?

I feel like this is a good test case for figuring out how to really instrument a proper testbed for that sort of thing - obviously we cant test every validation, but might be nice to test a large swath - or even have an action that we support to test old v new Lula version that we could provide people to run in their own environments to ensure that their validations are still responding as expected with Lula version upgrades...

@brandtkeller brandtkeller marked this pull request as draft November 5, 2024 15:54
@brandtkeller brandtkeller marked this pull request as ready for review November 6, 2024 00:22
@brandtkeller
Copy link
Member Author

I think this looks good, possibly agree with Kristin's comment above. Also wondering if you did any current validation tests and measured the outcomes against this branch v main (just to make sure partial resource return isn't flipping satisfaction on any existing validations, particularly those in UDS Core)?

Tested against uds-core and did not reduce counts of satisfied controls.

I feel like this is a good test case for figuring out how to really instrument a proper testbed for that sort of thing - obviously we cant test every validation, but might be nice to test a large swath - or even have an action that we support to test old v new Lula version that we could provide people to run in their own environments to ensure that their validations are still responding as expected with Lula version upgrades...

I like that idea!

@brandtkeller brandtkeller merged commit 9f29146 into main Nov 6, 2024
10 checks passed
@brandtkeller brandtkeller deleted the 589_zero_resources_allowed branch November 6, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Allow domain to return zero resources without failing
3 participants