From f91265beacce7d880cd95d2cb0dcd35e74644055 Mon Sep 17 00:00:00 2001 From: songhang Date: Fri, 14 Jun 2024 10:36:20 -0600 Subject: [PATCH] Change logic, add field DirectiveSources --- analyze.go | 29 ++++++++++-- analyze_test.go | 122 ++++++++++++++++++++++++++++++++++++++++++++++++ parse.go | 8 ++++ 3 files changed, 154 insertions(+), 5 deletions(-) diff --git a/analyze.go b/analyze.go index 70440827..09ce07e4 100644 --- a/analyze.go +++ b/analyze.go @@ -94,13 +94,32 @@ func enterBlockCtx(stmt *Directive, ctx blockCtx) blockCtx { //nolint:gocyclo,funlen,gocognit func analyze(fname string, stmt *Directive, term string, ctx blockCtx, options *ParseOptions) error { - masks, knownDirective := directives[stmt.Directive] + var masks []uint + knownDirective := false + currCtx, knownContext := contexts[ctx.key()] + directiveName := stmt.Directive + + // Find all bitmasks from the sources invoker provides. + for _, matchFn := range options.DirectiveSources { + if masksInFn, found := matchFn(directiveName); found { + masks = append(masks, masksInFn...) + knownDirective = true + } + } - if !knownDirective { - for _, matchFn := range options.MatchFuncs { - if masks, knownDirective = matchFn(stmt.Directive); knownDirective { - break + // If DirectiveSources was not provided, try to find the directive in legacy way. + // We want to use DirectiveSources to indicate the OSS/N+ version, and dynamic + // modules we want to include for validation. However, invokers used MatchFuncs + // and a directive map before. This is a transition plan. After MatchFuncs is deleted + // from ParseOptions, this if block should be deleted. + if len(options.DirectiveSources) == 0 { + masks, knownDirective = directives[directiveName] + if !knownDirective { + for _, matchFn := range options.MatchFuncs { + if masks, knownDirective = matchFn(stmt.Directive); knownDirective { + break + } } } } diff --git a/analyze_test.go b/analyze_test.go index 5010d867..709c61bf 100644 --- a/analyze_test.go +++ b/analyze_test.go @@ -2427,3 +2427,125 @@ func TestAnalyze_headers_more(t *testing.T) { }) } } + +//nolint:funlen +func TestAnalyze_directiveSources(t *testing.T) { + t.Parallel() + // two self defined maps and matchFn to ensure it is a separate test + testDirectiveMap1 := map[string][]uint{ + "common_dir": {ngxAnyConf | ngxConfTake1}, + "test_dir1": {ngxAnyConf | ngxConfTake1}, + } + testSource1 := func(directive string) ([]uint, bool) { + masks, matched := testDirectiveMap1[directive] + return masks, matched + } + + testDirectiveMap2 := map[string][]uint{ + "common_dir": {ngxAnyConf | ngxConfTake2}, + "test_dir2": {ngxAnyConf | ngxConfTake2}, + } + testSource2 := func(directive string) ([]uint, bool) { + masks, matched := testDirectiveMap2[directive] + return masks, matched + } + + testcases := map[string]struct { + stmt *Directive + ctx blockCtx + wantErr bool + }{ + // The directive only found in source1 and satisfies the bitmask in it + "DirectiveFoundOnlyInSource1_pass": { + &Directive{ + Directive: "test_dir1", + Args: []string{"arg1"}, + Line: 5, + }, + blockCtx{"http", "upstream"}, + false, + }, + // The directive only found in source2 and satisfies the bitmask in it + "DirectiveFoundOnlyInSource2_pass": { + &Directive{ + Directive: "test_dir2", + Args: []string{"arg1", "arg2"}, + Line: 5, + }, + blockCtx{"http", "upstream"}, + false, + }, + // The directive only found in source2 but not satisfies the bitmask in it + "DirectiveFoundOnlyInsource2_fail": { + &Directive{ + Directive: "test_dir2", + Args: []string{"arg1"}, + Line: 5, + }, + blockCtx{"http", "upstream"}, + true, + }, + // The directive found in both sources, + // but only satisfies bitmasks in source1 it should still pass validation + "DirectiveFoundInBothSources_pass_case1": { + &Directive{ + Directive: "common_dir", + Args: []string{"arg1"}, + Line: 5, + }, + blockCtx{"http", "upstream"}, + false, + }, + // The directive found in both Sources, + // but only satisfies bitmasks in source2 it should still pass validation + "DirectiveFoundInBothSources_pass_case2": { + &Directive{ + Directive: "common_dir", + Args: []string{"arg1", "arg2"}, + Line: 5, + }, + blockCtx{"http", "upstream"}, + false, + }, + // The directive found in both sources, + // but doesn't satisfy bitmask in any of them + "DirectiveFoundInBothSources_fail": { + &Directive{ + Directive: "common_dir", + Args: []string{"arg1", "arg2", "arg3"}, + Line: 5, + }, + blockCtx{"http", "upstream"}, + true, + }, + // The directive not found in any source + "DirectiveNotFoundInAnySource_fail": { + &Directive{ + Directive: "no_exist", + Args: []string{}, + Line: 5, + }, + blockCtx{"http", "location"}, + true, + }, + } + + for name, tc := range testcases { + tc := tc + t.Run(name, func(t *testing.T) { + t.Parallel() + err := analyze("nginx.conf", tc.stmt, ";", tc.ctx, &ParseOptions{ + DirectiveSources: []MatchFunc{testSource1, testSource2}, + ErrorOnUnknownDirectives: true, + }) + + if !tc.wantErr && err != nil { + t.Fatal(err) + } + + if tc.wantErr && err == nil { + t.Fatal("expected error, got nil") + } + }) + } +} diff --git a/parse.go b/parse.go index 00e3faed..27ee824d 100644 --- a/parse.go +++ b/parse.go @@ -108,7 +108,15 @@ type ParseOptions struct { // encountered by the parser to determine the valid contexts and argument count of the // directive. Set this option to enable parsing of directives belonging to non-core or // dynamic NGINX modules that follow the usual grammar rules of an NGINX configuration. + // If DirectiveSources was provided, this will be ignored. MatchFuncs []MatchFunc + + // An array of MatchFunc, which is used to indicate the OSS/N+ version, and + // dynamic modules you want to include for validation. If a directive matches + // any of them, and satisfies the corresponding bitmask, it should pass the validation. + // If we provide this, MatchFuncs will be ignored + DirectiveSources []MatchFunc + LexOptions LexOptions }