hook-reverse-proxy: First implementation - #5345
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughAdds a configurable GitHub webhook reverse proxy. The service validates and routes webhook requests, reloads YAML configuration, exposes readiness health, supports graceful shutdown, and includes a UBI9-based container image. ChangesReverse Proxy Service
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 3 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
images/hook-reverse-proxy/Dockerfile (1)
1-5: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMissing non-root
USER,ADDinstead ofCOPY, and noHEALTHCHECK.Three separate guideline violations here:
- No
USERdirective — the image runs as root by default.ADDused for a plain local file copy;COPYis the correct, less "magic" instruction.- No
HEALTHCHECK, even though the binary exposes a readiness endpoint viapjutil.NewHealthOnPort.As per path instructions, "USER non-root; never run as root" and "HEALTHCHECK defined."🔒 Proposed fix
FROM registry.access.redhat.com/ubi9/ubi-minimal:latest -ADD hook-reverse-proxy /usr/bin/hook-reverse-proxy +COPY hook-reverse-proxy /usr/bin/hook-reverse-proxy +USER 65534:65534 ENTRYPOINT ["/usr/bin/hook-reverse-proxy"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@images/hook-reverse-proxy/Dockerfile` around lines 1 - 5, Update the hook-reverse-proxy Dockerfile to replace ADD with COPY, create or select a non-root user and set it with USER before ENTRYPOINT, and add a HEALTHCHECK targeting the readiness endpoint exposed by pjutil.NewHealthOnPort. Preserve the existing binary path and startup command.Sources: Path instructions, Linters/SAST tools
🧹 Nitpick comments (3)
cmd/hook-reverse-proxy/main.go (3)
142-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLoop variable
rshadows the*routerreceiver.Inside
matchRoute,for _, r := range m.Reposat line 171 shadows the method receiverr *routerin scope for the rest of the function. It's harmless today (the inner block never touchesr.config), but it's a readability trap for future edits — someone adding router-state access inside that inner loop would silently reference the wrongr.As per coding guidelines, "Use good naming conventions that are long enough to communicate fully without being hard to read."🏷️ Proposed rename
- for _, r := range m.Repos { - if r == "*" { + for _, repoPattern := range m.Repos { + if repoPattern == "*" { log.Infof("Match found: match org %q and * repo wildcard", org) return route.Target.URL } - if r == repo { + if repoPattern == repo { log.Infof("Match found: match org %q and repo %q", org, repo) return route.Target.URL } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hook-reverse-proxy/main.go` around lines 142 - 186, Rename the inner loop variable in matchRoute that iterates over m.Repos so it no longer shadows the *router receiver r. Update its comparison with repo accordingly, preserving the existing wildcard and exact-match behavior.Source: Coding guidelines
85-97: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNil-deref risk in fallback if
DefaultRouteis unset.Line 91 dereferences
r.config.DefaultRoute.Target.URLwithout a nil check. Today this is safe only becauseConfig.validate()(line 48-64) is always run inloadConfigbefore arouteris constructed inmain(). ButnewRouteritself accepts any*Config, so any future caller that skipsloadConfig/validate()(e.g. programmatic construction, a different entrypoint) will panic here. Worth guarding defensively given this runs per-request.As per path instructions, "check for nil before dereferencing pointers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hook-reverse-proxy/main.go` around lines 85 - 97, The deferred fallback in the router response handling can panic when DefaultRoute or its Target is nil. Update the targetURL fallback in the defer block to check each pointer before accessing URL, while preserving the existing default-route behavior when the nested values are present.Source: Path instructions
20-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate import of the same package under two aliases.
Lines 20-21 import
sigs.k8s.io/prow/pkg/flagutiltwice — once unaliased (flagutil, used at line 210) and once aliased (prowflagutil, used at line 200). This compiles, but is a known code smell flagged by linters like go-critic'sdupImport/revive'sduplicated-importsand invites confusion about whether these are different packages.🧹 Proposed fix
- "sigs.k8s.io/prow/pkg/flagutil" - prowflagutil "sigs.k8s.io/prow/pkg/flagutil" + "sigs.k8s.io/prow/pkg/flagutil"And then replace
prowflagutil.InstrumentationOptionsat line 200 withflagutil.InstrumentationOptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hook-reverse-proxy/main.go` around lines 20 - 21, Remove the duplicate aliased import of sigs.k8s.io/prow/pkg/flagutil and use the existing flagutil import consistently. Update the prowflagutil.InstrumentationOptions reference in the surrounding initialization code to flagutil.InstrumentationOptions, while preserving the existing flagutil usage elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/hook-reverse-proxy/main.go`:
- Around line 263-268: Update the http.Server initialization in the
reverse-proxy setup to configure finite ReadHeaderTimeout, ReadTimeout,
WriteTimeout, and IdleTimeout values appropriate for GitHub webhook traffic.
Keep the existing Addr and ReverseProxy handler unchanged while ensuring slow or
idle clients cannot hold connections indefinitely.
---
Outside diff comments:
In `@images/hook-reverse-proxy/Dockerfile`:
- Around line 1-5: Update the hook-reverse-proxy Dockerfile to replace ADD with
COPY, create or select a non-root user and set it with USER before ENTRYPOINT,
and add a HEALTHCHECK targeting the readiness endpoint exposed by
pjutil.NewHealthOnPort. Preserve the existing binary path and startup command.
---
Nitpick comments:
In `@cmd/hook-reverse-proxy/main.go`:
- Around line 142-186: Rename the inner loop variable in matchRoute that
iterates over m.Repos so it no longer shadows the *router receiver r. Update its
comparison with repo accordingly, preserving the existing wildcard and
exact-match behavior.
- Around line 85-97: The deferred fallback in the router response handling can
panic when DefaultRoute or its Target is nil. Update the targetURL fallback in
the defer block to check each pointer before accessing URL, while preserving the
existing default-route behavior when the nested values are present.
- Around line 20-21: Remove the duplicate aliased import of
sigs.k8s.io/prow/pkg/flagutil and use the existing flagutil import consistently.
Update the prowflagutil.InstrumentationOptions reference in the surrounding
initialization code to flagutil.InstrumentationOptions, while preserving the
existing flagutil usage elsewhere.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 28ccc0f2-1cb0-4c05-b97a-47947d346bdd
📒 Files selected for processing (3)
cmd/hook-reverse-proxy/main.gocmd/hook-reverse-proxy/main_test.goimages/hook-reverse-proxy/Dockerfile
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
6208ea0 to
e8643c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
cmd/hook-reverse-proxy/main.go (3)
172-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLoop variable
rshadows the method receiverr *router.Harmless today since the receiver isn't referenced inside the inner loop, but it's a latent footgun if someone later adds router-scoped logic there. Rename to something like
repo.As per coding guidelines, use "good naming conventions that are long enough to communicate fully without being hard to read."
♻️ Proposed fix
- for _, r := range m.Repos { - if r == "*" { + for _, repo := range m.Repos { + if repo == "*" { log.Infof("Match found: match org %q and * repo wildcard", org) return route.Target.URL } - if r == repo { + if repo == event.Repo.Name { log.Infof("Match found: match org %q and repo %q", org, repo) return route.Target.URL } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hook-reverse-proxy/main.go` at line 172, Rename the inner loop variable `r` in the repository iteration over `m.Repos` to `repo` or another descriptive name, and update all references within that loop while preserving the method receiver `r *router` unchanged.Source: Coding guidelines
44-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd doc comments to exported config types.
Config,Route, andMatchare exported types defining the on-disk YAML schema but have no doc comments explaining their fields/semantics (e.g. what"*"means forOrg/Repos).As per coding guidelines, "Go documentation on Classes/Functions/Fields should be written properly."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hook-reverse-proxy/main.go` around lines 44 - 75, Add Go doc comments for the exported Config, Route, and Match types, describing their YAML schema roles and field semantics, including that "*" is supported as a wildcard for Match.Org and Match.Repos. Keep validation behavior unchanged.Source: Coding guidelines
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate import of the same package under two aliases.
Lines 21-22 import
sigs.k8s.io/prow/pkg/flagutiltwice, once unaliased and once asprowflagutil.flagutil.OptionGroup(Line 211) andprowflagutil.InstrumentationOptions(Line 201) refer to the same package. Consolidate to a single import name.♻️ Proposed fix
- "github.com/sirupsen/logrus" - "gopkg.in/yaml.v3" - - aggerrs "k8s.io/apimachinery/pkg/util/errors" - "sigs.k8s.io/prow/pkg/flagutil" - prowflagutil "sigs.k8s.io/prow/pkg/flagutil" + "github.com/sirupsen/logrus" + "gopkg.in/yaml.v3" + + aggerrs "k8s.io/apimachinery/pkg/util/errors" + "sigs.k8s.io/prow/pkg/flagutil"Then use
flagutil.InstrumentationOptionsat Line 201 instead ofprowflagutil.InstrumentationOptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hook-reverse-proxy/main.go` around lines 21 - 22, Remove the duplicate unaliased/aliased imports of sigs.k8s.io/prow/pkg/flagutil, retain the single flagutil import, and update the InstrumentationOptions reference to use flagutil while preserving the existing flagutil.OptionGroup usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/hook-reverse-proxy/main.go`:
- Around line 49-65: Update Config.validate to reject unusable target URLs, not
just nil pointers: validate both c.DefaultRoute.Target and each route.Target for
the required parsed URL fields (including non-empty scheme and host), and append
the existing aggregate validation errors with context identifying the affected
route. Preserve the current nil-target checks and return through
aggerrs.NewAggregate.
- Around line 127-132: Update the request-body handling around io.ReadAll in the
webhook handler to read through io.LimitReader with a 25 MB limit, then detect
when the payload exceeds that limit and reject it before processing. Preserve
the existing read-error logging and return behavior for actual read failures.
In `@images/hook-reverse-proxy/Dockerfile`:
- Around line 1-4: Add a dedicated non-root user to the Dockerfile after the
hook-reverse-proxy binary is copied, then set the image’s USER to that account
before ENTRYPOINT so the proxy runs without root privileges.
---
Nitpick comments:
In `@cmd/hook-reverse-proxy/main.go`:
- Line 172: Rename the inner loop variable `r` in the repository iteration over
`m.Repos` to `repo` or another descriptive name, and update all references
within that loop while preserving the method receiver `r *router` unchanged.
- Around line 44-75: Add Go doc comments for the exported Config, Route, and
Match types, describing their YAML schema roles and field semantics, including
that "*" is supported as a wildcard for Match.Org and Match.Repos. Keep
validation behavior unchanged.
- Around line 21-22: Remove the duplicate unaliased/aliased imports of
sigs.k8s.io/prow/pkg/flagutil, retain the single flagutil import, and update the
InstrumentationOptions reference to use flagutil while preserving the existing
flagutil.OptionGroup usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 51fd7b85-8843-4b9f-86e1-a4bc38603c15
📒 Files selected for processing (3)
cmd/hook-reverse-proxy/main.gocmd/hook-reverse-proxy/main_test.goimages/hook-reverse-proxy/Dockerfile
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
| func (c *Config) validate() error { | ||
| var errs []error | ||
|
|
||
| if c.DefaultRoute == nil { | ||
| errs = append(errs, errors.New("default route is not defined")) | ||
| } else if c.DefaultRoute.Target == nil { | ||
| errs = append(errs, errors.New("default route target URL is not defined")) | ||
| } | ||
|
|
||
| for i, route := range c.Routes { | ||
| if route.Target == nil { | ||
| errs = append(errs, fmt.Errorf("route[%d]: target is nil", i)) | ||
| } | ||
| } | ||
|
|
||
| return aggerrs.NewAggregate(errs) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
validate() doesn't catch empty/invalid target URLs.
It only checks Target == nil, not that the parsed URL is actually usable. A config with target: "" produces a non-nil *URL wrapping an empty url.URL (no Host/Scheme), which passes validation but will fail silently at proxy time (outbound request with empty Host).
🛡️ Proposed fix
if c.DefaultRoute == nil {
errs = append(errs, errors.New("default route is not defined"))
} else if c.DefaultRoute.Target == nil {
errs = append(errs, errors.New("default route target URL is not defined"))
+ } else if c.DefaultRoute.Target.Host == "" {
+ errs = append(errs, errors.New("default route target URL has no host"))
}
for i, route := range c.Routes {
if route.Target == nil {
errs = append(errs, fmt.Errorf("route[%d]: target is nil", i))
+ } else if route.Target.Host == "" {
+ errs = append(errs, fmt.Errorf("route[%d]: target URL has no host", i))
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *Config) validate() error { | |
| var errs []error | |
| if c.DefaultRoute == nil { | |
| errs = append(errs, errors.New("default route is not defined")) | |
| } else if c.DefaultRoute.Target == nil { | |
| errs = append(errs, errors.New("default route target URL is not defined")) | |
| } | |
| for i, route := range c.Routes { | |
| if route.Target == nil { | |
| errs = append(errs, fmt.Errorf("route[%d]: target is nil", i)) | |
| } | |
| } | |
| return aggerrs.NewAggregate(errs) | |
| } | |
| func (c *Config) validate() error { | |
| var errs []error | |
| if c.DefaultRoute == nil { | |
| errs = append(errs, errors.New("default route is not defined")) | |
| } else if c.DefaultRoute.Target == nil { | |
| errs = append(errs, errors.New("default route target URL is not defined")) | |
| } else if c.DefaultRoute.Target.Host == "" { | |
| errs = append(errs, errors.New("default route target URL has no host")) | |
| } | |
| for i, route := range c.Routes { | |
| if route.Target == nil { | |
| errs = append(errs, fmt.Errorf("route[%d]: target is nil", i)) | |
| } else if route.Target.Host == "" { | |
| errs = append(errs, fmt.Errorf("route[%d]: target URL has no host", i)) | |
| } | |
| } | |
| return aggerrs.NewAggregate(errs) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/hook-reverse-proxy/main.go` around lines 49 - 65, Update Config.validate
to reject unusable target URLs, not just nil pointers: validate both
c.DefaultRoute.Target and each route.Target for the required parsed URL fields
(including non-empty scheme and host), and append the existing aggregate
validation errors with context identifying the affected route. Preserve the
current nil-target checks and return through aggerrs.NewAggregate.
There was a problem hiding this comment.
If the URL is not valid then the custom unmarshal function would fail:
// UnmarshalText implements [encoding.UnmarshalText].
func (u *URL) UnmarshalText(text []byte) error {
parsed, err := url.Parse(string(text))
if err != nil {
return err
}
u.URL = parsed
return nil
}I think that makes the validation code you are proposing pointless.
There was a problem hiding this comment.
@danilo-gemoli, url.Parse does not require an absolute URL.
For example, url.Parse("") and url.Parse("/hook") return no error. Both results have an empty Host. A relative URL is valid URL syntax, but it is not a usable upstream target for this reverse proxy.
UnmarshalText should continue to reject malformed URL syntax. Config.validate() should also reject parsed targets that lack the required Scheme and Host.
The finding is therefore still valid. I will update the wording to make this distinction clear.
You are interacting with an AI system.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/hook-reverse-proxy/router.go (1)
90-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLoop variable
rshadows the method receiverr *router.
matchRouteis a method onr *router. Inside the nested loop at Line 119,for _, r := range m.Reposreintroducesras astring, shadowing the receiver for the rest of that inner loop. No bug results today because the receiver is not referenced inside the loop, but this is a latent trap: a future edit that tries to call a router method (r.something) inside this block will silently bind to the wrongror fail to compile with a confusing error.Rename the loop variable to avoid the collision.
♻️ Proposed fix
- for _, r := range m.Repos { - if r == "*" { + for _, repoPattern := range m.Repos { + if repoPattern == "*" { log.Infof("Match found: match org %q and * repo wildcard", org) return route.Target.URL } - if r == repo { + if repoPattern == repo { log.Infof("Match found: match org %q and repo %q", org, repo) return route.Target.URL } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hook-reverse-proxy/router.go` around lines 90 - 134, In the matchRoute method, rename the nested m.Repos loop variable r to a non-conflicting name and update its comparisons accordingly, preserving the existing wildcard and repository-match behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/hook-reverse-proxy/config.go`:
- Around line 40-53: Update validate() so every non-nil target URL must also be
an absolute URL with a non-empty Host, rejecting empty and relative targets for
both DefaultRoute.Target and entries in c.Routes. Preserve the existing
nil-target aggregate errors and route-specific error context.
- Around line 65-110: Update the watcher producer returned by
GetWatcher/startWatching so every send to EventCh and ErrCh is
cancellation-aware, selecting between the channel send and ctx.Done(). Ensure
cancellation allows the producer to exit and w.Close() to run even when
watchConfig’s consumer goroutine has already stopped; preserve the existing
event and error handling behavior while avoiding blocked sends.
---
Nitpick comments:
In `@cmd/hook-reverse-proxy/router.go`:
- Around line 90-134: In the matchRoute method, rename the nested m.Repos loop
variable r to a non-conflicting name and update its comparisons accordingly,
preserving the existing wildcard and repository-match behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 81fbbfcc-5051-47ac-906e-343149fbef8b
📒 Files selected for processing (4)
cmd/hook-reverse-proxy/config.gocmd/hook-reverse-proxy/main.gocmd/hook-reverse-proxy/router.gocmd/hook-reverse-proxy/router_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
| if c.DefaultRoute == nil { | ||
| errs = append(errs, errors.New("default route is not defined")) | ||
| } else if c.DefaultRoute.Target == nil { | ||
| errs = append(errs, errors.New("default route target URL is not defined")) | ||
| } | ||
|
|
||
| for i, route := range c.Routes { | ||
| if route.Target == nil { | ||
| errs = append(errs, fmt.Errorf("route[%d]: target is nil", i)) | ||
| } | ||
| } | ||
|
|
||
| return aggerrs.NewAggregate(errs) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
validate() accepts syntactically valid but unusable target URLs.
validate() only checks Target == nil. A config with target: "" or target: "/hook" produces a non-nil *URL since url.Parse accepts relative and empty URLs without error. Host stays empty, and the check at Line 42-44 and Line 47-49 passes, but the reverse proxy fails silently at request time because pr.SetURL/pr.Out.URL end up pointing at an unusable target.
This concern was already raised on a previous commit of this PR, when this validation logic lived in main.go. The discussion concluded the finding is still valid because url.Parse accepts relative URLs; it has not yet been fixed and the same logic is now in config.go.
🛡️ Proposed fix
if c.DefaultRoute == nil {
errs = append(errs, errors.New("default route is not defined"))
} else if c.DefaultRoute.Target == nil {
errs = append(errs, errors.New("default route target URL is not defined"))
+ } else if c.DefaultRoute.Target.Host == "" {
+ errs = append(errs, errors.New("default route target URL has no host"))
}
for i, route := range c.Routes {
if route.Target == nil {
errs = append(errs, fmt.Errorf("route[%d]: target is nil", i))
+ } else if route.Target.Host == "" {
+ errs = append(errs, fmt.Errorf("route[%d]: target URL has no host", i))
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/hook-reverse-proxy/config.go` around lines 40 - 53, Update validate() so
every non-nil target URL must also be an absolute URL with a non-empty Host,
rejecting empty and relative targets for both DefaultRoute.Target and entries in
c.Routes. Preserve the existing nil-target aggregate errors and route-specific
error context.
| func watchConfig(ctx context.Context, log *logrus.Entry, path string, onNewConfig func() error) error { | ||
| log = log.WithField("config_path", path) | ||
|
|
||
| events := make(chan fsnotify.Event) | ||
| errs := make(chan error) | ||
| w := agents.UniversalSymlinkWatcher{ | ||
| EventCh: events, | ||
| ErrCh: errs, | ||
| WatchPath: path, | ||
| } | ||
|
|
||
| startWatching, err := w.GetWatcher() | ||
| if err != nil { | ||
| return fmt.Errorf("get watcher: %w", err) | ||
| } | ||
|
|
||
| go func() { | ||
| for { | ||
| select { | ||
| case _, ok := <-events: | ||
| if !ok { | ||
| log.Warn("Watch config: events channel closed") | ||
| return | ||
| } | ||
| if err := onNewConfig(); err != nil { | ||
| log.WithError(err).Error("Failed to reload the configuration") | ||
| } else { | ||
| log.Info("Configuration reloaded") | ||
| } | ||
| case err, ok := <-errs: | ||
| if !ok { | ||
| log.Warn("Watch config: errors channel closed") | ||
| return | ||
| } | ||
| log.WithError(err).Error("Failed to watch the configuration file") | ||
| case <-ctx.Done(): | ||
| log.WithError(context.Cause(ctx)).Info("Configuration watcher stopped") | ||
| return | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| go startWatching(ctx) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f . pkg/load/agents | xargs rg -n -C 15 'UniversalSymlinkWatcher|GetWatcher'Repository: openshift/ci-tools
Length of output: 13186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '107,190p' pkg/load/agents/utils.go
printf '\n-- watchConfig call sites --\n'
rg -n -C 8 'watchConfig\(' cmd pkg
printf '\n-- watcher tests --\n'
rg -n -C 12 'GetWatcher|UniversalSymlinkWatcher|fsnotify watcher|watch.*config' pkg/load/agents cmd/hook-reverse-proxyRepository: openshift/ci-tools
Length of output: 22378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("pkg/load/agents/utils.go")
lines = p.read_text().splitlines()
for i in range(129, 175):
print(f"{i+1:4}: {lines[i]}")
PYRepository: openshift/ci-tools
Length of output: 1157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
source = Path("pkg/load/agents/utils.go").read_text()
checks = {
"context cancellation closes fsnotify watcher": "case <-ctx.Done():" in source and "w.Close()" in source,
"event send is cancellation-aware": 'select {\n\t\t\t\t\tcase u.EventCh <- event:' in source,
"close-error send is cancellation-aware": 'select {\n\t\t\t\t\tcase u.ErrCh <- fmt.Errorf("failed to close' in source,
"fsnotify-error send is cancellation-aware": 'select {\n\t\t\t\t\tcase u.ErrCh <- fmt.Errorf("received fsnotify error' in source,
}
for name, result in checks.items():
print(f"{name}: {'yes' if result else 'no'}")
PYRepository: openshift/ci-tools
Length of output: 332
Make watcher output sends cancellation-aware.
GetWatcher handles ctx.Done() and calls w.Close(), but startWatching can block on the unguarded u.EventCh and u.ErrCh sends. If watchConfig exits first, the watcher goroutine can remain blocked, and an event send can prevent w.Close() from running. Guard each send with ctx.Done() or coordinate producer shutdown before consumer shutdown.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/hook-reverse-proxy/config.go` around lines 65 - 110, Update the watcher
producer returned by GetWatcher/startWatching so every send to EventCh and ErrCh
is cancellation-aware, selecting between the channel send and ctx.Done(). Ensure
cancellation allows the producer to exit and w.Close() to run even when
watchConfig’s consumer goroutine has already stopped; preserve the existing
event and error handling behavior while avoiding blocked sends.
There was a problem hiding this comment.
Propose a fix
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '-- UniversalSymlinkWatcher definitions --'
rg -n -C 20 '\btype UniversalSymlinkWatcher\b|\bfunc \(u UniversalSymlinkWatcher\) GetWatcher\b' pkg/load/agents
printf '%s\n' '-- watcher call sites --'
rg -n -C 6 '\.GetWatcher\(\)|UniversalSymlinkWatcher' --glob '*.go'Length of output: 2892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '120,205p' pkg/load/agents/utils.go
printf '%s\n' '-- call sites --'
rg -n -C 6 '\bGetWatcher\s*\(' --glob '*.go' . || trueLength of output: 5852
@danilo-gemoli, the fix belongs in the shared UniversalSymlinkWatcher implementation. This protects the reverse proxy and the existing watcher consumers.
- Update every
u.EventChandu.ErrChsend inpkg/load/agents/utils.goto select onctx.Done(). - Return from the watcher function when cancellation wins.
- Add a focused regression test for cancellation with unread output channels.
- 📌 Create a pull request with these changes
You are interacting with an AI system.
|
/override ci/prow/integration |
|
/lgtm |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
@danilo-gemoli: Overrode contexts on behalf of danilo-gemoli: ci/prow/integration DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, deepsm007 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@deepsm007: Overrode contexts on behalf of deepsm007: ci/prow/integration DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@danilo-gemoli: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
This is an ad-hoc reverse proxy that will sit in front of hook, created to ease the migration of Prow from
app.citocore-ci.Since shutting down Prow on
app.ci, recreating it oncore-ciand migrate everything is too risk, we are going to follow this plan:core-ci.hook-reverse-proxyonapp.ci.hook-reverse-proxy.The reverse proxy reads a config file in order to match an endpoint to dispatch the next GitHub event to:
GitHub events generated by
openshift/ci-toolswill be forwarded to hook oncore-ci, everything else to standard hook onapp.ci. In this way we can gradually add new orgs/repos and assess the result.Summary
app.citocore-ci.core-ci, while unmatched events use the defaultapp.ciHook endpoint.