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

Add support for SSH Authentication to ObsRsync Plugin #5360

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

josegomezr
Copy link
Contributor

@josegomezr josegomezr commented Nov 14, 2023

ObsRsync Plugin now supports the new authentication mechanism of Build Service.

Heavily inspired by:

Addresses poo#139073


Important: The production openqa.ini config must be updated. obs_rsync.project_status_url must not include /public/.

etc/openqa/openqa.ini Outdated Show resolved Hide resolved
lib/OpenQA/WebAPI/Plugin/ObsRsync.pm Outdated Show resolved Hide resolved
lib/OpenQA/WebAPI/Plugin/ObsRsync.pm Show resolved Hide resolved
t/ui/27-plugin_obs_rsync_obs_status.t Show resolved Hide resolved
lib/OpenQA/WebAPI/Plugin/ObsRsync.pm Outdated Show resolved Hide resolved
lib/OpenQA/WebAPI/Plugin/ObsRsync.pm Outdated Show resolved Hide resolved
@kraih
Copy link
Member

kraih commented Nov 14, 2023

One problem i'm seeing is that the URLs still use the /public/... API endpoints. Those will be shut down completely for IBS and cannot be used anymore. So this PR is still incomplete.

Edit: According to new information the /public endpoints should remain available with authentication.
Edit2: And now i got the update that /public will indeed become unavailable.

@josegomezr
Copy link
Contributor Author

Thanks @kraih, Leaving a note for all watchers (and updated the PR description too).

Important: The production openqa.ini config must be updated. obs_rsync.project_status_url must not include /public/.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

Please make sure this is tested against both

  1. build.opensuse.org
  2. build.suse.de

dependencies.yaml Outdated Show resolved Hide resolved
etc/openqa/openqa.ini Outdated Show resolved Hide resolved
lib/OpenQA/WebAPI/Plugin/ObsRsync.pm Show resolved Hide resolved
t/ui/27-plugin_obs_rsync_obs_status.t Outdated Show resolved Hide resolved
@okurz
Copy link
Member

okurz commented Nov 15, 2023

See https://app.circleci.com/pipelines/github/os-autoinst/openQA/12483/workflows/c08ea15d-a06f-46f6-ab2e-dc3f7a5cf878/jobs/116513 . We still need to install ssh-keygen for tests. I guess we already do a similar approach for other packages?

Take a look how rsync is part of hard test dependencies put only recommended for installing on user systems. So in dependencies.yaml add the package to "devel_no_selenium_requires:"

@perlpunk
Copy link
Contributor

For new packages I think we just put them into https://github.com/os-autoinst/openQA/blob/master/tools/ci/ci-packages.txt without a version to make CI happy. The next dependency PR will add a version.

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e6799a9) 98.32% compared to head (b08b00c) 98.35%.
Report is 14 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5360      +/-   ##
==========================================
+ Coverage   98.32%   98.35%   +0.03%     
==========================================
  Files         389      389              
  Lines       37330    37391      +61     
==========================================
+ Hits        36704    36777      +73     
+ Misses        626      614      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@josegomezr josegomezr force-pushed the obs_rsync_ssh_auth branch 2 times, most recently from a5661c0 to da55360 Compare November 15, 2023 18:52
dependencies.yaml Outdated Show resolved Hide resolved
@josegomezr josegomezr force-pushed the obs_rsync_ssh_auth branch 2 times, most recently from c590841 to 94d95f0 Compare November 16, 2023 09:42
lib/OpenQA/WebAPI/Plugin/ObsRsync.pm Outdated Show resolved Hide resolved
t/ui/27-plugin_obs_rsync_obs_status.t Outdated Show resolved Hide resolved
t/ui/27-plugin_obs_rsync_obs_status.t Outdated Show resolved Hide resolved
Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

Actually, the condition mentioned in my last review should be double-checked. (I might be wrong but we should at least double-check.)

t/ui/27-plugin_obs_rsync_obs_status.t Outdated Show resolved Hide resolved
t/ui/27-plugin_obs_rsync_obs_status.t Outdated Show resolved Hide resolved
t/ui/27-plugin_obs_rsync_obs_status.t Outdated Show resolved Hide resolved
@josegomezr josegomezr force-pushed the obs_rsync_ssh_auth branch 2 times, most recently from 87b3f46 to d019776 Compare November 16, 2023 13:57
t/ui/27-plugin_obs_rsync_obs_status.t Outdated Show resolved Hide resolved
t/ui/27-plugin_obs_rsync_obs_status.t Outdated Show resolved Hide resolved
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

content all good, commits need to be squashd

@josegomezr
Copy link
Contributor Author

josegomezr commented Nov 16, 2023

Waiting for the ->touch discussion resolution.

Squashd 🚀

Heavily inspired by:

- https://github.com/openSUSE/obs-build/blob/master/PBuild/SigAuth.pm
- openSUSE/cavil@583009d
- https://www.suse.com/c/multi-factor-authentication-on-suses-build-service/

Addresses poo#139073

- Add openssh to devel dependencies for OBSRsync.

Co-authored-by: Martchus <[email protected]>
Co-authored-by: Tina Müller (tinita) <[email protected]>
Co-authored-by: Oliver Kurz <[email protected]>
@Martchus
Copy link
Contributor

Ok, nice. Even after squashing we normally keep starting the commit message with a verb in imperative. I would also rephrase it a bit: Add support for HTTP authentication in ObsRsync plugin

@mergify mergify bot merged commit 0130cfb into os-autoinst:master Nov 16, 2023
13 checks passed
os-autoinst-bot pushed a commit to os-autoinst-bot/openQA that referenced this pull request Nov 17, 2023
commit 0130cfb
Merge: 90cbb53 b08b00c
Author:     mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
AuthorDate: Thu Nov 16 17:38:09 2023 +0000
Commit:     GitHub <[email protected]>
CommitDate: Thu Nov 16 17:38:09 2023 +0000

    Merge pull request os-autoinst#5360 from josegomezr/obs_rsync_ssh_auth

    Add support for SSH Authentication to ObsRsync Plugin
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.

5 participants