On 20.5.2016 15:32, Martin Basti wrote:


On 20.05.2016 12:30, Petr Spacek wrote:
On 18.5.2016 12:43, Martin Basti wrote:

On 12.05.2016 16:16, Martin Basti wrote:


On 12.05.2016 11:01, Martin Basti wrote:

On 11.05.2016 09:41, Martin Basti wrote:

On 10.05.2016 18:56, Petr Spacek wrote:
On 10.5.2016 15:38, Petr Spacek wrote:
On 10.5.2016 15:26, Martin Basti wrote:
On 10.05.2016 15:23, Petr Spacek wrote:
On 10.5.2016 14:44, Martin Basti wrote:
On 10.05.2016 14:33, Petr Spacek wrote:
On 6.5.2016 10:20, Martin Basti wrote:
https://fedorahosted.org/freeipa/ticket/2008

Patches attached.


freeipa-mbasti-0473-DNS-Locations-Always-create-DNS-related-privileges.patch




    From 9a936740da7cdacec150acc92a45041a98ce7cb3 Mon Sep 17
00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Wed, 4 May 2016 17:33:52 +0200
Subject: [PATCH 1/4] DNS Locations: Always create DNS related
privileges

DNS privileges are important for handling DNS locations
which can be
created without DNS servers in IPA topology. We will also
need this
privileges presented for future feature 'External DNS support'
Seems reasonable, ACK.


freeipa-mbasti-0474-DNS-Locations-add-new-attributes-and-objectclasses.patch




    From a7766da5fd1a72884308d4206c9cde262f5c8d35 Mon Sep 17
00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Thu, 5 May 2016 11:12:00 +0200
Subject: [PATCH 2/4] DNS Locations: add new attributes and
objectclasses

http://www.freeipa.org/page/V4/DNS_Location_Mechanism

https://fedorahosted.org/freeipa/ticket/2008
---
     install/share/60ipadns.ldif | 4 ++++
     1 file changed, 4 insertions(+)

diff --git a/install/share/60ipadns.ldif
b/install/share/60ipadns.ldif
index
e0ed0ab869cea0478d9640bb509c6267abed1a01..31c2f71f8566d04a05709f1359b20e6fa51921ce



