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

[NDMII-3058] Add synchronous rDNS lookup to rdnsquerier component #30002

Merged
merged 33 commits into from
Oct 28, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
28291f0
Allow rdsquerier to run in snmp core check
vicweiss Oct 9, 2024
6fdcbf4
Move Sync rDNS lookup to querier
vicweiss Oct 10, 2024
14d6fd0
add release notes
vicweiss Oct 10, 2024
e33a05e
Add component for rdnsQuerierImplNone
vicweiss Oct 10, 2024
b186c38
Add component mock
vicweiss Oct 16, 2024
96d868f
debug: add log line
vicweiss Oct 16, 2024
44a9d29
Fix: SNMP CLI command, missing rdsquerier fx import
vicweiss Oct 17, 2024
13be360
Add timeout the sync rDNS resolver
vicweiss Oct 18, 2024
050bda0
Make it thread safe
vicweiss Oct 18, 2024
03aeb57
Make the linters :)
vicweiss Oct 18, 2024
8630743
fix race condition
vicweiss Oct 18, 2024
0327484
Fix timeout, added tests
vicweiss Oct 18, 2024
05da484
Remove debug logs
vicweiss Oct 21, 2024
1638c35
Log errors
vicweiss Oct 21, 2024
64ebedf
Merge branch 'main' into vic.weiss/snmp_rdns_enrichment
vicweiss Oct 21, 2024
d2f57ae
Linter fixes
vicweiss Oct 21, 2024
a456375
More linting
vicweiss Oct 21, 2024
d115431
Update releasenotes/notes/sndm-rdns-hostname-enrichment-1ca16478f8ebe…
vicweiss Oct 22, 2024
0f734f1
rename GetHostname to GetHostnameAsync
ken-schneider Oct 23, 2024
b2c0cca
use channels instead of a lock
ken-schneider Oct 23, 2024
4d29545
use table test for unit test
ken-schneider Oct 23, 2024
5df33c0
Remove rDNS querier from SNMP core check (#30439)
vicweiss Oct 23, 2024
4da5e29
add function for multiple IPs parallel
ken-schneider Oct 24, 2024
f9978d9
add tests for GetHostnames
ken-schneider Oct 24, 2024
66e64d8
fix linter error
ken-schneider Oct 24, 2024
6073f3f
update component definition, add mock
ken-schneider Oct 24, 2024
c69817b
udpate naming
ken-schneider Oct 24, 2024
312a20c
add import to test
ken-schneider Oct 24, 2024
de28955
remove newline
ken-schneider Oct 25, 2024
92fcea3
separate out single query logic
ken-schneider Oct 25, 2024
2ebb1ae
add delay to test
ken-schneider Oct 25, 2024
a77b6c9
increase test delay
ken-schneider Oct 25, 2024
c704f60
fix race condition
ken-schneider Oct 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions cmd/agent/subcommands/run/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ import (
"github.com/DataDog/datadog-agent/pkg/util/optional"
"github.com/DataDog/datadog-agent/pkg/version"

rdnsquerier "github.com/DataDog/datadog-agent/comp/rdnsquerier/def"

// runtime init routines
ddruntime "github.com/DataDog/datadog-agent/pkg/runtime"
)
Expand Down Expand Up @@ -255,6 +257,7 @@ func run(log log.Component,
settings settings.Component,
_ optional.Option[gui.Component],
_ agenttelemetry.Component,
rdnsquerier rdnsquerier.Component,
) error {
defer func() {
stopAgent()
Expand Down Expand Up @@ -319,6 +322,7 @@ func run(log log.Component,
cloudfoundrycontainer,
jmxlogger,
settings,
rdnsquerier,
); err != nil {
return err
}
Expand Down Expand Up @@ -494,6 +498,7 @@ func startAgent(
_ cloudfoundrycontainer.Component,
jmxLogger jmxlogger.Component,
settings settings.Component,
rdnsquerier rdnsquerier.Component,
) error {
var err error

Expand Down Expand Up @@ -571,6 +576,7 @@ func startAgent(

// TODO: (components) - Until the checks are components we set there context so they can depends on components.
check.InitializeInventoryChecksContext(invChecks)
check.InitializeRDNSQuerierContext(rdnsquerier)
vicweiss marked this conversation as resolved.
Show resolved Hide resolved

// Init JMX runner and inject dogstatsd component
jmxfetch.InitRunner(server, jmxLogger)
Expand Down
1 change: 1 addition & 0 deletions comp/rdnsquerier/def/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@ package rdnsquerier
// Component is the component type.
type Component interface {
GetHostname([]byte, func(string), func(string, error)) error
GetHostnameSync(string) (string, error)
}
5 changes: 5 additions & 0 deletions comp/rdnsquerier/impl-none/none.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,8 @@ func (q *rdnsQuerierImplNone) GetHostname(_ []byte, _ func(string), _ func(strin
// noop
return nil
}

func (q *rdnsQuerierImplNone) GetHostnameSync(_ string) (string, error) {
// noop
return "", nil
}
48 changes: 48 additions & 0 deletions comp/rdnsquerier/impl/rdnsquerier.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ package rdnsquerierimpl
import (
"context"
"fmt"
"net"
"net/netip"
"sync"

"github.com/DataDog/datadog-agent/comp/core/config"
log "github.com/DataDog/datadog-agent/comp/core/log/def"
Expand Down Expand Up @@ -181,6 +183,52 @@ func (q *rdnsQuerierImpl) GetHostname(ipAddr []byte, updateHostnameSync func(str
return nil
}

// GetHostnameSync attempts to resolve the hostname for the given IP address synchronously.
// If the IP address is invalid then an error is returned.
// If the IP address is not in the private address space then it is ignored - no lookup is performed and nil error is returned.
// If the IP address is in the private address space then the IP address will be resolved to a hostname.
func (q *rdnsQuerierImpl) GetHostnameSync(ipAddr string) (string, error) {
q.internalTelemetry.total.Inc()
Copy link
Contributor

Choose a reason for hiding this comment

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

This function calls GetHostname which also increments this value, so every use of GetHostnameSync will be counted twice. Same for the private counter.

Is there any reason to duplicate the telemetry and address verification from GetHostname here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, thinking about this maybe it makes sense to have the public endpoints call a helper function that just contains the logic. That way the metrics can be incremented separately for each one more easily.


// netipAddr, ok := netip.AddrFromSlice(ipAddr)
netipAddr := net.ParseIP(ipAddr).To4()
ken-schneider marked this conversation as resolved.
Show resolved Hide resolved
if netipAddr == nil {
q.internalTelemetry.invalidIPAddress.Inc()
return "", fmt.Errorf("invalid IP address %v", ipAddr)
}

if !netipAddr.IsPrivate() {
AlexandreYang marked this conversation as resolved.
Show resolved Hide resolved
q.logger.Tracef("Reverse DNS Enrichment IP address %s is not in the private address space", netipAddr)
return "", nil
}
q.internalTelemetry.private.Inc()

var hostname string
var err error
var wg sync.WaitGroup

wg.Add(1)
err = q.GetHostname(
netipAddr,
func(h string) {
hostname = h
wg.Done()
},
func(h string, e error) {
hostname = h
err = e
wg.Done()
},
)
if err != nil {
q.logger.Tracef("Error resolving reverse DNS enrichment for source IP address: %v error: %v", ipAddr, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to return the error here?

wg.Done()
}
wg.Wait()

return hostname, err
}

func (q *rdnsQuerierImpl) start(_ context.Context) error {
if q.started {
q.logger.Debugf("Reverse DNS Enrichment already started")
Expand Down
43 changes: 43 additions & 0 deletions comp/rdnsquerier/impl/rdnsquerier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -986,3 +986,46 @@ func TestCachePersist(t *testing.T) {
})
ts.validateExpected(t, expectedTelemetry)
}

func TestGetHostnameSync(t *testing.T) {
overrides := map[string]interface{}{
"network_devices.netflow.reverse_dns_enrichment_enabled": true,
}
ts := testSetup(t, overrides, true, nil)
internalRDNSQuerier := ts.rdnsQuerier.(*rdnsQuerierImpl)

// Test with invalid IP address
hostname, err := internalRDNSQuerier.GetHostnameSync("invalid_ip")
assert.Error(t, err)
ken-schneider marked this conversation as resolved.
Show resolved Hide resolved
assert.Equal(t, "", hostname)

// Test with IP address not in private range
hostname, err = internalRDNSQuerier.GetHostnameSync("8.8.8.8")
assert.NoError(t, err)
assert.Equal(t, "", hostname)

// Test with IP address in private range
hostname, err = internalRDNSQuerier.GetHostnameSync("192.168.1.100")
assert.NoError(t, err)
assert.NotEqual(t, "", hostname)

// Test with IP address in private range but cache miss
hostname, err = internalRDNSQuerier.GetHostnameSync("192.168.1.101")
assert.NoError(t, err)
assert.NotEqual(t, "", hostname)

// // Test with a valid IP address that resolves to a hostname
hostname, err = internalRDNSQuerier.GetHostnameSync("192.168.1.100") // cached from earlier test
assert.NoError(t, err)
assert.Equal(t, "fakehostname-192.168.1.100", hostname)

// Test with an empty string as input
hostname, err = internalRDNSQuerier.GetHostnameSync("")
assert.Error(t, err)
assert.Equal(t, "", hostname)

// Test with an IPv6 address
hostname, err = internalRDNSQuerier.GetHostnameSync("2001:4860:4860::8888") // Google Public DNS
assert.Error(t, err)
assert.Equal(t, "", hostname)
}
4 changes: 4 additions & 0 deletions pkg/cli/subcommands/check/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ import (
integrations "github.com/DataDog/datadog-agent/comp/logs/integrations/def"
"github.com/DataDog/datadog-agent/comp/metadata/inventorychecks"
"github.com/DataDog/datadog-agent/comp/metadata/inventorychecks/inventorychecksimpl"
rdnsquerier "github.com/DataDog/datadog-agent/comp/rdnsquerier/def"
"github.com/DataDog/datadog-agent/comp/remote-config/rcservice"
"github.com/DataDog/datadog-agent/comp/remote-config/rcservicemrf"
"github.com/DataDog/datadog-agent/comp/serializer/compression/compressionimpl"
Expand Down Expand Up @@ -264,6 +265,7 @@ func run(
jmxLogger jmxlogger.Component,
telemetry telemetry.Component,
logReceiver optional.Option[integrations.Component],
rdnsquerier rdnsquerier.Component,
) error {
previousIntegrationTracing := false
previousIntegrationTracingExhaustive := false
Expand All @@ -288,6 +290,8 @@ func run(

// TODO: (components) - Until the checks are components we set there context so they can depends on components.
check.InitializeInventoryChecksContext(invChecks)
check.InitializeRDNSQuerierContext(rdnsquerier)

pkgcollector.InitPython(common.GetPythonPaths()...)
commonchecks.RegisterChecks(wmeta, config, telemetry)

Expand Down
26 changes: 25 additions & 1 deletion pkg/collector/check/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"sync"

"github.com/DataDog/datadog-agent/comp/metadata/inventorychecks"
rdnsquerier "github.com/DataDog/datadog-agent/comp/rdnsquerier/def"
)

// checkContext holds a list of reference to different components used by Go and Python checks.
Expand All @@ -20,7 +21,8 @@ import (
// of C to Go. This way python checks can submit metadata to inventorychecks through the 'SetCheckMetadata' python
// method.
type checkContext struct {
ic inventorychecks.Component
ic inventorychecks.Component
rdnsquerier rdnsquerier.Component
vicweiss marked this conversation as resolved.
Show resolved Hide resolved
}

var ctx checkContext
Expand All @@ -47,10 +49,32 @@ func InitializeInventoryChecksContext(ic inventorychecks.Component) {
}
}

// GetRDNSQuerierContext returns a reference to the rdnsquerier component for Python and Go checks to use.
func GetRDNSQuerierContext() (rdnsquerier.Component, error) {
checkContextMutex.Lock()
defer checkContextMutex.Unlock()

if ctx.rdnsquerier == nil {
return nil, errors.New("rdnsquerier context was not set")
}
return ctx.rdnsquerier, nil
}

// InitializeRDNSQuerierContext set the reference to rdnsquerier in checkContext
func InitializeRDNSQuerierContext(rdnsquerier rdnsquerier.Component) {
checkContextMutex.Lock()
defer checkContextMutex.Unlock()

if ctx.rdnsquerier == nil {
ctx.rdnsquerier = rdnsquerier
}
}

// ReleaseContext reset to nil all the references hold by the current context
func ReleaseContext() {
checkContextMutex.Lock()
defer checkContextMutex.Unlock()

ctx.ic = nil
ctx.rdnsquerier = nil
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/DataDog/datadog-agent/pkg/networkdevice/profile/profiledefinition"
"github.com/DataDog/datadog-agent/pkg/networkdevice/utils"

"github.com/DataDog/datadog-agent/pkg/collector/check"
"github.com/DataDog/datadog-agent/pkg/collector/corechecks/snmp/internal/checkconfig"
"github.com/DataDog/datadog-agent/pkg/collector/corechecks/snmp/internal/common"
"github.com/DataDog/datadog-agent/pkg/collector/corechecks/snmp/internal/lldp"
Expand Down Expand Up @@ -214,6 +215,11 @@ func buildNetworkDeviceMetadata(deviceID string, idTags []string, config *checkc
vendor = config.ProfileDef.Device.Vendor
}

hostname := ""
if rdnsquerier, err := check.GetRDNSQuerierContext(); err == nil {
hostname, _ = rdnsquerier.GetHostnameSync(config.IPAddress)
vicweiss marked this conversation as resolved.
Show resolved Hide resolved
}

return devicemetadata.DeviceMetadata{
ID: deviceID,
IDTags: idTags,
Expand All @@ -238,6 +244,7 @@ func buildNetworkDeviceMetadata(deviceID string, idTags []string, config *checkc
OsHostname: osHostname,
DeviceType: deviceType,
Integration: common.SnmpIntegrationName,
RDNSHostname: hostname,
}
}

Expand Down
1 change: 1 addition & 0 deletions pkg/networkdevice/metadata/payload.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ type DeviceMetadata struct {
OsHostname string `json:"os_hostname,omitempty"`
Integration string `json:"integration,omitempty"` // indicates the source of the data SNMP, meraki_api, etc.
DeviceType string `json:"device_type,omitempty"`
RDNSHostname string `json:"rdns_hostname,omitempty"`
}

// DeviceOID device scan oid data
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# Each section from every release note are combined when the
# CHANGELOG.rst is rendered. So the text needs to be worded so that
# it does not depend on any information only available in another
# section. This may mean repeating some details, but each section
# must be readable independently of the other.
#
# Each section note must be formatted as reStructuredText.
---
features:
- |
Added support for enrighing SNMP IPs with hostnames via rDNS lookups
vicweiss marked this conversation as resolved.
Show resolved Hide resolved
Loading