Repository navigation
feat: build streams on krm-stream 0.7.0, and log every refusal once with its user #29
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -126,7 +126,10 @@ A grant taken away ends a shared stream within: | |
| The last line is there because krm-stream delivers events and rechecks one at a time: | ||
| a recheck waits for the write in progress. Without a bound on writes, a browser that | ||
| stopped reading would hold its stream open, unchecked, until the 30-minute response | ||
| duration. With it, the write fails and the stream ends. | ||
| duration. With it, the write fails and the stream ends. Since 0.7.0 krm-stream refuses | ||
| timed rechecks without a write timeout, and its | ||
| [revocation budget](https://github.com/ConfigButler/krm-stream/blob/main/docs/auth.md#revocation-budget) | ||
| breaks down the same parts. | ||
|
Comment on lines
+131
to
+132
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Correct the revocation-budget comparison. The linked v0.7.0 budget includes a delivery-gate wait that has no overall bound. 🤖 Prompt for AI Agents |
||
|
|
||
| Logout and session expiry are not affected: they end every stream within the | ||
| session-check interval, as before. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -165,15 +165,30 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |
| if err := checkResponse(resp); err != nil { | ||
| return err | ||
| } | ||
| if refusals[resp.StatusCode] { | ||
| a.Refused(gate.ByKubernetes, "status", resp.StatusCode, "reason", http.StatusText(resp.StatusCode)) | ||
| } | ||
|
Comment on lines
+168
to
+170
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
ast-grep outline internal/proxy/proxy.go --match limitBytes --view expanded
rg -n -C 18 'func \(p \*Proxy\) limitBytes' internal/proxy/proxy.goRepository: ConfigButler/krm-foyer Length of output: 1932 🏁 Script executed: sed -n '1,205p' internal/proxy/proxy.go | cat -n
printf '\\n--- response limit and helper ---\\n'
sed -n '237,280p' internal/proxy/proxy.go | cat -n
printf '\\n--- bound refusal / interruption declarations ---\\n'
rg -n -C 5 'func \\(.*\\) Refused|func \\(.*\\) Interrupt|func tooLarge|type Admission|ErrorHandler' internal/proxy internal/gate internal/interruptionRepository: ConfigButler/krm-foyer Length of output: 14223 🏁 Script executed: sed -n '246,330p' internal/gate/gate.go | cat -n
rg -n -C 5 'func \\(a \\*Admission\\) (Refused|Interrupt)|func \\(.*\\) Refused|func \\(.*\\) Interrupt' internal/gateRepository: ConfigButler/krm-foyer Length of output: 3767 🏁 Script executed: rg -n -C 6 'Refused|Interrupt' internal/gate/gate.goRepository: ConfigButler/krm-foyer Length of output: 3146 🏁 Script executed: sed -n '192,235p' internal/proxy/proxy.go | cat -nRepository: ConfigButler/krm-foyer Length of output: 2480 Check the declared size before logging the upstream refusal. When 🐛 Suggested fix- if refusals[resp.StatusCode] {
- a.Refused(gate.ByKubernetes, "status", resp.StatusCode, "reason", http.StatusText(resp.StatusCode))
+ if err := p.limitBytes(a, resp); err != nil {
+ return err
}
- return p.limitBytes(a, resp)
+ if refusals[resp.StatusCode] {
+ a.Refused(gate.ByKubernetes, "status", resp.StatusCode, "reason", http.StatusText(resp.StatusCode))
+ }
+ return nil🤖 Prompt for AI Agents |
||
| return p.limitBytes(a, resp) | ||
| }, | ||
| // ReverseProxy hands its error handler the outgoing request, whose headers | ||
| // are the allowlisted ones. How to answer depends on the browser's request. | ||
| ErrorHandler: func(w http.ResponseWriter, _ *http.Request, err error) { p.upstreamError(w, r, err) }, | ||
| ErrorHandler: func(w http.ResponseWriter, _ *http.Request, err error) { | ||
| if refused := p.upstreamError(r, err); refused != nil { | ||
| a.Interrupt(w, refused) | ||
| } | ||
| }, | ||
| } | ||
| rp.ServeHTTP(&headWriter{ResponseWriter: w}, r) | ||
| } | ||
|
|
||
| // refusals are the API server's answers that refuse the user, rather than fail: | ||
| // not signed in, not allowed, in conflict, or invalid. Each is logged, as | ||
| // krm-foyer's own refusals are. | ||
| var refusals = map[int]bool{ | ||
| http.StatusUnauthorized: true, http.StatusForbidden: true, | ||
| http.StatusConflict: true, http.StatusUnprocessableEntity: true, | ||
| } | ||
|
|
||
| // checkResponse applies the upstream response rules (docs/design.md). An error it | ||
| // returns is an *interruption.Interruption, answered by upstreamError. | ||
| func checkResponse(resp *http.Response) error { | ||
|
|
@@ -253,24 +268,26 @@ func heldBack(field, value string) *interruption.Interruption { | |
| } | ||
| } | ||
|
|
||
| func (p *Proxy) upstreamError(w http.ResponseWriter, r *http.Request, err error) { | ||
| // upstreamError is the interruption to answer r with for err, or nil when nobody | ||
| // is left to answer. It panics with http.ErrAbortHandler when the gate cut r short. | ||
| func (p *Proxy) upstreamError(r *http.Request, err error) *interruption.Interruption { | ||
| var refused *interruption.Interruption | ||
| if !errors.As(err, &refused) { | ||
| if gate.CutShort(r.Context()) { | ||
| // The request may have reached Kubernetes, so no interruption may say it | ||
| // did not, and an empty answer would pass for a complete one. | ||
| panic(http.ErrAbortHandler) | ||
| } | ||
| if r.Context().Err() != nil { | ||
| return // the browser went away; nobody is left to answer | ||
| } | ||
| p.logger.Warn("API server unreachable", "cause", upstream.Class(err)) | ||
| refused = &interruption.Interruption{ | ||
| Status: http.StatusBadGateway, Reason: "BadGateway", | ||
| Message: "the API server could not be reached", | ||
| } | ||
| if errors.As(err, &refused) { | ||
| return refused | ||
| } | ||
| if gate.CutShort(r.Context()) { | ||
| // The request may have reached Kubernetes, so no interruption may say it | ||
| // did not, and an empty answer would pass for a complete one. | ||
| panic(http.ErrAbortHandler) | ||
| } | ||
| if r.Context().Err() != nil { | ||
| return nil // the browser went away; nobody is left to answer | ||
| } | ||
| p.logger.Warn("API server unreachable", "cause", upstream.Class(err)) | ||
| return &interruption.Interruption{ | ||
| Status: http.StatusBadGateway, Reason: "BadGateway", | ||
| Message: "the API server could not be reached", | ||
| } | ||
| p.interrupt(w, r, refused) | ||
| } | ||
|
|
||
| func (p *Proxy) interrupt(w http.ResponseWriter, r *http.Request, i *interruption.Interruption) { | ||
|
|
||
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the held-back HTML description.
This sentence says the page shows the HTML source as escaped text. The interruption table at Line 186 says the page currently shows the content type and that escaped response text is future work. Describe the current behavior instead.
🤖 Prompt for AI Agents