fix: rule readiness check should require at least one rule to be loaded #1061
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zepatrik
force-pushed
the
fix/readiness-checker
branch
from
4dba2db
to
6e6e33d
Compare
|
Hm actually I am not sure if we want to keep 3365b84 Edit: changed the patch to also match on invalid rules, for the health check however we only count valid rules. |
Codecov Report
@@ Coverage Diff @@
## master #1061 +/- ##
==========================================
- Coverage 78.12% 77.79% -0.33%
==========================================
Files 83 81 -2
Lines 4013 3967 -46
==========================================
- Hits 3135 3086 -49
- Misses 599 600 +1
- Partials 279 281 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
zepatrik
commented
Feb 7, 2023
| @@ -25,7 +25,11 @@ import ( | |||
|
|
|||
| func TestCredentialsHandler(t *testing.T) { | |||
| conf := internal.NewConfigurationWithDefaults(configx.SkipValidation()) | |||
| conf.SetForTest(t, configuration.MutatorIDTokenIsEnabled, true) | |||
There was a problem hiding this comment.
All these are required because now only valid rules are considered by the rule repository.
| @@ -28,7 +28,7 @@ Note: | |||
| Run: func(cmd *cobra.Command, _ []string) { | |||
| client := newClient(cmd) | |||
|
|
|||
| r, err := client.API.IsInstanceAlive(api.NewIsInstanceAliveParams()) | |||
| r, err := client.Health.IsInstanceAlive(health.NewIsInstanceAliveParams()) | |||
There was a problem hiding this comment.
I honestly don't know why the SDK changed, but 🤷
There was a problem hiding this comment.
I think it comes from changes in github.com/ory/x/healthx. Fine with me.
| @@ -35,7 +36,7 @@ import ( | |||
| "github.com/ory/oathkeeper/rule" | |||
| ) | |||
|
|
|||
| const testRule = `[{"id":"test-rule-5","upstream":{"preserve_host":true,"strip_path":"/api","url":"mybackend.com/api"},"match":{"url":"myproxy.com/api","methods":["GET","POST"]},"authenticators":[{"handler":"noop"},{"handler":"anonymous"}],"authorizer":{"handler":"allow"},"mutators":[{"handler":"noop"}]}]` | |||
| const testRule = `[{"id":"test-rule-5","upstream":{"preserve_host":true,"strip_path":"/api","url":"https://mybackend.com/api"},"match":{"url":"myproxy.com/api","methods":["GET","POST"]},"authenticators":[{"handler":"noop"},{"handler":"anonymous"}],"authorizer":{"handler":"allow"},"mutators":[{"handler":"noop"}]}]` | |||
There was a problem hiding this comment.
A lot of the test rules were actually invalid 😅
| return rules[0], nil | ||
| } | ||
|
|
||
| func (m *RepositoryMemory) ReadyChecker(r *http.Request) error { |
There was a problem hiding this comment.
This simple function is so much nicer than that random event pipeline, with actually zero meaning in the end 😅 but that pre-dated the ory/x/healthx package, so the author is excused 😉
| @@ -113,7 +113,7 @@ | |||
| "2018-11-09" | |||
| ] | |||
| }, | |||
| "Body": "WwogIHsKICAgICJpZCI6ICJ0ZXN0LXJ1bGUtMSIsCiAgICAidXBzdHJlYW0iOiB7CiAgICAgICJwcmVzZXJ2ZV9ob3N0IjogdHJ1ZSwKICAgICAgInN0cmlwX3BhdGgiOiAiL2FwaSIsCiAgICAgICJ1cmwiOiAibXliYWNrZW5kLmNvbS9hcGkiCiAgICB9LAogICAgIm1hdGNoIjogewogICAgICAidXJsIjogIm15cHJveHkuY29tL2FwaSIsCiAgICAgICJtZXRob2RzIjogWwogICAgICAgICJHRVQiLAogICAgICAgICJQT1NUIgogICAgICBdCiAgICB9LAogICAgImF1dGhlbnRpY2F0b3JzIjogWwogICAgICB7CiAgICAgICAgImhhbmRsZXIiOiAibm9vcCIKICAgICAgfSwKICAgICAgewogICAgICAgICJoYW5kbGVyIjogImFub255bW91cyIKICAgICAgfQogICAgXSwKICAgICJhdXRob3JpemVyIjogewogICAgICAiaGFuZGxlciI6ICJhbGxvdyIKICAgIH0sCiAgICAibXV0YXRvcnMiOiBbCiAgICAgIHsKICAgICAgICAiaGFuZGxlciI6ICJub29wIgogICAgICB9CiAgICBdCiAgfSwKICB7CiAgICAiaWQiOiAidGVzdC1ydWxlLTIiLAogICAgInVwc3RyZWFtIjogewogICAgICAicHJlc2VydmVfaG9zdCI6IHRydWUsCiAgICAgICJzdHJpcF9wYXRoIjogIi9hcGkiLAogICAgICAidXJsIjogIm15YmFja2VuZC5jb20vYXBpIgogICAgfSwKICAgICJtYXRjaCI6IHsKICAgICAgInVybCI6ICJteXByb3h5LmNvbS9hcGkiLAogICAgICAibWV0aG9kcyI6IFsKICAgICAgICAiR0VUIiwKICAgICAgICAiUE9TVCIKICAgICAgXQogICAgfSwKICAgICJhdXRoZW50aWNhdG9ycyI6IFsKICAgICAgewogICAgICAgICJoYW5kbGVyIjogIm5vb3AiCiAgICAgIH0sCiAgICAgIHsKICAgICAgICAiaGFuZGxlciI6ICJhbm9ueW1vdXMiCiAgICAgIH0KICAgIF0sCiAgICAiYXV0aG9yaXplciI6IHsKICAgICAgImhhbmRsZXIiOiAiZGVueSIKICAgIH0sCiAgICAibXV0YXRvcnMiOiBbCiAgICAgIHsKICAgICAgICAiaGFuZGxlciI6ICJpZF90b2tlbiIKICAgICAgfSwKICAgICAgewogICAgICAgICJoYW5kbGVyIjogImhlYWRlcnMiLAogICAgICAgICJjb25maWciOiB7CiAgICAgICAgICAiaGVhZGVycyI6IHsKICAgICAgICAgICAgIlgtVXNlciI6ICJ7eyBwcmludCAuU3ViamVjdCB9fSIKICAgICAgICAgIH0KICAgICAgICB9CiAgICAgIH0KICAgIF0KICB9LAogIHsKICAgICJpZCI6ICJ0ZXN0LXJ1bGUtMyIsCiAgICAidXBzdHJlYW0iOiB7CiAgICAgICJwcmVzZXJ2ZV9ob3N0IjogdHJ1ZSwKICAgICAgInN0cmlwX3BhdGgiOiAiL2FwaSIsCiAgICAgICJ1cmwiOiAibXliYWNrZW5kLmNvbS9hcGkiCiAgICB9LAogICAgIm1hdGNoIjogewogICAgICAidXJsIjogIm15cHJveHkuY29tL2FwaSIsCiAgICAgICJtZXRob2RzIjogWwogICAgICAgICJHRVQiLAogICAgICAgICJQT1NUIgogICAgICBdCiAgICB9LAogICAgImF1dGhlbnRpY2F0b3JzIjogWwogICAgICB7CiAgICAgICAgImhhbmRsZXIiOiAibm9vcCIKICAgICAgfSwKICAgICAgewogICAgICAgICJoYW5kbGVyIjogImFub255bW91cyIKICAgICAgfQogICAgXSwKICAgICJhdXRob3JpemVyIjogewogICAgICAiaGFuZGxlciI6ICJhbGxvdyIKICAgIH0sCiAgICAibXV0YXRvcnMiOiBbCiAgICAgIHsKICAgICAgICAiaGFuZGxlciI6ICJpZF90b2tlbiIsCiAgICAgICAgImNvbmZpZyI6IHsKICAgICAgICAgICJqd2tzX3VybCI6ICJodHRwOi8vc3R1Yi8iCiAgICAgICAgfQogICAgICB9CiAgICBdCiAgfQpd" | |||
| "Body": "WwogIHsKICAgICJpZCI6ICJ0ZXN0LXJ1bGUtMSIsCiAgICAidXBzdHJlYW0iOiB7CiAgICAgICJwcmVzZXJ2ZV9ob3N0IjogdHJ1ZSwKICAgICAgInN0cmlwX3BhdGgiOiAiL2FwaSIsCiAgICAgICJ1cmwiOiAiaHR0cHM6Ly9teWJhY2tlbmQuY29tL2FwaSIKICAgIH0sCiAgICAibWF0Y2giOiB7CiAgICAgICJ1cmwiOiAibXlwcm94eS5jb20vYXBpIiwKICAgICAgIm1ldGhvZHMiOiBbCiAgICAgICAgIkdFVCIsCiAgICAgICAgIlBPU1QiCiAgICAgIF0KICAgIH0sCiAgICAiYXV0aGVudGljYXRvcnMiOiBbCiAgICAgIHsKICAgICAgICAiaGFuZGxlciI6ICJub29wIgogICAgICB9LAogICAgICB7CiAgICAgICAgImhhbmRsZXIiOiAiYW5vbnltb3VzIgogICAgICB9CiAgICBdLAogICAgImF1dGhvcml6ZXIiOiB7CiAgICAgICJoYW5kbGVyIjogImFsbG93IgogICAgfSwKICAgICJtdXRhdG9ycyI6IFsKICAgICAgewogICAgICAgICJoYW5kbGVyIjogIm5vb3AiCiAgICAgIH0KICAgIF0KICB9LAogIHsKICAgICJpZCI6ICJ0ZXN0LXJ1bGUtMiIsCiAgICAidXBzdHJlYW0iOiB7CiAgICAgICJwcmVzZXJ2ZV9ob3N0IjogdHJ1ZSwKICAgICAgInN0cmlwX3BhdGgiOiAiL2FwaSIsCiAgICAgICJ1cmwiOiAiaHR0cHM6Ly9teWJhY2tlbmQuY29tL2FwaSIKICAgIH0sCiAgICAibWF0Y2giOiB7CiAgICAgICJ1cmwiOiAibXlwcm94eS5jb20vYXBpIiwKICAgICAgIm1ldGhvZHMiOiBbCiAgICAgICAgIkdFVCIsCiAgICAgICAgIlBPU1QiCiAgICAgIF0KICAgIH0sCiAgICAiYXV0aGVudGljYXRvcnMiOiBbCiAgICAgIHsKICAgICAgICAiaGFuZGxlciI6ICJub29wIgogICAgICB9LAogICAgICB7CiAgICAgICAgImhhbmRsZXIiOiAiYW5vbnltb3VzIgogICAgICB9CiAgICBdLAogICAgImF1dGhvcml6ZXIiOiB7CiAgICAgICJoYW5kbGVyIjogImRlbnkiCiAgICB9LAogICAgIm11dGF0b3JzIjogWwogICAgICB7CiAgICAgICAgImhhbmRsZXIiOiAiaWRfdG9rZW4iCiAgICAgIH0sCiAgICAgIHsKICAgICAgICAiaGFuZGxlciI6ICJoZWFkZXIiLAogICAgICAgICJjb25maWciOiB7CiAgICAgICAgICAiaGVhZGVycyI6IHsKICAgICAgICAgICAgIlgtVXNlciI6ICJ7eyBwcmludCAuU3ViamVjdCB9fSIKICAgICAgICAgIH0KICAgICAgICB9CiAgICAgIH0KICAgIF0KICB9LAogIHsKICAgICJpZCI6ICJ0ZXN0LXJ1bGUtMyIsCiAgICAidXBzdHJlYW0iOiB7CiAgICAgICJwcmVzZXJ2ZV9ob3N0IjogdHJ1ZSwKICAgICAgInN0cmlwX3BhdGgiOiAiL2FwaSIsCiAgICAgICJ1cmwiOiAiaHR0cHM6Ly9teWJhY2tlbmQuY29tL2FwaSIKICAgIH0sCiAgICAibWF0Y2giOiB7CiAgICAgICJ1cmwiOiAibXlwcm94eS5jb20vYXBpIiwKICAgICAgIm1ldGhvZHMiOiBbCiAgICAgICAgIkdFVCIsCiAgICAgICAgIlBPU1QiCiAgICAgIF0KICAgIH0sCiAgICAiYXV0aGVudGljYXRvcnMiOiBbCiAgICAgIHsKICAgICAgICAiaGFuZGxlciI6ICJub29wIgogICAgICB9LAogICAgICB7CiAgICAgICAgImhhbmRsZXIiOiAiYW5vbnltb3VzIgogICAgICB9CiAgICBdLAogICAgImF1dGhvcml6ZXIiOiB7CiAgICAgICJoYW5kbGVyIjogImFsbG93IgogICAgfSwKICAgICJtdXRhdG9ycyI6IFsKICAgICAgewogICAgICAgICJoYW5kbGVyIjogImlkX3Rva2VuIiwKICAgICAgICAiY29uZmlnIjogewogICAgICAgICAgImp3a3NfdXJsIjogImh0dHA6Ly9zdHViLyIKICAgICAgICB9CiAgICAgIH0KICAgIF0KICB9Cl0=" | |||
There was a problem hiding this comment.
Just making the rules actually valid...
zepatrik
force-pushed
the
fix/readiness-checker
branch
from
0388ee5
to
25a8fc7
Compare
hperl
approved these changes
Feb 7, 2023
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, all we do is wait for an event to be dispatched. That event is dispatched regardless of whether there actually exists any rule or not. Oathkeeper with zero rules is not very helpful though, so IMO it is a good idea to change this like proposed.