Add ability for complex filtering on replicating UpstreamPulps #5733
Conversation
gerrod3
force-pushed
the
upstream-label-select
branch
from
67f661a to
a9ea8c1
Compare
gerrod3
force-pushed
the
upstream-label-select
branch
2 times, most recently
from
bb86823 to
f3eac16
Compare
| """Ensure we have a valid q_select expression.""" | ||
| from pulpcore.app.viewsets import DistributionFilter | ||
|
|
||
| DistributionFilter().filters["q"].field.clean(value) |
There was a problem hiding this comment.
That sounds like much better targeted.
👍
gerrod3
force-pushed
the
upstream-label-select
branch
2 times, most recently
from
5d4fdf2 to
c0178b2
Compare
|
Ok, I've "removed" |
gerrod3
force-pushed
the
upstream-label-select
branch
2 times, most recently
from
35158df to
6a833aa
Compare
There was a problem hiding this comment.
About the ZDU:
There's a case we may not be able to cover, I think. During the update, it is possible to configure an UpstreamPulp object using the new filter with a new api worker. Then, calling replicate on an old api worker, which will not realize there is an issue and trigger a task claiming to be compatible with the old worker code. Now if an old worker will pick up the task, it will not be able to apply the q_filter that may not even be translatable into label_select anymore. This could lead to an unfiltered replicate of every thing.
Can we make old workers fail when reading the label_select_filter, to limit the damage?
gerrod3
force-pushed
the
upstream-label-select
branch
from
6a833aa to
22cfaca
Compare
No, I don't think I can. Worst case the user can rerun replication if this happens after the upgrade is complete. |
Yes, I agree. The thing I'm worried about is that accidentally replicating without the filter can bust all the resource limits of the replica. And it can expose unwanted content in a certain Geo. But as you say, there is no good way to solve that, we should probably just leave a warning somewhere. |
gerrod3
force-pushed
the
upstream-label-select
branch
from
22cfaca to
3569223
Compare
I added a warning in the changelog. |
gerrod3
force-pushed
the
upstream-label-select
branch
from
3569223 to
32b45d8
Compare
| batch = [] | ||
| for upstream in UpstreamPulp.objects.filter(pulp_label_select__isnull=False).iterator(): | ||
| if upstream.pulp_label_select: | ||
| upstream.q_select = f"pulp_label_select='{upstream.pulp_label_select}'" |
There was a problem hiding this comment.
I think, we may need to think about proper quotation here.
There was a problem hiding this comment.
I think f"pulp_label_select={upstream.pulp_label_select!r}" should be sufficient, correct?
| pulp_settings, | ||
| gen_object_with_cleanup, | ||
| ): | ||
| """Test complex q_select replication.""" |
There was a problem hiding this comment.
Can we add a test where we need to quote the value inside of the quoted pulp_label_select filter string?
There was a problem hiding this comment.
Do you have an example of what one mine look like? I'm just wondering what users would be quoting in their labels.
There was a problem hiding this comment.
One thing i learned is, that users are creative...
Consider:
pulp file repository label set --repository test1 --key "asdf" --value "'asdf/+*&\""
There was a problem hiding this comment.
Yeah maybe we shouldn't allow quotes in labels. The ambiguity of it is very annoying and what value does a user gain by being able to use them?
I've been struggling with trying to get your example to work with the q=pulp_label_select filter and I'm not sure it is possible with how query expressions work. If you can get it to work then I'll add a test for it, else I'll just leave this as an edge case that hopefully users will be smart enough to avoid.
There was a problem hiding this comment.
So you are saying we cannot even select for these values today?
Can you add some sanitation for the label values to make sure we will never get there?
Maybe it's a separate PR, and I'm happy to have a migration fail in case a user has done that already.
gerrod3
force-pushed
the
upstream-label-select
branch
from
32b45d8 to
4da3b4b
Compare
3 participants
fixes: #5087
Also added tests for
pulp_label_selectreplications since I couldn't find any (maybe they are there and I'm bad at grep).