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

add config with no exists config #630

Closed
wants to merge 0 commits into from

Conversation

himrock922
Copy link

@himrock922 himrock922 commented Feb 3, 2019

I commit of parameter that was said "unknown" with config.
For example, The config property 'vnet_default_interface' is unknown
Because I don't know very well about config detail, I need refactoring it.

This PR addresses #626

@igalic
Copy link
Collaborator

igalic commented Feb 3, 2019

hrm… with the amount of config options we have to add, i think we should hurry up and make the config better structured.

as far as i can see it, there are jail(8) options, libioc (config) options, iocell and iocage compat options.
We have a json like structure but, but currently, except for provisioning it's entirely flat.

@igalic
Copy link
Collaborator

igalic commented Feb 3, 2019

@himrock922 may i recommend either adding the email address from this commit to your github email addresses — or else change it, so you're correctly credited!

@himrock922
Copy link
Author

Hi @igalic.

may i recommend either adding the email address from this commit to your github email addresses — or else change it, so you're correctly credited!

I'm sorry, after reset HEAD~, I did forcely commit that changes of email address.

Copy link
Member

@gronke gronke left a comment

Choose a reason for hiding this comment

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

@himrock922 thanks for your contribution. You have caused a whole bunch of new Issues that reflect new and missing features 😍Do you agree to deal with all the enhancements in separate PRs?

"devfs_ruleset": 4,
"enforce_statfs": 2,
"children_max": 0,
"allow_set_hostname": 1,
"allow_sysvipc": 0,
"allow_raw_sockets": 0,
"allow_chflags": 0,
"allow_mlock": 0,
Copy link
Member

Choose a reason for hiding this comment

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

👍 this param was introduced in FreeBSD 12. Therefore a check for the host version should apply before starting a jail with this option set. The default should be None, so that any value set by a user results in the parameter being set on jail creation.

Copy link
Author

Choose a reason for hiding this comment

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

OK! this parameter changes "0" from to "None".

"allow_mount": 0,
"allow_mount_devfs": 0,
"allow_mount_fusefs": 0,
Copy link
Member

Choose a reason for hiding this comment

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

👍

Privileged users inside the jail will be able to mount and unmount fuse-based file systems. This permission is effective only together with allow.mount and only when enforce_statfs is set to a value lower than 2.

The checks mentioned in the man page should be implemented as pre-checks before starting a Jail, meaning that allow_mount_fusefs raises an error before starting the Jail when enforce_statfs is lower than 2 or or allow_mount is disabled.

"allow_mount_nullfs": 0,
"allow_mount_procfs": 0,
"allow_mount_fdescfs": 0,
"allow_mount_zfs": 0,
"allow_mount_tmpfs": 0,
"allow_quotas": 0,
"allow_socket_af": 0,
"allow_tun": 0,
Copy link
Member

Choose a reason for hiding this comment

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

Tun devices are passed to a jail using devfs rules. Although iocage has introduced this config parameter, it will not be supported by ioc. Internally iocage modifies devfs rules when this parameter is enabled, but this abstraction introduces unnecessary complexity. The same goal can be achieved in a similar way to USB support #140.

"allow_vmm": 0,
"available": 0,
"bpf": None,
"comment": None,
Copy link
Member

Choose a reason for hiding this comment

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

Comments can be added in the user.* namespace. For instance jail.config["user.comment"].

"allow_tun": 0,
"allow_vmm": 0,
"available": 0,
"bpf": None,
Copy link
Member

Choose a reason for hiding this comment

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

This is another config parameter that is already handled in devfs_rules. It is set automatically when using DHCP in a jail, but can also be enabled individually. The USB Feature PR #140 shows a similar implementation of the required steps.

"exec_timeout": "600",
"stop_timeout": "30",
"mount_procfs": "0",
"mount_devfs": "1",
"mount_fdescfs": "0",
"mount_linprocfs": "0",
"mountpoint": "0",
"notes": None,
Copy link
Member

Choose a reason for hiding this comment

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

This appears to be yet another way to leave a comment. Both should go into the user.* namespace.

