Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
310dabc
refactor error handling, move validation to scrape function
michael-burt May 28, 2025
77416d0
add changelog
michael-burt May 28, 2025
89259d7
reorder validation for cross-platform consistency
michael-burt May 28, 2025
15c7ff6
use errors.Is to confirm file exists
michael-burt May 28, 2025
cd22a4e
fix windows issues
michael-burt May 28, 2025
349c311
fix windows issues
michael-burt May 28, 2025
f446f5a
fix windows issues
michael-burt May 28, 2025
fdeb228
Merge branch 'main' into tlscheck-warn-on-err-targets
michael-burt May 28, 2025
7400b26
Merge branch 'main' into tlscheck-warn-on-err-targets
michael-burt May 28, 2025
4a98797
Merge branch 'main' into tlscheck-warn-on-err-targets
michael-burt May 28, 2025
0895937
address PR comments
michael-burt May 29, 2025
981ba62
fix test
michael-burt May 29, 2025
bcbba1b
Merge branch 'main' into tlscheck-warn-on-err-targets
michael-burt May 29, 2025
6809403
Merge branch 'main' into tlscheck-warn-on-err-targets
michael-burt May 29, 2025
6e87da3
Merge branch 'main' into tlscheck-warn-on-err-targets
michael-burt Jun 2, 2025
a868491
Merge branch 'main' into tlscheck-warn-on-err-targets
michael-burt Jun 3, 2025
8122029
Merge branch 'main' into tlscheck-warn-on-err-targets
michael-burt Jun 3, 2025
d5c93bf
Merge branch 'main' into tlscheck-warn-on-err-targets
michael-burt Jun 4, 2025
cce662f
Merge branch 'main' into tlscheck-warn-on-err-targets
atoulme Jun 4, 2025
75be6f0
Merge branch 'main' into tlscheck-warn-on-err-targets
michael-burt Jun 4, 2025
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
27 changes: 27 additions & 0 deletions .chloggen/tlscheck-error-handling.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: tlscheckreceiver

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Do not crash on target validation & implement better scrape errors

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [40341]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [user]
70 changes: 0 additions & 70 deletions receiver/tlscheckreceiver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,6 @@ package tlscheckreceiver // import "github.com/open-telemetry/opentelemetry-coll

