-
Notifications
You must be signed in to change notification settings - Fork 292
CP-46944: Update yum plugins to dnf plugins #5526
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
"""dnf plugin to set accesstoken http header for enabled repos""" | ||
import json | ||
import logging | ||
# Disable the error, it can be import in production env | ||
# and mocked out in unitttest | ||
#pylint: disable=import-error | ||
import dnf | ||
import urlgrabber | ||
|
||
|
||
class InvalidToken(Exception): | ||
"""Token is invlaid""" | ||
def __init__(self, token): | ||
super().__init__(f"Invalid token: {token}") | ||
|
||
|
||
#pylint: disable=too-few-public-methods | ||
class AccessToken(dnf.Plugin): | ||
"""dnf accesstoken plugin class""" | ||
|
||
name = "accesstoken" | ||
|
||
def config(self): | ||
""" DNF plugin config hook, | ||
refer to https://dnf.readthedocs.io/en/latest/api_plugins.html""" | ||
|
||
for repo_name in self.base.repos: | ||
repo = self.base.repos[repo_name] | ||
|
||
token_url = repo.accesstoken | ||
if not token_url or token_url == '': | ||
continue | ||
try: | ||
token_str = urlgrabber.urlopen(token_url).read().strip() | ||
token = json.loads(token_str) | ||
except Exception: #pylint: disable=broad-except | ||
logging.debug("Failed to load token from: %s", token_url) | ||
continue | ||
|
||
if not (token.get('token') and token.get('token_id')): | ||
raise InvalidToken(token) | ||
|
||
access_token = f'X-Access-Token:{str(token["token"])}' | ||
referer = f'Referer:{str(token["token_id"])}' | ||
repo.set_http_headers([access_token, referer]) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
"""dnf plugin to add ptoken for repos""" | ||
import logging | ||
import dnf | ||
|
||
PTOKEN_PATH = "/etc/xensource/ptoken" | ||
|
||
#pylint: disable=too-few-public-methods | ||
class Ptoken(dnf.Plugin): | ||
"""ptoken plugin class""" | ||
|
||
name = "ptoken" | ||
|
||
def config(self): | ||
""" DNF plugin config hook, | ||
refer to https://dnf.readthedocs.io/en/latest/api_plugins.html""" | ||
try: | ||
with open('/etc/xensource/ptoken', encoding="utf-8") as file: | ||
ptoken = file.read().strip() | ||
except Exception: #pylint: disable=broad-exception-caught | ||
logging.error("Failed to open %s", PTOKEN_PATH) | ||
raise | ||
|
||
for repo_name in self.base.repos: | ||
repo = self.base.repos[repo_name] | ||
|
||
if len(repo.baseurl) > 0 and repo.baseurl[0].startswith("http://127.0.0.1") \ | ||
and repo.ptoken: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a newly added config item for dnf? May I please know the reason to add this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this require changes on xapi code which populate the repo configurations? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you mean No, this is an optional configure item for the repo, to decide whether we should enable pool token for a repo. No extra xapi code change is required for this, on the other side, dnf is pathed to support this as xapi use it. |
||
secret = "pool_secret=" + ptoken | ||
repo.set_http_headers([f'cookie:{secret}']) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
"""This is a stub module for dnf.base | ||
|
||
This module is introduced to decouple the dependencies from dnf | ||
modules during unittest, as the github CI ubuntu container has | ||
issues with the python-dnf and python3.11 | ||
""" | ||
#pylint: disable=too-few-public-methods | ||
class Plugin: | ||
"""Dnf plugin interface""" | ||
def __init__(self, base, cli): | ||
self.base = base | ||
self.cli = cli |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
"""Test module for dnf accesstoken""" | ||
import unittest | ||
import sys | ||
import json | ||
from unittest.mock import MagicMock, patch | ||
|
||
sys.modules["urlgrabber"] = MagicMock() | ||
|
||
# Disable wrong import postition as need to mock some sys modules first | ||
#pylint: disable=wrong-import-position | ||
|
||
# Disable unused-argument as some mock obj is not used | ||
#pylint: disable=unused-argument | ||
|
||
# Some test case does not use self | ||
|
||
from dnf_plugins import accesstoken | ||
from dnf_plugins import ptoken | ||
|
||
REPO_NAME = "testrepo" | ||
|
||
|
||
def _mock_repo(a_token=None, p_token=None, baseurl=None): | ||
mock_repo = MagicMock() | ||
mock_repo.accesstoken = a_token | ||
mock_repo.ptoken = p_token | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there is a good reason to add this "ptoken" config in dnf repo, then what information it would contain? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a boolean to indicate whether enable pool token for this repo. refer to https://github.com/xapi-project/xen-api/blob/master/ocaml/xapi/repository_helpers.ml#L432 |
||
mock_repo.baseurl = baseurl | ||
mock_base = MagicMock() | ||
mock_base.repos = {REPO_NAME: mock_repo} | ||
mock_repo.base = mock_base | ||
return mock_repo | ||
|
||
|
||
@patch("dnf_plugins.accesstoken.urlgrabber") | ||
class TestAccesstoken(unittest.TestCase): | ||
"""Test class for dnf access plugin""" | ||
|
||
def test_set_http_header_with_access_token(self, mock_grabber): | ||
"""test config succeed with accesstokan""" | ||
mock_repo = _mock_repo(a_token="file:///mock_accesstoken_url") | ||
mock_grabber.urlopen.return_value.read.return_value = json.dumps({ | ||
"token": "valid_token", | ||
"token_id": "valid_token_id", | ||
}) | ||
accesstoken.AccessToken(mock_repo.base, MagicMock()).config() | ||
mock_repo.set_http_headers.assert_called_with( | ||
['X-Access-Token:valid_token','Referer:valid_token_id'] | ||
) | ||
|
||
def test_repo_without_access_token(self, mock_grabber): | ||
"""If repo has not accestoken, it should not be blocked""" | ||
mock_repo = _mock_repo() | ||
accesstoken.AccessToken(mock_repo.base, MagicMock()).config() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As comments, the target is to test repo without accesstoken still work, so this case does not care whether it is called or not, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Edit: just updated to follow the suggestion. |
||
mock_repo.set_http_headers.assert_not_called() | ||
|
||
def test_ignore_invalid_token_url(self, mock_grabber): | ||
"""If repo provided an invalid token url, it should be ignored""" | ||
mock_repo = _mock_repo(a_token="Not_existed") | ||
mock_grabber.urlopen.side_effect = FileNotFoundError('') | ||
accesstoken.AccessToken(mock_repo.base, MagicMock()).config() | ||
assert not mock_repo.set_http_headers.called | ||
|
||
def test_invalid_token_raise_exception(self, mock_grabber): | ||
"""Token with right json format, bad content should raise""" | ||
mock_repo = _mock_repo(a_token="file:///file_contain_invalid_token") | ||
mock_grabber.urlopen.return_value.read.return_value = json.dumps({ | ||
"bad_token": "I am bad guy" | ||
}) | ||
with self.assertRaises(accesstoken.InvalidToken): | ||
accesstoken.AccessToken(mock_repo.base, MagicMock()).config() | ||
|
||
|
||
class TestPtoken(unittest.TestCase): | ||
"""Test class for ptoken dnf plugin""" | ||
def test_failed_to_open_ptoken_file(self): | ||
"""Exception should raised if the system does not have PTOKEN_PATH""" | ||
ptoken.PTOKEN_PATH = "/some/not/exist/path" | ||
with self.assertRaises(Exception): | ||
ptoken.Ptoken(MagicMock(), MagicMock()).config() | ||
|
||
@patch("builtins.open") | ||
def test_set_ptoken_to_http_header(self, mock_open): | ||
"""Local repo with ptoken enabled should set the ptoken to its http header""" | ||
mock_open.return_value.__enter__.return_value.read.return_value = "valid_ptoken" | ||
mock_repo = _mock_repo(p_token=True, baseurl=["http://127.0.0.1/some_local_path"]) | ||
ptoken.Ptoken(mock_repo.base, MagicMock()).config() | ||
mock_repo.set_http_headers.assert_called_with(["cookie:pool_secret=valid_ptoken"]) | ||
|
||
@patch("builtins.open") | ||
def test_remote_repo_ignore_ptoken(self, mock_open): | ||
"""non-local repo should just ignore the ptoken""" | ||
mock_open.return_value.__enter__.return_value.read.return_value = "valid_ptoken" | ||
mock_repo = _mock_repo(p_token=True, baseurl=["http://some_remote_token/some_local_path"]) | ||
ptoken.Ptoken(mock_repo.base, MagicMock()).config() | ||
assert not mock_repo.set_http_headers.called | ||
|
||
@patch("builtins.open") | ||
def test_local_repo_does_not_enable_ptoken_should_ignore_ptoken(self, mock_open): | ||
"""local repo which has not enabled ptoken should just ignore the ptoken""" | ||
mock_open.return_value.__enter__.return_value.read.return_value = "valid_ptoken" | ||
mock_repo = _mock_repo(p_token=False, baseurl=["http://127.0.0.1/some_local_path"]) | ||
ptoken.Ptoken(mock_repo.base, MagicMock()).config() | ||
assert not mock_repo.set_http_headers.called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this "name" is required by dnf plugin?
(Would be better to keep a consistent formating style for both plugins, e.g. the blank lines here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it just follow the dnf plugin protocols, find this in the comments:
refer to https://dnf.readthedocs.io/en/latest/api_plugins.html