Paramadon/instance store metrics - #1801
Conversation
…ramadon/fixingJmsDropTranslator
duhminick
left a comment
duhminick
left a comment
There was a problem hiding this comment.
I'm assuming that awsnvmereceiver is the final receiver for both EBS and instance store
| // Sanitize input by trimming whitespace and null bytes | ||
| device = strings.TrimSpace(device) | ||
| if strings.Contains(device, "\x00") { | ||
| return errors.New("device path cannot contain null bytes") | ||
| } | ||
|
|
||
| // Validate device path format first | ||
| if !strings.HasPrefix(device, "/dev/") { | ||
| return errors.New("device path must start with /dev/") | ||
| } | ||
|
|
||
| // Check for path traversal attempts - multiple patterns | ||
| if strings.Contains(device, "..") { | ||
| return errors.New("device path cannot contain '..'") | ||
| } | ||
| if strings.Contains(device, "./") { | ||
| return errors.New("device path cannot contain relative path components") | ||
| } | ||
| if strings.Contains(device, "//") { | ||
| return errors.New("device path cannot contain double slashes") | ||
| } | ||
|
|
||
| // Prevent directory traversal attacks | ||
| cleanPath := filepath.Clean(device) | ||
| if cleanPath != device { | ||
| return errors.New("device path contains invalid characters") | ||
| } | ||
|
|
||
| // Validate absolute path doesn't escape /dev | ||
| absPath, err := filepath.Abs(device) | ||
| if err != nil { | ||
| return errors.New("device path is not a valid absolute path") | ||
| } | ||
| if !strings.HasPrefix(absPath, "/dev/") { | ||
| return errors.New("device path must resolve to /dev/ directory") | ||
| } | ||
|
|
||
| // Validate NVMe device naming pattern with stricter regex-like validation | ||
| if !strings.HasPrefix(device, "/dev/nvme") { | ||
| return errors.New("device path must be an NVMe device (/dev/nvme*)") | ||
| } | ||
|
|
||
| // Additional validation for NVMe device name format | ||
| deviceName := strings.TrimPrefix(device, "/dev/nvme") | ||
| if len(deviceName) == 0 { | ||
| return errors.New("invalid NVMe device name format") | ||
| } | ||
|
|
||
| // Check for suspicious characters that could be used in attacks | ||
| for _, char := range deviceName { | ||
| if !isValidNVMeDeviceChar(char) { | ||
| return errors.New("device path contains invalid characters for NVMe device") | ||
| } | ||
| } | ||
|
|
||
| // Validate maximum path length to prevent buffer overflow attacks | ||
| if len(device) > 255 { | ||
| return errors.New("device path exceeds maximum allowed length") | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
I don't think we do this for the other diskio related receivers. We could add it (?) though I don't know if we really wanna fail by returning an error. Maybe just a log message
| @@ -0,0 +1,182 @@ | |||
| [comment]: <> (Code generated by mdatagen. DO NOT EDIT.) | |||
There was a problem hiding this comment.
This will need to get updated by using mdatagen
| // Get InstanceId from EC2 metadata service | ||
| instanceID, err := s.metadataProvider.InstanceID(ctx) | ||
| if err != nil { | ||
| s.logger.Warn("unable to get instance ID from metadata service, using placeholder", zap.Error(err)) | ||
| instanceID = "unknown" | ||
| } |
There was a problem hiding this comment.
There's a processor in the pipeline that should handle this already. User can specify that they want instance ID in the append dimensions
| zap.Int("controllerID", device.Controller()), | ||
| zap.Error(err)) | ||
| // Use a placeholder serial number to allow metrics collection | ||
| serial = fmt.Sprintf("unknown-controller-%d", device.Controller()) |
There was a problem hiding this comment.
Can't think of what I want here but unknown-controller-%d is bit iffy to me
| } | ||
|
|
||
| // For EBS devices, format the serial as volume ID | ||
| if deviceType == "ebs" { |
There was a problem hiding this comment.
Would make ebs/InstanceStore an enum or something
| @@ -0,0 +1,20 @@ | |||
| //go:build !linux | |||
There was a problem hiding this comment.
Should rename these instance_store_metrics_* files to be related to be broadly NVMe instead
| @@ -1,31 +1,116 @@ | |||
| # Maximum buffer size in MB (minimum 3). Choose 0 to use 1% of host memory. | |||
There was a problem hiding this comment.
This needs to be changed back.
| @@ -52,7 +54,9 @@ func NewTranslators(conf *confmap.Conf, configSection, os string) (common.Transl | |||
| }) | |||
| } | |||
|
|
|||
| if shouldAddEbsReceiver(conf, configSection) { | |||
| if shouldAddUnifiedNvmeReceiver(conf, configSection) { | |||
There was a problem hiding this comment.
This needs to be just one receiver.
58cf3c6 to
bfe1fdf
Compare
| } | ||
|
|
||
| // validateDevice validates a single device path with comprehensive security checks | ||
| func (cfg *Config) validateDevice(device string) error { |
There was a problem hiding this comment.
Let's not fail outright nor do we need to have a lot of error handling here, let's look at what we did for ebsnvme
|
|
||
| // InstanceStoreMetrics represents the parsed metrics from the Instance Store NVMe log page 0xC0. | ||
| // Similar to EBS but skips EBS-specific fields (EBSIOPSExceeded, EBSThroughputExceeded). | ||
| type InstanceStoreMetrics struct { |
There was a problem hiding this comment.
Change this to have bins and update the reserved to 888. And you can add some debug statements to see output.
| @@ -0,0 +1,385 @@ | |||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
| // SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
We don't need all the error handling
| @@ -0,0 +1,108 @@ | |||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
| // SPDX-License-Identifier: MIT | |||
|
|
|||
There was a problem hiding this comment.
This should basically be the same awsebsnvme receiver.
2 participants
Description of the issue
Describe the problem or feature in addition to a link to the issues.
Description of changes
How does this change address the problem?
License
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
Describe what tests you have done.
Requirements
Before commiting your code, please do the following steps.
make fmtandmake fmt-shmake lintIntegration Tests
To run integration tests against this PR, add the
ready for testinglabel.