From: giovanni@paclan.it
Subject: Re: relayd disable/enable all hosts with same name
To: tech@openbsd.org
Date: Wed, 12 Jun 2024 18:10:03 +0200

The diff looks good to me.
ok giovanni@
  Cheers
    Giovanni

On 6/3/24 4:05 PM, Kapetanakis Giannis wrote:
> ping
> 
> updated diff at the end with corrected line width
> 
> G
> 
> On 24/05/2024 10:07, Kapetanakis Giannis wrote:
>>
>>
>> On 24/05/2024 09:13, Alexandr Nedvedicky wrote:
>>> Hello,
>>>
>>> I'm not using a relayd so I can't tell exactly. The relayctl(8) says:
>>>
>>>      host disable [name | id]
>>>               Disable a host.  Treat it as though it were always down.
>>>
>>>      host enable [name | id]
>>>               Enable the host.  Start checking its health again.
>>>
>>> so the whole issue feels like a kind of bug. See further below for comments on
>>> change itself. Note I'm not very familiar with relayctl code.
>>>
>>>
>>> +
>>> +		/* Disable hosts with same name on all tables */
>>> +		if (host_byname)
>>> +			TAILQ_FOREACH(t, env->sc_tables, entry)
>>> +				TAILQ_FOREACH(h, &t->hosts, entry)
>>> +					if (strcmp(h->conf.name, host->conf.name) == 0 &&
>>> +					    h->conf.id != host->conf.id && !h->conf.parentid)
>>> +						disable_host(c, id, h);
>>> 			^^^^
>>> 			I'm not sure if it is right place for TAILQ_FOREACH()
>>> 			over all tables here. The thing is the loop does call
>>> 			disable_host() recursively. So each level of
>>> 			recursion is going to walk over the same set of
>>> 			all tables relayd keeps.
>>>
>>> 			also lines exceed 80 characters.
>>>
>>> </snip>
>>>
>>> thanks and
>>> regards
>>> sashan
>>
>> Look at host_findbyname() which is called early on disable_host()
>> I took the code from there. It does the same thing.
>> host_findbyname(struct relayd *env, const char *name)
>> {
>>     struct table   *table;
>>     struct host *host;
>>
>>     TAILQ_FOREACH(table, env->sc_tables, entry)
>>        TAILQ_FOREACH(host, &table->hosts, entry)
>>           if (strcmp(host->conf.name, name) == 0)
>>              return (host);
>>     return (NULL);
>> }
>>
>> but returns uppon first entry found. So only the first host found is disabled/enabled and the rest (same host) are left intact.
>> So if you have for example table <webhosts> { www1, www2 }
>> redirect http { listen on $ext_ip port 80 forward to <webhosts> } redirect https { listen on $ext_if port 443 forward to <webhosts> } redirect service1 { listen on $ext_if port 8080 forward to <webhosts> } etc.. you get my point, many services on same servers and issue relayctl host disable www1 www1 will be only removed from http@relayd/http table which is the first and will be left enabled on rest of services. So the only functional way of putting www1 on maintenance is to manually issue disable on all host ids. Intentionally I didn't touch enable/disable by id. Maybe someone wants to only disable a specific host/port and not all services on host. But by name should do it on all names found. G
> 
> 
> Index: pfe.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/pfe.c,v
> retrieving revision 1.90
> diff -u -p -u -p -r1.90 pfe.c
> --- pfe.c	14 Sep 2020 11:30:25 -0000	1.90
> +++ pfe.c	3 Jun 2024 14:00:46 -0000
> @@ -584,11 +584,14 @@ int
>   disable_host(struct ctl_conn *c, struct ctl_id *id, struct host *host)
>   {
>   	struct host	*h;
> -	struct table	*table;
> +	struct table	*table, *t;
> +	int	 host_byname = 0;
>   
>   	if (host == NULL) {
> -		if (id->id == EMPTY_ID)
> +		if (id->id == EMPTY_ID) {
>   			host = host_findbyname(env, id->name);
> +			host_byname = 1;
> +		}
>   		else
>   			host = host_find(env, id->id);
>   		if (host == NULL || host->conf.parentid)
> @@ -625,6 +628,16 @@ disable_host(struct ctl_conn *c, struct
>   		/* Disable all children */
>   		SLIST_FOREACH(h, &host->children, child)
>   			disable_host(c, id, h);
> +
> +		/* Disable hosts with same name on all tables */
> +		if (host_byname)
> +			TAILQ_FOREACH(t, env->sc_tables, entry)
> +				TAILQ_FOREACH(h, &t->hosts, entry)
> +					if (strcmp(h->conf.name,
> +					    host->conf.name) == 0 &&
> +					    h->conf.id != host->conf.id &&
> +					    !h->conf.parentid)
> +						disable_host(c, id, h);
>   		pfe_sync();
>   	}
>   	return (0);
> @@ -634,10 +647,15 @@ int
>   enable_host(struct ctl_conn *c, struct ctl_id *id, struct host *host)
>   {
>   	struct host	*h;
> +	struct table	*t;
> +	int	 host_byname = 0;
> +
>   
>   	if (host == NULL) {
> -		if (id->id == EMPTY_ID)
> +		if (id->id == EMPTY_ID) {
>   			host = host_findbyname(env, id->name);
> +			host_byname = 1;
> +		}
>   		else
>   			host = host_find(env, id->id);
>   		if (host == NULL || host->conf.parentid)
> @@ -666,6 +684,16 @@ enable_host(struct ctl_conn *c, struct c
>   		/* Enable all children */
>   		SLIST_FOREACH(h, &host->children, child)
>   			enable_host(c, id, h);
> +
> +		/* Enable hosts with same name on all tables */
> +		if (host_byname)
> +			TAILQ_FOREACH(t, env->sc_tables, entry)
> +				TAILQ_FOREACH(h, &t->hosts, entry)
> +					if (strcmp(h->conf.name,
> +					    host->conf.name) == 0 &&
> +					    h->conf.id != host->conf.id &&
> +					    !h->conf.parentid)
> +						enable_host(c, id, h);
>   		pfe_sync();
>   	}
>   	return (0);
>