Re: pcache LDAP_MATCHING_RULE_IN_CHAIN support

Howard Chu wrote:
> Johan wrote:
>> Hello all,
> 
>> We have an OpenLDAP instance proxying an active directory with back_meta and> mr_passthru.
>> We also have pcache on top, and as it do not support
>> LDAP_MATCHING_RULE_IN_CHAIN, I looked about implementing it.
> 
>> I found that we could retrieve cached results with pcacheQueryID, but I can't
>> get why this is not used in the current code?
> 
> It is of course used in the current code, for refreshing and expiring queries.

>> Plesae see attached patch

Please read https://openldap.org/devel/programming.html

> --- servers/slapd/overlays/pcache.c.ori	2023-02-04 10:56:07.910235675 +0100
> +++ servers/slapd/overlays/pcache.c	2023-02-04 10:57:14.319386931 +0100
> @@ -53,6 +53,16 @@
>   */
>  #define PCACHE_MONITOR
>  
> +// Not cacheable reasons
> +#define PCACHEMAXQUERIES_REACHED	0x1
> +#define PCACHEATTRSONLY				0x2
> +#define PCACHENOTEMPLATE			0x4
> +#define PCACHENOATTRSET				0x8

These values should be aligned. OpenLDAP source uses 4-column hard tabs for indents.
> +
> +// https://ldapwiki.com/wiki/LDAP_MATCHING_RULE_IN_CHAIN

This URL is currently unreachable. Probably should link to MSDN instead.

> +#define LDAP_MATCHING_RULE_IN_CHAIN "1.2.840.113556.1.4.1941"
> +
> +
>  /* query cache structs */
>  /* query */
>  
> @@ -512,7 +522,7 @@
>  	int attr_cnt;
>  	int t_cnt = 0;
>  	struct berval bv;
> -	char *p1, *p2;
> +	char *p1, *p2, *p3;
>  	AttributeDescription *ad;
>  	AttributeName *attrs;
>  
> @@ -532,9 +542,18 @@
>  		p2 = strchr( p1, '=' );
>  		if ( !p2 )
>  			break;
> -		if ( p2[-1] == '<' || p2[-1] == '>' ) p2--;
> -		bv.bv_val = p1;
> -		bv.bv_len = p2 - p1;
> +		// Incase of extended operation
> +		p3 = strchr( p1, ':' );
> +		if ( p3 && p3 < p2 ) {
> +			// FIXME: Is this valid syntax : "member:1.2.840.113556.1.4.1941:>=3" ?

Read RFC4515.

> +			if ( p3[-1] == '<' || p3[-1] == '>' ) p3--;
> +			bv.bv_val = p1;
> +			bv.bv_len = p3 - p1;
> +		} else {
> +			if ( p2[-1] == '<' || p2[-1] == '>' ) p2--;
> +			bv.bv_val = p1;
> +			bv.bv_len = p2 - p1;
> +		}
>  		ad = NULL;
>  		i = slap_bv2ad( &bv, &ad, text );
>  		if ( i ) {
> @@ -1136,7 +1155,6 @@
>  	return ret;
>  }
>  
> -
>  static struct berval*
>  merge_init_final(Operation *op, struct berval* init, struct berval* any,
>  	struct berval* final)
> @@ -1342,8 +1360,13 @@
>  			case LDAP_FILTER_LE:
>  				mrule = fs->f_ava->aa_desc->ad_type->sat_ordering;
>  				break;
> +			case LDAP_FILTER_EXT:
> +				if (0 == strncmp(fi->f_mr_rule_text.bv_val, LDAP_MATCHING_RULE_IN_CHAIN, fi->f_mr_rule_text.bv_len)) {

Probably should define the OID in a berval and use ber_bvcmp. We don't use "if (0 == xx)" we use "if (!xx)".

> +					mrule = fs->f_ava->aa_desc->ad_type->sat_equality;
> +				}
> +				break;
>  			default:
> -				mrule = NULL; 
> +				mrule = NULL;
>  			}
>  			if (mrule) {
>  				const char *text;
> @@ -1394,6 +1417,7 @@
>  				fi=fi->f_next;
>  				break;
>  			case LDAP_FILTER_EQUALITY:
> +			case LDAP_FILTER_EXT:
>  				if (ret == 0)
>  					res = 1;
>  				fs=fs->f_next;
> @@ -1941,6 +1965,19 @@
>  		fstr->bv_len += len;
>  		break;
>  
> +	case LDAP_FILTER_EXT:
> +		// Concatenate attribute name
> +		if ( f->f_mr_desc ) {
> +			ad = f->f_mr_desc;
> +			len = STRLENOF( "(::=)" ) + ad->ad_cname.bv_len + f->f_mr_rule_text.bv_len;
> +			ret = snprintf( fstr->bv_val+fstr->bv_len, len + 1, "(%s:%s:=)", ad->ad_cname.bv_val, f->f_mr_rule_text.bv_val );
> +			assert( ret == len );
> +			fstr->bv_len += len;
> +		} else {
> +			return -1;
> +		}

Again, read RFC4515.

> +		break;
> +
>  	case LDAP_FILTER_AND:
>  	case LDAP_FILTER_OR:
>  	case LDAP_FILTER_NOT: {
> @@ -2955,17 +2992,22 @@
>  {
>  	slap_overinst *on = (slap_overinst *)op->o_bd->bd_info;
>  	cache_manager *cm = on->on_bi.bi_private;
> -	query_manager*		qm = cm->qm;
> +	query_manager*	qm = cm->qm;

Don't make gratuitous whitespace changes.
>
>  	int i = -1;
>  
>  	Query		query;
>  	QueryTemplate	*qtemp = NULL;
> -	bindinfo *pbi = NULL;
> +	bindinfo	*pbi = NULL;
>  
> -	int 		attr_set = -1;
> -	CachedQuery 	*answerable = NULL;
> -	int 		cacheable = 0;
> +	int			attr_set = -1;
> +	CachedQuery	*answerable = NULL;
> +	int			cacheable = 0;
> +
> +	char	*fstr;
> +	Filter	*f;
> +
> +	int ncachereason = 0;
>  
>  	struct berval	tempstr;
>  
> @@ -3051,6 +3093,10 @@
>  				if (answerable)
>  					break;
>  			}
> +			if (cacheable == 0)
> +				ncachereason |= PCACHENOTEMPLATE;
> +		} else {
> +			ncachereason |= PCACHENOATTRSET;
>  		}
>  		op->o_tmpfree( tempstr.bv_val, op->o_tmpmemctx );
>  	}
> @@ -3098,7 +3144,16 @@
>  					}
>  				}
>  			}
> +
> +			/* Replace original query with query to cache ID */
> +			fstr = op->o_tmpalloc( sizeof("(pcacheQueryID=12345678-abcd-1234-abcd-123456789abc)")+1, op->o_tmpmemctx );
> +			sprintf(fstr, "(pcacheQueryID=%s)", answerable->q_uuid.bv_val);
> +			Debug( pcache_debug, "Replace search filter with %s\n", fstr, 0, 0 );

