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.

-- 
Petr^2 Spacek

-- 
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