feat: Add `external_data` function to Gator Test command #2789

fardin01 · 2023-05-24T13:53:52Z

Signed-off-by: Fardin Khanjani fardin.khanjani@tradeshift.com

What this PR does / why we need it:
gator test should be able to handle external data providers.

gator test returns undefined function external_data if external_data is being used in a ConstraintTemplate. This means that policies that query an external data source cannot be tested except for manual tests against a live cluster.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #2659

Special notes for your reviewer:

fardin01 · 2023-05-24T13:56:03Z

@maxsmythe @sozercan Can I please get some initial feedback here? I'll add tests afterward.

codecov-commenter · 2023-05-24T17:08:12Z

Codecov Report

Patch coverage: 27.77% and project coverage change: -0.16 ⚠️

Comparison is base (9da7226) 53.56% compared to head (fdffa22) 53.41%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2789      +/-   ##
==========================================
- Coverage   53.56%   53.41%   -0.16%     
==========================================
  Files         128      128              
  Lines       11411    11462      +51     
==========================================
+ Hits         6112     6122      +10     
- Misses       4822     4861      +39     
- Partials      477      479       +2

Flag	Coverage Δ
unittests	`53.41% <27.77%> (-0.16%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/gator/reader/filereader.go	`0.00% <0.00%> (ø)`
pkg/gator/test/test.go	`55.41% <30.95%> (-8.94%)`	⬇️
cmd/gator/test/test.go	`32.71% <50.00%> (+1.28%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

acpana

Thanks for opening this PR! Excited to see this take shape. Before acting on any feedback, please wait on max/ sertac to see what other comments they may have

acpana · 2023-05-25T00:07:00Z

cmd/gator/test/test.go

+	Cmd.Flags().BoolVarP(&flagEnableExternalData, "external-data", "e", false, "enable external_data function in Constraint Templates.")
+	Cmd.Flags().StringArrayVarP(&flagExternalDataProviders, "external-data-providers", "", []string{}, "a file containing External Data Provider. Can be specified multiple times.")


I think the existence of external-data-providers implies that external-data is true so we could get away with just using the latter flag.

Similarly, probably don't need two options in test.Opts

What is the reason for using a different flag to source providers vs. the rest of the cluster config? Avoiding letting untrusted config reach out to arbitrary HTTP endpoints?

No specific reason. :) I agree with Alex's suggestion that the existence of external-data-providers implies that external-data is true.

Gotcha. I think security is a good consideration as far as separating out the providers folder (at least potentially). And, if we do have a providers folder, that can probably take the place of an enabled/disabled flag. Maybe users who have a hard requirement of "absolutely no reaching out to external services" might take comfort in an explicit flag though.

@sozercan thoughts?

@sozercan Kind reminder. :)

+1 on having an explicit flag to enable/disable external data. though I prefer something like enable-external-data

fardin01 · 2023-05-31T17:04:22Z

@maxsmythe @sozercan A kind reminder. :)

maxsmythe

Sorry for the delay in review

maxsmythe · 2023-06-10T01:09:10Z

cmd/gator/test/test.go

+	Cmd.Flags().BoolVarP(&flagEnableExternalData, "external-data", "e", false, "enable external_data function in Constraint Templates.")
+	Cmd.Flags().StringArrayVarP(&flagExternalDataProviders, "external-data-providers", "", []string{}, "a file containing External Data Provider. Can be specified multiple times.")


What is the reason for using a different flag to source providers vs. the rest of the cluster config? Avoiding letting untrusted config reach out to arbitrary HTTP endpoints?

maxsmythe · 2023-06-10T01:11:10Z

pkg/gator/test/test.go

