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

ssh exporter #2138

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

ssh exporter #2138

wants to merge 5 commits into from

Conversation

EHSchmitt4395
Copy link
Contributor

@EHSchmitt4395 EHSchmitt4395 commented Nov 21, 2024

PR Description

ssh exporter for remote hosts

define ip along with password or key for the creation of custom metrics from a remote host via bash interpolation

ex alloy config:

prometheus.exporter.ssh "example" {
  verbose_logging = true

  targets {
    address         = "192.168.1.10"
    port            = 22
    username        = "admin"
    password        = "password"
    command_timeout = 10

    custom_metrics {
      name    = "load_average"
      command = "cat /proc/loadavg | awk '{print $1}'"
      type    = "gauge"
      help    = "Load average over 1 minute"
    }
  }

  targets {
    address         = "192.168.1.11"
    port            = 22
    username        = "monitor"
    key_file        = "/path/to/private.key"
    command_timeout = 15

    custom_metrics {
      name        = "disk_usage"
      command     = "df / | tail -1 | awk '{print $5}'"
      type        = "gauge"
      help        = "Disk usage percentage"
      parse_regex = "(\\d+)%"
    }
  }
}

prometheus.scrape "demo" {
  targets    = prometheus.exporter.ssh.example.targets
  forward_to = [prometheus.remote_write.demo.receiver]
}

prometheus.remote_write "demo" {
  endpoint {
    url = PROMETHEUS_REMOTE_WRITE_URL

    basic_auth {
      username = USERNAME
      password = PASSWORD
    }
  }
}

Notes to the Reviewer

plz let me know if issue or something missing with my pr - cheers :)

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated

@ptodev ptodev self-assigned this Nov 21, 2024
| Name | Type | Description | Default | Required |
| ----------------- | -------- | -------------------------------------------------- | ------- | -------- |
| `verbose_logging` | `bool` | Enable verbose logging for debugging purposes. | `false` | no |
| `targets` | `block` | One or more target configurations for SSH metrics. | | yes |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `targets` | `block` | One or more target configurations for SSH metrics. | | yes |

We don't list blocks in the table in the Arguments section. They are listed in the table in the Blocks section.

| `port` | `int` | SSH port number. | `22` | no |
| `username` | `string` | SSH username for authentication. | | yes |
| `password` | `secret` | Password for password-based SSH authentication. | | Required if `key_file` is not provided |
| `key_file` | `string` | Path to the private key file for key-based SSH authentication. | | Required if `password` is not provided |
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to just have a key attribute which is a secret string. People can still get that string from a file if they use local.file. The benefit is that it's more flexible, because you can also get the secret from somewhere like remote.vault or remote.kubernetes.secret.

| `address` | `string` | The IP address or hostname of the target server. | | yes |
| `port` | `int` | SSH port number. | `22` | no |
| `username` | `string` | SSH username for authentication. | | yes |
| `password` | `secret` | Password for password-based SSH authentication. | | Required if `key_file` is not provided |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `password` | `secret` | Password for password-based SSH authentication. | | Required if `key_file` is not provided |
| `password` | `secret` | Password for password-based SSH authentication. | | no

If an argument is not always required, we'll usually just say "no" in the table and then we'll explain under the table that one of those two arguments must be supplied.

Comment on lines +71 to +76
#### Metric Types

- `gauge`: Represents a numerical value that can go up or down.
- `counter`: Represents a cumulative value that only increases.

#### parse_regex
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#### Metric Types
- `gauge`: Represents a numerical value that can go up or down.
- `counter`: Represents a cumulative value that only increases.
#### parse_regex
The `type` argument can have either of the following values:
- `gauge`: Represents a numerical value that can go up or down.
- `counter`: Represents a cumulative value that only increases.

We usually don't use sub-headings under the tables, unless there's too much text and there's no other way to structure the information.

The `prometheus.exporter.ssh` component uses the `known_hosts` file to validate host keys and protect against man-in-the-middle (MITM) attacks. Here's how it handles this:

