Use env var IPA_CONFDIR to get confdir for 'cli' context

[tiran]

For 'cli' contexts, the environment variable IPA_CONFDIR overrides the
default confdir path. The value of the environment variable must be an
absolute path to an existing directory. The new variable simplifies
local configuration. Server and installer contexts do not use it.

Signed-off-by: Christian Heimes cheimes@redhat.com

[HonzaCholasta]

This seems rather unnecessary to me, as you can do:

$ ipa -e confdir=/path/to/confdir command

In case there is some shortcoming in the above, I would expect to see it described in the commit message.

[tiran]

It's explained in the title. The -e confdir=/path/to/confdir only works for ipa command. The env var affects all cli contexts including custom Python script that use ipalib and ipaclient libs directly.

Also it is much easier to set an env var in a shell session than to add a lengthy argument to every call of the ipa command.

[HonzaCholasta]

In custom Python scripts, you have to call api.bootstrap(), so you may as well call api.bootstrap(confdir='/path/to/confdir'). You may even call api.bootstrap(confdir=os.environ['IPA_CONFDIR']), so this PR is not necessary for custom scripts at all.

Instead of setting an env var, you can create an alias for ipa -e confdir=/path/to/confdir, which is equally easy.

[tiran]

Your proposals are hacks / workarounds, not a proper API to point ipa/ipalib to a custom configuration location. I'm proposing a proper API for integrators that works similar to KRB5_CONFIG (https://web.mit.edu/kerberos/krb5-1.14/doc/admin/env_variables.html). The PR is part of a larger effort to simplify integration of ipalib and ipa CLI into Ansible and other systems. Such integration needs to set KRB5_CONFIG anyway. It makes perfectly sense to have IPA_CONFDIR, too. It's not IPA_CONFIG because a local enrolment needs ca.crt and nssdb.

[tkrizek]

I also think IPA_CONFDIR environment variable is the proper way to configure the config directory with use cases such as Ansible.

However, with the current solution, if the AttributeError is raised, the command will fail and show a traceback. I'd really prefer to only see the error message itself. Perhaps this could be solved by using ScriptError?

# IPA_CONFDIR=/root/ipa ipa ping
[2016-11-07T14:38:11Z ipa] <ERROR>: AttributeError: IPA_CONFDIR must be an absolute path to an existing directory.
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/ipalib/cli.py", line 1345, in run
    (_options, argv) = api.bootstrap_with_global_options(context='cli')
  File "/usr/lib/python2.7/site-packages/ipalib/plugable.py", line 580, in bootstrap_with_global_options
    self.bootstrap(parser, **overrides)
  File "/usr/lib/python2.7/site-packages/ipalib/plugable.py", line 436, in bootstrap
    self.env._bootstrap(**overrides)
  File "/usr/lib/python2.7/site-packages/ipalib/config.py", line 469, in _bootstrap
    'IPA_CONFDIR must be an absolute path to an '
AttributeError: IPA_CONFDIR must be an absolute path to an existing directory.
[2016-11-07T14:38:11Z ipa] <ERROR>: an internal error has occurred

[tiran]

ipapython.admintool.ScriptError still prints the full traceback:

$ IPA_CONFDIR=/tmp/ipa ./ipa
[2016-11-09T16:35:38Z ipa] <ERROR>: ScriptError: IPA_CONFDIR must be an absolute path to an existing directory.
Traceback (most recent call last):
  File "/home/heimes/redhat/freeipa/ipalib/cli.py", line 1345, in run
    (_options, argv) = api.bootstrap_with_global_options(context='cli')
  File "/home/heimes/redhat/freeipa/ipalib/plugable.py", line 580, in bootstrap_with_global_options
    self.bootstrap(parser, **overrides)
  File "/home/heimes/redhat/freeipa/ipalib/plugable.py", line 436, in bootstrap
    self.env._bootstrap(**overrides)
  File "/home/heimes/redhat/freeipa/ipalib/config.py", line 470, in _bootstrap
    'IPA_CONFDIR must be an absolute path to an '
ScriptError: IPA_CONFDIR must be an absolute path to an existing directory.
[2016-11-09T16:35:38Z ipa] <ERROR>: an internal error has occurred

[tkrizek]

In that case, we probably need to properly handle the exception somewhere.

Since that's out of the scope of this PR, I'm going to ACK this. We can either open a ticket for this or wait until someone encounters the issue. I think it's a rather rare use case with low priority and low impact.

[HonzaCholasta]

