-
Notifications
You must be signed in to change notification settings - Fork 581
Add FreeBSD as a platform #1286
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
base: main
Are you sure you want to change the base?
Conversation
Cc @samuelkarp |
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.
A few initial (high level) thoughts 👍❤️
99dd807
to
ee03193
Compare
Fixed the type of FreeBSDDevice.Mode and fixed a typo in the json mapping for FreeBSDJail.SysVShm. |
I took another pass over this today and made a couple of minor changes. I think the only current open question is whether the Does anyone else have comments or suggestions for the FreeBSD runtime extension? What should our next steps be to make this acceptable for the runtime spec? |
@dfr we attended the developer meeting but it was Juneteenth and the attendance was relatively low. They suggested that you ping Tianon and Sam to ask for them to review it. If that doesn't work, we can try to attend the dev call again. |
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.
I've no specific comments on the FreeBSD part, but I can help to move this forward.
I've left an inline comment
Thanks! I will check out your comment on the PR but my internet connection is flaky at the moment which is slowing me down a bit (hopefully that will be fixed over the weekend). |
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.
This looks pretty decent to me, although admittedly I'm not super familiar with FreeBSD / jails internals.
If we're not in a hurry to merge, I'd love to wait until @samuelkarp has a chance to review too, but if we've got something urgent that needs this, I think it's probably in a reasonable state (and hopefully he doesn't disagree).
051877a
to
bb9e6b2
Compare
It looks like Sam is going to be away for a while focusing on family matters and he is unlikely to be able to give feedback in the near future. He was involved in the working group which led to this pull request and was a key reviewer during that process if that helps. As far as I know, he has not read through the exact text from this pull request. Timing wise, I am not in a great hurry. It would be great if it makes it into the 1.3.0 release if at all possible. |
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.
A couple minor comments, but generally this LGTM. I may not be available to review again after my suggestions are addressed, so please consider this to be my approval either way.
Edit: see further comments.
ac99f06
to
5a74ea4
Compare
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.
I started trying to migrate runj to this and immediately ran into challenges with vnet interfaces. Are we expecting to require CNI in order to do networking? That seems at odds with the work that was just merged for Linux in #1271.
| `jid` | - | | ||
| `name` | see below | | ||
| `path` | `root.path` | | ||
| `ip4.addr` | - | |
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.
Is there a reason we didn't define ip4.addr
and ip6.addr
?
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.
IP address management currently happens on the host using CNI and/or netavark on both FreeBSD and Linux.
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.
Since IP address management is supported by the kernel's jail interface directly, I don't think it's reasonable to require an OCI runtime to depend on CNI or netavark.
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.
Thinking about ip4.addr and ip6.addr, my main question is about how to represent IPv4 and IPv6 addresses. My inclination is to use json arrays of strings where each string is a numeric IPv4/IPv6 address. These could be passed to jail(8)
via a config file or for ocijail (which uses the syscall interface directly), converted to what the kernel expects using getaddrinfo(3)
. As far as I can tell, #1271 only covers network interfaces, not the addresses on those interfaces.
| `ip6.addr` | - | | ||
| `ip6.saddrsel` | - | | ||
| `ip6` | `freebsd.jail.ip6` | | ||
| `vnet` | `freebsd.jail.vnet` | |
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.
vnet.interface
seems to be missing.
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.
vnet.interface
seems to be missing.
Injecting interfaces into the vnet also currently happens on the host using CNI or netavark
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.
As above, I don't think it's reasonable to require CNI or netavark here. This is especially true in light of #1271.
This uses FreeBSD jails to implement container isolation. Signed-off-by: Doug Rabson <dfr@rabson.org>
I just pushed a small update which adds support for jail's |
The interface added in #1271 looks useful. Currently it seems to be Linux-specific and I'm not sure how it handles the firewall rules which are typically added for NAT and port forwarding. |
Yes, but we have equivalent FreeBSD-specific capabilities provided by the jail interface in the kernel and we are defining the FreeBSD-specific config section here.
It doesn't. This is also true for the jail interface in FreeBSD; that's left up to things like pf. |
I think I'm currently 👎 on this PR as-is. There are two known OCI jail implementations (well, they're not currently OCI given the spec doesn't have FreeBSD support yet, but they are planned to be): runj and ocijail. The config defined here does not satisfy the requirements of runj as it exists today. I think it'd be a reasonable requirement from the spec maintainers to say that there should at least be a POC PR that demonstrates the proposed changes working in both runj and ocijail before merging. I'm tracking that for runj in samuelkarp/runj#60 and welcome contributions to that (I also have some WIP code on my laptop, but my time here is extremely limited right now). |
Per Sam's latest, which I agree with / support
I still think that CNI etc. adds value for some use-cases but it certainly doesn't hurt to add some support for simple use-cases where we just need to add aliases to a loopback interface and allow the jail to use them. This is a very common pattern for jail users and has been requested by others. I will add support for specifying addresses and/or interfaces in this pull request. |
Sam, that is a very good point. I do have a branch in ocijail which tracks the progress of this PR but its currently private. I'll bring that up to date with the latest iteration of the PR and make it public on github. |
We're happy to test and provide feedback on both ocijail and runj when it gets support |
I pushed my OCI development branch to github here. |
This uses FreeBSD jails to implement container isolation.