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

Code refactoring: Include AgentIdentity #925

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sarroutbi
Copy link
Contributor

@sarroutbi sarroutbi commented Jan 30, 2025

Add agent_registration.rs to include all code
relevant to this kind of operation

Signed-off-by: Sergio Arroutbi [email protected]

@sarroutbi sarroutbi force-pushed the 202501301552-refactor-agent-activation branch 8 times, most recently from 136d47a to ce6d65e Compare January 30, 2025 16:06
Copy link

codecov bot commented Jan 30, 2025

Codecov Report

Attention: Patch coverage is 82.89474% with 13 lines in your changes missing coverage. Please review.

Project coverage is 62.96%. Comparing base (a18dbe2) to head (c3285ed).

Files with missing lines Patch % Lines
keylime-agent/src/agent_registration.rs 90.00% 7 Missing ⚠️
keylime-agent/src/main.rs 0.00% 6 Missing ⚠️
Additional details and impacted files
Flag Coverage Δ
e2e-testsuite 62.96% <82.89%> (+0.66%) ⬆️
upstream-unit-tests 62.96% <82.89%> (+0.66%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
keylime-agent/src/main.rs 19.58% <0.00%> (+1.68%) ⬆️
keylime-agent/src/agent_registration.rs 90.00% <90.00%> (ø)

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sarroutbi sarroutbi force-pushed the 202501301552-refactor-agent-activation branch 3 times, most recently from 82e517f to b8c4700 Compare February 3, 2025 11:22
@sarroutbi sarroutbi changed the title [DRAFT]: Code refactoring Code refactoring Feb 3, 2025
@sarroutbi sarroutbi marked this pull request as ready for review February 3, 2025 11:22
@sarroutbi sarroutbi requested a review from ansasaki February 3, 2025 12:05
use tss_esapi::traits::Marshall;

#[derive(Debug)]
pub struct AgentActivation {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this structure could be called AgentIdentity or similar as it is combining all the info that identifies the agent (UUID, keys, certificates, etc.)

Also it raises to me a question: is it possible that this structure containing the agent identity could be useful out of the context of the agent?

To me it seems it could be useful, for example, if we decide to implement other components in Rust in future which could benefit for having the agent identity structure available as a library.

For this reason I believe this structure could be moved to the keylime library, out of the keylime-agent application.

Also, I believe that we should modify the current implementation of the RegistrarClient in the keylime lib to not include the Agent information, but only the data related with "making requests to the registrar" (e.g. ip, port, etc.).

Then, you could move the code from there that is related to building the structure that contains the agent info to your AgentIdentity structure. Probably using an AgentIdentityBuilder to set the options.

The register_agent method in RegistrarClient could receive an instance of your AgentIdentity and make the registration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello Anderson. Regarding names (AgentActivation->AgentIdentity), I am OK with this.

However, I am still not confident about moving the whole thing to the library, so, if possible, I would like to leave that for another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another issue I find to extract the code of this PR to the library is the usage of certain code which are parts of the agent itself, such as the Error and the Config

@ansasaki
Copy link
Contributor

ansasaki commented Feb 3, 2025

Another thing that I would like to ask is to be more descriptive and specific in the PR title, so that when we are going through the PRs, we can get an idea on the changes it is making. "Code refactoring" is not a good title. "Introduce AgentIdentity structure" or "Move agent registration to dedicated module" would be better.

@sarroutbi sarroutbi force-pushed the 202501301552-refactor-agent-activation branch 3 times, most recently from 065c8c3 to 083ce08 Compare February 5, 2025 10:14
@sarroutbi sarroutbi changed the title Code refactoring Code refactoring: Include AgentIdentity Feb 5, 2025
@sarroutbi sarroutbi requested a review from ansasaki February 5, 2025 11:10
@sarroutbi sarroutbi force-pushed the 202501301552-refactor-agent-activation branch 4 times, most recently from 56d9148 to 8b9db7c Compare February 7, 2025 15:16
@sarroutbi sarroutbi force-pushed the 202501301552-refactor-agent-activation branch 2 times, most recently from 4007e50 to 6eded5b Compare February 13, 2025 14:54
@sarroutbi sarroutbi force-pushed the 202501301552-refactor-agent-activation branch from 6eded5b to 6cb24ac Compare February 24, 2025 17:18
Add agent_registration.rs to include all code
relevant to this kind of operation

Signed-off-by: Sergio Arroutbi <[email protected]>
@ansasaki ansasaki force-pushed the 202501301552-refactor-agent-activation branch from 6cb24ac to c3285ed Compare March 14, 2025 09:02
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.

2 participants