1. **First-Time Setup**:
- If the `known_hosts` file does not exist, the component creates it and fetches the host key using `ssh-keyscan`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- If the `known_hosts` file does not exist, the component creates it and fetches the host key using `ssh-keyscan`.
- If the `~/.ssh/known_hosts` file does not exist, the component creates it and fetches the host key using `ssh-keyscan`.

Would be good to know where the file is located.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need static mode converters, since static mode doesn't support this exporter :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this file? The reason other exporters have it is that they come from Agent Static mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the code copied from an already existing prometheus exporter? If so, we should probably check the license to make sure it's ok to copy it. But it might be better to just import the other exporter if possible?

time.Sleep(time.Second)
}
if len(output) == 0 {
return fmt.Errorf("failed to fetch host key for %s after 3 attempts: last error: %w", targetAddress, scanErr)
Copy link
Contributor

Choose a reason for hiding this comment

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

If a user fixes the issue with this, would they have to restart Alloy for things to start working? Ideally the component should just keep trying periodically until it works.




func (c *SSHClient) RunCommand(command string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any SSH commands that should not be allowed? I realise this may be hard to enforce, but I just think some users might want extra security and I wonder if there are any further checks we can do.

@ptodev
Copy link
Contributor

ptodev commented Nov 21, 2024

Once the component is merged, it would be useful to have modules in the alloy-modules repo which show how to extract various metrics in Linux.

# prometheus.exporter.ssh

The `prometheus.exporter.ssh` component embeds an SSH exporter for collecting metrics from remote servers over SSH and exporting them as Prometheus metrics.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the experimental snippet to document the fact that the component is experimental

---
canonical: https://grafana.com/docs/alloy/latest/reference/components/prometheus/prometheus.exporter.ssh/
aliases:
- ../prometheus.exporter.ssh/ # /docs/alloy/latest/reference/components/prometheus.exporter.ssh/
Copy link
Contributor

Choose a reason for hiding this comment

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

aliases are not need for this component because it's brand new (this was used for mapping after the structure of the doc was changed)

continue
}

level.Debug(c.logger).Log("msg", "executed custom command", "command", cm.Command, "value", value)
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add debug statements like this to Live Debugging. They can also include the output of the command.

| `username` | `string` | SSH username for authentication. | | yes |
| `password` | `secret` | Password for password-based SSH authentication. | | Required if `key_file` is not provided |
| `key_file` | `string` | Path to the private key file for key-based SSH authentication. | | Required if `password` is not provided |
| `command_timeout` | `int` | Timeout in seconds for each command execution over SSH. | `30` | no |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be a duration? We rarely use dedicated seconds.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the expected behavior if command timeout > scrape timeout?

@@ -0,0 +1,194 @@
package ssh_exporter
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this windows compatible?

timeout time.Duration
}
var sshKeyscanCommand = func(targetAddress string) ([]byte, error) {
cmd := exec.Command("ssh-keyscan", "-H", targetAddress)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can target address be hijacked for any injection? We should ensure it is an valid URI.

}
}

file, err := os.OpenFile(knownHostsPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this safe? It likely is because its only called from NewSSHClient but maybe a global mutex?

}

func (c *SSHCollector) executeCustomCommand(cm CustomMetric) (float64, error) {
output, err := c.client.RunCommand(cm.Command)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The underlying component context should likely also be passed here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And checked against.

Comment on lines +23 to +24
The following arguments can be used to configure the exporter's behavior.
All arguments are optional unless specified. Omitted fields take their default values.
Copy link
Contributor

Choose a reason for hiding this comment

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

Observation: In reviewing this I've noticed that the wording of some sections in the Prometheus topics are not consistent with the rest of the components. For example, Arguments in other sections simply state: The following arguments are supported:

I'll create an Issue to follow up on this. No action or suggestions for this PR.


The following blocks are supported inside the definition of `prometheus.exporter.ssh`:

| Block | Description | Required |
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add/include the Hierarchy column in this table?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants