Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repository_memory: Add rule id to malformed configuration error #386

Merged
merged 2 commits into from
Mar 25, 2020

Conversation

hefekranz
Copy link
Contributor

Related issue

#383

Proposed changes

.

Checklist

  • I have read the contributing guidelines
  • I have read the security policy
  • I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security
    vulnerability, I confirm that I got green light (please contact [email protected]) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation within the code base (if appropriate)
  • I have documented my changes in the developer guide (if appropriate)

Further comments

I didn't think a test was necessary here...

@CLAassistant
Copy link

CLAassistant commented Mar 24, 2020

CLA assistant check
All committers have signed the CLA.

@hefekranz hefekranz force-pushed the log-rule-id-on-malformed-config branch from c8f233e to 5b97f57 Compare March 24, 2020 16:29
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! I've got an idea how to improve it even further :)

@@ -111,7 +111,7 @@ func (m *RepositoryMemory) Set(ctx context.Context, rules []Rule) error {
for _, check := range rules {
if err := m.r.RuleValidator().Validate(&check); err != nil {
viperx.LoggerWithValidationErrorFields(m.r.Logger(), err).WithError(err).
Errorf("A rule uses a malformed configuration and all URLs matching this rule will not work. You should resolve this issue now.")
Errorf("Rule %s uses a malformed configuration and all URLs matching this rule will not work. You should resolve this issue now.", check.ID)
Copy link
Member

@aeneasr aeneasr Mar 24, 2020

Choose a reason for hiding this comment

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

We can actually include the full rule here :)

Suggested change
Errorf("Rule %s uses a malformed configuration and all URLs matching this rule will not work. You should resolve this issue now.", check.ID)
WithField("rule_id", check.ID).
WithField("rule", check).
Error("A rule uses a malformed configuration and all URLs matching this rule will not work. You should resolve this issue now.")

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks good in the code, but it will print out bytes.

ERRO[0000] Rule chatbot-webhook uses a malformed configuration and all URLs matching this rule will not work. You should resolve this issue now. error="authenticator matching this route is misconfigured or disabled" rule="{chatbot-webhook 0xc0004c20c0 [{oauth2_client_credentials []}] {keto_engine_acp_ory [123 10 32 32 32 32 32 32 32 32 34 114 101 113 117 105 114 101 100 95 97 99 116 105 111 110 34 58 32 34 99 97 108 108 34 44 10 32 32 32 32 32 32 32 32 34 114 101 113 117 105 114 101 100 95 114 101 115 111 117 114 99 101 34 58 32 34 99 104 97 116 98 111 116 58 119 101 98 104 111 111 107 34 10 32 32 32 32 32 32 125]} [{noop []}] [] {false } <nil>}" rule_id=chatbot-webhook
Maybe just the rule_id field then?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, makes sense!

@aeneasr
Copy link
Member

aeneasr commented Mar 25, 2020

Thank you!

@aeneasr aeneasr merged commit 7688a8d into ory:master Mar 25, 2020
@hefekranz hefekranz deleted the log-rule-id-on-malformed-config branch March 25, 2020 16:28
@hefekranz
Copy link
Contributor Author

👏

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