Skip to content

Public API: Load()#17

Merged
Julio-Guerra merged 12 commits into
mainfrom
feature-load-public-api
Jul 5, 2023
Merged

Public API: Load()#17
Julio-Guerra merged 12 commits into
mainfrom
feature-load-public-api

Conversation

@Julio-Guerra

Copy link
Copy Markdown
Collaborator

Sorry, this is a big-bang PR implementing:

  • func Load() (ok bool, err error) to ease the integration in dd-trace-go so that we can distinguish the different possible error cases:
    • Panic errors that must be used to gracefully abort the caller and prevent it from continuing. Such errors are represented by the new PanicError error type.
    • Unsupported target OS and/or architecture, which must be handled as an informative error, is worth logging to the user, and prevents the caller from continuing. This error is represented by the new UnsupportedTarget error type.
    • Every other unexpected error that can possible happen is returned as-is, but ok is false, so that the caller knows it cannot use the package.
    • Informative errors, about internal non-critical failures, can also happen (eg. if os.Remove fails removing the temporary files), and are returned as-is, but with ok being true so that the caller can log the error and continue its startup.
    • The dd-trace-go integration of this PR can be found here appsec: integrate the new purego-based go-libddwaf dd-trace-go#2090
    • Note that Health() has been kept for the sake of simplicity health-checking cases such as in the tests, where it returns the Load() error only if ok is false, meaning that go-libddwaf is not healthy.
  • Simplified build tags, removed everywhere it was possible, down to purego which isn't properly stubbed for unsupported targets and which we stub ourselves at our level in the new waf_dl_unsupported.go file.

@Julio-Guerra
Julio-Guerra requested a review from eliottness July 3, 2023 21:30
@Julio-Guerra
Julio-Guerra requested a review from Hellzy July 4, 2023 08:04

@eliottness eliottness left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM (except maybe for the little comment i left)

Comment thread waf_test.go Outdated
Co-authored-by: Eliott Bouhana <[email protected]>

@Hellzy Hellzy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Couple remarks, nothing too mean

Comment thread safe.go Outdated
Comment thread waf.go Outdated
Comment thread waf_dl.go Outdated
Comment thread waf_dl.go Outdated
Comment thread waf.go Outdated

@Hellzy Hellzy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍

@Julio-Guerra
Julio-Guerra merged commit 2a0301d into main Jul 5, 2023
@Julio-Guerra
Julio-Guerra deleted the feature-load-public-api branch July 5, 2023 08:10
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.

3 participants