Add crypto extern to behavioral-model

[hesingh]

Please see this Issue:

https://github.com/p4lang/behavioral-model/issues/697

A .cpp file is added where the new extern is implemented. A .p4 filed is added where the extern is defined and used.

Thanks to the folks who developed this code and put it in at this repo:

https://github.com/uni-tue-kn/p4-macsec

[jafingerhut]

I am sure Antonin will have his own comments, but my high level comment is that I would be happy to try this out to see if it works, but only if there is an explicit README file somewhere, in the simple_switch/externs/crypto directory, that gave step-by-step instructions for:

How to compile the .cpp file, with actual working commands, and all options used. It is reasonable to do so only for the main supported platform, i.e. Ubuntu 16.04 Linux.

How to compile the P4 program, with all command line options specified fully.

How to run simple_switch or simple_switch_grpc, with all command line options specified fully.

Bonus points if the README explains which C++ functions are called by simple_switch directly, and an example of what the parameter values are in the P4 program, and what they become (if it isn't obvious) in the C++ function call, what the C++ function return value is (unless it is void), and if that value becomes visible in the P4 program somehow. Also how out and inout parameters are handled, if they are relevant for the example.

[jafingerhut]

Also good to mention what the license is on the code that you copied or based this on, and preserve the names of the original authors, and link to the original in comments. In particular, it seems best not to have a boilerplate copyright with Barefoot Networks in there, unless someone at Barefoot Networks wrote it.

We should not relicense code in a legally incompatible way, unless the original authors are willing to release it under that license, too.

[hesingh]

Sure, will add README.md. However, I am little lost for instructions because the original repo has no README.md file but the code uses a controller, mininet, and also simple_switch. These instructions may take a little while to put together.

Regarding license, please see one file they have changed at this link:

https://github.com/uni-tue-kn/p4-macsec/blob/master/p4/target/simple_switch.cpp

The original Apache 2.0 license is retained.

I have pinged the folks by filing this issue: https://github.com/uni-tue-kn/p4-macsec/issues/1

[jafingerhut]

I don't think it is necessary to give a working example that uses Mininet here.

Instructions for running a single simple_switch or simple_switch_grpc process, with all command line options needed to use the new extern, should be enough. Let those who wish to use the new extern in a Mininet network figure out the changes to their own software outside of simple_switch.

[hesingh]

OK.

I am looking at the Travis failure right now. I have let the authors know what would help in the README.md file. thanks.

[hesingh]

I could not reproduce the Travis build failure on my local machine using Ubuntu 18.04. I will see how to force another Travis build.

[jafingerhut]

Most of the messages I saw looked like style warnings in the syntax/formatting of the C++ source code file, e.g. must be a space after "//" comment beginning, or line too long. Search for the string "should have a space" on this Travis log page: https://travis-ci.org/p4lang/behavioral-model/jobs/624343929?utm_medium=notification&utm_source=github_status

[hesingh]

Right - I did see that and fixed all those already. I am fixing other style issues - thanks.

[codecov-io]

Codecov Report

Merging #834 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #834   +/-   ##
=======================================
  Coverage   74.77%   74.77%           
=======================================
  Files         115      115           
  Lines       10193    10193           
=======================================
  Hits         7622     7622           
  Misses       2571     2571

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9ef3248...f187058. Read the comment docs.

[jafingerhut]

@hesingh Thanks for continuing to enhance this PR with more details. I would like to try them out when there are directions I can try following. I see a recent commit that claims it adds a README.md file, but there is no README.md file in the current PR. Did you forget to add that file using git on your end?

[jafingerhut]

Ignore my previous comment. I see a README.md file. Let me see if it looks complete. Thanks.

[hesingh]

Sorry, during a checkin, part of the README.md file was not incorporated. So, I checked in another version that now has the complete README.md with one TODO and maybe requiring more clarifications.

[hesingh]

The original macsec code uses send.py and receive.py in https://github.com/uni-tue-kn/p4-macsec/p4/p4. There is no README.md to show which IP address/mac-address to send a packet to or how is simple_grpc used to send and receive packets. This is why such notes are pending for README.md or this PR.

[hesingh]

Also good to mention what the license is on the code that you copied or based this on, and preserve the names of the original authors, and link to the original in comments. In particular, it seems best not to have a boilerplate copyright with Barefoot Networks in there, unless someone at Barefoot Networks wrote it.

We should not relicense code in a legally incompatible way, unless the original authors are willing to release it under that license, too.

Few days back, I asked the authors to add a CONTRIBUTING.md file to their repo so that their code's licensing is clear. They didn't get back yet. This is why my code changes do not have licensing changes.

[hesingh]

@jafingerhut I am compiling their basic.p4 after fixing mark_to_drop(). I see an error with recirculate. I checked your p4-info code but didn't find a use for recirculate like what this code has. Please see basic.p4 in this PR which fails to compile.

hemant@ubuntu:~/p4c/build$ p4c-bm2-ss --std p4-16 --emit-externs --p4runtime-files foo.txt /tmp/int/behavioral-model/targets/simple_switch/externs/sym_crypto/basic.p4 
/tmp/int/behavioral-model/targets/simple_switch/externs/sym_crypto/basic.p4(288): [--Wwarn=unsupported] warning: recirculate: recirculate with non-empty argument not supported
                recirculate({meta.intrinsic_metadata, standard_metadata, meta.user_metadata});
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[jafingerhut]

@hesingh I don't see any strong reason that an example P4 program for a crypto (or any) extern function needs to also have a resubmit/recirculate/clone operation. Does the code compile if you just delete or comment out that part of the P4 code?

[jafingerhut]

And if we do not get the authors to give their permission, like I said before, a working example of an extern function that adds 7 to its in parameter and sends it back as the value of an out parameter, which can be compiled into simple_switch and used from a P4_16 program compiled with p4c, that is a more complete example of an extern than anything in the behavioral-model repository now. We could save crypto example for later when someone chooses to contribute one.

[hesingh]

@hesingh I don't see any strong reason that an example P4 program for a crypto (or any) extern function needs to also have a resubmit/recirculate/clone operation. Does the code compile if you just delete or comment out that part of the P4 code?

Yes, the code does compile after commenting out the recirculate line of code from the P4 file. I think a decrypted packet could go to cpu, bu tI guess, the authors just decided to send the packet to recirculate.

[hesingh]

And if we do not get the authors to give their permission, like I said before, a working example of an extern function that adds 7 to its in parameter and sends it back as the value of an out parameter, which can be compiled into simple_switch and used from a P4_16 program compiled with p4c, that is a more complete example of an extern than anything in the behavioral-model repository now. We could save crypto example for later when someone chooses to contribute one.

Ok, sounds reasonable to me. I have also narrowed down the test to be in https://github.com/uni-tue-kn/p4-macsec/tree/master/p4/p4/ directory with use of s1-commands.txt, etc. and .py send and receive. I am not sure how to run these when simple_switch/simple_switch_grpc is launched.

[hesingh]

The macsec folks came through today. Have granted Apache 2.0 license which is same as the p4c license. They also added a README.md file. Sincere thanks is owed to them.

[hesingh]

@hesingh Thanks for continuing to enhance this PR with more details. I would like to try them out when there are directions I can try following. I see a recent commit that claims it adds a README.md file, but there is no README.md file in the current PR. Did you forget to add that file using git on your end?

If you'd like you can try the steps at macsec repo to test the crypto extern. The scripts use an older version of p4c, behavoiral-model, and PI code. Or you can wait till the repo updates code using latest p4c, etc. thanks.

[jafingerhut]

I'd like to wait until their README is updated, to see if that version looks accurate, and provide comments on it.

Is there any reason to have both a .cpp file and the diffs file with nearly identical contents in this PR?

Is there any reason to have either one of those, or the .p4 program, if they also exist in the linked other Github repository, along with the instructions for how to use them?

It seems to me either:

(a) this PR should be a README with a link to the other repo, and pretty much nothing else

or

(b) it should be possible without installing mininet, or anything else not absolutely required to install behavioral-model simple_switch according to behavioral-model's current README instructions, to follow README instructions completely within behavioral-model's repository to run a single simple_switch executable with an extra extern added.

Personally, I am content either way. It seems the current PR is much more than (a), and much different than (b).

[hesingh]

I'd like to wait until their README is updated, to see if that version looks accurate, and provide comments on it.

Is there any reason to have both a .cpp file and the diffs file with nearly identical contents in this PR?

Is there any reason to have either one of those, or the .p4 program, if they also exist in the linked other Github repository, along with the instructions for how to use them?

It seems to me either:

(a) this PR should be a README with a link to the other repo, and pretty much nothing else

or

(b) it should be possible without installing mininet, or anything else not absolutely required to install behavioral-model simple_switch according to behavioral-model's current README instructions, to follow README instructions completely within behavioral-model's repository to run a single simple_switch executable with an extra extern added.

Personally, I am content either way. It seems the current PR is much more than (a), and much different than (b).

I went with (a). Please see a checkin I made right now. I think the README.md instructions should get updated in a day or two for the macsec repo.

[hesingh]

I'd like to wait until their README is updated, to see if that version looks accurate, and provide comments on it.

The README has been updated today. Please take a look - thanks.

[hesingh]

Is there any reason to have either one of those, or the .p4 program, if they also exist in the linked other Github repository, along with the instructions for how to use them?

It seems to me either:

(a) this PR should be a README with a link to the other repo, and pretty much nothing else

There is a reason. In the macsec repo, they have an old version of simple_switch.cpp in p4/targets/ with the crypto extern. For a new user, why sift through an old version of simple_switch.cpp in the macsec repo to find the extern. This is why I have added just the extern implementation in crypt.cpp of this PR. Also, even if the macsec repo grabs the latest simple_switch.cpp, in one month, simple_switch.cpp is old again. With code in crypt.cpp, a user can take the code and add to any version of simple_switch.cpp with ease.

basic.p4 is self-contained implementation and this is why I have deleted it from this repo and referenced it in the README to macsec repo

[hesingh]

File a new issue with macsec repo to fix cpplint errors in their code.

https://github.com/uni-tue-kn/p4-macsec/issues/2

[hesingh]

The macsec repo accepted my cpplint fixes in their code and updated their repo. Then, I copied their extern to crypto.cpp of this PR. We could consider committing this PR's code change.

[hesingh]

Now, the macsec repo README.md has its final edit made one week back. Just run ./setup.sh in the root directory and the software is installed.

The code changes are this PR are also final until any other changes are suggested.

[antoninbas]

Where does the name "sym_crypto" comes from? Why not "macsec" to make it clear what this does?

[antoninbas]

We shouldn't place this directly under the "externs" directory because to me it implies that this is an official extern for simple_switch / v1model. Maybe a "sample_externs" directory would be better.

I think @jafingerhut may have pointed this out somewhere else previously: while I think there is value in having a more complex extern type such as this one, maybe we should start with a very basic one that just prints the time or something like this. We can add a more complex one when we figure out exactly how we want to organize the "sample_externs" directory.

Edit: if we go with a basic extern, we still want something that takes a parameter, in order to keep things interesting. Maybe some accumulator with one method to "accumulate" (i.e. increment an internal counter by a specified amount) and one method to log the current counter value.