Skip to content

Commit 4c0074f

Browse files
feat(IDGetter): split implementation according to environment (#266)
* feat(IDGetter): split implementation according to environment
1 parent 279a7e2 commit 4c0074f

File tree

23 files changed

+406
-89
lines changed

23 files changed

+406
-89
lines changed

.github/workflows/check.yml

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,12 @@ jobs:
6363
uses: actions-rs/clippy-check@v1
6464
with:
6565
token: ${{ secrets.GITHUB_TOKEN }}
66-
args: --all-features -- -D clippy::all
66+
args: --features=onhost -- -D clippy::all
67+
- name: cargo clippy
68+
uses: actions-rs/clippy-check@v1
69+
with:
70+
token: ${{ secrets.GITHUB_TOKEN }}
71+
args: --features=k8s -- -D clippy::all
6772
doc:
6873
runs-on: ubuntu-latest
6974
name: stable / doc
@@ -99,8 +104,11 @@ jobs:
99104
uses: dtolnay/rust-toolchain@4f366e621dc8fa63f557ca04b8f4361824a35a45
100105
with:
101106
toolchain: ${{ matrix.msrv }}
102-
- name: cargo +${{ matrix.msrv }} check
103-
run: cargo check --all-features
107+
- name: cargo +${{ matrix.msrv }} check onK8s
108+
run: cargo check --features=k8s
109+
- name: cargo +${{ matrix.msrv }} check onHost
110+
run: cargo check --features=onhost
111+
104112

105113
licenses:
106114
name: Validate third party libraries

.github/workflows/coverage.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ jobs:
3131
- name: cargo generate-lockfile
3232
if: hashFiles('Cargo.lock') == ''
3333
run: cargo generate-lockfile
34-
- name: cargo llvm-cov
34+
- name: cargo llvm-cov for onHost version
3535
run: cargo llvm-cov --locked --all-features --json --output-path jcov.info -- --skip as_root
3636
- name: Calculate and print total coverage
3737
run: |

.github/workflows/test.yml

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,15 @@ jobs:
3030
- name: cargo generate-lockfile
3131
if: hashFiles('Cargo.lock') == ''
3232
run: cargo generate-lockfile
33-
- name: Run tests excluding root-required tests
34-
run: cargo test --locked --all-features --all-targets -- --skip as_root
35-
- name: Run documentation tests
36-
run: cargo test --locked --all-features --doc
33+
- name: Run tests onHost excluding root-required tests
34+
run: cargo test --locked --features=onhost --all-targets -- --skip as_root
35+
- name: Run tests onK8s
36+
run: cargo test --locked --features=k8s --all-targets
37+
- name: Run documentation tests onhost
38+
run: cargo test --locked --features=onhost --doc
39+
- name: Run documentation tests k8st
40+
run: cargo test --locked --features=k8s --doc
41+
3742
# Test using root can be troublesome to perform as usually the Rust toolchain is installed local to the user.
3843
# To test for root privileges, we can run from a container (disabling the test that fail if running non-root).
3944
test-with-root:
@@ -48,8 +53,10 @@ jobs:
4853
uses: webfactory/[email protected]
4954
with:
5055
ssh-private-key: ${{ secrets.CAOS_RUST_CRATES }}
51-
- name: Run root-required tests only
52-
run: cargo test --locked --all-features --all-targets -- --skip no_root
56+
- name: Run k8s root-required tests only
57+
run: cargo test --locked --features=k8s --all-targets
58+
- name: Run onHost root-required tests only
59+
run: cargo test --locked --features=onhost --all-targets -- --skip no_root
5360
# Windows and macos not required at the moment (ci failing while fetching
5461
# private crates)
5562
# os-check:

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ opamp-client = { git = "ssh://[email protected]/newrelic/opamp-rs.git", tag = "0.0.
2323
futures = "0.3.28"
2424
async-trait = "0.1.71"
2525
tokio = "1.32.0"
26-
ulid = "1.0.1"
26+
ulid = { version = "1.0.1", features = ["serde"]}
2727
openssl = { version = "0.10.57", features = ["vendored"] }
2828
aquamarine = "0.3.2"
2929
duration-str = "0.7.0"

src/bin/main.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,11 @@ use newrelic_super_agent::super_agent::error::AgentError;
44
use tracing::{error, info};
55

66
use newrelic_super_agent::config::store::{SuperAgentConfigStore, SuperAgentConfigStoreFile};
7+
use newrelic_super_agent::opamp::instance_id::getter::ULIDInstanceIDGetter;
8+
use newrelic_super_agent::opamp::instance_id::Storer;
79
use newrelic_super_agent::opamp::remote_config_hash::HashRepositoryFile;
810
use newrelic_super_agent::sub_agent::values::values_repository::ValuesRepositoryFile;
911
use newrelic_super_agent::super_agent::effective_agents_assembler::LocalEffectiveAgentsAssembler;
10-
use newrelic_super_agent::super_agent::instance_id::ULIDInstanceIDGetter;
1112
use newrelic_super_agent::super_agent::opamp::client_builder::SuperAgentOpAMPHttpBuilder;
1213
use newrelic_super_agent::super_agent::super_agent::{SuperAgent, SuperAgentEvent};
1314
use newrelic_super_agent::{cli::Cli, context::Context, logging::Logging};
@@ -59,7 +60,7 @@ fn run_super_agent(
5960
config_storer: SuperAgentConfigStoreFile,
6061
ctx: Context<Option<SuperAgentEvent>>,
6162
opamp_client_builder: Option<SuperAgentOpAMPHttpBuilder>,
62-
instance_id_getter: ULIDInstanceIDGetter,
63+
instance_id_getter: ULIDInstanceIDGetter<Storer>,
6364
) -> Result<(), AgentError> {
6465
#[cfg(unix)]
6566
if !nix::unistd::Uid::effective().is_root() {
@@ -98,12 +99,11 @@ fn run_super_agent(
9899
config_storer: SuperAgentConfigStoreFile,
99100
ctx: Context<Option<SuperAgentEvent>>,
100101
opamp_client_builder: Option<SuperAgentOpAMPHttpBuilder>,
101-
instance_id_getter: ULIDInstanceIDGetter,
102+
instance_id_getter: ULIDInstanceIDGetter<Storer>,
102103
) -> Result<(), AgentError> {
103104
let hash_repository = HashRepositoryFile::default();
104105
let sub_agent_hash_repository = HashRepositoryFile::new_sub_agent_repository();
105106

106-
// Disabled when --all-features
107107
#[cfg(all(not(feature = "onhost"), feature = "k8s"))]
108108
let sub_agent_builder = newrelic_super_agent::sub_agent::k8s::builder::K8sSubAgentBuilder::new(
109109
opamp_client_builder.as_ref(),

src/opamp/client_builder.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ use tracing::error;
99
use crate::config::super_agent_configs::{AgentID, OpAMPClientConfig};
1010

1111
use crate::context::Context;
12+
use crate::opamp::instance_id::getter;
1213
use crate::super_agent::super_agent::SuperAgentEvent;
1314

1415
#[derive(Error, Debug)]
@@ -23,6 +24,8 @@ pub enum OpAMPClientBuilderError {
2324
StartedOpAMPlientError(#[from] opamp_client::error::ClientError),
2425
#[error("system time error: `{0}`")]
2526
SystemTimeError(#[from] SystemTimeError),
27+
#[error("error getting agent ulid: `{0}`")]
28+
GetUlidError(#[from] getter::GetterError),
2629
}
2730

2831
pub trait OpAMPClientBuilder {

src/opamp/instance_id/getter.rs

Lines changed: 234 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
use crate::opamp::instance_id::storer::{InstanceIDStorer, StorerError};
2+
use crate::opamp::instance_id::{Identifiers, Storer};
3+
use serde::{Deserialize, Serialize};
4+
use std::fmt::{Display, Formatter};
5+
use tracing::debug;
6+
use ulid::Ulid;
7+
8+
#[derive(thiserror::Error, Debug)]
9+
pub enum GetterError {
10+
#[error("failed to persist Data: `{0}`")]
11+
Persisting(#[from] StorerError),
12+
}
13+
14+
// InstanceID holds the to_string of Ulid assigned to a Agent
15+
#[derive(Default, Debug, Deserialize, Serialize, PartialEq, Clone)]
16+
pub struct InstanceID(String);
17+
18+
impl InstanceID {
19+
pub(crate) fn new(id: String) -> InstanceID {
20+
InstanceID(id)
21+
}
22+
}
23+
24+
impl Display for InstanceID {
25+
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
26+
write!(f, "{}", self.0)
27+
}
28+
}
29+
30+
// InstanceIDGetter returns an InstanceID for a specific agentID.
31+
pub trait InstanceIDGetter {
32+
fn get(&self, agent_id: &str) -> Result<InstanceID, GetterError>;
33+
}
34+
35+
pub struct ULIDInstanceIDGetter<S>
36+
where
37+
S: InstanceIDStorer,
38+
{
39+
storer: S,
40+
identifiers: Identifiers,
41+
}
42+
43+
impl<S> ULIDInstanceIDGetter<S>
44+
where
45+
S: InstanceIDStorer,
46+
{
47+
pub fn new(storer: S, identifiers: Identifiers) -> Self {
48+
Self {
49+
storer,
50+
identifiers,
51+
}
52+
}
53+
}
54+
55+
impl<S> InstanceIDGetter for ULIDInstanceIDGetter<S>
56+
where
57+
S: InstanceIDStorer,
58+
{
59+
fn get(&self, agent_id: &str) -> Result<InstanceID, GetterError> {
60+
debug!("retrieving ulid");
61+
let data = self.storer.get(agent_id)?;
62+
63+
match data {
64+
None => {
65+
debug!("storer returned no data");
66+
}
67+
Some(d) if d.identifiers == self.identifiers => return Ok(d.ulid),
68+
Some(d) => debug!(
69+
"stored data had different identifiers {:?}!={:?}",
70+
d.identifiers, self.identifiers
71+
),
72+
}
73+
74+
let new_data = DataStored {
75+
ulid: InstanceID(Ulid::new().to_string()),
76+
identifiers: self.identifiers.clone(),
77+
};
78+
79+
debug!("persisting ulid {}", new_data.ulid);
80+
self.storer.set(agent_id, &new_data)?;
81+
82+
Ok(new_data.ulid)
83+
}
84+
}
85+
86+
impl Default for ULIDInstanceIDGetter<Storer> {
87+
fn default() -> Self {
88+
Self::new(Storer {}, Identifiers::default())
89+
}
90+
}
91+
92+
#[derive(Deserialize, Serialize)]
93+
pub struct DataStored {
94+
pub ulid: InstanceID,
95+
pub identifiers: Identifiers,
96+
}
97+
98+
#[cfg(test)]
99+
pub mod test {
100+
use super::*;
101+
use crate::opamp::instance_id::getter::{DataStored, InstanceIDGetter, ULIDInstanceIDGetter};
102+
use crate::opamp::instance_id::storer::test::MockInstanceIDStorerMock;
103+
use crate::opamp::instance_id::storer::StorerError;
104+
use mockall::{mock, predicate};
105+
106+
mock! {
107+
pub InstanceIDGetterMock {}
108+
109+
impl InstanceIDGetter for InstanceIDGetterMock {
110+
fn get(&self, agent_id: &str) -> Result<InstanceID, GetterError>;
111+
}
112+
}
113+
114+
impl MockInstanceIDGetterMock {
115+
pub fn should_get(&mut self, agent_id: String, ulid: String) {
116+
self.expect_get()
117+
.once()
118+
.with(predicate::eq(agent_id.clone()))
119+
.returning(move |_| Ok(InstanceID(ulid.clone())));
120+
}
121+
}
122+
123+
const AGENT_NAME: &str = "agent1";
124+
125+
#[test]
126+
fn test_not_found() {
127+
let mut mock = MockInstanceIDStorerMock::new();
128+
129+
mock.expect_get()
130+
.once()
131+
.with(predicate::eq(AGENT_NAME))
132+
.returning(|_| Ok(None));
133+
mock.expect_set()
134+
.once()
135+
.with(predicate::eq(AGENT_NAME), predicate::always())
136+
.returning(|_, _| Ok(()));
137+
let getter = ULIDInstanceIDGetter::new(mock, Identifiers::default());
138+
let res = getter.get(AGENT_NAME);
139+
140+
assert!(res.is_ok());
141+
}
142+
143+
#[test]
144+
fn test_error_get() {
145+
let mut mock = MockInstanceIDStorerMock::new();
146+
147+
mock.expect_get()
148+
.once()
149+
.with(predicate::eq(AGENT_NAME))
150+
.returning(|_| Err(StorerError::Generic));
151+
let getter = ULIDInstanceIDGetter::new(mock, Identifiers::default());
152+
let res = getter.get(AGENT_NAME);
153+
154+
assert!(res.is_err());
155+
}
156+
157+
#[test]
158+
fn test_error_set() {
159+
let mut mock = MockInstanceIDStorerMock::new();
160+
161+
mock.expect_get()
162+
.once()
163+
.with(predicate::eq(AGENT_NAME))
164+
.returning(|_| Ok(None));
165+
mock.expect_set()
166+
.once()
167+
.with(predicate::eq(AGENT_NAME), predicate::always())
168+
.returning(|_, _| Err(StorerError::Generic));
169+
170+
let getter = ULIDInstanceIDGetter::new(mock, Identifiers::default());
171+
let res = getter.get(AGENT_NAME);
172+
173+
assert!(res.is_err());
174+
}
175+
176+
#[test]
177+
fn test_ulid_already_present() {
178+
let mut mock = MockInstanceIDStorerMock::new();
179+
let ulid = Ulid::new();
180+
181+
mock.expect_get()
182+
.once()
183+
.with(predicate::eq(AGENT_NAME))
184+
.returning(move |_| {
185+
Ok(Some(DataStored {
186+
ulid: InstanceID(ulid.to_string()),
187+
identifiers: Default::default(),
188+
}))
189+
});
190+
let getter = ULIDInstanceIDGetter::new(mock, Identifiers::default());
191+
let res = getter.get(AGENT_NAME);
192+
193+
assert!(res.is_ok());
194+
assert_eq!(InstanceID(ulid.to_string()), res.unwrap());
195+
}
196+
197+
#[test]
198+
fn test_ulid_present_but_different_identifiers() {
199+
let mut mock = MockInstanceIDStorerMock::new();
200+
let ulid = ulid::Ulid::new();
201+
202+
mock.expect_get()
203+
.once()
204+
.with(predicate::eq(AGENT_NAME))
205+
.returning(move |_| {
206+
Ok(Some(DataStored {
207+
ulid: InstanceID(ulid.to_string()),
208+
identifiers: get_different_identifier(),
209+
}))
210+
});
211+
mock.expect_set()
212+
.once()
213+
.with(predicate::eq(AGENT_NAME), predicate::always())
214+
.returning(|_, _| Ok(()));
215+
let getter = ULIDInstanceIDGetter::new(mock, Identifiers::default());
216+
let res = getter.get(AGENT_NAME);
217+
218+
assert!(res.is_ok());
219+
assert_ne!(InstanceID(ulid.to_string()), res.unwrap());
220+
}
221+
222+
fn get_different_identifier() -> Identifiers {
223+
#[cfg(all(not(feature = "onhost"), feature = "k8s"))]
224+
return Identifiers {
225+
cluster_name: "test".to_string(),
226+
};
227+
228+
#[cfg(feature = "onhost")]
229+
return Identifiers {
230+
machine_id: "different".to_string(),
231+
hostname: "different".to_string(),
232+
};
233+
}
234+
}
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
use serde::{Deserialize, Serialize};
2+
3+
#[derive(Default, Debug, Deserialize, Serialize, PartialEq, Clone)]
4+
pub struct Identifiers {
5+
pub cluster_name: String,
6+
}
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
use serde::{Deserialize, Serialize};
2+
3+
#[derive(Default, Debug, Deserialize, Serialize, PartialEq, Clone)]
4+
pub struct Identifiers {
5+
pub hostname: String,
6+
pub machine_id: String,
7+
}

0 commit comments

Comments
 (0)