"mount_linprocfs": "0",
"mountpoint": "0",
"notes": None,
"origin": None,
Copy link
Member

Choose a reason for hiding this comment

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

Does this refer to the ZFS origin property? Not sure what it is used for.

@himrock922
Copy link
Author

himrock922 commented Feb 4, 2019

Hi @gronke
Thanks for reply.

thanks for your contribution. You have caused a whole bunch of new Issues that reflect new and missing features 😍Do you agree to deal with all the enhancements in separate PRs?

All right!, I don't know how much of can enhancements.
But, I want cooperate of that enhancements.

Regards.

@himrock922
Copy link
Author

himrock922 commented Feb 4, 2019

I beginning refactoring it.
Question, Should I use not 0, but False?
Also, Should I use not 1, but True?

@gronke
Copy link
Member

gronke commented Feb 4, 2019

@himrock922 you should use boolean types. The jail parameters are translated for the jail command in the JailGenerator._get_value method

libioc/libioc/Jail.py

Lines 1804 to 1811 in 72cbed7

def _get_value(self, key: str) -> str:
"""Return jail command consumable config value string."""
return str(libioc.helpers.to_string(
self.config[key],
true="1",
false="0",
none=""
))

@fabianfreyer brought up the idea to read valid jail parameters from syctl, as seen here on a FreeBSD 12.0-RELEASE:

