feature: Register consul services with agent API #567

Merged
lx merged 10 commits from unrob/garage:roberto/consul-agent-registration into main 2023-06-02 14:35:00 +00:00
3 changed files with 43 additions and 51 deletions
Showing only changes of commit 6b69404f1a - Show all commits

View file

@ -318,16 +318,16 @@ Garage supports discovering other nodes of the cluster using Consul. For this
to work correctly, nodes need to know their IP address by which they can be
reached by other nodes of the cluster, which should be set in `rpc_public_addr`.
### `mode`
Two modes of service discovery are supported: `node` and `service`. `node`, the default will register a service using
the `/v1/catalog` endpoints and mTLS (if `client_cert` and `client_key` are provided). `service` mode uses the
`v1/agent` endpoints instead, where an optional `consul_http_token` may be provided.
### `consul_http_addr` and `service_name`
The `consul_http_addr` parameter should be set to the full HTTP(S) address of the Consul server.
### `consul_http_api`
Two APIs for service registration are supported: `catalog` and `agent`. `catalog`, the default, will register a service using
the `/v1/catalog` endpoints and mTLS (if `client_cert` and `client_key` are provided). The `agent` API uses the
`v1/agent` endpoints instead, where an optional `consul_http_token` may be provided.
unrob marked this conversation as resolved Outdated
Outdated
Review

token instead of consul_http_token