@tiran, setting confdir explicitly is not a hack, but the proper way to set the config directory path and there is nothing that makes the environment variable better as an API for integrators. I would argue that it's actually worse, because it is implicit and optimized towards the less common usage (everyone who wants to use the default path has to unset the variable now to make sure that's what they actually get), and while some software does indeed allow changing configuration using environment variables, there is other software (such as GNU grep) which is actually deprecating this way of changing configuration.

If majority of people think it is a good idea, I won't push back, but NACK on respecting the variable only in certain contexts.

[tiran]

No, env vars are the standard way to change the behavior of a program for a local session. They are used all over the place: MIT KRB5 as KRB5_CONFIG, Python has PYTHONHOME and more, OpenSSL has SSL_CERT_FILE/DIR, Freedesktop has XDG_DATA_HOME, XDG_CONFIG_HOME...

I could bring up the same argument against your proposal to use a shell alias. Shell aliases are even worse because they work only in shells and not for execve() calls. Env vars are common to change the environment of a program (hence the name) while shell aliases are a hack.

It is not only a good idea, it's required to make integration of FreeIPA's client libraries in 3rd party applications feasible.

[HonzaCholasta]

"Everyone else does it" is not really a good argument to anything. Just saying.

Also you still haven't provided a single example of where explicitly setting confdir can't be used and thus the environment variable must be used, and just keep repeating how required it is, so sorry I'm a little bit sceptical.

[tiran]

Everyone else does it is a very good argument. Standards and common practices provide a good user and developer experience. I detest Not Invented Here solutions.

By the way did you read my integration improvement proposal? I haven't released it yet because it's not finished.

[HonzaCholasta]

Care to point me to some actual standard which recommends this? Using explicit configuration via library initialization arguments is no NIH, everyone else does it as well and it is a solution we already have in place.

Still zero examples to support you claim that environment variable is a must.

EDIT: There is no link to your proposal here nor is there a thread on freeipa-devel. I would be glad to read it but please follow our process for new feature designs.

[tiran]

The env var is an initialization argument. In comment #182 (comment) I already explained why library initialization on Python level is not a viable solution.

Let me google that for you

• https://specifications.freedesktop.org/basedir-spec/latest/ar01s03.html
• https://web.mit.edu/kerberos/krb5-1.14/doc/admin/env_variables.html
• https://docs.python.org/2/using/cmdline.html#environment-variables
• https://pip.pypa.io/en/stable/user_guide/#environment-variables

https://en.wikipedia.org/wiki/Environment_variable defines env vars as

Environment variables are a set of dynamic named values that can affect the way running processes will behave on a computer.

Examples

• local installation in a virtual environment
• unified experience for non-root configuration
• user shell session with custom KRB5 and IPA settings
• Ansible playbook modules
• application in a root-less container that cannot write to /etc (OpenShift)
• unit and integration tests with custom config file location

You can find more detailed examples in my integration document.

[HonzaCholasta]

Sorry, but I just don't see an explanation in the comment you linked, just that you think it's easier to set an environment variable rather than an argument. Yes, it is easier, but it also make the configuration implicit - say this PR was merged, now look at this:

$ ipa ping

Can you tell me which configuration directory will this command use? The fact is you can't, as opposed to:

$ ipa -e confdir=/path/to/confdir

where it is clear just by looking at the command. This is the part I have a problem with.

The links you posted only show that environment variables are used to override configuration in a few pieces of software, not that it is a standard like you say. I could as easily compile a list of software which doesn't do it.

All of the examples are doable by setting confdir explicitly in ipa -e or api.bootstrap() as well. I would like to see something more concrete.

I will read your proposal once you send it to freeipa-devel for review.

[MartinBasti]

For the long history of IPA we haven't had need for our own environment variables. I agree with Honza, why we should have the another way how to pass config dir to IPA commands.

Also handling env variables in IPA is inconsistent, so this should be fixed as well, see #204 somewhere environ variables are not passed to subprocesses at all.

[tiran]

For a long time FreeIPA ignored Python packaging guidelines. It did neither support pip and wheels nor virtual envs or local configuration. There is pressing demand from multiple projects like OpenStack and Ansible to support proper Python packages. Ask @rcritten, @admiyo

@mbasti-rh the proposal is not just about command line scripts. It's for Python applications that use ipalib, too. You are free to come up with another solution that works for all use cases.

[tiran]

The argument is mood. Even now you can't tell which config file ipa ping is going to load. There are tons of ways to modify behavior, e.g. mount binds, LD_PRELOAD, a sitecustomize.py or a .pth file in the users site-packages which mokey-patches ipaplatform.paths.paths...