$ sysctl -t security.jail
security.jail.param.sysvshm.: integer
security.jail.param.sysvsem.: integer
security.jail.param.sysvmsg.: integer
security.jail.param.allow.mount.zfs: integer
security.jail.param.allow.mount.tmpfs: integer
security.jail.param.allow.mount.linsysfs: integer
security.jail.param.allow.mount.linprocfs: integer
security.jail.param.allow.mount.procfs: integer
security.jail.param.allow.mount.nullfs: integer
security.jail.param.allow.mount.fdescfs: integer
security.jail.param.allow.mount.devfs: integer
security.jail.param.allow.mount.: integer
security.jail.param.allow.socket_af: integer
security.jail.param.allow.quotas: integer
security.jail.param.allow.chflags: integer
security.jail.param.allow.raw_sockets: integer
security.jail.param.allow.sysvipc: integer
security.jail.param.allow.set_hostname: integer
security.jail.param.ip6.saddrsel: integer
security.jail.param.ip6.addr: opaque
security.jail.param.ip6.: integer
security.jail.param.ip4.saddrsel: integer
security.jail.param.ip4.addr: opaque
security.jail.param.ip4.: integer
security.jail.param.cpuset.id: integer
security.jail.param.host.hostid: unsigned long
security.jail.param.host.hostuuid: string
security.jail.param.host.domainname: string
security.jail.param.host.hostname: string
security.jail.param.host.: integer
security.jail.param.children.max: integer
security.jail.param.children.cur: integer
security.jail.param.dying: integer
security.jail.param.vnet: integer
security.jail.param.persist: integer
security.jail.param.devfs_ruleset: integer
security.jail.param.enforce_statfs: integer
security.jail.param.osrelease: string
security.jail.param.osreldate: integer
security.jail.param.securelevel: integer
security.jail.param.path: string
security.jail.param.name: string
security.jail.param.parent: integer
security.jail.param.jid: integer
security.jail.devfs_ruleset: integer
security.jail.enforce_statfs: integer
security.jail.mount_zfs_allowed: integer
security.jail.mount_tmpfs_allowed: integer
security.jail.mount_linsysfs_allowed: integer
security.jail.mount_linprocfs_allowed: integer
security.jail.mount_procfs_allowed: integer
security.jail.mount_nullfs_allowed: integer
security.jail.mount_fdescfs_allowed: integer
security.jail.mount_devfs_allowed: integer
security.jail.mount_allowed: integer
security.jail.chflags_allowed: integer
security.jail.allow_raw_sockets: integer
security.jail.sysvipc_allowed: integer
security.jail.socket_unixiproute_only: integer
security.jail.set_hostname_allowed: integer
security.jail.jail_max_af_ips: unsigned integer
security.jail.vnet: integer
security.jail.jailed: integer
security.jail.list: opaque
$ sysctl security.jail 
security.jail.param.sysvshm.: 0
security.jail.param.sysvsem.: 0
security.jail.param.sysvmsg.: 0
security.jail.param.allow.mount.zfs: 0
security.jail.param.allow.mount.tmpfs: 0
security.jail.param.allow.mount.linsysfs: 0
security.jail.param.allow.mount.linprocfs: 0
security.jail.param.allow.mount.procfs: 0
security.jail.param.allow.mount.nullfs: 0
security.jail.param.allow.mount.fdescfs: 0
security.jail.param.allow.mount.devfs: 0
security.jail.param.allow.mount.: 0
security.jail.param.allow.socket_af: 0
security.jail.param.allow.quotas: 0
security.jail.param.allow.chflags: 0
security.jail.param.allow.raw_sockets: 0
security.jail.param.allow.sysvipc: 0
security.jail.param.allow.set_hostname: 0
security.jail.param.ip6.saddrsel: 0
security.jail.param.ip6.: 0
security.jail.param.ip4.saddrsel: 0
security.jail.param.ip4.: 0
security.jail.param.cpuset.id: 0
security.jail.param.host.hostid: 0
security.jail.param.host.hostuuid: 64
security.jail.param.host.domainname: 256
security.jail.param.host.hostname: 256
security.jail.param.host.: 0
security.jail.param.children.max: 0
security.jail.param.children.cur: 0
security.jail.param.dying: 0
security.jail.param.vnet: 0
security.jail.param.persist: 0
security.jail.param.devfs_ruleset: 0
security.jail.param.enforce_statfs: 0
security.jail.param.osrelease: 32
security.jail.param.osreldate: 0
security.jail.param.securelevel: 0
security.jail.param.path: 1024
security.jail.param.name: 256
security.jail.param.parent: 0
security.jail.param.jid: 0
security.jail.devfs_ruleset: 0
security.jail.enforce_statfs: 2
security.jail.mount_zfs_allowed: 0
security.jail.mount_tmpfs_allowed: 0
security.jail.mount_linsysfs_allowed: 0
security.jail.mount_linprocfs_allowed: 0
security.jail.mount_procfs_allowed: 0
security.jail.mount_nullfs_allowed: 0
security.jail.mount_fdescfs_allowed: 0
security.jail.mount_devfs_allowed: 0
security.jail.mount_allowed: 0
security.jail.chflags_allowed: 0
security.jail.allow_raw_sockets: 0
security.jail.sysvipc_allowed: 0
security.jail.socket_unixiproute_only: 1
security.jail.set_hostname_allowed: 1
security.jail.jail_max_af_ips: 255
security.jail.vnet: 0
security.jail.jailed: 0

The first command prints each supported jail parameter type, the second one their length. To efficiently read those values, we'd need a native Python sysctl module - preferably without using Cython.

After reading those values and structuring the jail params in the config files properly (nesting with . notation), we can transparently pass through all valid jail parameters. When a config file defines one that is unknown, an according error can be triggered. That way we do not need to "support" new jail parameters, but always match with the host OS capabilities.

I left a lot comments on the added lines that, whenever necessary, resulted in a new issue. We should target each new issue in a separate PRs. @himrock922 please file a new Issue in case I forgot anything. In summary that is:

@himrock922
Copy link
Author

Hi @gronke.

I left a lot comments on the added lines that, whenever necessary, resulted in a new issue. We should target each new issue in a separate PRs. @himrock922 please file a new Issue in case I forgot anything. In summary that is:
Some config parameters are state, that should not be stored in a config, but read from the jail itself including ZFS properties (compressratio, ...)
Read (and write) support for ZFS properties on creation #633
Mount linprocfs #637
add cpuset to resource limits #636
Inheritance of host configuration (locale, timezone) #635
Host ID information stored in jails #634
last_started date of Jails #632
opt out from rtsold when accept_rtadv in an ip6_addr #631

All right! First, I do commit PR related to above issue.
By the way, May I create PR not master branch but this PR as a target?

