Validate configuration after unmarshalling
ClosedPublic

Authored by dduvall on Nov 3 2017, 12:53 AM.

Details

Maniphest Tasks
T175186: Blubber config input validation
Reviewers
thcipriani
hashar
Jrbranaa
Joe
mobrovac
Group Reviewers
Release-Engineering-Team
Commits
rGBLBR1d8f07a35c5a: Validate configuration after unmarshalling
Patch without arc
git checkout -b D868 && curl -L https://phabricator.wikimedia.org/D868?download=true | git apply
Summary

Implemented a validation system using the
github.com/go-playground/validator package, extending it with custom
validation tags, and implemented translation of validation errors into
somewhat human-friendly messages.

Fixes T175186
Depends on D845

Test Plan

Run the unit tests and try running blubber against some bad config.

Diff Detail

Repository
rGBLBR Blubber
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dduvall created this revision.Nov 3 2017, 12:53 AM
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald TranscriptNov 3 2017, 12:53 AM
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript
dduvall updated this revision to Diff 2298.Nov 6 2017, 5:17 PM

Rebased

dduvall updated this revision to Diff 2299.Nov 6 2017, 5:31 PM

Follow golint's func comment advice and use a const of unexported type as the context key for the root config during validation.

thcipriani accepted this revision.Nov 7 2017, 12:49 AM

Everything seems to work as intended, I have a few thoughts/questions inline about your intentions, but overall works well!

It's also kind of confusing to me why you don't have to include gopkg.in/go-playground/validator.v9 to use things like dive in structs(?), but it seems to work without it...

config/runs.go
21

May consider some validation for UID/GID for the sake of prettier messages:
line n: cannot unmarshal !!str foo into uint

config/runs_test.go
137

Unclear if you're testing to ensure the path isn't the root path, or if you're testing to ensure the path doesn't include any relative elements. FWIW, path.IsAbs("/foo/bar/..") == true, so relative elements are fine (which I think is fine, just want to clarify with you that this is a test about / not about ..)

config/validation.go
19

hrm, this prevents installing specific versions of a package, e.g., scap=3.7.0-1. Not sure if that's a problem.

21

very nice :)

This revision is now accepted and ready to land.Nov 7 2017, 12:49 AM
dduvall added inline comments.Nov 7 2017, 6:17 PM
config/runs.go
21

I would have except that handling of unmarshalling errors preempt the validator. By the time the validations run, the input for those fields should be jolly good.

config/runs_test.go
137

Right, I was more concerned with testing that the validator can't be fooled with relative path components.

config/validation.go
19

Ooooh, true dat. I'll integrate versions into the pattern.

21

Nothing like nerding out on some IEEE docs. :)

dduvall updated this revision to Diff 2310.Nov 7 2017, 7:49 PM

Added support for Debian package version or release specified in name.

dduvall requested review of this revision.Nov 7 2017, 7:50 PM
thcipriani accepted this revision.Nov 7 2017, 8:39 PM

awesome! LGTM.

This revision is now accepted and ready to land.Nov 7 2017, 8:39 PM
This revision was automatically updated to reflect the committed changes.