Skip to content

Commit

Permalink
rules: Resolves an issue with cached matchers
Browse files Browse the repository at this point in the history
This patch resolves an issue where updates would not properly propagate. This caused deleted rules to still be available in the proxy.

Closes #73
  • Loading branch information
arekkas committed Jun 15, 2018
1 parent 5c40f97 commit 76fbe89
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 13 deletions.
9 changes: 9 additions & 0 deletions rule/matcher_cached.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,17 @@ func (m *CachedMatcher) Refresh() error {
return errors.WithStack(err)
}

inserted := map[string]bool{}
for _, rule := range rules {
inserted[rule.ID] = true
m.Rules[rule.ID] = rule
}

for _, rule := range m.Rules {
if _, ok := inserted[rule.ID]; !ok {
delete(m.Rules, rule.ID)
}
}

return nil
}
8 changes: 8 additions & 0 deletions rule/matcher_cached_http.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ func (m *HTTPMatcher) Refresh() error {
return errors.Errorf("Unable to fetch rules from backend, got status code %d but expected %s", response.StatusCode, http.StatusOK)
}

inserted := map[string]bool{}
for _, r := range rules {
if len(r.Match.Methods) == 0 {
r.Match.Methods = []string{}
Expand All @@ -64,6 +65,7 @@ func (m *HTTPMatcher) Refresh() error {
}
}

inserted[r.Id] = true
m.Rules[r.Id] = Rule{
ID: r.Id,
Description: r.Description,
Expand All @@ -85,5 +87,11 @@ func (m *HTTPMatcher) Refresh() error {
}
}

for _, rule := range m.Rules {
if _, ok := inserted[rule.ID]; !ok {
delete(m.Rules, rule.ID)
}
}

return nil
}
48 changes: 35 additions & 13 deletions rule/matcher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,29 +100,51 @@ func TestMatcher(t *testing.T) {
handler.SetRoutes(router)
server := httptest.NewServer(router)

for _, tr := range testRules {
require.NoError(t, manager.CreateRule(&tr))
}

matchers := map[string]Matcher{
"memory": NewCachedMatcher(manager),
"http": NewHTTPMatcher(oathkeeper.NewSDK(server.URL)),
}

var testMatcher = func(t *testing.T, matcher Matcher, method string, url string, expectErr bool, expect *Rule) {
r, err := matcher.MatchRule(method, mustParseURL(t, url))
if expectErr {
require.Error(t, err)
} else {
require.NoError(t, err)
assert.EqualValues(t, *expect, *r)
}
}

for name, matcher := range matchers {
t.Run("matcher="+name, func(t *testing.T) {
t.Run("matcher="+name+"/case=empty", func(t *testing.T) {
require.NoError(t, matcher.Refresh())
testMatcher(t, matcher, "GET", "https://localhost:34/baz", true, nil)
testMatcher(t, matcher, "POST", "https://localhost:1234/foo", true, nil)
testMatcher(t, matcher, "DELETE", "https://localhost:1234/foo", true, nil)
})
}

r, err := matcher.MatchRule("GET", mustParseURL(t, "https://localhost:34/baz"))
require.NoError(t, err)
assert.EqualValues(t, testRules[1], *r)
for _, tr := range testRules {
require.NoError(t, manager.CreateRule(&tr))
}

r, err = matcher.MatchRule("POST", mustParseURL(t, "https://localhost:1234/foo"))
require.NoError(t, err)
assert.EqualValues(t, testRules[0], *r)
for name, matcher := range matchers {
t.Run("matcher="+name+"/case=created", func(t *testing.T) {
require.NoError(t, matcher.Refresh())
testMatcher(t, matcher, "GET", "https://localhost:34/baz", false, &testRules[1])
testMatcher(t, matcher, "POST", "https://localhost:1234/foo", false, &testRules[0])
testMatcher(t, matcher, "DELETE", "https://localhost:1234/foo", true, nil)
})
}

r, err = matcher.MatchRule("DELETE", mustParseURL(t, "https://localhost:1234/foo"))
require.Error(t, err)
require.NoError(t, manager.DeleteRule(testRules[0].ID))

for name, matcher := range matchers {
t.Run("matcher="+name+"/case=updated", func(t *testing.T) {
require.NoError(t, matcher.Refresh())
testMatcher(t, matcher, "GET", "https://localhost:34/baz", false, &testRules[1])
testMatcher(t, matcher, "POST", "https://localhost:1234/foo", true, nil)
testMatcher(t, matcher, "DELETE", "https://localhost:1234/foo", true, nil)
})
}
}

0 comments on commit 76fbe89

Please sign in to comment.