If I don't do that, I think hard to understand what I do fix.

Regards.

@gronke
Copy link
Member

gronke commented Feb 4, 2019

All right! First, I do commit PR related to above issue.
By the way, May I create PR not master branch but this PR as a target?

If I don't do that, I think hard to understand what I do fix.

Of course you can do that. I'm unsure how GitHub handles it when we later merge to master instead of this branch, but let's find out!

Still I believe what you want to do is to cherry-pick your commit in your other PR branches and remove it before pushing your changes. That way you can point to master, but still have your ideas from this PR in place. Another supportive idea is that we can tick off lines in this PR as soon as we merge related changes elsewhere. However you wish :)

@igalic
Copy link
Collaborator

igalic commented Feb 8, 2019

@himrock922 may i suggest you rebase instead of merging master into your branch?

@himrock922
Copy link
Author

himrock922 commented Feb 8, 2019

Hi @igalic.

may i suggest you rebase instead of merging master into your branch?

right. Is my understanding of correct? After from my branch reverting merge master branch, that does rebase.

@igalic
Copy link
Collaborator

igalic commented Feb 8, 2019

a rebase "simply" puts your changes on top of the latest master

@himrock922
Copy link
Author

Hi @igalic.
Now I've rebased this branch on top latest branch.
I'm sorry for the mistake.

@igalic
Copy link
Collaborator

igalic commented Feb 8, 2019

no need to apologise

i am always happy to help people — with git or anything else!

@himrock922 himrock922 force-pushed the feature/add_config branch 2 times, most recently from c099944 to c283c63 Compare February 19, 2019 00:53
@@ -88,22 +103,38 @@
"exec_prestop": None,
"exec_stop": "/bin/sh /etc/rc.shutdown",
"exec_poststop": None,
"exec_system_user": "root",
"exec_system_jail_user": "root",
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference to the exec_jail_user config property, that already exists?

Copy link
Author

@himrock922 himrock922 Feb 19, 2019

Choose a reason for hiding this comment

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

Hi @gronke.
I'm sorry. This parameter is property via iocage config(First, After My case did FreeBSD host machine installed iocage, jail did created or deleted via libioc).Therefore, FreeBSD host machine had return not libioc response, but iocage response(response is example like as The config property 'exec_system_user' is unknown...)
Therefore, this parameter doesn't need config it(Maybe Other too).
I remove config it.

Copy link
Author

Choose a reason for hiding this comment

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

Now, My ZFS filesystem had recreate iocage directory from iocage tool to libioc tool.

Therefore, it isn't said unknown config propetery.

Copy link
Member

Choose a reason for hiding this comment

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

The config property indeed exists in python-iocage, see:

https://github.com/iocage/iocage/blob/528f007e92f6e081614d8a09186c4831fc9f3e32/iocage.8#L1299-L1314

Name Default Description
exec_system_jail_user 0 This boolean option looks for the exec_jail_user file in the system rather than the jail's file.
exec_system_user root Run commands as this user in the system environment. The default is to run commands as the current user.

We can track the missing feature in #662.

@himrock922
Copy link
Author

ref #668
This PR related each parameters does migrate that it's from Defaults.py to Globals.py.

@gronke
Copy link
Member

gronke commented Feb 22, 2019

This PR related each parameters does migrate that it's from Defaults.py to Globals.py.

Yes, indeed the hardcoded defaults were moved to another file that stores the hardcoded defaults. To keep in mind we have three different places where a configuration is derived from:

  1. Global Hard-Coded defaults
  2. A defaults.json file in the top-level of a ioc_dataset (or activated pool)
  3. The jail dataset itself

@himrock922 himrock922 closed this Feb 23, 2019
himrock922 added a commit to himrock922/libioc that referenced this pull request Feb 24, 2019
allow_mlock propetry changes from 0 to None.
himrock922 added a commit to himrock922/libioc that referenced this pull request Feb 24, 2019
ref2 https://www.freebsd.org/cgi/man.cgi?jail(8)
enforce_staffs property equal 2 So, set allow_mount_fusefs values False(0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants