Re: [pve-devel] [PATCH firewall] fix #4204: automatically update usages of group when it is renamed

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Tue, Sep 27, 2022 at 12:13:30PM +0200, Leo Nunner wrote:
> On 9/27/22 11:59, Wolfgang Bumiller wrote:
> 
> > On Tue, Sep 27, 2022 at 11:28:26AM +0200, Leo Nunner wrote:
> > > On 9/27/22 10:46, Wolfgang Bumiller wrote:
> > > > On Mon, Sep 26, 2022 at 11:45:07AM +0200, Leo Nunner wrote:
> > > > > +		$cluster_conf->{groups}->{$param->{group}} = $cluster_conf->{groups}->{$param->{rename}};
> > > > > +		$cluster_conf->{group_comments}->{$param->{group}} = $cluster_conf->{group_comments}->{$param->{rename}};
> > > > > +
> > > > > +		# Update comment if provided
> > > > >    		$cluster_conf->{group_comments}->{$param->{group}} = $param->{comment} if defined($param->{comment});
> > > > > +
> > > > At this point you'd need to also store the cluster fw config, because
> > > > *reading* the configs isn't necessarily done with a lock on the cluster
> > > > config, and you don't want to race against readers seeing the new group
> > > > being referred to without actually having the it in the config.
> > > > 
> > > > You'll still be racing against clients having read the cluster config
> > > > while you're *here* and then reading their host config *after* you've
> > > > updated it...
> > > Is there actually a way around this? Unless we use something like inotify,
> > > there'll be no way for them to actually know about the new group if they've
> > > read the cluster config before I updated it.
> > Well, not yet, and we'd need to distinguish between the race and the
> > group *actually* not existing.
> > Currently it'll produce a warning in the log which we might consider to
> > be "good enough".
> > We *could* try to remember which groups were missing in the previous run
> > and assume new missing groups are part of a race, but I'm not sure it's
> > worth it. Though syncing up would be simple enough as we only need to
> > lock/unlock the cluster fw config once.
> 
> Would this still be in the scope of this patch or should we just
> leave it like this for now?

That would be a separate patch.