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

Implement REST datasource driver #5009

Merged
merged 8 commits into from
Nov 21, 2024
Merged

Conversation

JAORMX
Copy link
Contributor

@JAORMX JAORMX commented Nov 20, 2024

Summary

This adds an initial implementation of the REST datasource driver. This
allows the user to register multiple REST endpoints to call a data
soruce and thus interact with it.

This initial implementation left several TODO items which need to be
addressed before unleashing the implementation.

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@JAORMX JAORMX self-assigned this Nov 20, 2024
@coveralls
Copy link

coveralls commented Nov 20, 2024

Coverage Status

coverage: 54.624% (+0.03%) from 54.599%
when pulling 549f6f4 on JAORMX:datasource-rest-impl
into a81830b on mindersec:main.

@JAORMX JAORMX force-pushed the datasource-rest-impl branch 2 times, most recently from fe4cde6 to 852f8ba Compare November 20, 2024 13:52
@JAORMX JAORMX marked this pull request as ready for review November 20, 2024 13:52
@JAORMX JAORMX requested a review from a team as a code owner November 20, 2024 13:52
@JAORMX JAORMX force-pushed the datasource-rest-impl branch from fbb974b to f508ead Compare November 20, 2024 14:19
Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

I like the common schema validation, I'm not sure if this is done yet (I saw more changes sneak in while reviewing)

internal/datasources/rest/handler.go Outdated Show resolved Hide resolved
internal/datasources/rest/handler.go Outdated Show resolved Hide resolved
@@ -19,18 +19,26 @@ import (
// ValidateSchemaUpdate validates that the new json schema doesn't break
// profiles using this rule type
func ValidateSchemaUpdate(oldRuleSchema *structpb.Struct, newRuleSchema *structpb.Struct) error {
Copy link
Member

Choose a reason for hiding this comment

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

Given the lovely new functions in schemavalidate for conversion, would it make sense to file a cleanup ticket to make these take a pair of jsonschema.Schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would make sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for _, desc := range errs {
problems = append(problems, desc.Error())
}
return fmt.Errorf("invalid json schema: %s", strings.TrimSpace(strings.Join(problems, "\n")))
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to use errors.Join(...error) here (or implement Unwrap() []error upstream) to allow callers to pick apart the errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Join would make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave this as a TODO for another PR since this was moved from existing code.

@JAORMX JAORMX force-pushed the datasource-rest-impl branch 2 times, most recently from 329d34d to fc6d9ae Compare November 21, 2024 06:26
@JAORMX
Copy link
Contributor Author

JAORMX commented Nov 21, 2024

The security scan failure is a false positive. I don't even know where it's getting that file from 😕

@JAORMX
Copy link
Contributor Author

JAORMX commented Nov 21, 2024

We're hitting aquasecurity/trivy#7970

rdimitrov
rdimitrov previously approved these changes Nov 21, 2024
eleftherias
eleftherias previously approved these changes Nov 21, 2024
internal/datasources/rest/handler.go Outdated Show resolved Hide resolved
This adds an initial implementation of the REST datasource driver. This
allows the user to register multiple REST endpoints to call a data
soruce and thus interact with it.

This initial implementation left several TODO items which need to be
addressed before unleashing the implementation.

Signed-off-by: Juan Antonio Osorio <[email protected]>
Signed-off-by: Juan Antonio Osorio <[email protected]>
This is instead handled by the registry.

Signed-off-by: Juan Antonio Osorio <[email protected]>
Signed-off-by: Juan Antonio Osorio <[email protected]>
Signed-off-by: Juan Antonio Osorio <[email protected]>
Signed-off-by: Juan Antonio Osorio <[email protected]>
@JAORMX JAORMX dismissed stale reviews from eleftherias and rdimitrov via 549f6f4 November 21, 2024 08:40
@JAORMX JAORMX force-pushed the datasource-rest-impl branch from fc6d9ae to 549f6f4 Compare November 21, 2024 08:40
@JAORMX JAORMX merged commit 3cc4ae8 into mindersec:main Nov 21, 2024
23 of 24 checks passed
@JAORMX JAORMX deleted the datasource-rest-impl branch November 21, 2024 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants