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

Adding /agent/info API to agent #758

Merged
merged 1 commit into from
May 27, 2024
Merged

Conversation

mpeters
Copy link
Member

@mpeters mpeters commented Mar 5, 2024

This is a new read-only API on the agent to allow for 3rd party integrations (starting with SPIRE) that will need certain information about the agent.

@mpeters mpeters self-assigned this Mar 5, 2024
@keylime-bot keylime-bot assigned THS-on, aplanas and ueno and unassigned mpeters Mar 5, 2024
keylime-agent/src/errors_handler.rs Outdated Show resolved Hide resolved
keylime-agent/src/errors_handler.rs Outdated Show resolved Hide resolved
@ansasaki
Copy link
Contributor

ansasaki commented Mar 7, 2024

The feature seems straightforward.

About the test issues, it seems the nightly rust compiler changed and is now failing to compile. Let's see if that gets fixed "automagically" or if we will have to make changes to the compiler, probably pinning the version with the coverage measurement support instead of using the nightly version.

The formatting issue normally can be fixed automatically by running cargo fmt.

@mpeters mpeters force-pushed the spire-apis branch 3 times, most recently from 0ed81b5 to 524a6b8 Compare March 12, 2024 16:07
@mpeters
Copy link
Member Author

mpeters commented Mar 12, 2024

The formatting issue normally can be fixed automatically by running cargo fmt.

Thanks, done.

@ansasaki
Copy link
Contributor

I hope this fixes the CI issues: RedHat-SP-Security/keylime-tests#560

@mpeters mpeters requested a review from aplanas May 7, 2024 14:46
@mpeters mpeters changed the title WIP: Adding /agent/info API to agent Adding /agent/info API to agent May 7, 2024
keylime-agent/src/agent_handler.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 62.71186% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 61.95%. Comparing base (2f7b3ad) to head (42872f0).
Report is 21 commits behind head on master.

Additional details and impacted files
Flag Coverage Δ
e2e-testsuite 61.95% <62.71%> (+4.36%) ⬆️
upstream-unit-tests 61.95% <62.71%> (+10.94%) ⬆️

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

Files Coverage Δ
keylime-agent/src/main.rs 28.52% <100.00%> (+2.51%) ⬆️
keylime-agent/src/agent_handler.rs 55.55% <55.55%> (ø)
keylime-agent/src/errors_handler.rs 55.50% <56.25%> (-0.66%) ⬇️

... and 15 files with indirect coverage changes

Copy link
Contributor

@ueno ueno left a comment

Choose a reason for hiding this comment

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

Looks good to me!

keylime-agent/src/agent_handler.rs Outdated Show resolved Hide resolved
keylime-agent/src/agent_handler.rs Outdated Show resolved Hide resolved
@ansasaki
Copy link
Contributor

ansasaki commented May 8, 2024

Should the API version be bumped due to this addition?

@mpeters
Copy link
Member Author

mpeters commented May 13, 2024

Should the API version be bumped due to this addition?

Yes. I don't know what process we want for that. Do we want to bump the version when we make a single change? Or before we do a release? I'm open to either one.

@mpeters
Copy link
Member Author

mpeters commented May 13, 2024

/packit retest-failed

@ansasaki
Copy link
Contributor

Should the API version be bumped due to this addition?

Yes. I don't know what process we want for that. Do we want to bump the version when we make a single change? Or before we do a release? I'm open to either one.

I think the best would be to bump the version with the first API change made in the release, but do a single bump including all the changes made to the API per release.

This would better test the bump itself during the release development. It would be risky and error prone to bump the version only when creating the new release.

@mpeters
Copy link
Member Author

mpeters commented May 13, 2024

@ansasaki It's an interesting problem with the version numbers: If I bump them in the code in the agent not only will the agent's URLs be /v2.2 but it will be trying to use the /v2.2 APIs of the registrar and verifier which don't yet exist. Now in my case I do have a companion PR (keylime/keylime#1532) on the server side so I'll go ahead and bump up the versions there too... but what do we do if it's just a change in the agent's APIs? Do we have to have a "dummy" PR in the server side to bump up the APIs first there before we can do it in the agent?

@ansasaki
Copy link
Contributor

t what do we do if it's just a change in the agent's APIs? Do we have to have a "dummy" PR in the serve

For now, I think the easiest way would be to add the new supported version there and then bump the version here. We should discuss with the community what will be the process, and also think if it is worth to increase the complexity of the agent to add more than one supported API version.

We agreed in the past that the agent would provide only the latest API version, while the python components would handle the various possible supported agent API's.

I also hit this problem when updating some tests and created an issue to track this: #790

We should consider dissociating the API versions, but that will come with the need to manage on the agent side which versions of the registrar API are supported. Not an easy decision.