[tiran]

http://www.freeipa.org/page/V4/Integration_Improvements#API_for_local_configuration_directory

[MartinBasti]

This should have test because it is completely new so it is not part of any current test suites.

However final review will wait for @jcholast

[tiran]

Integration test will be fairly easy after #247 has landed.

[pvoborni]

If I understand it correctly, the review is stalled for some time given that there is misalignment if this pull request is needed. As described in Christian's design page: http://www.freeipa.org/page/V4/Integration_Improvements#API_for_local_configuration_directory there is clear method how to do it with current code.

So this cannot be regarded as a blocker for the whole effort.

It is only a convenience method for people who rather uses env variable instead of conf dir option.

From maintenance perspective it is just another use case to support.

[tiran]

Latest PR depends on PR #280 .

[rcritten]

I don't see this as a convenience method. I'd find it less likely to use directly with the ipa tool (though having to specify -e <some_long_thing> every time I used a command get old pretty quickly.

From a library perspective it is going to call api.bootstrap(options). Sure one can pass the config file location through that but then EVERY SINGLE app using IPA is going to have to create an option to allow that, creating disparate means of doing so, when IPA can more simply accept an environment variable, like many other libraries do.

If you want to change the location of krb5.conf what do you do? Right, set an environment variable. In fact, IPA leverages this. Treat ipalib the same way, giving lots of rope, and let people utilize that power (or hang themselves).

[HonzaCholasta]

Please raise a PublicError subclass here (add one to ipalib.errors if necessary), this is a generic library code not tied to scripts.

[HonzaCholasta]

This isn't what we agreed on on Monday ("raise runtime error" - although a PublicError subclass would be nicer). Please fix.

[pvoborni]

Honza, would this work?

diff --git a/ipalib/config.py b/ipalib/config.py
index 9d87782..6c3a0b1 100644
--- a/ipalib/config.py
+++ b/ipalib/config.py
@@ -43,7 +43,7 @@ from ipapython.dn import DN
 from ipalib.base import check_name
 from ipalib.constants import CONFIG_SECTION
 from ipalib.constants import OVERRIDE_ERROR, SET_ERROR, DEL_ERROR
-from ipapython.admintool import ScriptError
+from ipalib.errors import EnvirontmentError
 
 if six.PY3:
     unicode = str
@@ -466,7 +466,7 @@ class Env(object):
             if self.env_confdir is not None:
                 if (not path.isabs(self.env_confdir)
                         or not path.isdir(self.env_confdir)):
-                    raise ScriptError(
+                    raise EnvirontmentError(
                         "IPA_CONFDIR env var must be an absolute path to an "
                         "existing directory, got '{}'.".format(
                             self.env_confdir))
diff --git a/ipalib/errors.py b/ipalib/errors.py
index d1fe5f0..dfd74d7 100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -865,6 +865,13 @@ class NotAForestRootError(InvocationError):
     errno = 3016
     format = _("Domain '%(domain)s' is not a root domain for forest '%(forest)s'")
 
+class EnvirontmentError(InvocationError):
+    """
+    **3017** Raised when a command is called with invalid environment settings
+    """
+
+    errno = 3017
+
 ##############################################################################
 # 4000 - 4999: Execution errors
 
diff --git a/ipalib/plugable.py b/ipalib/plugable.py
index 142b3e6..95e0a8a 100644
--- a/ipalib/plugable.py
+++ b/ipalib/plugable.py
@@ -718,9 +718,9 @@ class API(ReadOnly):
                 self.log.info(
                     "IPA_CONFDIR env sets confdir to '%s'.", self.env.confdir)
             else:
-                self.log.warn(
-                    "IPA_CONFDIR env is overridden by an explicit confdir "
-                    "argument.")
+                raise errors.EnvirontmentError(
+                    "IPA_CONFDIR env cannot be set because explicit confdir is"
+                    "used")
 
         for plugin in self.__plugins:
             if not self.env.validate_api:

[tiran]

You have a copy and paste typo: EnvironmentError, not EnvirontmentError

[pvoborni]

True, also thinking that it might not be the best name because it can be confused with buildin EnvironmentError so the usages should be either errors.EnvironmentError to distinguish it or different name should be used.

[HonzaCholasta]

@pvoborni, yes, although it should not inherit from InvocationError, since it does not happen during command invocation, but way before, during API initialization. I would inherit it directly from PublicError and put it into the 900-1000 errno range.

[HonzaCholasta]

Pushed with #302.