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 50 additions and 75 deletions
Showing only changes of commit b770504126 - Show all commits

View file

@ -35,14 +35,14 @@ bootstrap_peers = [
[consul_discovery] [consul_discovery]
mode = "node" api = "catalog"
unrob marked this conversation as resolved Outdated
Outdated
Review

Is this the old name of consul_http_api ?

I'd suggest calling this parameter api, as it is already under the [consul_discovery] section so there is no need to specify that it is related to Consul.

For all parameters: take care of making sure the names in the docs are consistent with the actual code

Is this the old name of `consul_http_api` ? I'd suggest calling this parameter `api`, as it is already under the `[consul_discovery]` section so there is no need to specify that it is related to Consul. For all parameters: take care of making sure the names in the docs are consistent with the actual code
consul_http_addr = "http://127.0.0.1:8500" consul_http_addr = "http://127.0.0.1:8500"
service_name = "garage-daemon" service_name = "garage-daemon"
ca_cert = "/etc/consul/consul-ca.crt" ca_cert = "/etc/consul/consul-ca.crt"
client_cert = "/etc/consul/consul-client.crt" client_cert = "/etc/consul/consul-client.crt"
client_key = "/etc/consul/consul-key.crt" client_key = "/etc/consul/consul-key.crt"
# for `service` mode, unset client_cert and client_key, and optionally enable `consul_http_token` # for `catalog` API mode, unset client_cert and client_key, and optionally enable `token`
unrob marked this conversation as resolved Outdated
Outdated
Review

I think you meant for agent API mode instead?

I think you meant for `agent` API mode instead?
# consul_http_token = "abcdef-01234-56789" # token = "abcdef-01234-56789"
unrob marked this conversation as resolved Outdated
Outdated
Review

Similarly, I'd suggest calling this simply token

Similarly, I'd suggest calling this simply `token`
tls_skip_verify = false tls_skip_verify = false
tags = [ "dns-enabled" ] tags = [ "dns-enabled" ]
meta = { dns-acl = "allow trusted" } meta = { dns-acl = "allow trusted" }

View file

@ -19,10 +19,15 @@ struct ConsulQueryEntry {
address: String, address: String,
#[serde(rename = "ServicePort")] #[serde(rename = "ServicePort")]
service_port: u16, service_port: u16,
#[serde(rename = "NodeMeta")]
node_meta: HashMap<String, String>,
#[serde(rename = "ServiceMeta")] #[serde(rename = "ServiceMeta")]
service_meta: HashMap<String, String>, meta: HashMap<String, String>,
}
#[derive(Serialize, Clone, Debug)]
#[serde(untagged)]
enum PublishRequest {
Catalog(ConsulPublishEntry),
Service(ConsulPublishService),
} }
#[derive(Serialize, Clone, Debug)] #[derive(Serialize, Clone, Debug)]
@ -31,8 +36,6 @@ struct ConsulPublishEntry {
node: String, node: String,
#[serde(rename = "Address")] #[serde(rename = "Address")]
address: IpAddr, address: IpAddr,
#[serde(rename = "NodeMeta")]
node_meta: HashMap<String, String>,
#[serde(rename = "Service")] #[serde(rename = "Service")]
service: ConsulPublishCatalogService, service: ConsulPublishCatalogService,
} }
@ -46,7 +49,7 @@ struct ConsulPublishCatalogService {
#[serde(rename = "Tags")] #[serde(rename = "Tags")]
unrob marked this conversation as resolved Outdated
Outdated
Review

It's called service_meta here but meta in the other definition below. Let's just call it meta everywhere

It's called `service_meta` here but `meta` in the other definition below. Let's just call it `meta` everywhere
tags: Vec<String>, tags: Vec<String>,
#[serde(rename = "Meta")] #[serde(rename = "Meta")]
service_meta: HashMap<String, String>, meta: HashMap<String, String>,
#[serde(rename = "Address")] #[serde(rename = "Address")]
address: IpAddr, address: IpAddr,
#[serde(rename = "Port")] #[serde(rename = "Port")]
@ -77,42 +80,36 @@ pub struct ConsulDiscovery {
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)
impl ConsulDiscovery { impl ConsulDiscovery {
pub fn new(config: ConsulDiscoveryConfig) -> Result<Self, ConsulError> { pub fn new(config: ConsulDiscoveryConfig) -> Result<Self, ConsulError> {
let mut builder: reqwest::ClientBuilder = reqwest::Client::builder(); let mut builder: reqwest::ClientBuilder = reqwest::Client::builder().use_rustls_tls();
if config.tls_skip_verify { if config.tls_skip_verify {
builder = builder.danger_accept_invalid_certs(true); builder = builder.danger_accept_invalid_certs(true);
} else if let Some(ca_cert) = &config.ca_cert { } else if let Some(ca_cert) = &config.ca_cert {
let mut ca_cert_buf = vec![]; let mut ca_cert_buf = vec![];
File::open(ca_cert)?.read_to_end(&mut ca_cert_buf)?; File::open(ca_cert)?.read_to_end(&mut ca_cert_buf)?;
builder = builder.use_rustls_tls();
builder = builder =
builder.add_root_certificate(reqwest::Certificate::from_pem(&ca_cert_buf[..])?); builder.add_root_certificate(reqwest::Certificate::from_pem(&ca_cert_buf[..])?);
} }
let client: reqwest::Client = match &config.consul_http_api { match &config.api {
ConsulDiscoveryAPI::Catalog => { ConsulDiscoveryAPI::Catalog => match (&config.client_cert, &config.client_key) {
match (&config.client_cert, &config.client_key) { (Some(client_cert), Some(client_key)) => {
(Some(client_cert), Some(client_key)) => { let mut client_cert_buf = vec![];
let mut client_cert_buf = vec![]; File::open(client_cert)?.read_to_end(&mut client_cert_buf)?;
File::open(client_cert)?.read_to_end(&mut client_cert_buf)?;
let mut client_key_buf = vec![]; let mut client_key_buf = vec![];
File::open(client_key)?.read_to_end(&mut client_key_buf)?; File::open(client_key)?.read_to_end(&mut client_key_buf)?;
let identity = reqwest::Identity::from_pem( let identity = reqwest::Identity::from_pem(
&[&client_cert_buf[..], &client_key_buf[..]].concat()[..], &[&client_cert_buf[..], &client_key_buf[..]].concat()[..],
)?; )?;
builder = builder.use_rustls_tls(); builder = builder.identity(identity);
builder = builder.identity(identity);
}
(None, None) => {}
_ => return Err(ConsulError::InvalidTLSConfig),
} }
(None, None) => {}
builder.build()? _ => return Err(ConsulError::InvalidTLSConfig),
} },
ConsulDiscoveryAPI::Agent => { ConsulDiscoveryAPI::Agent => {
if let Some(token) = &config.consul_http_token { if let Some(token) = &config.token {
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()?;`
let mut headers = reqwest::header::HeaderMap::new(); let mut headers = reqwest::header::HeaderMap::new();
headers.insert( headers.insert(
"x-consul-token", "x-consul-token",
@ -120,11 +117,11 @@ impl ConsulDiscovery {
); );
builder = builder.default_headers(headers); builder = builder.default_headers(headers);
} }
builder.build()?
} }
}; };
let client: reqwest::Client = builder.build()?;
Ok(Self { client, config }) Ok(Self { client, config })
} }
@ -142,14 +139,11 @@ impl ConsulDiscovery {
let mut ret = vec![]; let mut ret = vec![];
for ent in entries { for ent in entries {
let ip = ent.address.parse::<IpAddr>().ok(); let ip = ent.address.parse::<IpAddr>().ok();
let pubkey = match &self.config.consul_http_api { let pubkey = ent
ConsulDiscoveryAPI::Catalog => ent.node_meta.get("pubkey"), .meta
ConsulDiscoveryAPI::Agent => { .get(&format!("{}-pubkey", META_PREFIX))
ent.service_meta.get(&format!("{}-pubkey", META_PREFIX)) .and_then(|k| hex::decode(k).ok())
} .and_then(|k| NodeID::from_slice(&k[..]));
}
.and_then(|k| hex::decode(k).ok())
.and_then(|k| NodeID::from_slice(&k[..]));
if let (Some(ip), Some(pubkey)) = (ip, pubkey) { if let (Some(ip), Some(pubkey)) = (ip, pubkey) {
ret.push((pubkey, SocketAddr::new(ip, ent.service_port))); ret.push((pubkey, SocketAddr::new(ip, ent.service_port)));
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.
} else { } else {
@ -179,47 +173,34 @@ impl ConsulDiscovery {
] ]
.concat(); .concat();
let meta_prefix: String = match &self.config.consul_http_api { let mut meta = self.config.meta.clone().unwrap_or_default();
ConsulDiscoveryAPI::Catalog => "".to_string(), meta.insert(format!("{}-pubkey", META_PREFIX), hex::encode(node_id));
ConsulDiscoveryAPI::Agent => format!("{}-", META_PREFIX), meta.insert(format!("{}-hostname", META_PREFIX), hostname.to_string());
};
let mut meta = HashMap::from([
(format!("{}pubkey", meta_prefix), hex::encode(node_id)),
(format!("{}hostname", meta_prefix), hostname.to_string()),
]);
if let Some(global_meta) = &self.config.meta {
for (key, value) in global_meta.into_iter() {
meta.insert(key.clone(), value.clone());
}
}
let url = format!( let url = format!(
"{}/v1/{}", "{}/v1/{}",
self.config.consul_http_addr, self.config.consul_http_addr,
(match &self.config.consul_http_api { (match &self.config.api {
ConsulDiscoveryAPI::Catalog => "catalog/register", ConsulDiscoveryAPI::Catalog => "catalog/register",
ConsulDiscoveryAPI::Agent => "agent/service/register?replace-existing-checks", ConsulDiscoveryAPI::Agent => "agent/service/register?replace-existing-checks",
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 req = self.client.put(&url); let req = self.client.put(&url);
let http = (match &self.config.consul_http_api { let advertisement: PublishRequest = match &self.config.api {
ConsulDiscoveryAPI::Catalog => req.json(&ConsulPublishEntry { ConsulDiscoveryAPI::Catalog => PublishRequest::Catalog(ConsulPublishEntry {
node: node.clone(), node: node.clone(),
address: rpc_public_addr.ip(), address: rpc_public_addr.ip(),
node_meta: meta.clone(),
service: ConsulPublishCatalogService { service: ConsulPublishCatalogService {
service_id: node.clone(), service_id: node.clone(),
service_name: self.config.service_name.clone(), service_name: self.config.service_name.clone(),
unrob marked this conversation as resolved Outdated
Outdated
Review

I'd replace 187-195 by the following:

let mut meta = self.config.meta.clone().unwrap_or_default();
meta.insert(format!("{}-pubkey", META_PREFIX), hex::encode(node_id));
meta.insert(format!("{}-hostname", META_PREFIX), hostname.to_string());
I'd replace 187-195 by the following: ```rust let mut meta = self.config.meta.clone().unwrap_or_default(); meta.insert(format!("{}-pubkey", META_PREFIX), hex::encode(node_id)); meta.insert(format!("{}-hostname", META_PREFIX), hostname.to_string()); ```
tags, tags,
service_meta: meta.clone(), meta: meta.clone(),
address: rpc_public_addr.ip(), address: rpc_public_addr.ip(),
port: rpc_public_addr.port(), port: rpc_public_addr.port(),
}, },
}), }),
ConsulDiscoveryAPI::Agent => req.json(&ConsulPublishService { ConsulDiscoveryAPI::Agent => PublishRequest::Service(ConsulPublishService {
service_id: node.clone(), service_id: node.clone(),
service_name: self.config.service_name.clone(), service_name: self.config.service_name.clone(),
tags, tags,
@ -227,9 +208,8 @@ impl ConsulDiscovery {
address: rpc_public_addr.ip(), address: rpc_public_addr.ip(),
port: rpc_public_addr.port(), port: rpc_public_addr.port(),
}), }),
}) };
.send() let http = req.json(&advertisement).send().await?;
.await?;
http.error_for_status()?; http.error_for_status()?;
Ok(()) Ok(())

View file

@ -135,23 +135,18 @@ pub struct AdminConfig {
pub trace_sink: Option<String>, pub trace_sink: Option<String>,
} }
#[derive(Deserialize, Debug, Clone)] #[derive(Deserialize, Debug, Clone, Default)]
#[serde(rename_all = "lowercase")]
pub enum ConsulDiscoveryAPI { pub enum ConsulDiscoveryAPI {
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?
#[serde(rename_all = "lowercase")] #[default]
Catalog, Catalog,
Agent, Agent,
} }
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.
impl ConsulDiscoveryAPI {
fn default() -> Self {
ConsulDiscoveryAPI::Catalog
}
}
#[derive(Deserialize, Debug, Clone)] #[derive(Deserialize, Debug, Clone)]
pub struct ConsulDiscoveryConfig { pub struct ConsulDiscoveryConfig {
/// The consul api to use when registering: either `catalog` (the default) or `agent` /// The consul api to use when registering: either `catalog` (the default) or `agent`
#[serde(default = "ConsulDiscoveryAPI::default")] pub api: ConsulDiscoveryAPI,
pub consul_http_api: ConsulDiscoveryAPI,
/// Consul http or https address to connect to to discover more peers /// Consul http or https address to connect to to discover more peers
pub consul_http_addr: String, pub consul_http_addr: String,
/// Consul service name to use /// Consul service name to use
@ -163,7 +158,7 @@ pub struct ConsulDiscoveryConfig {
/// Client TLS key to use when connecting to Consul /// Client TLS key to use when connecting to Consul
pub client_key: Option<String>, pub client_key: Option<String>,
/// /// Token to use for connecting to consul /// /// Token to use for connecting to consul
pub consul_http_token: Option<String>, pub token: Option<String>,
/// Skip TLS hostname verification /// Skip TLS hostname verification
#[serde(default)] #[serde(default)]
pub tls_skip_verify: bool, pub tls_skip_verify: bool,