+	// this has to happen before creating the client, because rego.AddExternalDataProviderCache arg to the driver must have
+	// a ProviderCache already set up
+	providerCache := frameworksexternaldata.NewCache()
+	for _, obj := range objs {


If having a separate flag is for security purposes, we'd also need to load in the data separately, currently providers defined in any file would be ingested.

I think we should figure out our stance vis-a-vis security in the other comment (link below). Then this comment will either be irrelevant, or very relevant.

#2789 (comment)

@maxsmythe Could you please give me some pointers on how to implement this?

Should ReadSources() function contain something like

s, err = readExternalDataSourceFiles(externalDataProviderFiles) ...

If we want to load the external data provider config from its dedicated directory, then we should change this line to skip over external data provider config, and then either

Reuse readFiles function like: s, err = readFiles(externalDataProviderFiles)

Create a new function like: readExternalDataSourceFiles(externalDataProviderFiles).

and then append the result to sources. Am I on the right path?

@maxsmythe @ritazh Please take another look. I'll add the tests afterward.

sozercan · 2023-06-13T00:36:39Z

Sorry for the delay. +1 on @acpana and @maxsmythe's comments.

fardin01 · 2023-06-13T17:00:24Z

In terms of unit tests, what do we do about the external_data() function request and response?

sozercan · 2023-06-13T19:04:30Z

@fardin01

for go http unit tests, we might be able to use https://pkg.go.dev/net/http/httptest

for mock rego tests, see #2826

acpana · 2023-07-17T17:51:25Z

@fardin01 thanks again your PR here and for your contributions.

I'm curious if you still had the bandwidth to follow up here.

I think there's still the issue of figuring out the right semantics for the flags here @maxsmythe @sozercan and testing IIUC.

fardin01 · 2023-07-17T18:06:40Z

@acpana No problem! I do have the bandwidth, just been waiting for input before I spend time on it.

We are actually going to go live soon with Gator CLI built off of this PR (to give our developers a tool to verify their changes before deploying), so we are very eager for this PR to be merged and released. 😃

maxsmythe · 2023-07-19T02:19:14Z

To be clear, I'd prefer having a separate flag for specifying provider configs (for security purposes), and provider configs should be loaded from a different path from the rest of the code. This is to make it possible to avoid loading malicious configurations.

ritazh · 2023-07-19T03:55:41Z

Let's also make sure to add the new flag(s) to docs.

Signed-off-by: Fardin Khanjani <fardin.khanjani@tradeshift.com>

ritazh · 2023-08-16T14:07:02Z

@fardin01 Let us know if this is ready for review!

fardin01 · 2023-08-16T18:11:26Z

@fardin01 Let us know if this is ready for review!

@ritazh Unfortunately I will not be working on this anymore. 😞

stale · 2023-10-15T21:20:09Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

fardin01 force-pushed the external_data_gator branch from ebf63e4 to 4fbc9be Compare May 24, 2023 13:54

fardin01 force-pushed the external_data_gator branch from 4fbc9be to fdffa22 Compare May 24, 2023 13:56

fardin01 force-pushed the external_data_gator branch from fdffa22 to 6551ae3 Compare May 24, 2023 17:55

acpana reviewed May 25, 2023

View reviewed changes

maxsmythe reviewed Jun 10, 2023

View reviewed changes

fardin01 force-pushed the external_data_gator branch from 6551ae3 to b283ea9 Compare June 13, 2023 13:35

fardin01 requested review from maxsmythe and acpana June 13, 2023 16:14

fardin01 force-pushed the external_data_gator branch 3 times, most recently from 83e9946 to e322225 Compare June 21, 2023 18:23

fardin01 force-pushed the external_data_gator branch 3 times, most recently from 5bd79e2 to 4e07859 Compare July 27, 2023 18:55

feat: Add external_data function to Gator Test command

49eb331

Signed-off-by: Fardin Khanjani <fardin.khanjani@tradeshift.com>

fardin01 force-pushed the external_data_gator branch from 4e07859 to 49eb331 Compare July 27, 2023 19:06

stale bot added the stale label Oct 15, 2023

stale bot closed this Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `external_data` function to Gator Test command #2789

feat: Add `external_data` function to Gator Test command #2789

fardin01 commented May 24, 2023 •

edited

Loading

fardin01 commented May 24, 2023

codecov-commenter commented May 24, 2023

acpana left a comment

acpana May 25, 2023 •

edited

Loading

acpana May 25, 2023

maxsmythe Jun 10, 2023

fardin01 Jun 13, 2023

maxsmythe Jun 13, 2023

fardin01 Jun 27, 2023

ritazh Jul 19, 2023

fardin01 commented May 31, 2023

maxsmythe left a comment

maxsmythe Jun 10, 2023

maxsmythe Jun 10, 2023

maxsmythe Jun 13, 2023

fardin01 Jul 19, 2023

fardin01 Jul 20, 2023

maxsmythe Jul 21, 2023

fardin01 Jul 27, 2023

sozercan commented Jun 13, 2023

fardin01 commented Jun 13, 2023

sozercan commented Jun 13, 2023 •

edited

Loading

acpana commented Jul 17, 2023

fardin01 commented Jul 17, 2023

maxsmythe commented Jul 19, 2023

ritazh commented Jul 19, 2023

ritazh commented Aug 16, 2023

fardin01 commented Aug 16, 2023

stale bot commented Oct 15, 2023

		Cmd.Flags().BoolVarP(&flagEnableExternalData, "external-data", "e", false, "enable external_data function in Constraint Templates.")
		Cmd.Flags().StringArrayVarP(&flagExternalDataProviders, "external-data-providers", "", []string{}, "a file containing External Data Provider. Can be specified multiple times.")

feat: Add external_data function to Gator Test command #2789

feat: Add external_data function to Gator Test command #2789

Conversation

fardin01 commented May 24, 2023 • edited Loading

fardin01 commented May 24, 2023

codecov-commenter commented May 24, 2023

Codecov Report

acpana left a comment

Choose a reason for hiding this comment

acpana May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fardin01 commented May 31, 2023

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan commented Jun 13, 2023

fardin01 commented Jun 13, 2023

sozercan commented Jun 13, 2023 • edited Loading

acpana commented Jul 17, 2023

fardin01 commented Jul 17, 2023

maxsmythe commented Jul 19, 2023

ritazh commented Jul 19, 2023

ritazh commented Aug 16, 2023

fardin01 commented Aug 16, 2023

stale bot commented Oct 15, 2023

feat: Add `external_data` function to Gator Test command #2789

feat: Add `external_data` function to Gator Test command #2789

fardin01 commented May 24, 2023 •

edited

Loading

acpana May 25, 2023 •

edited

Loading

sozercan commented Jun 13, 2023 •

edited

Loading