Feature/systemd query events #1728
Conversation
📝 WalkthroughWalkthroughThe pull request introduces updates to the dependency management in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (15)
service/firewall/bypassing.go (2)
49-49: Consider strengthening the system resolver check.The TODO comment suggests uncertainty about the specificity of the check. Consider adding additional validation such as verifying the process path or using a more comprehensive identification method.
46-56: Document the resolver disabled behavior.Given that this introduces significant new behavior when the resolver is disabled, consider adding a function-level comment explaining:
- When and why the resolver might be disabled
- Security implications of the disabled state
- Interaction with systemd-resolver
service/firewall/interception/dnslistener/varlinktypes.go (3)
1-4: Enhance package documentation.Consider adding more detailed documentation about:
- The varlink protocol and its role in systemd-resolver
- The relationship between these types and the DNS event handling
- Any relevant systemd-resolver documentation references
package dnslistener -// List of struct that define the systemd-resolver varlink dns event protocol. +// Package dnslistener provides types for interacting with systemd-resolver's varlink DNS event protocol. +// These types facilitate the communication between the application and systemd-resolver for DNS query events. +// +// For more information about the protocol, see: +// https://www.freedesktop.org/software/systemd/man/org.freedesktop.resolve1.html
5-9: Add field documentation and consider using DNS-specific types.The struct would benefit from:
- Documentation explaining the purpose of each field
- Using DNS-specific constants for Class and Type fields
+// ResourceKey represents a unique identifier for a DNS resource record. type ResourceKey struct { + // Class represents the DNS class (e.g., IN for Internet) Class int `json:"class"` + // Type represents the DNS record type (e.g., A, AAAA, MX) Type int `json:"type"` + // Name is the domain name for this resource Name string `json:"name"` }Consider defining constants for common DNS classes and types:
// DNS Classes const ( ClassIN = 1 // Internet ClassCS = 2 // CSNET ClassCH = 3 // CHAOS ClassHS = 4 // Hesiod ) // DNS Types const ( TypeA = 1 // IPv4 address TypeNS = 2 // Nameserver TypeCNAME = 5 // Canonical name TypeSOA = 6 // Start of authority TypePTR = 12 // Pointer TypeMX = 15 // Mail exchange TypeTXT = 16 // Text TypeAAAA = 28 // IPv6 address )
65-69: Add struct and field documentation.The struct would benefit from clear documentation explaining its purpose and the significance of each field.
+// Answer represents a DNS response containing either a parsed resource record +// or the raw DNS response data. type Answer struct { + // RR is the parsed DNS resource record, if available RR *ResourceRecord `json:"rr,omitempty"` + // Raw contains the unparsed DNS response string Raw string `json:"raw"` + // IfIndex is the network interface index where this answer was received IfIndex *int `json:"ifindex,omitempty"` }service/compat/module.go (2)
102-106: Consider standardizing skip handling witherrSelfcheckSkipped.The early return for disabled resolver is logically sound, but for consistency with other skip conditions (like the one handled by
errSelfcheckSkipped), consider using the same error pattern.res := module.instance.Resolver() if res.IsDisabled.IsSet() { - log.Debugf("compat: skipping self-check: resolver is disabled") - return nil + return nil, fmt.Errorf("%w: resolver is disabled", errSelfcheckSkipped) }
192-192: LGTM! Consider documenting interface changes.The new
Resolver()method is a good addition for accessing resolver state. Consider adding interface documentation to explain the contract, especially regarding initialization guarantees and thread safety requirements.go.mod (1)
Line range hint
5-6: Track temporary winres dependency replacement.The TODO comment indicates this is a temporary fix waiting for upstream PR #4. Consider:
- Adding a GitHub issue to track the removal of this replacement
- Adding a link to the upstream PR for better context
Would you like me to create a GitHub issue to track this?
service/resolver/main.go (2)
32-33: Add documentation for the exported field.Since
IsDisabledis an exported field that other packages may use, please add a documentation comment explaining its purpose, when it's set, and how it should be used.+// IsDisabled indicates whether the resolver module is currently disabled. +// This is typically set when no resolvers are configured. IsDisabled abool.AtomicBool
32-33: Consider reflecting the disabled state in the state manager.The module uses
mgr.StateMgrfor state management, but the newIsDisabledstate is not reflected there. Consider adding the disabled state to the state manager to maintain consistency and enable proper monitoring.This could be done in the
loadResolversfunction (whereIsDisabledis set) by adding:if module.IsDisabled.IsSet() { module.states.Add("resolver:disabled", "The resolver module is currently disabled as no resolvers are configured") } else { module.states.Remove("resolver:disabled") }Also applies to: 272-274
service/instance.go (2)
192-195: Fix error message for consistencyThe error message uses "dns-listener" with a hyphen, while other module names in error messages don't use hyphens. Consider changing for consistency:
- return instance, fmt.Errorf("create dns-listener module: %w", err) + return instance, fmt.Errorf("create dnslistener module: %w", err)
473-476: Fix comment style for consistencyThe comment uses "dns-listener" with a hyphen, while other module names in comments don't use hyphens. Consider changing for consistency:
-// DNSListener returns the dns-listener module. +// DNSListener returns the dnslistener module.service/firewall/packet_handler.go (1)
Line range hint
447-459: Security review of DNS redirection logic.The implementation maintains robust security controls while adding flexibility:
- Multiple validation layers (resolver status, nameserver IP, port checks)
- Special handling for broadcast queries
- Proper separation between system and user DNS queries
Consider adding debug logging when DNS redirection is skipped due to disabled resolver to aid in troubleshooting.
Add debug logging:
case dnsQueryInterception() && module.instance.Resolver().IsDisabled.IsNotSet() && pkt.IsOutbound() && + // Log when redirection is skipped due to disabled resolver + func() bool { + if module.instance.Resolver().IsDisabled.IsSet() { + log.Tracer(pkt.Ctx()).Debugf("filter: skipping DNS redirection, resolver is disabled: %s", pkt) + } + return true + }() && pkt.Info().DstPort == 53 &&service/firewall/interception/dnslistener/module.go (2)
9-14: Organize imports according togcirecommendationsThe imports are not properly organized as indicated by the
gcistatic analysis tool. Consider runninggciwith--skip-generated -s standard -s defaultto sort and group imports according to Go standards.🧰 Tools
🪛 GitHub Check: Linter
[failure] 9-9:
File is notgci-ed with --skip-generated -s standard -s default (gci)
[failure] 14-14:
File is notgci-ed with --skip-generated -s standard -s default (gci)
144-146: Handle error frominfo.Save()appropriatelyCurrently, when
info.Save()returns an error, it is logged but not returned or handled further. Consider whether this error should be propagated or if additional error handling is necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.mod(2 hunks)service/compat/module.go(3 hunks)service/firewall/bypassing.go(1 hunks)service/firewall/interception/dnslistener/module.go(1 hunks)service/firewall/interception/dnslistener/varlinktypes.go(1 hunks)service/firewall/module.go(3 hunks)service/firewall/packet_handler.go(1 hunks)service/instance.go(5 hunks)service/resolver/main.go(2 hunks)service/resolver/resolvers.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Linter
service/firewall/interception/dnslistener/module.go
[failure] 9-9:
File is not gci-ed with --skip-generated -s standard -s default (gci)
[failure] 14-14:
File is not gci-ed with --skip-generated -s standard -s default (gci)
[failure] 48-48:
type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
[failure] 51-51:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
[failure] 60-60:
type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
[failure] 63-63:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
🔇 Additional comments (13)
service/firewall/bypassing.go (2)
46-56: Verify interaction with secure DNS bypass reporting.
The new exceptions for disabled resolver state are placed before the secure DNS filter list checks. This means connections that would normally trigger secure DNS bypass reporting might be allowed when the resolver is disabled.
Let's check the interaction:
#!/bin/bash
# Description: Verify how secure DNS bypass reporting works with disabled resolver
# Test: Check for any conditions or checks around secure DNS bypass reporting
rg "ReportSecureDNSBypassIssue"46-56: Verify security implications of bypass exceptions.
The new conditional block creates exceptions to bypass prevention when the Portmaster resolver is disabled. While the intent is clear, we should ensure these exceptions don't create security vulnerabilities.
Let's verify the impact and usage:
✅ Verification successful
The bypass exceptions are properly implemented with adequate security measures
The verification reveals that the bypass exceptions are well-integrated into the system's security architecture:
-
The system has comprehensive DNS bypass prevention:
- Bypass attempts trigger security notifications via
ReportSecureDNSBypassIssue - There's a global setting
Block Secure DNS Bypassingthat's enabled by default - The firewall maintains protection even when bypassing is attempted
- Bypass attempts trigger security notifications via
-
The localhost and system resolver exceptions are specifically handled:
- System resolver checks are consistently implemented across the codebase
- Localhost scope checks are used in multiple security contexts with proper validation
- The exceptions only apply when the Portmaster resolver is explicitly disabled
-
The conditional block maintains security by:
- Only allowing necessary traffic when the resolver is disabled
- Properly identifying system resolver processes
- Limiting localhost exceptions to DNS port 53
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other security measures or validations when resolver is disabled
# Test 1: Check for any additional security checks or validations when resolver is disabled
rg -A 5 "IsDisabled.IsSet\(\)"
# Test 2: Check how localhost connections are handled elsewhere
rg -A 5 "IsLocalhost\(\)"
Length of output: 400
Script:
#!/bin/bash
# Let's try a more comprehensive verification approach
# Check for IsDisabled usage with proper quoting
rg -A 5 "IsDisabled\.IsSet"
# Check how localhost connections are validated
rg -A 5 "IsLocalhost"
# Check for system resolver related checks
rg -A 5 "IsSystemResolver"
# Check for other DNS bypass prevention mechanisms
rg -A 5 "DNS.*bypass" -i
# Look for related security validations
rg -A 5 "ReportSecureDNSBypassIssue"
Length of output: 13343
service/firewall/interception/dnslistener/varlinktypes.go (1)
71-79: Add documentation and validate pointer usage.
The struct needs documentation, and some fields might not need to be pointers.
+// QueryResult represents the complete result of a DNS query from systemd-resolver,
+// including the query state, any errors, and the received answers.
type QueryResult struct {
+ // Ready indicates if the query has completed
Ready *bool `json:"ready,omitempty"`
+ // State describes the current state of the query
State *string `json:"state,omitempty"`
+ // Rcode is the DNS response code
Rcode *int `json:"rcode,omitempty"`
+ // Errno is the system error number, if any
Errno *int `json:"errno,omitempty"`
+ // Question contains the original DNS query
Question *[]ResourceKey `json:"question,omitempty"`
+ // CollectedQuestions contains additional related queries
CollectedQuestions *[]ResourceKey `json:"collectedQuestions,omitempty"`
+ // Answer contains the DNS responses
Answer *[]Answer `json:"answer,omitempty"`
}Consider whether some fields really need to be pointers. For example:
Readycould be a non-pointerboolwith a default value offalseStatecould be an enum type instead of a string pointerRcodeandErrnocould be non-pointer integers with sentinel values (-1) for "not set"
service/firewall/module.go (1)
19-19: LGTM: Clean integration of resolver module access.
The addition of the resolver module access follows the established pattern of module getter methods in the interface. This change maintains consistency with the existing architecture.
Let's verify that all implementations of the instance interface have been updated:
Also applies to: 168-168
✅ Verification successful
Let me gather more information to verify the interface implementation comprehensively.
LGTM: The resolver module integration is properly implemented.
The verification confirms that:
- The
Instancestruct is the only implementation of theinstanceinterface - The
Instancestruct has both the required fieldresolver *resolver.ResolverModuleand the methodResolver() *resolver.ResolverModule - All interface methods are implemented by the
Instancestruct
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all potential implementations of the instance interface
# to ensure they've been updated with the new Resolver() method.
# Search for struct types that might implement the interface
ast-grep --pattern 'type $name struct {
$$$
}'
# Search for interface method implementations
rg -A 1 'func \([^)]+\) Resolver\(\)'
Length of output: 206
Script:
#!/bin/bash
# First, let's find the interface definition to understand all required methods
rg -A 10 "type instance interface" service/firewall/module.go
# Then find all structs that could implement this interface
rg "type.*struct" service/instance.go
# Finally, check if Instance is the only implementation
fd -e go | xargs rg "type.*struct.*{"
Length of output: 496
Script:
#!/bin/bash
# Let's fix the regex and search for struct implementations
fd -e go | xargs rg "type.*struct"
# Also check if Instance implements all interface methods
rg "func \(i \*Instance\)" service/instance.go
# And verify the resolver field exists in Instance
rg "resolver.*\*resolver\.ResolverModule" service/instance.go
Length of output: 40121
service/compat/module.go (1)
127-128:
Potential race condition in resolver state handling.
The early return (lines 102-106) should prevent execution from reaching this point when the resolver is disabled. The fact that we're checking again suggests the resolver state could change during self-check execution. Consider:
- Adding atomic operations or proper synchronization
- Removing this redundant check
- Adding a comment explaining why both checks are necessary
Let's verify if the resolver state can change during execution:
go.mod (2)
37-37: Document new dependency purposes.
Two new direct dependencies have been added:
github.com/maruel/panicparse/v2: Consider documenting why this debugging tool is neededgithub.com/varlink/go: Required for systemd-resolver DNS event protocol integration
#!/bin/bash
# Check for usage of new dependencies in codebase
echo "Checking panicparse usage..."
rg "panicparse" --type go
echo "Checking varlink usage..."
rg "varlink" --type goConsider adding comments in the code or documentation explaining the purpose of these dependencies.
Also applies to: 61-61
Line range hint 3-3: Verify Go 1.22.0 compatibility requirements.
Go 1.22.0 is a very recent version (released Feb 2024). Please ensure all deployment environments and CI/CD pipelines support this version.
service/instance.go (4)
22-22: LGTM!
The import is correctly placed and follows the project's import grouping convention.
78-78: LGTM!
The dnslistener field is correctly placed and follows the established pattern for module fields.
297-297: LGTM!
The dnslistener module is correctly registered in the service group, maintaining the logical grouping with other interception-related modules.
78-78: Verify SleepyModule interface implementation
Since this is a new module in the service group, verify if it needs to implement the SleepyModule interface like other modules. This interface is used by the SetSleep method to manage module sleep states.
Also applies to: 192-195, 297-297, 473-476
service/resolver/resolvers.go (1)
401-408: 🛠️ Refactor suggestion
Consider moving the disabled state management after resolver validation.
The current implementation sets the module's disabled state before validating resolvers and potentially loading defaults. This could lead to incorrect state representation if:
- The configured resolvers are invalid and defaults are loaded
- Other modules check the disabled state while resolvers are being loaded
Consider moving this block after the resolver validation and default loading (after line 440) to ensure the state accurately reflects the final resolver configuration:
- // If no resolvers are configure set the disabled state. So other modules knows that the users does not want to use Portmaster resolver.
- if len(newResolverConfig) == 0 {
- module.IsDisabled.Set()
- } else {
- module.IsDisabled.UnSet()
- }
currentResolverConfig = newResolverConfig
newResolvers := append(
getConfiguredResolvers(newResolverConfig),
getSystemResolvers()...,
)
if len(newResolvers) == 0 {
// load defaults directly, overriding config system
newResolvers = getConfiguredResolvers(defaultNameServers)
...
}
+
+ // Set the disabled state after resolver validation and default loading
+ if len(newResolvers) == 0 {
+ module.IsDisabled.Set()
+ } else {
+ module.IsDisabled.UnSet()
+ }Let's verify the impact of this state management on other modules:
service/firewall/packet_handler.go (1)
447-449: LGTM! Verify DNS redirection behavior.
The additional check for resolver status is correctly implemented, ensuring DNS queries are only redirected when the resolver is enabled.
Let's verify the resolver status handling:
✅ Verification successful
Let me gather more information about the DNS redirection logic to ensure a comprehensive verification.
DNS redirection logic is correctly implemented with resolver status check
The verification confirms that:
- The resolver status check
IsDisabled.IsNotSet()is consistently used across the codebase for DNS handling - The DNS redirection logic is properly integrated with the resolver status:
- In packet handling (
service/firewall/packet_handler.go) - In bypassing logic (
service/firewall/bypassing.go) - In compatibility checks (
service/compat/module.go)
- In packet handling (
- The implementation correctly ensures DNS queries are only redirected when:
- The resolver is enabled (IsDisabled.IsNotSet())
- The packet is outbound
- The destination port is 53
- The destination is not our nameserver
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent resolver status handling across the codebase
# Test: Search for other DNS redirection logic that might need similar checks
rg -A 5 "DstPort == 53"
# Test: Search for other resolver status checks to ensure consistent patterns
ast-grep --pattern 'Resolver().IsDisabled'
Length of output: 895
Script:
#!/bin/bash
# Search for DNS query interception function and its usage
ast-grep --pattern 'dnsQueryInterception()'
# Search for resolver status checks in a broader context
rg -A 5 "IsDisabled"
# Look for nameserver related functions to understand DNS handling
rg -A 5 "RerouteToNameserver"
Length of output: 11331
| type ResourceRecord struct { | ||
| Key ResourceKey `json:"key"` | ||
| Name *string `json:"name,omitempty"` | ||
| Address *[]byte `json:"address,omitempty"` | ||
| // Rest of the fields are not used. | ||
| // Priority *int `json:"priority,omitempty"` | ||
| // Weight *int `json:"weight,omitempty"` | ||
| // Port *int `json:"port,omitempty"` | ||
| // CPU *string `json:"cpu,omitempty"` | ||
| // OS *string `json:"os,omitempty"` | ||
| // Items *[]string `json:"items,omitempty"` | ||
| // MName *string `json:"mname,omitempty"` | ||
| // RName *string `json:"rname,omitempty"` | ||
| // Serial *int `json:"serial,omitempty"` | ||
| // Refresh *int `json:"refresh,omitempty"` | ||
| // Expire *int `json:"expire,omitempty"` | ||
| // Minimum *int `json:"minimum,omitempty"` | ||
| // Exchange *string `json:"exchange,omitempty"` | ||
| // Version *int `json:"version,omitempty"` | ||
| // Size *int `json:"size,omitempty"` | ||
| // HorizPre *int `json:"horiz_pre,omitempty"` | ||
| // VertPre *int `json:"vert_pre,omitempty"` | ||
| // Latitude *int `json:"latitude,omitempty"` | ||
| // Longitude *int `json:"longitude,omitempty"` | ||
| // Altitude *int `json:"altitude,omitempty"` | ||
| // KeyTag *int `json:"key_tag,omitempty"` | ||
| // Algorithm *int `json:"algorithm,omitempty"` | ||
| // DigestType *int `json:"digest_type,omitempty"` | ||
| // Digest *string `json:"digest,omitempty"` | ||
| // FPType *int `json:"fptype,omitempty"` | ||
| // Fingerprint *string `json:"fingerprint,omitempty"` | ||
| // Flags *int `json:"flags,omitempty"` | ||
| // Protocol *int `json:"protocol,omitempty"` | ||
| // DNSKey *string `json:"dnskey,omitempty"` | ||
| // Signer *string `json:"signer,omitempty"` | ||
| // TypeCovered *int `json:"type_covered,omitempty"` | ||
| // Labels *int `json:"labels,omitempty"` | ||
| // OriginalTTL *int `json:"original_ttl,omitempty"` | ||
| // Expiration *int `json:"expiration,omitempty"` | ||
| // Inception *int `json:"inception,omitempty"` | ||
| // Signature *string `json:"signature,omitempty"` | ||
| // NextDomain *string `json:"next_domain,omitempty"` | ||
| // Types *[]int `json:"types,omitempty"` | ||
| // Iterations *int `json:"iterations,omitempty"` | ||
| // Salt *string `json:"salt,omitempty"` | ||
| // Hash *string `json:"hash,omitempty"` | ||
| // CertUsage *int `json:"cert_usage,omitempty"` | ||
| // Selector *int `json:"selector,omitempty"` | ||
| // MatchingType *int `json:"matching_type,omitempty"` | ||
| // Data *string `json:"data,omitempty"` | ||
| // Tag *string `json:"tag,omitempty"` | ||
| // Value *string `json:"value,omitempty"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve documentation and consider separating unused fields.
The current approach of commenting out unused fields makes the code harder to maintain and understand.
Consider either:
- Moving unused fields to a separate file/type for future implementation
- Or documenting why these fields exist and when they might be needed
+// ResourceRecord represents a DNS resource record with its associated data.
+// Currently, only a subset of DNS record types are supported, focusing on
+// name resolution and addressing. Additional fields for other DNS record
+// types are planned for future implementation.
type ResourceRecord struct {
Key ResourceKey `json:"key"`
+ // Name is the canonical name for CNAME records
Name *string `json:"name,omitempty"`
+ // Address contains the IP address for A/AAAA records
Address *[]byte `json:"address,omitempty"`
- // Rest of the fields are not used.
- // Priority *int `json:"priority,omitempty"`
- // Weight *int `json:"weight,omitempty"`
- // Port *int `json:"port,omitempty"`
- // ... (other commented fields)
}Create a new file (e.g., future_types.go) for the unused fields:
// future_types.go
// FutureResourceRecord contains fields for additional DNS record types
// that may be implemented in the future.
type FutureResourceRecord struct {
Priority *int `json:"priority,omitempty"`
Weight *int `json:"weight,omitempty"`
Port *int `json:"port,omitempty"`
// ... (other fields)
}| mgr: m, | ||
| instance: instance, | ||
| IsDisabled: *abool.New(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary dereferencing of abool.New().
The abool.New() function returns a pointer to AtomicBool, but the field type is not a pointer. The dereferencing is unnecessary and could be error-prone.
- IsDisabled: *abool.New(),
+ IsDisabled: abool.New(),📝 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.
| mgr: m, | |
| instance: instance, | |
| IsDisabled: *abool.New(), | |
| mgr: m, | |
| instance: instance, | |
| IsDisabled: abool.New(), |
| if err != nil { | ||
| log.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %s", err) | ||
| return nil |
There was a problem hiding this comment.
Return error instead of nil on connection failure
In the Start() method, when failing to establish a varlink connection, the error is logged but nil is returned. Consider returning the error to allow the caller to handle it appropriately.
Apply this diff to fix the issue:
if err != nil {
log.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %s", err)
- return nil
+ return err
}📝 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.
| if err != nil { | |
| log.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %s", err) | |
| return nil | |
| if err != nil { | |
| log.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %s", err) | |
| return err |
| if varlinkErr, ok := err.(*varlink.Error); ok { | ||
| return fmt.Errorf("failed to issue Varlink call: %+v", varlinkErr.Parameters) | ||
| } else { | ||
| return fmt.Errorf("failed to issue Varlink call: %v", err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap errors using %w in fmt.Errorf
When returning errors with fmt.Errorf, use the %w verb to wrap the original error and maintain the error chain.
Apply this diff to refactor:
-return fmt.Errorf("failed to issue Varlink call: %v", err)
+return fmt.Errorf("failed to issue Varlink call: %w", err)📝 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.
| return fmt.Errorf("failed to issue Varlink call: %v", err) | |
| return fmt.Errorf("failed to issue Varlink call: %w", err) |
🧰 Tools
🪛 GitHub Check: Linter
[failure] 51-51:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
| if varlinkErr, ok := err.(*varlink.Error); ok { | ||
| return fmt.Errorf("failed to receive Varlink reply: %+v", varlinkErr.Parameters) | ||
| } else { | ||
| return fmt.Errorf("failed to receive Varlink reply: %v", err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap errors using %w in fmt.Errorf
Again, use %w to wrap errors when returning them with fmt.Errorf.
Apply this diff to refactor:
-return fmt.Errorf("failed to receive Varlink reply: %v", err)
+return fmt.Errorf("failed to receive Varlink reply: %w", err)📝 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.
| return fmt.Errorf("failed to receive Varlink reply: %v", err) | |
| return fmt.Errorf("failed to receive Varlink reply: %w", err) |
🧰 Tools
🪛 GitHub Check: Linter
[failure] 63-63:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
| if varlinkErr, ok := err.(*varlink.Error); ok { | ||
| return fmt.Errorf("failed to receive Varlink reply: %+v", varlinkErr.Parameters) | ||
| } else { | ||
| return fmt.Errorf("failed to receive Varlink reply: %v", err) |
There was a problem hiding this comment.
Use errors.As for error type assertions
Similar to the previous case, use errors.As to handle wrapped errors when checking for specific error types.
Apply this diff to fix the issue:
-if varlinkErr, ok := err.(*varlink.Error); ok {
+var varlinkErr *varlink.Error
+if errors.As(err, &varlinkErr) {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Linter
[failure] 60-60:
type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
[failure] 63-63:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
| if varlinkErr, ok := err.(*varlink.Error); ok { | ||
| return fmt.Errorf("failed to issue Varlink call: %+v", varlinkErr.Parameters) | ||
| } else { | ||
| return fmt.Errorf("failed to issue Varlink call: %v", err) |
There was a problem hiding this comment.
Use errors.As for error type assertions
Type assertions on errors may fail on wrapped errors. Use errors.As to check for specific error types, ensuring wrapped errors are properly handled.
Apply this diff to fix the issue:
-if varlinkErr, ok := err.(*varlink.Error); ok {
+var varlinkErr *varlink.Error
+if errors.As(err, &varlinkErr) {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Linter
[failure] 48-48:
type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
[failure] 51-51:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Enhancements
Documentation