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

Container firewall test #754

Merged
merged 4 commits into from
Oct 23, 2024
Merged

Container firewall test #754

merged 4 commits into from
Oct 23, 2024

Conversation

troglobit
Copy link
Contributor

@troglobit troglobit commented Oct 22, 2024

Description

Checklist

Tick relevant boxes, this PR is-a or has-a:

  • Bugfix
    • Regression tests
    • ChangeLog updates (for next release)
  • Feature
    • YANG model change => revision updated?
    • Regression tests added?
    • ChangeLog updates (for next release)
    • Documentation added?
  • Test changes
    • Checked in changed Readme.adoc (make test-spec)
    • Added new test to group Readme.adoc and yaml file
  • Code style update (formatting, renaming)
  • Refactoring (please detail in commit messages)
  • Build related changes
  • Documentation content changes
    • ChangeLog updated (for major changes)
  • Other (please describe):

@troglobit troglobit added the ci:main Build default defconfig, not minimal label Oct 22, 2024
@troglobit troglobit added this to the Infix v24.10.2 milestone Oct 22, 2024
@troglobit
Copy link
Contributor Author

Ping @jovatn, this PR also contains a proposal for how a test section/group could look. It's a litmus test to see if we're aligned or if we need to discuss this a bit more. (Re: discussion you, me, and @axkar had the other day.)

@jovatn
Copy link
Contributor

jovatn commented Oct 23, 2024

Great work!
Two comments:

  • On section/group: Although informative to have a description on each section, I realized there are some risks. (1) Easy to forget to update if tests are added, removed or modified. (2) What if we create a test-suite which excludes some of the tests within a section? Then the section description risks being incorrect. This said, I still think it is a good idea, but we ought to avoid being too specific in the section description.
  • On the firewall container test: Great! I only have a question on formatting; should we rename ext0/int0 to ext1/int1 or ext/int? No big deal, but we have changed the port naming to start by 1 instead of 0 in all(?) other tests.

Signed-off-by: Joachim Wiberg <[email protected]>
@troglobit
Copy link
Contributor Author

troglobit commented Oct 23, 2024

On section/group: Although informative to have a description on each section, I realized there are some risks. (1) Easy to forget to update if tests are added, removed or modified. (2) What if we create a test-suite which excludes some of the tests within a section? Then the section description risks being incorrect. This said, I still think it is a good idea, but we ought to avoid being too specific in the section description.

OK, I can drop it until we've agreed upon a strategy for these. Would that be better? (ping @mattiaswal)

On the firewall container test: Great! I only have a question on formatting; should we rename ext0/int0 to ext1/int1 or ext/int? No big deal, but we have changed the port naming to start by 1 instead of 0 in all(?) other tests.

Well, we have cases where we use veth0a<-->veth0b in examples and documentation, so I don't think this would be much of a difference.

@jovatn
Copy link
Contributor

jovatn commented Oct 23, 2024

Well, we have cases where we use veth0a<-->veth0b in examples and documentation, so I don't think this would be much of a difference.

I agree. We frequently use br0 as well.

@troglobit
Copy link
Contributor Author

Well, we have cases where we use veth0a<-->veth0b in examples and documentation, so I don't think this would be much of a difference.
I agree. We frequently use br0 as well.

Yeah, I think we talked about this at one point AFK as well. Iirc, the agreement was to use positive integers for physical interfaces (or interfaces supposed to emulate physical interfaces in virtual setups like Qemu/GNS3).

Side note, I wonder if the confusion in computer science, and computers in general, stems from the one in natural numbers, which some define as excluding zero?

This is a proposal to how a test section/group overview could look.

Signed-off-by: Joachim Wiberg <[email protected]>
@mattiaswal
Copy link
Contributor

Great work! Two comments:

  • On section/group: Although informative to have a description on each section, I realized there are some risks. (1) Easy to forget to update if tests are added, removed or modified. (2) What if we create a test-suite which excludes some of the tests within a section? Then the section description risks being incorrect. This said, I still think it is a good idea, but we ought to avoid being too specific in the section description.
  • On the firewall container test: Great! I only have a question on formatting; should we rename ext0/int0 to ext1/int1 or ext/int? No big deal, but we have changed the port naming to start by 1 instead of 0 in all(?) other tests.

I agree on the group part, but as I see this is better than nothing. The test specification will not be done with this release, we must continue to work with the graphical parts.

My idea of the group page is to describe the functions tested overall, but I doubt someone has time to do it now.

Copy link
Contributor

@mattiaswal mattiaswal left a comment

Choose a reason for hiding this comment

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

Great work! Gold star for you! ⭐ ⭐ ⭐ ⭐ ⭐

@troglobit troglobit merged commit d36296d into main Oct 23, 2024
3 checks passed
@troglobit troglobit deleted the firewall-test branch October 23, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:main Build default defconfig, not minimal
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants