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

🧹 Remove discovery filters parameter from Discover #4691

Merged
merged 11 commits into from
Sep 26, 2024

Conversation

VasilSirakov
Copy link
Contributor

@VasilSirakov VasilSirakov commented Sep 25, 2024

In this PR:

  • Removed DiscoveryFilters from Discover(...) function and use the parameters from the AwsConnection instead.
  • Fixed parsing of inventory config discovery filters into DiscoveryFilters struct.
  • Added new cases for ECS filters to be specified from inventory because they were only passed down from the removed parameter in Discovery(...).
  • Removed duplicate filtering of EC2 instances inside the AWS discovery that was already done by GetInstances().
  • Added corresponding exclude filters in Ec2DiscoveryFilters.
  • Filter out EC2 instances if they match the values specified in the exclude filters. ExcludeRegions happens before the call to AWS, ExcludeTags and ExcludeInstanceIds happen after (AWS doesn't support server-side filtering for those).
  • Added unit tests for a couple of functions.

Note:

Changing the signature for Discovery means everyone using the AWS provider and wanting to filter discovered instances must specify these filters in their inventory config discovery filters map.

…ilters from AwsConnection.

Signed-off-by: Vasil Sirakov <sirakov97@gmail.com>
Copy link
Contributor

github-actions bot commented Sep 25, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

Signed-off-by: Vasil Sirakov <sirakov97@gmail.com>
Copy link
Contributor

github-actions bot commented Sep 25, 2024

Test Results

3 111 tests  +12   3 110 ✅ +12   1m 22s ⏱️ -4s
  371 suites ± 0       1 💤 ± 0 
   28 files   ± 0       0 ❌ ± 0 

Results for commit 8a9f783. ± Comparison against base commit 77779d0.

♻️ This comment has been updated with latest results.

@VasilSirakov
Copy link
Contributor Author

I have read the Mondoo CLA Document and I hereby sign the CLA

Signed-off-by: Vasil Sirakov <sirakov97@gmail.com>
Signed-off-by: Vasil Sirakov <sirakov97@gmail.com>
@@ -803,6 +803,9 @@ func (a *mqlAwsEc2) getInstances(conn *connection.AwsConnection) []*jobpool.Job
if len(conn.Filters.Ec2DiscoveryFilters.Regions) > 0 {
regions = conn.Filters.Ec2DiscoveryFilters.Regions
}
for _, regionToExclude := range conn.Filters.Ec2DiscoveryFilters.ExcludeRegions {
regions = removeElement(regions, regionToExclude)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just adjust this to a single function that takes the regions and the regions to exclude and returns a new list?

Copy link
Contributor

@preslavgerchev preslavgerchev left a comment

Choose a reason for hiding this comment

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

this lgtm! @vjeffrey can you have another look since we're changing the AWS discovery here?

… should be queried.

Signed-off-by: Vasil Sirakov <sirakov97@gmail.com>
…cable.

Signed-off-by: Vasil Sirakov <sirakov97@gmail.com>
@VasilSirakov VasilSirakov marked this pull request as ready for review September 25, 2024 12:07
Signed-off-by: Vasil Sirakov <sirakov97@gmail.com>
@VasilSirakov VasilSirakov changed the title WIP: Remove discovery filters parameter from Discover 🧹 Remove discovery filters parameter from Discover Sep 25, 2024
…map and implement parsing of CLI flags to opts matching them to known filter prefixes.

Signed-off-by: Vasil Sirakov <sirakov97@gmail.com>
@VasilSirakov
Copy link
Contributor Author

VasilSirakov commented Sep 25, 2024

After a chat with Preslav, we decided it'd be nicer to have the filters as comma separated values as opposed to individual keys.
So, we went from ec2:instance-id:id1 = id1 and ec2:instance-id:id2 = id2 to just ec2:instance-ids = id1,id2. Did that for all applicable filters excl. tags.

Also, added the parsing of CLI flags to opts and now the CLI should have the same functionality.

Signed-off-by: Vasil Sirakov <sirakov97@gmail.com>
… longer embed the value into the key.

Signed-off-by: Vasil Sirakov <sirakov97@gmail.com>
…ven region but propagate an error when we attempt to find it from another region it is not in.

Signed-off-by: Vasil Sirakov <sirakov97@gmail.com>
Copy link
Contributor

@vjeffrey vjeffrey left a comment

Choose a reason for hiding this comment

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

nice, thank you!

@preslavgerchev preslavgerchev merged commit 8126971 into main Sep 26, 2024
15 checks passed
@preslavgerchev preslavgerchev deleted the vasil/unify-discovery-filters branch September 26, 2024 15:38
@github-actions github-actions bot locked and limited conversation to collaborators Sep 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants