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

Jump to bottom

Set explicit confdir option for global contexts #280

Closed
wants to merge 2 commits into from
Closed

Set explicit confdir option for global contexts #280

wants to merge 2 commits into from

Conversation

tiran
Copy link
Member

@tiran tiran commented Nov 28, 2016

Some API contexts are used to modify global state (e.g. files in /etc
and /var). These contexts do not support confdir overrides. Initialize
the API with an explicit confdir argument to paths.ETC_IPA.

The special contexts are:

  • backup
  • cli_installer
  • installer
  • ipctl
  • renew
  • restore
  • server
  • updates

The patch also corrects the context of the ipa-httpd-kdcproxy script to
'server'.

https://fedorahosted.org/freeipa/ticket/6389

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

@tiran
Copy link
Member Author

tiran commented Nov 28, 2016

For #182

@HonzaCholasta HonzaCholasta self-assigned this Nov 28, 2016
@tiran tiran mentioned this pull request Nov 28, 2016
Closed
@HonzaCholasta
Copy link
Contributor

You missed a few:

daemons/dnssec/ipa-dnskeysync-replica:124:ipalib.api.bootstrap(in_server=True, log=None)  # no logging to file
daemons/dnssec/ipa-dnskeysyncd:23:api.bootstrap(in_server=True, log=None)  # no logging to file
daemons/dnssec/ipa-ods-exporter:618:ipalib.api.bootstrap(in_server=True, log=None)  # no logging to file
doc/guide/wsgi.py.txt:9:env._bootstrap(context='server', log=None)
doc/guide/wsgi.py.txt:13:api.bootstrap(context='server', debug=env.debug, log=None) (ref:wsgi-app-bootstrap)
install/restart_scripts/renew_ra_cert:39:    api.bootstrap(in_server=True, context='restart')
install/tools/ipa-adtrust-install:269:    api.bootstrap(**cfg)
install/tools/ipa-ca-install:262:    api.bootstrap(in_server=True, ra_plugin='dogtag')
install/tools/ipa-compat-manage:105:    api.bootstrap(context='cli', in_server=True, debug=options.debug)
install/tools/ipa-csreplica-manage:418:    api.bootstrap(**api_env)
install/tools/ipa-dns-install:139:    api.bootstrap(**cfg)
install/tools/ipa-managed-entries:75:    api.bootstrap(context='cli', debug=options.debug)
install/tools/ipa-nis-manage:118:    api.bootstrap(context='cli', debug=options.debug, in_server=True)
install/tools/ipa-replica-manage:1512:    api.bootstrap(**api_env)
ipapython/dnssec/ldapkeydb.py:417:    ipalib.api.bootstrap(in_server=True, log=None)  # no logging to file
ipaserver/advise/base.py:238:        api.bootstrap(in_server=False, context='cli')
ipaserver/advise/base.py:240:        advise_api.bootstrap(in_server=False, context='cli')
ipaserver/install/ipa_cacert_manage.py:99:        api.bootstrap(in_server=True)
ipaserver/install/ipa_kra_install.py:80:        api.bootstrap(in_server=True)
ipaserver/install/ipa_otptoken_import.py:512:        api.bootstrap(in_server=True)
ipaserver/install/ipa_replica_prepare.py:183:        api.bootstrap(in_server=True)
ipaserver/install/ipa_server_certinstall.py:102:        api.bootstrap(in_server=True)
ipatests/test_ipaserver/test_ldap.py:114:        myapi.bootstrap(context='cli', in_server=True)
ipatests/test_ipaserver/test_serverroles.py:472:    test_api.bootstrap(in_server=True, ldap_uri=api.env.ldap_uri)
lite-server.py:130:    (options, args) = api.bootstrap_with_global_options(parser, context='lite')

@tiran
Copy link
Member Author

tiran commented Nov 29, 2016

I fixed a few. Some scripts deliberately do not have the confdir flag in bootstrap.

@HonzaCholasta
Copy link
Contributor

Please explain, all of the affected scripts are server-only and thus not related to the integration effort and most probably won't work correctly with non-server configuration anyway.

@tiran
Copy link
Member Author

tiran commented Nov 29, 2016

All bootstrap() calls without an explicit confdir argument are fine. If you think otherwise, please list all calls and give me a compelling reason to have them ignore IPA_CONFDIR.

@tiran
Set explicit confdir option for global contexts
86ddbbe 
Some API contexts are used to modify global state (e.g. files in /etc
and /var). These contexts do not support confdir overrides. Initialize
the API with an explicit confdir argument to paths.ETC_IPA.

The special contexts are:

* backup
* cli_installer
* installer
* ipctl
* renew
* restore
* server
* updates

The patch also corrects the context of the ipa-httpd-kdcproxy script to
'server'.

https://fedorahosted.org/freeipa/ticket/6389

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran
Set explicit confdir option for global contexts (2)
e0d1c7f 
Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran
Copy link
Member Author