@ansasaki
Copy link
Contributor

/packit retest-failed

4 similar comments
@mpeters
Copy link
Member Author

mpeters commented May 17, 2024

/packit retest-failed

@mpeters
Copy link
Member Author

mpeters commented May 17, 2024

/packit retest-failed

@mpeters
Copy link
Member Author

mpeters commented May 20, 2024

/packit retest-failed

@ansasaki
Copy link
Contributor

/packit retest-failed

@ansasaki
Copy link
Contributor

@kkaarreell It seems the new version of the coverage tool cargo-tarpaulin cannot be compiled with the rustc version we pinned in the test (1.74.1). The (maybe) good news is that it can be compiled with the current rustc version in Fedora (1.78.0). I think we will have to adjust the test install_upstream_rust_keylime test again 😞

@mpeters There are warnings that are generated by the new rust compiler, I proposed a fix for them in #793. Meanwhile, the tests in the CI will probably continue to fail. I tested your code manually on top of #793 and the test you added passes.

@mpeters
Copy link
Member Author

mpeters commented May 22, 2024

/packit retest-failed

@ansasaki
Copy link
Contributor

/packit retest-failed

@kkaarreell
Copy link
Contributor

kkaarreell commented May 23, 2024

I think that the build on F39 is failing due to some repodata (cache) issue. It is installing old i686 version of tpm2-tss-devel from fedora repo instead of installing it from updates repo.

tpm2-tss-4.0.1-6.fc39.x86_64
tpm2-tss-devel                    i686    4.0.1-4.fc39                     fedora

Moreover, on my F39 test system I have already 4.0.2-1 in updates.

@kkaarreell
Copy link
Contributor

You could try editing packit-ci.fmf and adding explicit call of
dnf makecache to https://github.com/mpeters/rust-keylime/blob/spire-apis/packit-ci.fmf#L19
It could fix this problem, though prolong test execution a bit.

@mpeters
Copy link
Member Author

mpeters commented May 23, 2024

You could try editing packit-ci.fmf and adding explicit call of dnf makecache to https://github.com/mpeters/rust-keylime/blob/spire-apis/packit-ci.fmf#L19 It could fix this problem, though prolong test execution a bit.

@kkaarreell Is this before or after the calls to disable dnf-makecache.service and dnf-makecache.timer?

Bumping API version to 2.2

Signed-off-by: Michael Peters <[email protected]>
@kkaarreell
Copy link
Contributor

Hm, it seems it didnt help, I'm sorry. Weird.

@kkaarreell
Copy link
Contributor

kkaarreell commented May 23, 2024

~~ I think I know whats going on. F39 image Is old and already contains old tpm2-tss package. Therefore the -devel subpackage gets installed in the same old version. Either we would have to explicitly update or ask Testing Farm to create newer image. I will do the later tomorrow for sure. ~~

EDIT: I will consult TF folks tomorrow. I have reserved a system from TF and it can install tpm2-tss-devel with the right version and architecture.

[root@48fb5785-be51-4cfe-9a7a-adbb16f2e439 ~]# rpm -q tpm2-tss
tpm2-tss-4.0.2-1.fc39.x86_64
[root@48fb5785-be51-4cfe-9a7a-adbb16f2e439 ~]# dnf makecache
Beaker Client - Fedora39                                                                    11 kB/s | 1.5 kB     00:00    
Beaker harness                                                                              18 kB/s | 1.3 kB     00:00    
Beaker tasks                                                                                14 MB/s | 5.9 MB     00:00    
Fedora 39 - x86_64                                                                          82 kB/s |  23 kB     00:00    
Fedora 39 openh264 (From Cisco) - x86_64                                                   7.3 kB/s | 989  B     00:00    
Fedora 39 - x86_64 - Updates                                                               205 kB/s |  22 kB     00:00    
Copr repo for qa-tools owned by lpol                                                        35 kB/s | 1.8 kB     00:00    
Metadata cache created.
[root@48fb5785-be51-4cfe-9a7a-adbb16f2e439 ~]# dnf install tpm2-tss-devel
Last metadata expiration check: 0:00:14 ago on Thu 23 May 2024 05:13:53 PM EDT.
Dependencies resolved.
===========================================================================================================================
 Package                           Architecture            Version                          Repository                Size
===========================================================================================================================
Installing:
 tpm2-tss-devel                    x86_64                  4.0.2-1.fc39                     updates                  169 k

@kkaarreell
Copy link
Contributor

Hi Michael, try rescheduling it once more. In #795 CI has passed for me even without an explicit makecache call.

@mpeters
Copy link
Member Author

mpeters commented May 26, 2024

/packit retest-failed

@mpeters mpeters merged commit e2b24db into keylime:master May 27, 2024
8 of 9 checks passed
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.

6 participants