Implement Policy Fetcher in framework #3539
Conversation
ciarams87
force-pushed
the
feat/waf-policy-fetcher
branch
from
dbe430a to
0d658ad
Compare
github-project-automation
bot
moved this from 🆕 New
to 🏗 In Progress
in NGINX Gateway Fabric
github-actions
bot
added
the
dependencies
|
|
||
| // S3Client defines the interface for S3 operations. | ||
| // | ||
| //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . S3Client |
There was a problem hiding this comment.
Nowadays you can just specify //go:generate go tool counterfeiter -generate once per package, and then wherever you need it, set //counterfeiter:generate . <interface-name>.
| // Try to load AWS config | ||
| // Note: We don't return an error if AWS config fails - HTTP fetching should still work | ||
| var s3Client S3Client | ||
| cfg, err := config.LoadDefaultConfig(context.TODO()) |
There was a problem hiding this comment.
Let's use a sane context here.
| options := defaults() | ||
| for _, opt := range opts { | ||
| opt(&options) | ||
| } |
There was a problem hiding this comment.
Feels like we should set these in the constructor, no?
| if err != nil { | ||
| return nil, fmt.Errorf("failed to fetch S3 file after retries: %w", err) | ||
| } |
There was a problem hiding this comment.
Looks like the only way this gets returned is if the checksum doesn't match. If so, then this message could probably be tweaked a bit.
| return false, nil //nolint:nilerr // Retry on S3 errors | ||
| } | ||
|
|
||
| if opts.checksumEnabled { |
There was a problem hiding this comment.
the validate function already checks this boolean
| if err != nil { | ||
| t.Fatalf("NewDefaultFetcher() failed: %v", err) | ||
| } |
There was a problem hiding this comment.
We can still use the gomega library to call the matchers to check for success.
See some of our other test files for examples. (g.Expect(err).ToNot(HaveOccurred()))
There was a problem hiding this comment.
Probably a lot in this test file that can be updated to use the proper matchers.
| tests := []struct { | ||
| setupServer func() *httptest.Server | ||
| setupFetcher func() Fetcher | ||
| validateFunc func(t *testing.T, data []byte, err error) |
There was a problem hiding this comment.
Doesn't look like this is used.
| } else { | ||
| w.WriteHeader(http.StatusOK) | ||
| _, _ = w.Write([]byte(fileContent)) | ||
| } |
There was a problem hiding this comment.
Are these secondary conditions called at all in these test cases?
| // Error cases specific to parsing logic (not covered in integration tests) | ||
| { | ||
| name: "missing key with trailing slash", | ||
| url: "s3://bucket/", | ||
| expectErr: true, | ||
| }, | ||
| { | ||
| name: "invalid URL encoding", | ||
| url: "s3://bucket/invalid%gg", | ||
| expectErr: true, | ||
| }, | ||
| } |
There was a problem hiding this comment.
IMO, with how simple this test is, I wouldn't even both with the test loop and just call the function directly with these two inputs.
| data, err := fetcher.GetRemoteFile(server.URL, | ||
| WithRetryAttempts(3), | ||
| WithRetryBackoff(RetryBackoffLinear)) |
There was a problem hiding this comment.
| data, err := fetcher.GetRemoteFile(server.URL, | |
| WithRetryAttempts(3), | |
| WithRetryBackoff(RetryBackoffLinear)) | |
| data, err := fetcher.GetRemoteFile( | |
| server.URL, | |
| WithRetryAttempts(3), | |
| WithRetryBackoff(RetryBackoffLinear, | |
| )) |
There was a problem hiding this comment.
(same formatting suggestion elsewhere)
|
I think I've fallen into an over engineering loop - I started out just converting s3 urls to https (no aws sdk required!), then I investigated using an aws library for this parsing, then I moved to using the s3 library with auth. I'm going to strip the whole thing back and remove s3 protocol support altogether - we can require https protocol for s3 objects and shift that requirement to the user. In the auth story we can decide how we want to implement aws auth - we can just use http support without the sdk there too. For clarity I'm going to close this PR and start again with a simpler set of changes. Thanks for the reviews, sorry for the time waste @salonichf5 @sjberman @bjee19!! |
github-project-automation
bot
moved this from 🏗 In Progress
to ✅ Done
in NGINX Gateway Fabric
Proposed changes
Problem: As a user
I want NGF to fetch my NAP WAF Policy bundle from a remote location
So that this bundle can be applied by the data plane to protect my traffic
Solution: Create a Policy Fetcher utility in framework which can pull files from a remote location, as configured in the WafPolicy resource.
Implement policy validation via Checksum validation (compute checksum locally and compare to remote provided checksum).
Testing: Ensured the policy can be downloaded from an http location as part of #3454
Closes #3456
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.