import (
"errors"
"fmt"
"net"
"os"
"path/filepath"
"strconv"
"strings"

"go.opentelemetry.io/collector/config/confignet"
"go.opentelemetry.io/collector/scraper/scraperhelper"
Expand Down Expand Up @@ -42,77 +36,13 @@ type Config struct {
_ struct{}
}

func validatePort(port string) error {
portNum, err := strconv.Atoi(port)
if err != nil {
return fmt.Errorf("provided port is not a number: %s", port)
}
if portNum < 1 || portNum > 65535 {
return fmt.Errorf("provided port is out of valid range (1-65535): %d", portNum)
}
return nil
}

func validateTarget(ct *CertificateTarget) error {
// Check that exactly one of endpoint or file_path is specified
if ct.Endpoint != "" && ct.FilePath != "" {
return errors.New("cannot specify both endpoint and file_path")
}
if ct.Endpoint == "" && ct.FilePath == "" {
return errors.New("must specify either endpoint or file_path")
}

// Validate endpoint if specified
if ct.Endpoint != "" {
Copy link
Member

Choose a reason for hiding this comment

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

Do these changes mean the collector can now start in an invalid state? We normally like to catch that at startup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still do this validation, but we do it in the scrape function. The user can now include an invalid target, but it will be logged, the otelcol_scraper_errored_metric_points will be incremented, and the collector will not crash. As currently implemented, the collector will fail to start if there is an invalid target in the TLS Check configuration, such as a nonexistent file certificate. I do not think this is desirable behavior for the user. The new behavior matches that of the File Stats Receiver, HTTP Check Receiver, and others.

if strings.Contains(ct.Endpoint, "://") {
return fmt.Errorf("endpoint contains a scheme, which is not allowed: %s", ct.Endpoint)
}

_, port, err := net.SplitHostPort(ct.Endpoint)
if err != nil {
return fmt.Errorf("%s: %w", errInvalidEndpoint.Error(), err)
}

if err := validatePort(port); err != nil {
return fmt.Errorf("%s: %w", errInvalidEndpoint.Error(), err)
}
}

// Validate file path if specified
if ct.FilePath != "" {
// Clean the path to handle different path separators
cleanPath := filepath.Clean(ct.FilePath)

// Check if the path is absolute
if !filepath.IsAbs(cleanPath) {
return fmt.Errorf("file path must be absolute: %s", ct.FilePath)
}

// Check if path exists and is a regular file
fileInfo, err := os.Stat(cleanPath)
if err != nil {
if os.IsNotExist(err) {
return fmt.Errorf("certificate file does not exist: %s", ct.FilePath)
}
return fmt.Errorf("error accessing certificate file %s: %w", ct.FilePath, err)
}

// check if it is a directory
if fileInfo.IsDir() {
return fmt.Errorf("path is a directory, not a file: %s", cleanPath)
}

// Check if it's a regular file (not a directory or special file)
if !fileInfo.Mode().IsRegular() {
return fmt.Errorf("certificate path is not a regular file: %s", ct.FilePath)
}

// Check if file is readable
if _, err := os.ReadFile(cleanPath); err != nil {
return fmt.Errorf("certificate file is not readable: %s", ct.FilePath)
}
}

return nil
}

Expand Down
80 changes: 0 additions & 80 deletions receiver/tlscheckreceiver/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@ package tlscheckreceiver // import "github.com/open-telemetry/opentelemetry-coll

import (
"errors"
"fmt"
"os"
"path/filepath"
"testing"
"time"

Expand Down Expand Up @@ -92,34 +90,6 @@ func TestValidate(t *testing.T) {
},
expectedErr: nil,
},
{
desc: "invalid endpoint",
cfg: &Config{
Targets: []*CertificateTarget{
{
TCPAddrConfig: confignet.TCPAddrConfig{
Endpoint: "bad-endpoint:12efg",
},
},
},
ControllerConfig: scraperhelper.NewDefaultControllerConfig(),
},
expectedErr: fmt.Errorf("%w: provided port is not a number: 12efg", errInvalidEndpoint),
},
{
desc: "endpoint with scheme",
cfg: &Config{
Targets: []*CertificateTarget{
{
TCPAddrConfig: confignet.TCPAddrConfig{
Endpoint: "https://example.com:443",
},
},
},
ControllerConfig: scraperhelper.NewDefaultControllerConfig(),
},
expectedErr: errors.New("endpoint contains a scheme, which is not allowed: https://example.com:443"),
},
{
desc: "both endpoint and file path",
cfg: &Config{
Expand All @@ -135,56 +105,6 @@ func TestValidate(t *testing.T) {
},
expectedErr: errors.New("cannot specify both endpoint and file_path"),
},
{
desc: "relative file path",
cfg: &Config{
Targets: []*CertificateTarget{
{
FilePath: "cert.pem",
},
},
ControllerConfig: scraperhelper.NewDefaultControllerConfig(),
},
expectedErr: errors.New("file path must be absolute: cert.pem"),
},
{
desc: "nonexistent file",
cfg: &Config{
Targets: []*CertificateTarget{
{
FilePath: filepath.Join(tmpDir, "nonexistent.pem"),
},
},
ControllerConfig: scraperhelper.NewDefaultControllerConfig(),
},
expectedErr: errors.New("certificate file does not exist"),
},
{
desc: "directory instead of file",
cfg: &Config{
Targets: []*CertificateTarget{
{
FilePath: tmpDir,
},
},
ControllerConfig: scraperhelper.NewDefaultControllerConfig(),
},
expectedErr: fmt.Errorf("path is a directory, not a file: %s", tmpDir),
},
{
desc: "port out of range",
cfg: &Config{
Targets: []*CertificateTarget{
{
TCPAddrConfig: confignet.TCPAddrConfig{
Endpoint: "example.com:67000",
},
},
},
ControllerConfig: scraperhelper.NewDefaultControllerConfig(),
},
expectedErr: fmt.Errorf("%w: provided port is out of valid range (1-65535): 67000", errInvalidEndpoint),
},
}

for _, tc := range testCases {
Expand Down
Loading
Loading