`token` instead of `consul_http_token`
### `service_name`
`service_name` should be set to the service name under which Garage's
@ -335,8 +335,8 @@ RPC ports are announced.
### `client_cert`, `client_key`
`node` mode only. TLS client certificate and client key to use when communicating with Consul over TLS.
Both are mandatory when doing so.
TLS client certificate and client key to use when communicating with Consul over TLS. Both are mandatory when doing so.
Only available when `consul_http_api = "catalog"`.
### `ca_cert`
@ -349,8 +349,8 @@ Skip server hostname verification in TLS handshake.
### `consul_http_token`
`service` mode only. Uses the provided token for communication with Consul. The policy assigned to this token
should at least have these rules:
Uses the provided token for communication with Consul. Only available when `consul_http_api = "agent"`.
The policy assigned to this token should at least have these rules:
```hcl
// the `service_name` specified above

View file

@ -8,8 +8,8 @@ use serde::{Deserialize, Serialize};
use netapp::NodeID;
use garage_util::config::ConsulDiscoveryAPI;
use garage_util::config::ConsulDiscoveryConfig;
use garage_util::config::ConsulDiscoveryMode;
const META_PREFIX: &str = "fr-deuxfleurs-garagehq";
@ -78,18 +78,18 @@ pub struct ConsulDiscovery {
impl ConsulDiscovery {
pub fn new(config: ConsulDiscoveryConfig) -> Result<Self, ConsulError> {
let mut builder: reqwest::ClientBuilder = reqwest::Client::builder();
unrob marked this conversation as resolved Outdated
Outdated
Review

add .use_rustls_tls() here and remove all other instances below (we are using rustls in all cases)

add `.use_rustls_tls()` here and remove all other instances below (we are using rustls in all cases)
builder = builder.danger_accept_invalid_certs(config.tls_skip_verify);
let client: reqwest::Client = match &config.mode {
ConsulDiscoveryMode::Node => {
if let Some(ca_cert) = &config.ca_cert {
let mut ca_cert_buf = vec![];
File::open(ca_cert)?.read_to_end(&mut ca_cert_buf)?;
builder = builder.use_rustls_tls();
builder = builder
.add_root_certificate(reqwest::Certificate::from_pem(&ca_cert_buf[..])?);
}
if config.tls_skip_verify {
builder = builder.danger_accept_invalid_certs(true);
} else if let Some(ca_cert) = &config.ca_cert {
let mut ca_cert_buf = vec![];
File::open(ca_cert)?.read_to_end(&mut ca_cert_buf)?;
builder = builder.use_rustls_tls();
builder =
builder.add_root_certificate(reqwest::Certificate::from_pem(&ca_cert_buf[..])?);
}
let client: reqwest::Client = match &config.consul_http_api {
ConsulDiscoveryAPI::Catalog => {
match (&config.client_cert, &config.client_key) {
(Some(client_cert), Some(client_key)) => {
let mut client_cert_buf = vec![];
@ -111,15 +111,7 @@ impl ConsulDiscovery {
builder.build()?
unrob marked this conversation as resolved Outdated
Outdated
Review

refactoring: move builder.build()? after the match

This means that there is no let client = match, just a match block that mutates builder

and after the match block there is let client = builder.build()?;

refactoring: move `builder.build()?` after the match This means that there is no `let client = match`, just a match block that mutates `builder` and after the match block there is `let client = builder.build()?;`
}
ConsulDiscoveryMode::Service => {
if let Some(ca_cert) = &config.ca_cert {
let mut ca_cert_buf = vec![];
File::open(ca_cert)?.read_to_end(&mut ca_cert_buf)?;
builder = builder
.add_root_certificate(reqwest::Certificate::from_pem(&ca_cert_buf[..])?);
builder = builder.use_rustls_tls();
}
ConsulDiscoveryAPI::Agent => {
if let Some(token) = &config.consul_http_token {
let mut headers = reqwest::header::HeaderMap::new();
headers.insert(
@ -150,9 +142,9 @@ impl ConsulDiscovery {
let mut ret = vec![];
for ent in entries {
let ip = ent.address.parse::<IpAddr>().ok();
let pubkey = match &self.config.mode {
ConsulDiscoveryMode::Node => ent.node_meta.get("pubkey"),
ConsulDiscoveryMode::Service => {
let pubkey = match &self.config.consul_http_api {
ConsulDiscoveryAPI::Catalog => ent.node_meta.get("pubkey"),
ConsulDiscoveryAPI::Agent => {
ent.service_meta.get(&format!("{}-pubkey", META_PREFIX))
unrob marked this conversation as resolved Outdated
Outdated
Review

I think we should also add the META_PREFIX to the old code that uses the Catalog API, it's a good practice to have and it will simplify the code. It will make Consul discovery incompatible between versions but that's fine, we are still beta.

I think we should also add the `META_PREFIX` to the old code that uses the Catalog API, it's a good practice to have and it will simplify the code. It will make Consul discovery incompatible between versions but that's fine, we are still beta.
}
}
@ -187,9 +179,9 @@ impl ConsulDiscovery {
]
.concat();
let meta_prefix: String = match &self.config.mode {
ConsulDiscoveryMode::Node => "".to_string(),
ConsulDiscoveryMode::Service => format!("{}-", META_PREFIX),
let meta_prefix: String = match &self.config.consul_http_api {
ConsulDiscoveryAPI::Catalog => "".to_string(),
ConsulDiscoveryAPI::Agent => format!("{}-", META_PREFIX),
};
unrob marked this conversation as resolved Outdated
Outdated
Review

this can also be simplified by using META_PREFIX in all cases

this can also be simplified by using META_PREFIX in all cases
let mut meta = HashMap::from([
@ -206,15 +198,15 @@ impl ConsulDiscovery {
let url = format!(
"{}/v1/{}",
self.config.consul_http_addr,
(match &self.config.mode {
ConsulDiscoveryMode::Node => "catalog/register",
ConsulDiscoveryMode::Service => "agent/service/register?replace-existing-checks",
(match &self.config.consul_http_api {
ConsulDiscoveryAPI::Catalog => "catalog/register",
ConsulDiscoveryAPI::Agent => "agent/service/register?replace-existing-checks",
})
);
let req = self.client.put(&url);
let http = (match &self.config.mode {
ConsulDiscoveryMode::Node => req.json(&ConsulPublishEntry {
let http = (match &self.config.consul_http_api {
ConsulDiscoveryAPI::Catalog => req.json(&ConsulPublishEntry {
node: node.clone(),
address: rpc_public_addr.ip(),
node_meta: meta.clone(),
@ -227,7 +219,7 @@ impl ConsulDiscovery {
port: rpc_public_addr.port(),
},
}),
ConsulDiscoveryMode::Service => req.json(&ConsulPublishService {
ConsulDiscoveryAPI::Agent => req.json(&ConsulPublishService {
service_id: node.clone(),
service_name: self.config.service_name.clone(),
tags,

View file

@ -136,22 +136,22 @@ pub struct AdminConfig {
}
#[derive(Deserialize, Debug, Clone)]
pub enum ConsulDiscoveryMode {
pub enum ConsulDiscoveryAPI {
#[serde(rename_all = "lowercase")]
unrob marked this conversation as resolved Outdated
Outdated
Review

I think the rename_all line has to go above the pub enum ConsulDiscoveryAPI line?

I think the `rename_all` line has to go above the `pub enum ConsulDiscoveryAPI` line?
Node,
Service,
Catalog,
Agent,
}
impl ConsulDiscoveryMode {
impl ConsulDiscoveryAPI {
unrob marked this conversation as resolved Outdated
Outdated
Review

This whole impl can be avoided by deriving Default on the enum. The default variant has to be marked with a #[default] tag.

This whole impl can be avoided by deriving `Default` on the enum. The default variant has to be marked with a `#[default]` tag.
fn default() -> Self {
ConsulDiscoveryMode::Node
ConsulDiscoveryAPI::Catalog
}
}
#[derive(Deserialize, Debug, Clone)]
pub struct ConsulDiscoveryConfig {
/// Mode of consul operation: either `node` (the default) or `service`
#[serde(default = "ConsulDiscoveryMode::default")]
pub mode: ConsulDiscoveryMode,
/// The consul api to use when registering: either `catalog` (the default) or `agent`
#[serde(default = "ConsulDiscoveryAPI::default")]
pub consul_http_api: ConsulDiscoveryAPI,
/// Consul http or https address to connect to to discover more peers
pub consul_http_addr: String,
/// Consul service name to use