2014-03-25 18:14 GMT+01:00 Stephen Gallagher <sgallagh@redhat.com>:
> On 03/24/2014 05:02 PM, Miloslav Trmač wrote:
>> 2014-03-19 14:47 GMT+01:00 Stephen Gallagher <sgallagh@redhat.com
>> Wouldn't it be cleaner to have generic ValidateSettings(settings)
>> DeployRole(settings) as the abstraction used by the "universal"
>> clients (with "settings" possibly having some universal settings,
>> like our favorite firewall_enabled_after_deploy, but mostly
>> role-specific, only using a standard format, perhaps JSON or some
>> object graph with similar capabilities)?
> That's an idea. I'm not sure it's doing anything other than moving the
> problem to the settings object instead of the method call, but *shrug*.

Right, it would be even better to provide role-specific APIs (or a generic API with schema enforcement) to change values within the settings object :)  in order to make typos in settings names obvious, and that point it is really similar.

It does only one structurally significant thing: encode the "settings format" (JSON/whatever) as a canonical way to describe/dump/persistently store role deployment configuration, giving the API users precisely the same facilities they have during installation.

>> * methods: * GetFirewallPorts: list of port/protocol pairs that the
>> Role needs
>> I'd much rather have FirewallRestriction enum (LocalhostOnly,
>> Unrestricted, Complex) or something like that, or, if we decided
>> to depend on firewalld, an object reference to a firewalld service.
>> Roles will be asking for ICMP, new IP protocols, and whatnot, and
>> we don't need to start with that complexity in the API and the
>> clients.
> Sorry, that was a little thin. Could you go into a bit more detail
> here, please?

Stepping back a little, the role implementation and external API should be different.  The implementation obviously does need to actually contain port numbers and the like somewhere, but I don't think we want them in the role's public API.

The reason is that this introduces too much complexity into the API, at a level that isn't interesting to many important users of the API.  The complexity is in all the things a role might need (TCP ports, UDP ports, IP protocols, specific multicast addresses, perhaps even conntrack modules), and the presumed callers of a "Role deploy API" just don't care, they want the role deployed firewalled/not firewalled, and a service dashboard needs to to know whether it has been firewalled.  Receiving a list of half-a-dozen different individual firewall concepts to check isn't all that helpful, the public API should be able to give a simple yes/no answer (well, a "yes/no/"it's complicated" answer, which are the badly-explained LocalhostOnly/Unrestricted/Complex values above).

Depending on firewalld, it seems simplest for roles to provide firewalld services, and refer to callers to them.  We might still want a simple facade above, providing only the ability set LocalhostOnly/Unrestricted, and to receive current status as one of the three values.  That would be appropriate for a simple dashboard, more detailed management would be done on the firewalld service object directly using the firewalld API.

>> * AddCertificateAuthority:       Add a certificate authority to
>> this domain controller * EnableDisableDNS:              Enable or
>> disable the DNS server on this server
>> This would of course work.  A philosophical question, though: would
>> we rather move to closer to the "cattle" model, i.e.
>> settings = role.GetCurrentSettings() settings.enableDNS = False
>> role.Redeploy(settings)
>> ?
>> A "redeploy" is more work for the role, having to compare the
>> settings with current deployment and apply them, but it also ensure
>> that that code path works; with the individual configuration
>> modification operations, the roles could still provide a
>> GetCurrentSettings method, but we would have two completely
>> separate paths (EnableDisableDNS vs. Deploy) to test, and the risk
>> of them diverging.
> Hmm, but redeploy may not be possible (or idempotent) for all possible
> setting changes.

It's always possible to delete all data and start fresh :)  An API to redeploy that doesn't delete data needs the option to fail, sure—but any functionality that can be done using a GetValue/SetValue-like API should be equally possible to do using a Redeploy model; in the role-already-deployed case, it's just a series of
> if (settings.option_name != role.GetValue(option_name))
>    role.setValue(option_name, settings.option_name)
> if (settings.immutable_option_name != role.GetValue(immutable_option_name)
>    fail;

So semantically the two are sort of equivalent, this is primarily a matter of how we want to encourage the users to work with the system: keep a pet and modify it over time with the API, or have a settings file managed in a VCS somewhere and use the API only to sync the VCS and the deployment?

>> If we provide a limited firewall facade so that Fedora Server can
>> work without firewalld, IMHO it should literally be the simplest
>> possible "role.FirewallPreventsNonLocalhostAccess" value; not even
>> listing the ports involved.  If we provide something closer to a
>> full-featured D-Bus API firewall, we might just as well require
>> firewalld directly instead of writing a firewalld competitor.
> I think my original statement failed to get my point across, but I
> think I was pretty much saying the same thing you are.
I think so too.

> First of all,
> we *did* agree to use firewalld as the proper solution. I just meant
> that in terms of defining the Role-firewall interop, it should *not*
> try to be a complete set of firewall rules. For that, we should allow
> a Role to have a 'managed_firewall=False' setting[1] or something like
> it, and that admins would be expected to handle the firewall
> appropriately through firewalld/iptables. But if we *are* managing it,
> it's probably acceptable for it to be essentially either blocked or
> allowed on a whitelist of interfaces ('lo' being just a special case).
> Anyone requiring more control than that should set
> managed_firewall=False and do it themselves.

We could even have the managed_firewall=False implicit, as "service is not assigned to the Public or Drop zones" (... which would leave even the "allow on a whitelist of interfaces" case up to non-Role-API firewalld calls; I have no idea whether this would be appropriate).