Skip to content

Commit 232492e

Browse files
weili-broadcomchombium
authored andcommitted
Exempt non-network schemes from drain-specific network checks in poller
c62111f added secure-endpoint/metrics-endpoint/structured-format to allowedSchemes so non-syslog CUPS bindings (e.g. TAS Metric Registrar's documented tagging convention) aren't dropped from the shared /v2/bindings store. However, checkBindings still ran the syslog-drain network validations (hostname presence, log type filters, DNS resolution, IP blacklist) on these bindings too, which don't apply since they're used as opaque discovery tags rather than real drain endpoints - e.g. "metrics-endpoint:///metrics" has no host at all, and "structured-format://DogStatsD"'s host segment is not a resolvable DNS name. Both still got rejected, just with a different error. Skip those network checks for schemes in the new nonNetworkSchemes list; credential validation still applies to all schemes. Adds regression tests reproducing the exact URL shapes from the bug report (#727).
1 parent a7e9c6a commit 232492e

2 files changed

Lines changed: 80 additions & 35 deletions

File tree

src/pkg/binding/poller.go

Lines changed: 40 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"net"
1010
"net/http"
1111
"net/url"
12+
"slices"
1213
"strings"
1314
"time"
1415

@@ -78,6 +79,14 @@ type Setter interface {
7879
// belong to the Syslog Agent. If a particular downstream consumer doesn't support some scheme, it should handle the validation itself
7980
var allowedSchemes = []string{"syslog", "syslog-tls", "https", "https-batch", "secure-endpoint", "metrics-endpoint", "structured-format"}
8081

82+
// nonNetworkSchemes lists schemes used by downstream consumers other than the Syslog Agent as opaque
83+
// discovery tags on a CUPS binding (e.g. TAS Metric Registrar's documented "structured-format://" and
84+
// "metrics-endpoint://" convention) rather than as real syslog drain endpoints. Bindings using these
85+
// schemes aren't guaranteed to have a resolvable, or even present, hostname, so they must be exempt from
86+
// the syslog-drain-specific network checks below (hostname presence, log type filters, DNS resolution,
87+
// IP blacklist) - those only make sense for bindings that are actually dialed as drains.
88+
var nonNetworkSchemes = []string{"secure-endpoint", "metrics-endpoint", "structured-format"}
89+
8190
func NewPoller(
8291
ac client,
8392
pi time.Duration,
@@ -237,39 +246,41 @@ func (bc *bindingChecker) checkBindings(bindings []Binding) []Binding {
237246
continue
238247
}
239248

240-
if len(u.Host) == 0 {
241-
bc.rejectBinding(b.Credentials, fmt.Sprintf("No hostname found in syslog drain url %s", anonymousUrl.String()), true)
242-
continue
243-
}
249+
if !slices.Contains(nonNetworkSchemes, u.Scheme) {
250+
if len(u.Host) == 0 {
251+
bc.rejectBinding(b.Credentials, fmt.Sprintf("No hostname found in syslog drain url %s", anonymousUrl.String()), true)
252+
continue
253+
}
244254

245-
if invalidLogFilter(u) {
246-
bc.rejectBinding(b.Credentials, fmt.Sprintf("include-log-types and exclude-log-types cannot be used at the same time in syslog drain url %s", anonymousUrl.String()), true)
247-
continue
248-
}
255+
if invalidLogFilter(u) {
256+
bc.rejectBinding(b.Credentials, fmt.Sprintf("include-log-types and exclude-log-types cannot be used at the same time in syslog drain url %s", anonymousUrl.String()), true)
257+
continue
258+
}
249259

250-
sourceTypes := getUnknownSourceTypes(u.Query())
251-
if sourceTypes != nil {
252-
bc.rejectBinding(b.Credentials, fmt.Sprintf("Unknown log types '%s' in log type filter in syslog drain url %s", strings.Join(sourceTypes, ", "), anonymousUrl.String()), true)
253-
continue
254-
}
260+
sourceTypes := getUnknownSourceTypes(u.Query())
261+
if sourceTypes != nil {
262+
bc.rejectBinding(b.Credentials, fmt.Sprintf("Unknown log types '%s' in log type filter in syslog drain url %s", strings.Join(sourceTypes, ", "), anonymousUrl.String()), true)
263+
continue
264+
}
255265

256-
_, exists := bc.failedHostsCache.Get(u.Host)
257-
if exists {
258-
bc.rejectBinding(b.Credentials, fmt.Sprintf("Skipped resolve ip address for syslog drain with url %s due to prior failure", anonymousUrl.String()), false)
259-
continue
260-
}
266+
_, exists := bc.failedHostsCache.Get(u.Host)
267+
if exists {
268+
bc.rejectBinding(b.Credentials, fmt.Sprintf("Skipped resolve ip address for syslog drain with url %s due to prior failure", anonymousUrl.String()), false)
269+
continue
270+
}
261271

262-
ip, err := bc.checker.ResolveAddr(u.Host)
263-
if err != nil {
264-
bc.failedHostsCache.Set(u.Host, true)
265-
bc.rejectBinding(b.Credentials, fmt.Sprintf("Cannot resolve ip address for syslog drain with url %s", anonymousUrl.String()), true)
266-
continue
267-
}
272+
ip, err := bc.checker.ResolveAddr(u.Host)
273+
if err != nil {
274+
bc.failedHostsCache.Set(u.Host, true)
275+
bc.rejectBinding(b.Credentials, fmt.Sprintf("Cannot resolve ip address for syslog drain with url %s", anonymousUrl.String()), true)
276+
continue
277+
}
268278

269-
err = bc.checker.CheckBlacklist(ip)
270-
if err != nil {
271-
bc.rejectBinding(b.Credentials, fmt.Sprintf("Resolved ip address for syslog drain with url %s is blacklisted", anonymousUrl.String()), true, true)
272-
continue
279+
err = bc.checker.CheckBlacklist(ip)
280+
if err != nil {
281+
bc.rejectBinding(b.Credentials, fmt.Sprintf("Resolved ip address for syslog drain with url %s is blacklisted", anonymousUrl.String()), true, true)
282+
continue
283+
}
273284
}
274285

275286
var validCredentials []Credentials
@@ -316,13 +327,7 @@ func sendAppLogMessage(msg string, apps []App, appLogClient v2.LogClient, logger
316327
}
317328

318329
func invalidScheme(scheme string) bool {
319-
for _, s := range allowedSchemes {
320-
if s == scheme {
321-
return false
322-
}
323-
}
324-
325-
return true
330+
return !slices.Contains(allowedSchemes, scheme)
326331
}
327332

328333
// invalidLogFilter checks if both include-log-types and exclude-log-types are set

src/pkg/binding/poller_test.go

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -430,6 +430,46 @@ var _ = Describe("Poller", func() {
430430
Expect(bndChecker.blacklistedDrains).To(Equal(float64(0)))
431431
})
432432

433+
It("returns non-network-scheme bindings even without a hostname, like the Metric Registrar's documented convention", func() {
434+
// e.g. "metrics-endpoint:///metrics" - a discovery tag, not a real drain endpoint, and never
435+
// has a host. It must not be rejected by the "no hostname found" check.
436+
bindings := []Binding{
437+
{
438+
Url: "metrics-endpoint:///metrics",
439+
Credentials: []Credentials{
440+
{
441+
Apps: []App{{Hostname: "app-hostname0", AppID: "app-id-0"}},
442+
},
443+
},
444+
},
445+
}
446+
447+
filteredBindings := bndChecker.checkBindings(bindings)
448+
449+
Expect(filteredBindings).To(HaveLen(1))
450+
Expect(bndChecker.invalidDrains).To(Equal(float64(0)))
451+
})
452+
453+
It("returns non-network-scheme bindings even when their host cannot be resolved via DNS", func() {
454+
// e.g. "structured-format://DogStatsD" - the host segment is an opaque tag, not a resolvable
455+
// hostname, so it must not be rejected by the DNS resolution check.
456+
bindings := []Binding{
457+
{
458+
Url: "structured-format://fail_to_resolve_ip",
459+
Credentials: []Credentials{
460+
{
461+
Apps: []App{{Hostname: "app-hostname0", AppID: "app-id-0"}},
462+
},
463+
},
464+
},
465+
}
466+
467+
filteredBindings := bndChecker.checkBindings(bindings)
468+
469+
Expect(filteredBindings).To(HaveLen(1))
470+
Expect(bndChecker.invalidDrains).To(Equal(float64(0)))
471+
})
472+
433473
It("returns no binding if a host cannot be parsed from the given url", func() {
434474
bindings := []Binding{
435475
{

0 commit comments

Comments
 (0)