tiran commented Nov 30, 2016

  • daemons/dnssec/ipa-dnskeysync-replica:124:ipalib.api.bootstrap(in_server=True, log=None) # no logging to file
  • daemons/dnssec/ipa-dnskeysyncd:23:api.bootstrap(in_server=True, log=None) # no logging to file
  • daemons/dnssec/ipa-ods-exporter:618:ipalib.api.bootstrap(in_server=True, log=None) # no logging to file
  • doc/guide/wsgi.py.txt:9:env._bootstrap(context='server', log=None)
  • doc/guide/wsgi.py.txt:13:api.bootstrap(context='server', debug=env.debug, log=None) (ref:wsgi-app-bootstrap)
  • install/restart_scripts/renew_ra_cert:39: api.bootstrap(in_server=True, context='restart')
  • install/tools/ipa-adtrust-install:269: api.bootstrap(**cfg)
  • install/tools/ipa-ca-install:262: api.bootstrap(in_server=True, ra_plugin='dogtag')
  • install/tools/ipa-compat-manage:105: api.bootstrap(context='cli', in_server=True, debug=options.debug)
  • install/tools/ipa-csreplica-manage:418: api.bootstrap(**api_env)
  • install/tools/ipa-dns-install:139: api.bootstrap(**cfg)
  • install/tools/ipa-managed-entries:75: api.bootstrap(context='cli', debug=options.debug)
  • install/tools/ipa-nis-manage:118: api.bootstrap(context='cli', debug=options.debug, in_server=True)
  • install/tools/ipa-replica-manage:1512: api.bootstrap(**api_env)
  • ipaserver/dnssec/ldapkeydb.py:417: ipalib.api.bootstrap(in_server=True, log=None) # no logging to file
  • ipaserver/advise/base.py:238: api.bootstrap(in_server=False, context='cli')
  • ipaserver/advise/base.py:240: advise_api.bootstrap(in_server=False, context='cli')
  • ipaserver/install/ipa_cacert_manage.py:99: api.bootstrap(in_server=True)
  • ipaserver/install/ipa_kra_install.py:80: api.bootstrap(in_server=True)
  • ipaserver/install/ipa_otptoken_import.py:512: api.bootstrap(in_server=True)
  • ipaserver/install/ipa_replica_prepare.py:183: api.bootstrap(in_server=True)
  • ipaserver/install/ipa_server_certinstall.py:102: api.bootstrap(in_server=True)
  • ipatests/test_ipaserver/test_ldap.py:114: myapi.bootstrap(context='cli', in_server=True)
  • ipatests/test_ipaserver/test_serverroles.py:472: test_api.bootstrap(in_server=True, ldap_uri=api.env.ldap_uri)
  • lite-server.py:130: (options, args) = api.bootstrap_with_global_options(parser, context='lite')

@pvoborni
Copy link
Member

If I understand Christian right, it is not disagreement about something which needs to be done. But rather a proposal to address rest of the scripts later in other pull request. So that we can push this PR to unblock subsequent reviews.

Is it correct? If so can be proceed with checking if current code is OK and finished rest in other PR?

@HonzaCholasta
Copy link
Contributor

That may be so, but why do it later when it can be done now in 10 minutes?

More importantly, we try hard not to introduce new inconsistencies to our code and I don't see why this PR should be an exception (the incosistency is differing behavior of server-side scripts when IPA_CONFDIR is set, if it's not obvious).

@HonzaCholasta
Copy link
Contributor

BTW, @tiran, if you don't have 10 minutes of your time to make the changes, I would be happy to spend 10 minutes of my time to amend the patch before pusing it. If that's OK with you, ACK.

@tiran
Copy link
Member Author

tiran commented Dec 1, 2016

It's not a 10 minute change because I don't want to have a purely mechanical transition. It's going to take several hours of bike shedding and head banging.

Some calls to bootstrap are fine without confdir. Other places like test suite need another approach. I already have some local patches to replace some test hacks (e.g. ipalib/__init__.py) with proper pytest code.

@pvoborni
Copy link
Member

pvoborni commented Dec 1, 2016 •
edited

Christian, was your answer an agreement to Honza's proposal? I.e. push this PR? Do rest later by Honza?

@tiran
Copy link
Member Author

tiran commented Dec 1, 2016

@pvoborni No, my answer is an disagreement. Honza does not want the approve the PR as it stands now.

My proposal is

  • Review this PR under the premise that it does not break current code when IPA_CONFDIR is absent.
  • Review the disputable code lines after my deadline and within the next two weeks. Some of them might need an explicit confdir, some might not. I need to carefully think about each call.

@pvoborni
Copy link
Member

pvoborni commented Dec 1, 2016

Lets push this code if it is correct but only misses usecases mentioned above". Honza will implement the missing usecases in separate PR.

@HonzaCholasta HonzaCholasta added the ack Pull Request approved, can be merged label Dec 2, 2016
@HonzaCholasta
Copy link
Contributor

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/1e6a204b4372bbbfb722a00370a5ce4e34406b9f

@HonzaCholasta HonzaCholasta added the pushed Pull Request has already been pushed label Dec 2, 2016
@stlaz stlaz mentioned this pull request Feb 22, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@HonzaCholasta HonzaCholasta

Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
3 participants
@tiran @HonzaCholasta @pvoborni