Re: [pve-devel] [PATCH pve-access-control v2 1/1] permissions: add ACL paths for SDN fabrics

[Stefan Hanreich <s.hanreich@proxmox.com>]

On 4/7/25 11:34, Thomas Lamprecht wrote:
> Am 07.04.25 um 10:51 schrieb Stefan Hanreich:
>> On 4/7/25 10:12, Thomas Lamprecht wrote:
>>> Am 07.04.25 um 09:24 schrieb Fabian Grünbichler:
>>>> On April 4, 2025 7:20 pm, Thomas Lamprecht wrote:
>>>>> So, without looking at the implementation, fabrics have the IDs unique
>>>>> per sub-type? Could maybe also share an ID space, less confusion
>>>>> potential, but naturally also less flexibility – what do you think?
>>>>
>>>> they share a section config (and thus ID-space), so I guess we could
>>
>> There's one section config file per fabric, so its ID is unique per
>> protocol. We'd need to load *all* configuration files everytime we
>> change one configuration file (at least when adding a fabric) so we can
>> validate unique IDs across all fabric types.
> 
> I faintly remember some lunch talks, but what was the story behind the
> per type configs again? The per-node mappings?

Sorry, didn't see this email before sending the reply to Fabian:

Mostly the disjunct attributes in the fabric section, as well as the
interface key of the node section. OSPF and OpenFabric are relatively
similar, only differing in some protocol parameters. BGP and Wireguard
probably diverge even more and we wanted to avoid mixing too many
dissimilar sections.

Thinking more about it, I think we might have underexplored just putting
every type into one configuration file (as is the case with zones or
controllers for instance). I don't think it should be too hard to change
though with the current implementation.

> Can you post some sample configs for my convenience, or point to them
> if these patches here contain some (e.g., for testing), so that I (or
> someone other core maintainer) can take a closer look at this part.

There are some in the integration tests of proxmox-perl-rs, as well as
in the pve-network integration tests, but here they are for your
convenience (taken from my test cluster):


$ cat /etc/pve/sdn/fabrics/openfabric.cfg

fabric: evpn
	hello_interval 1
	loopback_prefix 172.20.30.0/24

node: evpn_deadeye
	fabric_id evpn
	interfaces name=ens19,ip=172.16.3.10/31
	node_id deadeye
	router_id 172.20.30.1

node: evpn_pathfinder
	fabric_id evpn
	interfaces name=ens19,ip=172.16.3.20/31
	node_id pathfinder
	router_id 172.20.30.2


$ cat /etc/pve/sdn/fabrics/ospf.cfg
fabric: ceph
	area 123
	loopback_prefix 172.20.3.0/24

node: ceph_deadeye
	fabric_id ceph
	interfaces name=ens20,unnumbered=true
	interfaces name=ens21,unnumbered=true
	node_id deadeye
	router_id 172.20.3.1

node: ceph_pathfinder
	fabric_id ceph
	interfaces name=ens20,unnumbered=true
	interfaces name=ens21,unnumbered=true
	node_id pathfinder
	router_id 172.20.3.2

node: ceph_raider
	fabric_id ceph
	interfaces name=ens20,unnumbered=true
	interfaces name=ens21,unnumbered=true
	node_id raider
	router_id 172.20.3.3


>> Since we have plans of moving over the existing IS-IS + BGP controllers,
>> as well as a new WireGuard fabric, we'd have to load and parse 5
>> different configuration files for cross-validation then.
>>
>>> Hmm, ok no real value for allowing a user use-access on such a fabric
>>> (for now), and not sure if it is useful to allow certain admins to create
>>> an openfabric fabric but not an ospf one (if the implementation even
>>> uses such fine-grained checks)
>>
>> I think that in practice, admins would create the fabrics and then only
>> hand out permissions to edit that specific fabric, without handing out
>> the permissions to create fabrics at all.
>>
>> It might make sense for Wireguard (where the possible implications
>> aren't nearly as bad as with e.g. BGP), but even then I think my
>> previous point applies that you probably don't want to hand out blanket
>> permissions for a whole subtype, in case you want to make heavy use of ACLs.
>>
>> The current schema is more of a direct result of how the section config
>> is structured, since IDs can be duplicated across different protocols,
>> so you need the additional distinguisher in the ACL path.
> 
> With the configs you describe it probably indeed make sense to have
> per-type ACL base paths, that said, I'd like to revisit the design
> again with more time at hand; IMO this is a bit to short notice and
> to big bags to hold for us to rush it in now.

Yes, I agree that we shouldn't rush this. I've thought about this
extensively today (and the weekend for that matter) and wanted to talk
to you about it today anyway.

After some consideration I strongly drifted towards not including it.
While I think this is functionally in a decent shape and I *really*
wanted to get this ready, I think we should take more time to carefully
consider and evaluate the underlying design. This is a fundamental
building block - particularly looking at further PDM integrations, so we
need to make sure to get this right. I don't think we can manage to do
this properly in a timely fashion, and I don't think this patch series
received the scrutiny it should given the importance and size.

> As of now I'd pretty certain that I'll only get around taking a
> closer look post releases, so this might be probably be a better fit
> for the next major release in a two to three months. Or would you
> prefer getting this in now to acquire feedback but then having to
> potentially rework the config/acl including migrations in a bigger
> way? Slapping some tech-preview label on it might set expectations
> roughly right for users in that case. I cannot give you a promise
> here but if you think it should go in and promise me to work on
> such a transformation then I will set some time aside to take a
> serious look today. But please consider whether a new feature at
> the end of the new feature release cycle is worth the effort for
> all of us. 

I think this might be for the better, this also has the potential to
affect existing EVPN setups as well with the refactoring portions. So
apart from discussing the design of the implementation, further
extensive testing would also be warranted imo (as evidenced by
Friedrich's report today).


> A side-benefit of waiting might be that you get around to also
> move bgp and is-is over too until then, ensuring the config/api
> design is really a good fit for that too.

I think so as well, same goes for a potential Wireguard integration.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel