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

[1.28.36] cockpit: code fix and CI fixes from subscription-manager-cockpit #3445

Merged

Conversation

ptoscano
Copy link
Contributor

@ptoscano ptoscano commented Aug 7, 2024

The file /etc/insights-client/.last-upload.results is watched to
determine when was the last upload done. A couple of important notes
about it:
1) even though it is read (and parsed), its content is actually never
   used currently
2) the file is written by insights-client (better: insights-core) only
   when "legacy_upload" is true (which is the default as of this
   writing)

While (1) is not much an issue, (2) means that the displaying of the
last upload (timestamp) to Insights is not available when switching
insights-client to non-legacy-upload.

To avoid both the issues, switch from .last-upload.results to
/etc/insights-client/.lastupload:
- .lastupload is always written, no matter the mode, and its
  modification timestamp is indeed accurate
- stop reading the file at all, saving a bit of resources

There should be no behaviour changes.

(cherry picked from commit 2ac616ffdf0d25afc10d72dc2a32b16032512dc8 in
the subscription-manager-cockpit repository)
Tweak their implementations a bit to prepare them for more substantial
changes; in particular:
- properly name the ID we get: the "insights_id" query parameter as
  such, and the UUID as Inventory ID
- use an helper variable to which add/set all the bits to the replies,
  dumping them as JSON only in one place

This is only a refactor with no behaviour change.

(cherry picked from commit 2cf70497eb9a1e8b87a0eb5d9910a3675a811929 in
the subscription-manager-cockpit repository)
Currently, the fake systems (or better, only one) is kept in the helper
"systems" dictionary by the machine ID; while this seems to work fine,
in practice it will not work for upcoming changes, and it does not match
what Inventory actually does.

Change the ID handling to represent better what Inventory does:
- assign "id" as Inventory ID for each newly registered system; in
  practice we have only one, and keep hardcoding "123-nice-id" for now
  (the "testInsights" test checks for it)
- use the "id" as key in the "systems" dictionary, rather than the
  "machine_id"
- when registering a new system, copy "machine_id" as "insights_id";
  this will help later on when implementing the non-legacy API endpoints
- adapt endpoints to search for the ID they need

Even with all the changes, there should be no behaviour changes.

(cherry picked from commit 40c42862eaf15b6f17b9a9f4e70a762574541b80 in
the subscription-manager-cockpit repository)
…dpoints

Implement a couple of missing platform endpoints needed to make
insights-client work in non-legacy-upload mode:
- the upload endpoint, which needs to get the system metadata from the
  MIME data sent with POST: because of this, the implementation needs a
  bit more work; because of testing reasons, the Inventory ID is still
  hardcoded as "123-nice-id"
- the delete endpoint, needed to unregister a system

(cherry picked from commit 5bafd3772f88ec2d3d8422f2aac6f53828ffe761 in
the subscription-manager-cockpit repository)
Switch the authentication method of insights-client to CERT-based, which
uses the consumer certificate & key of subscription-manager.

This means using mTLS for authentication, still without verifying them,
just like the previous fake credentials; this is still acceptable in the
testing environment of a transient guest.

(cherry picked from commit e682d429aab936718bb902380d56849ec568a141 in
the subscription-manager-cockpit repository)
Copy link

github-actions bot commented Aug 7, 2024

Coverage

Coverage •
FileStmtsMissCoverMissing
TOTAL22529995355% 
report-only-changed-files is enabled. No files were changed during this commit :)

Tests Skipped Failures Errors Time
2341 10 💤 0 ❌ 0 🔥 5m 6s ⏱️

Copy link
Contributor

@m-horky m-horky left a comment

Choose a reason for hiding this comment

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

LGTM

@m-horky m-horky merged commit 4425661 into subscription-manager-1.28.36 Aug 7, 2024
4 checks passed
@m-horky m-horky deleted the ptoscano/cockpit-fixes-1.28.36 branch August 7, 2024 12:49
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