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

chore(pipelined): blocking of local ipv6 addresses is supported #12339

Merged
merged 3 commits into from Apr 6, 2022

Conversation

nstng
Copy link
Contributor

@nstng nstng commented Mar 29, 2022

Summary

See #12074.

Couple of notes and questions @pshelar and @koolzz:

  • access_control.py uses netifaces which is outdated
  • please have a detailed look at our test: we replaced the suffix of fe80:: addresses as these are not static in the vm setup - and it is disproportionate to make them static. Is there a test mechanic we did not see that achieves the same?
  • in the ipv4 case all 127.* addresses are blocked - from our research we found that there is not really a fixed ipv6 block for loopback addresses. For now we only blocked in this case ::1. What is your expectation here?
  • should this feature be extended to the blocking list mechanics for ipv6?

Test Plan

Mainly test_access_control.py.

Additional Information

  • This change is backwards-breaking

@nstng nstng requested a review from a team March 29, 2022 15:33
@nstng nstng requested a review from a team as a code owner March 29, 2022 15:33
@nstng nstng requested a review from koolzz March 29, 2022 15:33
@pull-request-size pull-request-size bot added the size/XL Denotes a Pull Request that changes 500-999 lines. label Mar 29, 2022
@nstng nstng requested a review from pshelar March 29, 2022 15:33
@github-actions
Copy link
Contributor

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@github-actions github-actions bot added the component: agw Access gateway-related issue label Mar 29, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 2022

Oops! Looks like you failed the Python Format Check.

Howto

♻️ Updated: ✅ The check is passing the Python Format Check after the last commit.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 2022

feg-workflow

    2 files  202 suites   33s ⏱️
363 tests 363 ✔️ 0 💤 0
377 runs  377 ✔️ 0 💤 0

Results for commit 85f4a50.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 2022

agw-workflow

     77 files     122 suites   6m 27s ⏱️
1 134 tests 1 125 ✔️ 9 💤 0
1 135 runs  1 126 ✔️ 9 💤 0

Results for commit 85f4a50.

♻️ This comment has been updated with latest results.

@nstng nstng linked an issue Mar 29, 2022 that may be closed by this pull request
@nstng nstng force-pushed the pipelined_block_local_ipv6 branch 2 times, most recently from f7a8b7e to b9ccd38 Compare April 1, 2022 13:23
@github-actions
Copy link
Contributor

github-actions bot commented Apr 1, 2022

Oops! Looks like you failed the DCO check. Be sure to sign all your commits.

Howto

♻️ Updated: ✅ The check is passing the DCO check after the last commit.

@nstng nstng force-pushed the pipelined_block_local_ipv6 branch from a354e3e to 151cccc Compare April 1, 2022 13:45
@Neudrino Neudrino requested a review from pshelar April 4, 2022 09:26
),
ip_proto=IPPROTO_ICMP,
)
return MagmaMatch(
Copy link
Contributor

Choose a reason for hiding this comment

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

better readability you could add 'else' here.

Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>
Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>
Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>
@nstng nstng force-pushed the pipelined_block_local_ipv6 branch from 151cccc to 85f4a50 Compare April 5, 2022 10:05
@nstng nstng added ready2merge This PR is ready to be merged (is approved and passes all required checks) and removed ready2merge This PR is ready to be merged (is approved and passes all required checks) labels Apr 5, 2022
@nstng
Copy link
Contributor Author

nstng commented Apr 5, 2022

running integ tests on fork: https://github.com/nstng/magma/actions/runs/2096312195 (/)

@nstng nstng added the ready2merge This PR is ready to be merged (is approved and passes all required checks) label Apr 6, 2022
@sebathomas sebathomas merged commit 69d2344 into magma:master Apr 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2022

Unit Test Results

  1 files    1 suites   10m 59s ⏱️
40 tests 29 ✔️ 11 💤 0

Results for commit 69d2344.

♻️ This comment has been updated with latest results.

sreedharkumartn added a commit to sreedharkumartn/magma that referenced this pull request Apr 12, 2022
…a#12339)

Signed-off-by: sreedharkumartn <sreedhar.kumar@wavelabs.ai>
@panyogesh panyogesh added the backport-v1.7 MME fixes to be backported for 1.7 label Apr 25, 2022
emakeev pushed a commit to emakeev/magma that referenced this pull request Aug 5, 2022
…a#12339)

* chore(pipelined): access control test is refactored

Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

* chore(pipelined): blocking of local ipv6 addresses is supported

Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

* chore(pipelined): blocking of local ipv6 addresses is tested

Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v1.7 MME fixes to be backported for 1.7 component: agw Access gateway-related issue ready2merge This PR is ready to be merged (is approved and passes all required checks) size/XL Denotes a Pull Request that changes 500-999 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datapath: Add support for IPv6 local address in access control app.
5 participants