100644
--- a/install/share/60ipadns.ldif
+++ b/install/share/60ipadns.ldif
@@ -70,9 +70,13 @@ attributeTypes: (
2.16.840.1.113730.3.8.5.25 NAME
'idnsSecKeyRevoke' DESC 'DNSKE
     attributeTypes: ( 2.16.840.1.113730.3.8.5.26 NAME
'idnsSecKeySep' DESC
'DNSKEY SEP flag (equivalent to bit 15): RFC 4035' EQUALITY
booleanMatch
SYNTAX 1.3.6.1.4.1.1466.115.121.1.7 SINGLE-VALUE X-ORIGIN
'IPA v4.1' )
     attributeTypes: ( 2.16.840.1.113730.3.8.5.27 NAME
'idnsSecAlgorithm' DESC
'DNSKEY algorithm: string used as mnemonic' EQUALITY
caseIgnoreIA5Match
SUBSTR caseIgnoreIA5SubstringsMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.26
SINGLE-VALUE X-ORIGIN 'IPA v4.1' )
     attributeTypes: ( 2.16.840.1.113730.3.8.5.28 NAME
'idnsSecKeyRef' DESC
'PKCS#11 URI of the key' EQUALITY caseExactMatch
SINGLE-VALUE SYNTAX
1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'IPA v4.1' )
+attributeTypes: ( 2.16.840.1.113730.3.8.5.32 NAME
'ipaLocation' DESC
'Reference to IPA location' EQUALITY distinguishedNameMatch
SYNTAX
1.3.6.1.4.1.1466.115.121.1.12 SINGLE-VALUE X-ORIGIN 'IPA
v4.4' )
+attributeTypes: ( 2.16.840.1.113730.3.8.5.33 NAME
'ipaLocationWeight' DESC
'Weight for the server in IPA location' EQUALITY
integerMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.27 SINGLE-VALUE X-ORIGIN 'IPA
v4.4' )
     objectClasses: ( 2.16.840.1.113730.3.8.6.0 NAME
'idnsRecord'
DESC 'dns
Record, usually a host' SUP top STRUCTURAL MUST idnsName
MAY ( cn $
idnsAllowDynUpdate $ dNSTTL $ dNSClass $ aRecord $
aAAARecord $
a6Record $
nSRecord $ cNAMERecord $ pTRRecord $ sRVRecord $ tXTRecord $
mXRecord $
mDRecord $ hInfoRecord $ mInfoRecord $ aFSDBRecord $
SigRecord $
KeyRecord
$ LocRecord $ nXTRecord $ nAPTRRecord $ kXRecord $
certRecord $
dNameRecord
$ dSRecord $ sSHFPRecord $ rRSIGRecord $ nSECRecord $
DLVRecord $
TLSARecord $ UnknownRecord $ RPRecord $ APLRecord $
IPSECKEYRecord $
DHCIDRecord $ HIPRecord $ SPFRecord ) )
     objectClasses: ( 2.16.840.1.113730.3.8.6.1 NAME
'idnsZone' DESC
'Zone
class' SUP idnsRecord STRUCTURAL MUST ( idnsZoneActive $
idnsSOAmName $
idnsSOArName $ idnsSOAserial $ idnsSOArefresh $ idnsSOAretry $
idnsSOAexpire $ idnsSOAminimum ) MAY ( idnsUpdatePolicy $
idnsAllowQuery $
idnsAllowTransfer $ idnsAllowSyncPTR $ idnsForwardPolicy $
idnsForwarders $
idnsSecInlineSigning $ nSEC3PARAMRecord ) )
     objectClasses: ( 2.16.840.1.113730.3.8.6.2 NAME
'idnsConfigObject' DESC
'DNS global config options' STRUCTURAL MAY (
idnsForwardPolicy $
idnsForwarders $ idnsAllowSyncPTR $ idnsZoneRefresh $
idnsPersistentSearch ) )
     objectClasses: ( 2.16.840.1.113730.3.8.12.18 NAME
'ipaDNSZone'
SUP top
AUXILIARY MUST idnsName MAY managedBy X-ORIGIN 'IPA v3' )
     objectClasses: ( 2.16.840.1.113730.3.8.6.3 NAME
'idnsForwardZone' DESC
'Forward Zone class' SUP top STRUCTURAL MUST ( idnsName $
idnsZoneActive )
MAY ( idnsForwarders $ idnsForwardPolicy ) )
     objectClasses: ( 2.16.840.1.113730.3.8.6.4 NAME
'idnsSecKey'
DESC 'DNSSEC
key metadata' STRUCTURAL MUST ( idnsSecKeyRef $
idnsSecKeyCreated $
idnsSecAlgorithm ) MAY ( idnsSecKeyPublish $
idnsSecKeyActivate $
idnsSecKeyInactive $ idnsSecKeyDelete $ idnsSecKeyZone $
idnsSecKeyRevoke $
idnsSecKeySep $ cn ) X-ORIGIN 'IPA v4.1' )
+objectClasses: ( 2.16.840.1.113730.3.8.6.7 NAME
'ipaLocationObject' DESC
'Object for storing IPA server location' AUXILIARY MUST (
idnsName
) MAY (
description ) X-ORIGIN 'IPA v4.4' )
Why is it AUXILIARY? AFAIK it should be STRUCTURAL because
there
will not be
any other object class on the location object (at least not
in the
beginning).

+objectClasses: ( 2.16.840.1.113730.3.8.6.8 NAME
'ipaLocationMember' DESC
'Member object of IPA location' AUXILIARY MAY ( ipaLocation $
ipaLocationWeight ) X-ORIGIN 'IPA v4.4' )
Conditional ACK if you fix ipaLocationObject.


freeipa-mbasti-0475-DNS-Locations-Add-location-commands.patch


    From 407b935ecd6df0ed98c6df6d45a575229ef3cd09 Mon Sep 17
00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Thu, 5 May 2016 11:13:07 +0200
Subject: [PATCH 3/4] DNS Locations: Add location-* commands

Added location-{add,mod,del,find,show} commands. Command
are just
prototypes and does not provide any information about
server (will be
done later)

http://www.freeipa.org/page/V4/DNS_Location_Mechanism

https://fedorahosted.org/freeipa/ticket/2008
---
     ACI.txt                               |   8 ++
     API.txt                               |  59
++++++++++++++
     VERSION                               |   4 +-
     install/share/bootstrap-template.ldif |   6 ++
     install/updates/37-locations.update   |   4 +
     install/updates/Makefile.am           |   1 +
     ipalib/constants.py                   |   1 +
     ipalib/plugins/location.py            | 142
+++++++++++++++++++++++++++++++++-
     8 files changed, 222 insertions(+), 3 deletions(-)
[...]

diff --git a/VERSION b/VERSION
index
aedebd185821d42fa48608f4c5fdf9ff510ace3f..7e3def151e9986454509a580515b9d34dc220a60



100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=20100614120000
# #
########################################################
     IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=165
-# Last change: mbasti - limit ipamaxusernamelength value
to 255
+IPA_API_VERSION_MINOR=166
+# Last change: mbasti - location-* commands
Needs rebase.


diff --git a/install/share/bootstrap-template.ldif
b/install/share/bootstrap-template.ldif
index
628a8e2e0f5483b9f6f565b0c7d11eb000a5912d..83be4399508a905f8eae7e2f59140a6b4051b661



100644
--- a/install/share/bootstrap-template.ldif
+++ b/install/share/bootstrap-template.ldif
@@ -119,6 +119,12 @@ objectClass: nsContainer
     objectClass: top
     cn: etc
     +dn: cn=locations,cn=etc,$SUFFIX
+changetype: add
+objectClass: nsContainer
+objectClass: top
+cn: locations
+
     dn: cn=sysaccounts,cn=etc,$SUFFIX
     changetype: add
     objectClass: nsContainer
diff --git a/install/updates/37-locations.update
b/install/updates/37-locations.update
index
e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..cf47e6d6296af830a76aad2c9b9f5a6ea5d9f3a1



100644
--- a/install/updates/37-locations.update
+++ b/install/updates/37-locations.update
@@ -0,0 +1,4 @@
+dn: cn=locations,cn=etc,$SUFFIX
+default: objectClass: nsContainer
+default: objectClass: top
+default: cn: locations
Ok.

[...]

diff --git a/ipalib/plugins/location.py
b/ipalib/plugins/location.py
index
8090bb1637c4d826b9a746a82b98ece903e321cc..d52d2baeb8bfb2fddeac40b281268622d47c6aeb



100644
--- a/ipalib/plugins/location.py
+++ b/ipalib/plugins/location.py
[...]
+__doc__ = _("""
+IPA locations
+""") + _("""
+Manipulate with DNS locations
IMHO "with" should be omited. [...]


+@register()
+class location(LDAPObject):
+    """
+    IPA locations
+    """
[...]

+ permission_filter_objectclasses = ['ipaLocationObject']
+    managed_permissions = {
+        'System: Read IPA Locations': {
+            'ipapermright': {'read', 'search', 'compare'},
+            'ipapermdefaultattr': {
+                'objectclass', 'idnsname', 'description',
+            },
+            'default_privileges': {'DNS Administrators'},
+        },
+        'System: Add IPA Locations': {
+            'ipapermright': {'add'},
+            'default_privileges': {'DNS Administrators'},
+        },
+        'System: Remove IPA Locations': {
+            'ipapermright': {'delete'},
+            'default_privileges': {'DNS Administrators'},
+        },
+        'System: Modify IPA Locations': {
+            'ipapermright': {'write'},
+            'ipapermdefaultattr': {
+                'description',
+            },
+            'default_privileges': {'DNS Administrators'},
+        },
+    }
Sounds reasonable. ACI does not allow renaming location but
IMHO
this is
okay.
Less renames we support the better.


+
+    takes_params = (
+        DNSNameParam(
+            'idnsname',
+            cli_name='name',
+            primary_key=True,
+            label=_('Location name'),
+            doc=_('IPA location name'),
+            # dns name must be relative, we will put it into
middle of
+            # location domain name for location records
+            only_relative=True,
+        ),
Okay. We need to make sure that relative names with multiple
labels
work -
but
this should automagically work as long as we are handling
DNS names
using
proper data types (not as strings).


+        Str(
+            'description?',
+            label=_('Description'),
+            doc=_('IPA Location description'),
+        ),
After discussion with Honza we will keep description as
single-value
in the
IPA framework and ignore that description attribute is
multi-value
in LDAP.
This is done for consitency with mistakes from the past.

[...]

+@register()
+class location_mod(LDAPUpdate):
+    __doc__ = _('Modify information about an IPA location .')
This should say 'Modify description' because nothing else
can be
modified.
More specific text would hopefully stop some people from
looking for
rename
options.
I disagree, this is general description about the modify
command, see
privilege-add it is the same as I made. I can see in future
that we will
forgot to update description of command if we add something
new there.
This is really an invalid argument.

"We must not touch XYZ because its documentation might become
obsolete in
future if we forget to update it!" :-)

How about inconsistency with description of older commands? I
don't
think that
command description should describe attributes that are allowed
to change.
Allowed attributes are shown in --help output
I do not agree but push whatever variant you like, it costed too
much
already.
NACK anyway. ipa-dns-install screams if you install a server
without DNS and
run ipa-dns-install later on:

The log contains this:

add objectClass:
          top
          groupofnames
          nestedgroup
add cn:
          DNS Administrators
add description:
          DNS Administrators
adding new entry "cn=DNS
Administrators,cn=privileges,cn=pbac,dc=dom-033,dc=abc,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com"




2016-05-10T16:53:05Z DEBUG stderr=ldap_initialize(
ldapi://%2Fvar%2Frun%2Fslapd-DOM-033-ABC-IDM-LAB-ENG-BRQ-REDHAT-COM.socket/??base


)
SASL/EXTERNAL authentication started
SASL username:
gidNumber=0+uidNumber=0,cn=peercred,cn=external,cn=auth
SASL SSF: 0
ldap_add: Already exists (68)

2016-05-10T16:53:05Z CRITICAL Failed to load dns.ldif: Command
'/usr/bin/ldapmodify -v -f /tmp/tmpMvWMaT -H
ldapi://%2fvar%2frun%2fslapd-DOM-033-ABC-IDM-LAB-ENG-BRQ-REDHAT-COM.socket
-Y

EXTERNAL' returned non-zero exit status 68

Well I cannot reproduce it, this should be resolved by patch 473

Updated patches attached

I found that IDNA did not work with previous version, fixed + IDNA
tests added


Fixed here, I sent wrong patches before




More patches added, all patches are available here:
https://github.com/bastiak/freeipa/tree/DNS-locations
Functional NACK

location-show returns incorrect location weight for servers

# ipa server-show vm-046.abc.idm.lab.eng.brq.redhat.com --all
   dn:
cn=vm-046.abc.idm.lab.eng.brq.redhat.com,cn=masters,cn=ipa,cn=etc,dc=dom-058-082,dc=abc,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com

   Server name: vm-046.abc.idm.lab.eng.brq.redhat.com
   Managed suffixes: domain
   Min domain level: 0
   Max domain level: 1
   Location: l2
   Location weight: 200
   objectclass: ipalocationmember, ipaReplTopoManagedServer, top,
ipaConfigObject, nsContainer, ipaSupportedDomainLevelConfig

# ipa location-show l2
   Location name: l2
   Description: Loc 2
   Servers: vm-046.abc.idm.lab.eng.brq.redhat.com (weight: 100)

- Did you forget --all somewhere?
Nope, I forgot to put it to default attributes, nice catch, I will add
test for it.

Other than that I have only nitpick regarding error message:
ipa: ERROR: l2 cannot be deleted because IPA Server(s)
vm-046.abc.idm.lab.eng.brq.redhat.com requires it

- Please unify singular/plural.
This is how the exception is generated, so you can choose just label, so
which label is the right? {'IPA server', 'IPA servers', 'IPA server(s)'}
(I think server should be with lower cased 's')
                raise DependentEntry(
                    label=_('IPA Server(s)'),
                    key=keys[-1],
                    dependent=location_members
                )

Use ngettext here to handle singular/plural correctly.


I did not require the code because Honza will surely look into details.

"Allow to use non-Str attributes as keys for members":

There is no actual API change, so don't bump VERSION.


"Remove CSV from from member params":

Again, no actual API change, don't bump VERSION.

CSV params should be removed completely. I have a patch in my drawer which does exactly that, should I post it?


"DNS Locations: extend server-* command with locations":

I don't think ipalocationweight should be in server-find. Searching for servers by exact weight does not strike me as something useful. Or is it?

In server.normalize_location(), don't call location.get_dn() with **options, as it contains server options, not location options. Also you are calling kw.pop('ipalocation_location') twice, which is most likely a bug.

In server_mod.pre_callback(), don't raise NotFound manually, but rather use self.api.Object.location.handle_not_found().


Honza

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to