No. The fact the query is answerable doesn't mean you can replace the user's filter.
The user's filter may be more specific than the cached set, and will only return a
subset of entries. This code will break such queries.

> +
> +			f = str2filter(fstr);
> +			op->oq_search.rs_filter = f;
>  			i = cm->db.bd_info->bi_op_search( op, rs );
> +			op->o_tmpfree( fstr, op->o_tmpmemctx );
>  		}
>  		ldap_pvt_thread_rdwr_wunlock(&answerable->rwlock);
>  		/* locked by qtemp->qcfunc (query_containment) */
> @@ -3112,11 +3167,14 @@
>  	ldap_pvt_thread_mutex_lock(&cm->cache_mutex);
>  	if (cm->num_cached_queries >= cm->max_queries) {
>  		cacheable = 0;
> +		ncachereason |= PCACHEMAXQUERIES_REACHED;
>  	}
>  	ldap_pvt_thread_mutex_unlock(&cm->cache_mutex);
>  
> -	if (op->ors_attrsonly)
> +	if (op->ors_attrsonly) {
>  		cacheable = 0;
> +		ncachereason |= PCACHEATTRSONLY;
> +	}
>  
>  	if (cacheable) {
>  		slap_callback		*cb;
> @@ -3170,7 +3228,27 @@
>  		}
>  
>  	} else {
> -		Debug( pcache_debug, "QUERY NOT CACHEABLE\n" );
> +		switch (ncachereason) {

You defined the ncachereason as a set of bitflags. You can't use a switch() statement here, it
will just hit default if more than 1 bit is set, which defeats the purpose of tracking any reasons.

> +		case PCACHEMAXQUERIES_REACHED:
> +			Debug( pcache_debug, "QUERY NOT CACHEABLE (max_entries reached)\n",
> + 					0, 0, 0);
> +			break;
> +		case PCACHEATTRSONLY:
> +			Debug( pcache_debug, "QUERY NOT CACHEABLE (attrs only)\n",
> +					0, 0, 0);
> +			break;
> +		case PCACHENOTEMPLATE:
> +			Debug( pcache_debug, "QUERY NOT CACHEABLE (No template)\n",
> +					0, 0, 0);
> +			break;
> +		case PCACHENOATTRSET:
> +			Debug( pcache_debug, "QUERY NOT CACHEABLE (No Attrset)\n",
> +					0, 0, 0);
> +			break;
> +		default:
> +			Debug( pcache_debug, "QUERY NOT CACHEABLE (unkown reason)\n",
> +					0, 0, 0);
> +		}
>  	}
>  
>  	return SLAP_CB_CONTINUE;

-- 
  -- Howard Chu
  CTO, Symas Corp.           http://www.symas.com
  Director, Highland Sun     http://highlandsun.com/hyc/
  Chief Architect, OpenLDAP  http://www.openldap.org/project/