From: Jozef Urbanovsky jurbanov@redhat.com
Hi team,
This patch extends the documentation of IpsecEspAeadRecipe based on Jan's feedback. Adding forgotten reference to the specific_scenarios. Both PacketAssert modules (RecipeCommon and Tests) are extended with documentation.
Jozef Urbanovsky (4): docs.source.specific_scenarios: add ipsec_esp_aead_recipe lnst.RecipeCommon.PacketAssert: add documentation lnst.Recipes.ENRT.XfrmTools: add documentation lnst.Recipes.ENRT.IpsecEspAeadRecipe: extend and fix documentation
docs/source/specific_scenarios.rst | 1 + lnst/RecipeCommon/PacketAssert.py | 27 +++++++++++++++++++ lnst/Recipes/ENRT/IpsecEspAeadRecipe.py | 23 +++++++++++----- lnst/Recipes/ENRT/XfrmTools.py | 36 +++++++++++++++++++++++++ lnst/Tests/PacketAssert.py | 20 ++++++++++++++ 5 files changed, 100 insertions(+), 7 deletions(-)
From: Jozef Urbanovsky jurbanov@redhat.com
Signed-off-by: Jozef Urbanovsky jurbanov@redhat.com --- docs/source/specific_scenarios.rst | 1 + 1 file changed, 1 insertion(+)
diff --git a/docs/source/specific_scenarios.rst b/docs/source/specific_scenarios.rst index b78577c..fb027e4 100644 --- a/docs/source/specific_scenarios.rst +++ b/docs/source/specific_scenarios.rst @@ -6,3 +6,4 @@ Specific ENRT scenarios team_recipe vlans_recipe vlans_over_bond_recipe + ipsec_esp_aead_recipe
Wed, Sep 30, 2020 at 04:14:49PM CEST, jurbanov@redhat.com wrote:
From: Jozef Urbanovsky jurbanov@redhat.com
Signed-off-by: Jozef Urbanovsky jurbanov@redhat.com
docs/source/specific_scenarios.rst | 1 + 1 file changed, 1 insertion(+)
diff --git a/docs/source/specific_scenarios.rst b/docs/source/specific_scenarios.rst index b78577c..fb027e4 100644 --- a/docs/source/specific_scenarios.rst +++ b/docs/source/specific_scenarios.rst @@ -6,3 +6,4 @@ Specific ENRT scenarios team_recipe vlans_recipe vlans_over_bond_recipe
- ipsec_esp_aead_recipe
-- 2.25.4 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
Acked-by: Jan Tluka jtluka@redhat.com
From: Jozef Urbanovsky jurbanov@redhat.com
Signed-off-by: Jozef Urbanovsky jurbanov@redhat.com --- lnst/RecipeCommon/PacketAssert.py | 27 +++++++++++++++++++++++++++ lnst/Tests/PacketAssert.py | 20 ++++++++++++++++++++ 2 files changed, 47 insertions(+)
diff --git a/lnst/RecipeCommon/PacketAssert.py b/lnst/RecipeCommon/PacketAssert.py index 689250e..b42be74 100644 --- a/lnst/RecipeCommon/PacketAssert.py +++ b/lnst/RecipeCommon/PacketAssert.py @@ -4,6 +4,17 @@ from lnst.Tests import PacketAssert from lnst.Common.LnstError import LnstError
class PacketAssertConf(object): + """ + Class for the configuration of the :any:`PacketAssert` class + + :param host: client in :any:`PingConf` object used to specify host for the ping test + :param iface: interface used for the ping test + :param p_filter: string representation of desired packets in dump + :param grep_for: string representation of the content of given packet in dump + :param p_min: minimum count of the packets to be found in the dump + :param p_max: maximum count of the packets to be found in the dump + :param promiscuous: toggle of promiscuous mode + """ def __init__(self, host, iface, **kwargs): self._host = host self._iface = iface @@ -42,9 +53,19 @@ class PacketAssertConf(object): return self._promiscuous
class PacketAssertTestAndEvaluate(BaseRecipe): + """ + Class to control and execute :any:`PacketAssert` test. + """ started_job = None
def packet_assert_test_start(self, packet_assert_config): + """ + Method starts a :any:`PacketAssert` job and stores ***started_job*** attribute + containing LNST :any:`Job: object. + + :param packet_assert_config: configuration in a form of :any:`PacketAssertConf` + class + """ if self.started_job: raise LnstError("Only 1 packet_assert job is allowed to run at a time.")
@@ -54,6 +75,12 @@ class PacketAssertTestAndEvaluate(BaseRecipe): self.started_job = host.prepare_job(packet_assert).start(bg=True)
def packet_assert_test_stop(self): + """ + Method kills a process executing :any:`PacketAssert` job and + resets the value of the ***started_job*** attribute. + + :return: result of the packet assert + """ if not self.started_job: raise LnstError("No packet_assert job is running.")
diff --git a/lnst/Tests/PacketAssert.py b/lnst/Tests/PacketAssert.py index 25a92ba..901fdc0 100644 --- a/lnst/Tests/PacketAssert.py +++ b/lnst/Tests/PacketAssert.py @@ -8,6 +8,19 @@ from lnst.Tests.BaseTestModule import BaseTestModule from lnst.Common.LnstError import LnstError
class PacketAssert(BaseTestModule): + """ + Class to control tcpdump test + + This class runs tcpdump and searches through its output in order + to evaluate whether specified packets are traversing the network stack. + + :param interface: interface used for the :any:`PacketAssert` test + :param p_filter: string representation of desired packets in dump + :param grep_for: string representation of the content of given packet in dump + :param promiscuous: toggle of promiscuous mode + :param grep_exprs: concatenated expression to be used by grep for search + :param p_recv: amount of received packets + """ interface = DeviceParam(mandatory=True) p_filter = StrParam(default='') grep_for = ListParam(default=[]) @@ -57,6 +70,13 @@ class PacketAssert(BaseTestModule): return False
def run(self): + """ + Method used for the control of :any:`PacketAssert` test. + Process with tcpdump is spawned and its output is digested + in order to find and count specified packets through grep expressions. + + :return: result of the run + """ self._res_data = {} if not is_installed("tcpdump"): self._res_data["msg"] = "tcpdump is not installed on this machine!"
So the Tests.PacketAssert class and also the PacketAssert class are not linked into anywhere and doesn't even show up in the modules index.
I think for "lnst.Tests" we should create a new subsection "Test modules" inside the "tester api" where we can describe the base test module and link in the specific ones that we support.
You don't need to create the base test module documentation, but can you at least create the document hierarchy and link in the PacketAssert test module into it?
For lnst.RecipeCommon I'm undecided for now. I think for that some "Other supported packages" top level section may be interesting just as a temporary space until we think of a good way to organize this.
-Ondrej
On Wed, Sep 30, 2020 at 04:14:50PM +0200, jurbanov@redhat.com wrote:
From: Jozef Urbanovsky jurbanov@redhat.com
Signed-off-by: Jozef Urbanovsky jurbanov@redhat.com
lnst/RecipeCommon/PacketAssert.py | 27 +++++++++++++++++++++++++++ lnst/Tests/PacketAssert.py | 20 ++++++++++++++++++++ 2 files changed, 47 insertions(+)
diff --git a/lnst/RecipeCommon/PacketAssert.py b/lnst/RecipeCommon/PacketAssert.py index 689250e..b42be74 100644 --- a/lnst/RecipeCommon/PacketAssert.py +++ b/lnst/RecipeCommon/PacketAssert.py @@ -4,6 +4,17 @@ from lnst.Tests import PacketAssert from lnst.Common.LnstError import LnstError
class PacketAssertConf(object):
- """
- Class for the configuration of the :any:`PacketAssert` class
- :param host: client in :any:`PingConf` object used to specify host for the ping test
- :param iface: interface used for the ping test
- :param p_filter: string representation of desired packets in dump
- :param grep_for: string representation of the content of given packet in dump
- :param p_min: minimum count of the packets to be found in the dump
- :param p_max: maximum count of the packets to be found in the dump
- :param promiscuous: toggle of promiscuous mode
- """ def __init__(self, host, iface, **kwargs): self._host = host self._iface = iface
@@ -42,9 +53,19 @@ class PacketAssertConf(object): return self._promiscuous
class PacketAssertTestAndEvaluate(BaseRecipe):
"""
Class to control and execute :any:`PacketAssert` test.
""" started_job = None
def packet_assert_test_start(self, packet_assert_config):
"""Method starts a :any:`PacketAssert` job and stores ***started_job*** attributecontaining LNST :any:`Job: object.:param packet_assert_config: configuration in a form of :any:`PacketAssertConf`class""" if self.started_job: raise LnstError("Only 1 packet_assert job is allowed to run at a time.")@@ -54,6 +75,12 @@ class PacketAssertTestAndEvaluate(BaseRecipe): self.started_job = host.prepare_job(packet_assert).start(bg=True)
def packet_assert_test_stop(self):
"""Method kills a process executing :any:`PacketAssert` job andresets the value of the ***started_job*** attribute.:return: result of the packet assert""" if not self.started_job: raise LnstError("No packet_assert job is running.")diff --git a/lnst/Tests/PacketAssert.py b/lnst/Tests/PacketAssert.py index 25a92ba..901fdc0 100644 --- a/lnst/Tests/PacketAssert.py +++ b/lnst/Tests/PacketAssert.py @@ -8,6 +8,19 @@ from lnst.Tests.BaseTestModule import BaseTestModule from lnst.Common.LnstError import LnstError
class PacketAssert(BaseTestModule):
- """
- Class to control tcpdump test
- This class runs tcpdump and searches through its output in order
- to evaluate whether specified packets are traversing the network stack.
- :param interface: interface used for the :any:`PacketAssert` test
- :param p_filter: string representation of desired packets in dump
- :param grep_for: string representation of the content of given packet in dump
- :param promiscuous: toggle of promiscuous mode
- :param grep_exprs: concatenated expression to be used by grep for search
- :param p_recv: amount of received packets
- """ interface = DeviceParam(mandatory=True) p_filter = StrParam(default='') grep_for = ListParam(default=[])
@@ -57,6 +70,13 @@ class PacketAssert(BaseTestModule): return False
def run(self):
"""Method used for the control of :any:`PacketAssert` test.Process with tcpdump is spawned and its output is digestedin order to find and count specified packets through grep expressions.:return: result of the run""" self._res_data = {} if not is_installed("tcpdump"): self._res_data["msg"] = "tcpdump is not installed on this machine!"-- 2.25.4 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
Hello Jozef, please see my comments inline.
Wed, Sep 30, 2020 at 04:14:50PM CEST, jurbanov@redhat.com wrote:
From: Jozef Urbanovsky jurbanov@redhat.com
Signed-off-by: Jozef Urbanovsky jurbanov@redhat.com
lnst/RecipeCommon/PacketAssert.py | 27 +++++++++++++++++++++++++++ lnst/Tests/PacketAssert.py | 20 ++++++++++++++++++++ 2 files changed, 47 insertions(+)
This actually look weird. We should rename the file under RecipeCommon to avoid confusion. It's out of the scope of this patch set review but we should do a follow up fix at some point.
diff --git a/lnst/RecipeCommon/PacketAssert.py b/lnst/RecipeCommon/PacketAssert.py index 689250e..b42be74 100644 --- a/lnst/RecipeCommon/PacketAssert.py +++ b/lnst/RecipeCommon/PacketAssert.py @@ -4,6 +4,17 @@ from lnst.Tests import PacketAssert from lnst.Common.LnstError import LnstError
class PacketAssertConf(object):
- """
- Class for the configuration of the :any:`PacketAssert` class
- :param host: client in :any:`PingConf` object used to specify host for the ping test
- :param iface: interface used for the ping test
- :param p_filter: string representation of desired packets in dump
- :param grep_for: string representation of the content of given packet in dump
- :param p_min: minimum count of the packets to be found in the dump
- :param p_max: maximum count of the packets to be found in the dump
- :param promiscuous: toggle of promiscuous mode
Please take a look at my comments for Tests/PacketAssert.py about the parameters explanation.
- """ def __init__(self, host, iface, **kwargs): self._host = host self._iface = iface
@@ -42,9 +53,19 @@ class PacketAssertConf(object): return self._promiscuous
class PacketAssertTestAndEvaluate(BaseRecipe):
- """
- Class to control and execute :any:`PacketAssert` test.
I think this should be enhanced a bit.
" This class provides an extension to BaseRecipe class to perform an evaluation of the captured packets on an interface. The class uses :any:`PacketAssert` test module to capture the packets based on the filters defined in :any:`PacketAssertConf` configuration. The pass or fail decision is made upon whether the number of the captured packets matching the criteria fits the min/max interval defined through :any:`PacketAssertConf`. "
""" started_job = None
def packet_assert_test_start(self, packet_assert_config):
"""Method starts a :any:`PacketAssert` job and stores ***started_job*** attributecontaining LNST :any:`Job: object.:param packet_assert_config: configuration in a form of :any:`PacketAssertConf`class""" if self.started_job: raise LnstError("Only 1 packet_assert job is allowed to run at a time.")@@ -54,6 +75,12 @@ class PacketAssertTestAndEvaluate(BaseRecipe): self.started_job = host.prepare_job(packet_assert).start(bg=True)
def packet_assert_test_stop(self):
"""Method kills a process executing :any:`PacketAssert` job andresets the value of the ***started_job*** attribute.:return: result of the packet assert""" if not self.started_job: raise LnstError("No packet_assert job is running.")
Another method that should be documented here is packet_assert_evaluate_and_report() as it is going to be used by a user.
diff --git a/lnst/Tests/PacketAssert.py b/lnst/Tests/PacketAssert.py index 25a92ba..901fdc0 100644 --- a/lnst/Tests/PacketAssert.py +++ b/lnst/Tests/PacketAssert.py @@ -8,6 +8,19 @@ from lnst.Tests.BaseTestModule import BaseTestModule from lnst.Common.LnstError import LnstError
class PacketAssert(BaseTestModule):
- """
- Class to control tcpdump test
- This class runs tcpdump and searches through its output in order
- to evaluate whether specified packets are traversing the network stack.
Few corrections and suggestions.
"Class to control tcpdump test" - er, what is the "tcpdump test"? I'd remove this completely.
I'd extend this a bit to give user an overview how this can be used and why it could be useful for the user.
Something like:
"This test module utilizes tcpdump to capture packets on a network interface based on filters defined by the test module parameters. It returns the number of captured packets that match the filter criteria.
This test module is usually used in conjunction with :any:`Ping` test module, for example:
pa_job = host1.run(PacketAssert()) host1.run(Ping()) pa_job.kill()
if pa_job.res_data['p_recv'] < 10: print("failed") "
Please update the code example as I made this up and I'm not sure about the accuracy of the method/module names.
- :param interface: interface used for the :any:`PacketAssert` test
I'd rather refer to tcpdump than the class itself.
- :param p_filter: string representation of desired packets in dump
This is a tcpdump pcap filter (man tcpdump, man 7 pcap-filter).
- :param grep_for: string representation of the content of given packet in dump
This is a regular expression to be matched in the string representation of a packet in the tcpdump output.
- :param promiscuous: toggle of promiscuous mode
- :param grep_exprs: concatenated expression to be used by grep for search
This is not a parameter exposed to user, please remove it.
To expose a TestModule class attribute as a parameter to user it has to be an instance of *Param class.
- :param p_recv: amount of received packets
This is not a parameter exposed to user, please remove it.
- """ interface = DeviceParam(mandatory=True) p_filter = StrParam(default='') grep_for = ListParam(default=[])
@@ -57,6 +70,13 @@ class PacketAssert(BaseTestModule): return False
def run(self):
"""Method used for the control of :any:`PacketAssert` test.Process with tcpdump is spawned and its output is digestedin order to find and count specified packets through grep expressions.:return: result of the run
So I think this could be probably removed as it's basically the same that I proposed for the class docstring and IMO is a better place.
Just a note that ":return: result of the run" is too vague.
""" self._res_data = {} if not is_installed("tcpdump"): self._res_data["msg"] = "tcpdump is not installed on this machine!"-- 2.25.4 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
From: Jozef Urbanovsky jurbanov@redhat.com
Signed-off-by: Jozef Urbanovsky jurbanov@redhat.com --- lnst/Recipes/ENRT/XfrmTools.py | 36 ++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/lnst/Recipes/ENRT/XfrmTools.py b/lnst/Recipes/ENRT/XfrmTools.py index fd226fc..789b42d 100644 --- a/lnst/Recipes/ENRT/XfrmTools.py +++ b/lnst/Recipes/ENRT/XfrmTools.py @@ -1,12 +1,32 @@ from lnst.Common.LnstError import LnstError
def generate_key(length): + """ + Method generates key suitable for the IPsec. + + :param length: Desired length of the generated key + :return: key + """ key = "0x" key = key + (length//8) * "0b" return key
def configure_ipsec_esp_aead(m1, ip1, m2, ip2, algo, algo_key, icv_len, ipsec_mode, spi_vals): + """ + Configuration of the IPsec is executed through iproute2's ip-xfrm + tool. Method creates state and policy for the ESP AEAD IPsec. + + :param m1: :any:`RemoteDevice` class describing tunnel's endpoint + :param ip1: :any:`Device` class containing IP addresses of given device under test + :param m2: :any:`RemoteDevice` class describing other side of tunnel endpoint + :param ip2: :any:`Device` class containing IP addresses of other side of the tunnel + :param algo: Algorithm chosen for the tunnel's ESP + :param algo_key: Generated key for the encryption algorithm + :param icv_len: Length of ICV for the AEAD encryption protocol + :param ipsec_mode: Mode of the ESP + :param spi_vals: SPI value for the identification + """ for m, in1, in2, in [(m1, ip2, ip1), (m2, ip1, ip2)]: m.run("ip xfrm policy flush") m.run("ip xfrm state flush") @@ -37,6 +57,22 @@ def configure_ipsec_esp_aead(m1, ip1, m2, ip2, algo, algo_key, icv_len,
def configure_ipsec_esp_ah_comp(m1, ip1, m2, ip2, ciph_alg, ciph_key, hash_alg, hash_key, ipsec_mode, spi_vals): + """ + Configuration of the IPsec is executed through iproute2's ip-xfrm + tool. Method creates state and policy for the ESP AH IPsec. + + :param m1: :any:`RemoteDevice` class describing tunnel's endpoint + :param ip1: :any:`Device` class containing IP addresses of given device under test + :param m2: :any:`RemoteDevice` class describing other side of tunnel endpoint + :param ip2: :any:`Device` class containing IP addresses of other side of the tunnel + :param ciph_alg: Cipher algorithm chosen for the tunnel's ESP + :param ciph_key: Generated key for the encryption algorithm + :param hash_alg: Algorithm for AH part of IPsec + :param hash_key: Generated key for the authentication protocol + :param icv_len: Length of ICV for the AH encryption protocol + :param ipsec_mode: Mode of the ESP + :param spi_vals: SPI value for the identification + """ m_keys = [] for m in [m1, m2]: res = m.run("rpm -qa iproute")
Wed, Sep 30, 2020 at 04:14:51PM CEST, jurbanov@redhat.com wrote:
From: Jozef Urbanovsky jurbanov@redhat.com
Signed-off-by: Jozef Urbanovsky jurbanov@redhat.com
lnst/Recipes/ENRT/XfrmTools.py | 36 ++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/lnst/Recipes/ENRT/XfrmTools.py b/lnst/Recipes/ENRT/XfrmTools.py index fd226fc..789b42d 100644 --- a/lnst/Recipes/ENRT/XfrmTools.py +++ b/lnst/Recipes/ENRT/XfrmTools.py @@ -1,12 +1,32 @@ from lnst.Common.LnstError import LnstError
def generate_key(length):
- """
- Method generates key suitable for the IPsec.
- :param length: Desired length of the generated key
- :return: key
- """ key = "0x" key = key + (length//8) * "0b" return key
def configure_ipsec_esp_aead(m1, ip1, m2, ip2, algo, algo_key, icv_len, ipsec_mode, spi_vals):
- """
- Configuration of the IPsec is executed through iproute2's ip-xfrm
- tool. Method creates state and policy for the ESP AEAD IPsec.
Could you swap the two sentences? First I'd start with what the method is used for, so: " This method creates states and policies for the ESP AEAD IPsec tunnel between two endpoinsts according to the specified parameters. The iproute2's ip-xfrm is used for the configuration. "
- :param m1: :any:`RemoteDevice` class describing tunnel's endpoint
Actually m1/m2 are Namespace instances that represents a host API for the user. It is an abstraction (and an extension of Host object) of the root namespace and any created network namespace in LNST. The reference to RemoteDevice is incorrect here.
- :param ip1: :any:`Device` class containing IP addresses of given device under test
Both ip1/ip2 are instances of BaseIpAddress, so simply an IPv4/IPv6 address in this case is sufficient IMO.
- :param m2: :any:`RemoteDevice` class describing other side of tunnel endpoint
- :param ip2: :any:`Device` class containing IP addresses of other side of the tunnel
- :param algo: Algorithm chosen for the tunnel's ESP
While we're at it, I'd rephrase this and others to match following pattern: "(Encryption algorithm) that will be used for the configuration of (ESP)"
- :param algo_key: Generated key for the encryption algorithm
- :param icv_len: Length of ICV for the AEAD encryption protocol
- :param ipsec_mode: Mode of the ESP
- :param spi_vals: SPI value for the identification
- """ for m, in1, in2, in [(m1, ip2, ip1), (m2, ip1, ip2)]: m.run("ip xfrm policy flush") m.run("ip xfrm state flush")
@@ -37,6 +57,22 @@ def configure_ipsec_esp_aead(m1, ip1, m2, ip2, algo, algo_key, icv_len,
def configure_ipsec_esp_ah_comp(m1, ip1, m2, ip2, ciph_alg, ciph_key, hash_alg, hash_key, ipsec_mode, spi_vals):
- """
- Configuration of the IPsec is executed through iproute2's ip-xfrm
- tool. Method creates state and policy for the ESP AH IPsec.
This method includes the IPComp in addition to the ESP/AH.
Same comments as for the aead variant of this method applies.
- :param m1: :any:`RemoteDevice` class describing tunnel's endpoint
- :param ip1: :any:`Device` class containing IP addresses of given device under test
- :param m2: :any:`RemoteDevice` class describing other side of tunnel endpoint
- :param ip2: :any:`Device` class containing IP addresses of other side of the tunnel
- :param ciph_alg: Cipher algorithm chosen for the tunnel's ESP
- :param ciph_key: Generated key for the encryption algorithm
- :param hash_alg: Algorithm for AH part of IPsec
- :param hash_key: Generated key for the authentication protocol
- :param icv_len: Length of ICV for the AH encryption protocol
- :param ipsec_mode: Mode of the ESP
- :param spi_vals: SPI value for the identification
- """ m_keys = [] for m in [m1, m2]: res = m.run("rpm -qa iproute")
-- 2.25.4 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
Thanks for the notes, I will work on followup patch
On Thu, Oct 1, 2020 at 12:53 PM Jan Tluka jtluka@redhat.com wrote:
Wed, Sep 30, 2020 at 04:14:51PM CEST, jurbanov@redhat.com wrote:
From: Jozef Urbanovsky jurbanov@redhat.com
Signed-off-by: Jozef Urbanovsky jurbanov@redhat.com
lnst/Recipes/ENRT/XfrmTools.py | 36 ++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/lnst/Recipes/ENRT/XfrmTools.py
b/lnst/Recipes/ENRT/XfrmTools.py
index fd226fc..789b42d 100644 --- a/lnst/Recipes/ENRT/XfrmTools.py +++ b/lnst/Recipes/ENRT/XfrmTools.py @@ -1,12 +1,32 @@ from lnst.Common.LnstError import LnstError
def generate_key(length):
- """
- Method generates key suitable for the IPsec.
- :param length: Desired length of the generated key
- :return: key
- """ key = "0x" key = key + (length//8) * "0b" return key
def configure_ipsec_esp_aead(m1, ip1, m2, ip2, algo, algo_key, icv_len, ipsec_mode, spi_vals):
- """
- Configuration of the IPsec is executed through iproute2's ip-xfrm
- tool. Method creates state and policy for the ESP AEAD IPsec.
Could you swap the two sentences? First I'd start with what the method is used for, so: " This method creates states and policies for the ESP AEAD IPsec tunnel between two endpoinsts according to the specified parameters. The iproute2's ip-xfrm is used for the configuration. "
Sure, that sounds reasonable.
- :param m1: :any:`RemoteDevice` class describing tunnel's endpoint
Actually m1/m2 are Namespace instances that represents a host API for the user. It is an abstraction (and an extension of Host object) of the root namespace and any created network namespace in LNST. The reference to RemoteDevice is incorrect here.
As we talked on IRC, I was describing the real class that the parameter uses, instead of how it is meant and how it is being used. I will fix this and refer to m1/m2 parameters as "handles for the Host API to describe the endpoint".
- :param ip1: :any:`Device` class containing IP addresses of given
device under test
Both ip1/ip2 are instances of BaseIpAddress, so simply an IPv4/IPv6 address in this case is sufficient IMO.
Ok, I will simplify this.
- :param m2: :any:`RemoteDevice` class describing other side of tunnel
endpoint
- :param ip2: :any:`Device` class containing IP addresses of other
side of the tunnel
- :param algo: Algorithm chosen for the tunnel's ESP
While we're at it, I'd rephrase this and others to match following pattern: "(Encryption algorithm) that will be used for the configuration of (ESP)"
Good point, I'm going to fix it.
- :param algo_key: Generated key for the encryption algorithm
- :param icv_len: Length of ICV for the AEAD encryption protocol
- :param ipsec_mode: Mode of the ESP
- :param spi_vals: SPI value for the identification
- """ for m, in1, in2, in [(m1, ip2, ip1), (m2, ip1, ip2)]: m.run("ip xfrm policy flush") m.run("ip xfrm state flush")
@@ -37,6 +57,22 @@ def configure_ipsec_esp_aead(m1, ip1, m2, ip2, algo,
algo_key, icv_len,
def configure_ipsec_esp_ah_comp(m1, ip1, m2, ip2, ciph_alg, ciph_key,
hash_alg,
hash_key, ipsec_mode, spi_vals):
- """
- Configuration of the IPsec is executed through iproute2's ip-xfrm
- tool. Method creates state and policy for the ESP AH IPsec.
This method includes the IPComp in addition to the ESP/AH.
Same comments as for the aead variant of this method applies.
Ok, I will mention the compression protocol as well.
- :param m1: :any:`RemoteDevice` class describing tunnel's endpoint
- :param ip1: :any:`Device` class containing IP addresses of given
device under test
- :param m2: :any:`RemoteDevice` class describing other side of tunnel
endpoint
- :param ip2: :any:`Device` class containing IP addresses of other
side of the tunnel
- :param ciph_alg: Cipher algorithm chosen for the tunnel's ESP
- :param ciph_key: Generated key for the encryption algorithm
- :param hash_alg: Algorithm for AH part of IPsec
- :param hash_key: Generated key for the authentication protocol
- :param icv_len: Length of ICV for the AH encryption protocol
- :param ipsec_mode: Mode of the ESP
- :param spi_vals: SPI value for the identification
- """ m_keys = [] for m in [m1, m2]: res = m.run("rpm -qa iproute")
-- 2.25.4 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to
lnst-developers-leave@lists.fedorahosted.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives:
https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
This is also not linked from anywhere, I think creating an "Other" section to the ENRT package and linking this there is probably ok.
-Ondrej
On Wed, Sep 30, 2020 at 04:14:51PM +0200, jurbanov@redhat.com wrote:
From: Jozef Urbanovsky jurbanov@redhat.com
Signed-off-by: Jozef Urbanovsky jurbanov@redhat.com
lnst/Recipes/ENRT/XfrmTools.py | 36 ++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/lnst/Recipes/ENRT/XfrmTools.py b/lnst/Recipes/ENRT/XfrmTools.py index fd226fc..789b42d 100644 --- a/lnst/Recipes/ENRT/XfrmTools.py +++ b/lnst/Recipes/ENRT/XfrmTools.py @@ -1,12 +1,32 @@ from lnst.Common.LnstError import LnstError
def generate_key(length):
- """
- Method generates key suitable for the IPsec.
- :param length: Desired length of the generated key
- :return: key
- """ key = "0x" key = key + (length//8) * "0b" return key
def configure_ipsec_esp_aead(m1, ip1, m2, ip2, algo, algo_key, icv_len, ipsec_mode, spi_vals):
- """
- Configuration of the IPsec is executed through iproute2's ip-xfrm
- tool. Method creates state and policy for the ESP AEAD IPsec.
- :param m1: :any:`RemoteDevice` class describing tunnel's endpoint
- :param ip1: :any:`Device` class containing IP addresses of given device under test
- :param m2: :any:`RemoteDevice` class describing other side of tunnel endpoint
- :param ip2: :any:`Device` class containing IP addresses of other side of the tunnel
- :param algo: Algorithm chosen for the tunnel's ESP
- :param algo_key: Generated key for the encryption algorithm
- :param icv_len: Length of ICV for the AEAD encryption protocol
- :param ipsec_mode: Mode of the ESP
- :param spi_vals: SPI value for the identification
- """ for m, in1, in2, in [(m1, ip2, ip1), (m2, ip1, ip2)]: m.run("ip xfrm policy flush") m.run("ip xfrm state flush")
@@ -37,6 +57,22 @@ def configure_ipsec_esp_aead(m1, ip1, m2, ip2, algo, algo_key, icv_len,
def configure_ipsec_esp_ah_comp(m1, ip1, m2, ip2, ciph_alg, ciph_key, hash_alg, hash_key, ipsec_mode, spi_vals):
- """
- Configuration of the IPsec is executed through iproute2's ip-xfrm
- tool. Method creates state and policy for the ESP AH IPsec.
- :param m1: :any:`RemoteDevice` class describing tunnel's endpoint
- :param ip1: :any:`Device` class containing IP addresses of given device under test
- :param m2: :any:`RemoteDevice` class describing other side of tunnel endpoint
- :param ip2: :any:`Device` class containing IP addresses of other side of the tunnel
- :param ciph_alg: Cipher algorithm chosen for the tunnel's ESP
- :param ciph_key: Generated key for the encryption algorithm
- :param hash_alg: Algorithm for AH part of IPsec
- :param hash_key: Generated key for the authentication protocol
- :param icv_len: Length of ICV for the AH encryption protocol
- :param ipsec_mode: Mode of the ESP
- :param spi_vals: SPI value for the identification
- """ m_keys = [] for m in [m1, m2]: res = m.run("rpm -qa iproute")
-- 2.25.4 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
From: Jozef Urbanovsky jurbanov@redhat.com
Signed-off-by: Jozef Urbanovsky jurbanov@redhat.com --- lnst/Recipes/ENRT/IpsecEspAeadRecipe.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py index 015db74..30da2fb 100644 --- a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py +++ b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py @@ -57,8 +57,9 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
def test_wide_configuration(self): """ - Test wide configuration for this recipe involves just adding an IPv4 and - IPv6 address to the matched eth0 nics on both hosts and route between them. + Test wide configuration for this recipe involves adding an IPv4 and + IPv6 address to the matched eth0 nics on both hosts and creating a route + between them.
host1.eth0 = 192.168.101.1/24 and fc00::1/64
@@ -120,6 +121,11 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe, """ Test wide configuration is extended with subconfiguration containing IPsec tunnel with predefined parameters for both IP versions. + Required variables for the configuration of IPsec tunnel are generated + and yieled for later application of subconfiguration. + + Yielded as:: + (new_config) """ ipsec_mode = self.params.ipsec_mode spi_values = self.spi_values @@ -144,7 +150,7 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe, def apply_sub_configuration(self, config): """ Subconfiguration containing IPsec tunnel is applied through - XfrmTools class. + :any:`XfrmTools` class' methods using the previously generated configuration. """ super().apply_sub_configuration(config) ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns @@ -162,7 +168,7 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe, def generate_ping_configurations(self, config): """ The ping endpoints for this recipe are the configured endpoints of - the IPsec tunnel on both hosts. + the IPsec on both hosts. """ ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns ip1, ip2 = config.ips @@ -180,8 +186,9 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
def generate_flow_combinations(self, config): """ - Flow combinations are generated based on the tunnel endpoints - and test parameters. + Flow combinations are generated based on the IPsec endpoints + and test parameters specified. All combinations are looped over + and each flow is yielded. """ nic1, nic2 = config.endpoint1, config.endpoint2 ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns @@ -211,7 +218,9 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
def ping_test(self, ping_configs): """ - Ping test is utilizing PacketAssert class to search + Ping test method overrides method of class :any:`PingTestAndEvaluate`. + Method executes ping test according to the ping configuration through + the IPsec with the use of :any:`PacketAssert` class to search for the appropriate ESP IP packet. Result of ping test is handed to the super class' method.
Wed, Sep 30, 2020 at 04:14:52PM CEST, jurbanov@redhat.com wrote:
From: Jozef Urbanovsky jurbanov@redhat.com
Signed-off-by: Jozef Urbanovsky jurbanov@redhat.com
lnst/Recipes/ENRT/IpsecEspAeadRecipe.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py index 015db74..30da2fb 100644 --- a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py +++ b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py @@ -57,8 +57,9 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
def test_wide_configuration(self): """
Test wide configuration for this recipe involves just adding an IPv4 andIPv6 address to the matched eth0 nics on both hosts and route between them.
Test wide configuration for this recipe involves adding an IPv4 andIPv6 address to the matched eth0 nics on both hosts and creating a routebetween them. host1.eth0 = 192.168.101.1/24 and fc00::1/64@@ -120,6 +121,11 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe, """ Test wide configuration is extended with subconfiguration containing IPsec tunnel with predefined parameters for both IP versions.
Required variables for the configuration of IPsec tunnel are generatedand yieled for later application of subconfiguration.Yielded as::(new_config)
I'm not sure how much value the user gets from that.
I recommend following reading: https://www.writethedocs.org/guide/writing/beginners-guide-to-docs/#what-to-...
I think that info about yielding is quite redundant as this is obvious from the code itself.
I'd really focus on the user point of view here, rather than from a developer point of view.
In that case the main focus of documentation here should be to explain __why__ this method is different from the one used in other recipes (where it's inherited from the BaseSubConfigMixin).
Also, I think that you should mention that for this specific recipe the sub configurations are combinations of IPSec mode (transport, tunnel) and encryption algorithm that are passed through the recipe parameters.
Please, try to think about what should be in the documentation for the user, not the developer.
""" ipsec_mode = self.params.ipsec_mode spi_values = self.spi_values@@ -144,7 +150,7 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe, def apply_sub_configuration(self, config): """ Subconfiguration containing IPsec tunnel is applied through
XfrmTools class.
:any:`XfrmTools` class' methods using the previously generated configuration. """ super().apply_sub_configuration(config) ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns@@ -162,7 +168,7 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe, def generate_ping_configurations(self, config): """ The ping endpoints for this recipe are the configured endpoints of
the IPsec tunnel on both hosts.
the IPsec on both hosts.
Not sure why you omitted the tunnel word? I think it's ok to keep it.
""" ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns ip1, ip2 = config.ips@@ -180,8 +186,9 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
def generate_flow_combinations(self, config): """
Flow combinations are generated based on the tunnel endpointsand test parameters.
Flow combinations are generated based on the IPsec endpointsand test parameters specified. All combinations are looped overand each flow is yielded. """ nic1, nic2 = config.endpoint1, config.endpoint2 ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns@@ -211,7 +218,9 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
def ping_test(self, ping_configs): """
Ping test is utilizing PacketAssert class to search
Ping test method overrides method of class :any:`PingTestAndEvaluate`.Method executes ping test according to the ping configuration throughthe IPsec with the use of :any:`PacketAssert` class to search for the appropriate ESP IP packet. Result of ping test is handed to the super class' method.-- 2.25.4 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
On Thu, Oct 1, 2020 at 1:19 PM Jan Tluka jtluka@redhat.com wrote:
Wed, Sep 30, 2020 at 04:14:52PM CEST, jurbanov@redhat.com wrote:
From: Jozef Urbanovsky jurbanov@redhat.com
Signed-off-by: Jozef Urbanovsky jurbanov@redhat.com
lnst/Recipes/ENRT/IpsecEspAeadRecipe.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
index 015db74..30da2fb 100644 --- a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py +++ b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py @@ -57,8 +57,9 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin,
BaseEnrtRecipe,
def test_wide_configuration(self): """
Test wide configuration for this recipe involves just adding anIPv4 and
IPv6 address to the matched eth0 nics on both hosts and routebetween them.
Test wide configuration for this recipe involves adding an IPv4and
IPv6 address to the matched eth0 nics on both hosts and creatinga route
between them. host1.eth0 = 192.168.101.1/24 and fc00::1/64@@ -120,6 +121,11 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin,
BaseEnrtRecipe,
""" Test wide configuration is extended with subconfigurationcontaining
IPsec tunnel with predefined parameters for both IP versions.
Required variables for the configuration of IPsec tunnel aregenerated
and yieled for later application of subconfiguration.Yielded as::(new_config)I'm not sure how much value the user gets from that.
I recommend following reading: https://www.writethedocs.org/guide/writing/beginners-guide-to-docs/#what-to-...
I think that info about yielding is quite redundant as this is obvious from the code itself.
I'd really focus on the user point of view here, rather than from a developer point of view.
In that case the main focus of documentation here should be to explain __why__ this method is different from the one used in other recipes (where it's inherited from the BaseSubConfigMixin).
Also, I think that you should mention that for this specific recipe the sub configurations are combinations of IPSec mode (transport, tunnel) and encryption algorithm that are passed through the recipe parameters.
Please, try to think about what should be in the documentation for the user, not the developer.
Sure, I will work on more user-centric documentation and fix aforementioned issues.
""" ipsec_mode = self.params.ipsec_mode spi_values = self.spi_values@@ -144,7 +150,7 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin,
BaseEnrtRecipe,
def apply_sub_configuration(self, config): """ Subconfiguration containing IPsec tunnel is applied through
XfrmTools class.
:any:`XfrmTools` class' methods using the previously generatedconfiguration.
""" super().apply_sub_configuration(config) ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns@@ -162,7 +168,7 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin,
BaseEnrtRecipe,
def generate_ping_configurations(self, config): """ The ping endpoints for this recipe are the configured endpointsof
the IPsec tunnel on both hosts.
the IPsec on both hosts.Not sure why you omitted the tunnel word? I think it's ok to keep it.
I deleted the "tunnel" due to the IPsec's mode "tunnel" which might be confusing. We test both modes - tunnel and transport and it might mislead the user to think we actually only test tunnel mode.
""" ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns ip1, ip2 = config.ips@@ -180,8 +186,9 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin,
BaseEnrtRecipe,
def generate_flow_combinations(self, config): """
Flow combinations are generated based on the tunnel endpointsand test parameters.
Flow combinations are generated based on the IPsec endpointsand test parameters specified. All combinations are looped overand each flow is yielded. """ nic1, nic2 = config.endpoint1, config.endpoint2 ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns@@ -211,7 +218,9 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin,
BaseEnrtRecipe,
def ping_test(self, ping_configs): """
Ping test is utilizing PacketAssert class to search
Ping test method overrides method of class:any:`PingTestAndEvaluate`.
Method executes ping test according to the ping configurationthrough
the IPsec with the use of :any:`PacketAssert` class to search for the appropriate ESP IP packet. Result of ping test is handed to the super class' method.-- 2.25.4 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to
lnst-developers-leave@lists.fedorahosted.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives:
https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahos...
lnst-developers@lists.fedorahosted.org
-
Jan Tluka -
Jozef Urbanovsky -
jurbanov＠redhat.com
-
Ondrej Lichtner