-
Notifications
You must be signed in to change notification settings - Fork 848
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
Log policy checker error messages #9013
base: master
Are you sure you want to change the base?
Log policy checker error messages #9013
Conversation
…A server Signed-off-by: aliculPix4D <aleksandar.licul@pix4d.com>
…hecker Signed-off-by: aliculPix4D <aleksandar.licul@pix4d.com>
result, err := h.policyChecker.Check(h.action, acc, r) | ||
if err != nil { | ||
w.WriteHeader(http.StatusBadRequest) | ||
fmt.Fprintf(w, "policy check error: %s", err.Error()) | ||
userMsg := "policy-checker: unreachable or misconfigured" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember that in the fork I mentioned method SessionName()
of lager, hoping it would return a unique session ID. Since I do not see it here, does it mean that it returns instead "policy-checker" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... I am almost 100% sure I tried to use it but there was no ID... but honestly now I am confused and don't remember exactly...
From the source code:
https://github.com/cloudfoundry/lager/blob/8d31fc126ae626bbf33d7a05e405a2f4be414345/logger.go#L56
there is no mention of ID.
UPDATE: just double checked and as expected: SessionName()
would return only atc.policy-checker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, too bad. Thanks.
Wow didn't realize how many PRs you guys had opened! I'll get around to them once 7.12.0 is out |
Log policy checker error messages.
This will ensure that:
policy-checker: unreachable or misconfigured
)Originally, we wanted to go a step further and also log the success and log the actual policy checker decision and reason but that turned out to be too complicated with the current code to be done in a clean way. To improve this situation, in the future we plan to open a new issue to start the discussion on the proposed changes first.