Should we keep using FK with on_delete=SET_NULL?

ipanova · January 25, 2022, 4:47pm

We have number of instances where in our models we use FK with on_detete=SET_NULL

[ipanova@puffy models]$ git grep SET_NULL
exporter.py:        "PulpExport", related_name="last_export", on_delete=models.SET_NULL, null=True
publication.py:    content_guard = models.ForeignKey(ContentGuard, null=True, on_delete=models.SET_NULL)
publication.py:    remote = models.ForeignKey(Remote, null=True, on_delete=models.SET_NULL)
publication.py:    content_guard = models.ForeignKey(ContentGuard, null=True, on_delete=models.SET_NULL)
publication.py:    publication = models.ForeignKey(Publication, null=True, on_delete=models.SET_NULL)
publication.py:    remote = models.ForeignKey(Remote, null=True, on_delete=models.SET_NULL)
publication.py:        Repository, null=True, on_delete=models.SET_NULL, related_name="distributions"
publication.py:    repository_version = models.ForeignKey(RepositoryVersion, null=True, on_delete=models.SET_NULL)
repository.py:    remote = models.ForeignKey("Remote", null=True, on_delete=models.SET_NULL)
repository.py:    base_version = models.ForeignKey("RepositoryVersion", null=True, on_delete=models.SET_NULL)
task.py:    worker = models.ForeignKey("Worker", null=True, related_name="tasks", on_delete=models.SET_NULL)
task.py:        "Task", null=True, related_name="child_tasks", on_delete=models.SET_NULL
task.py:        "TaskGroup", null=True, related_name="tasks", on_delete=models.SET_NULL

I will state my case by providing couple of examples. There are more should we decide to audit whole pulp.

In a situation where a remote is being shared/used by N repositories, by removing the remote, all repos will lose reference to the remote.

(pulp) [vagrant@pulp3-source-fedora34 ~]$ http https://pulp3-source-fedora34.fluffy.example.com/pulp/api/v3/repositories/container/container/
HTTP/1.1 200 OK
Access-Control-Expose-Headers: Correlation-ID
Allow: GET, POST, HEAD, OPTIONS
Connection: keep-alive
Content-Length: 1212
Content-Type: application/json
Correlation-ID: 1b64ea433efb4f34aa94c1219071a999
Date: Tue, 25 Jan 2022 13:10:57 GMT
Referrer-Policy: same-origin
Server: nginx/1.20.1
Strict-Transport-Security: max-age=15768000
Vary: Accept, Cookie
X-Content-Type-Options: nosniff
X-Frame-Options: DENY

{
    "count": 2,
    "next": null,
    "previous": null,
    "results": [
        {
            "description": null,
            "latest_version_href": "/pulp/api/v3/repositories/container/container/335e97e5-4f4f-4ede-ba1e-a4e1d07c8e87/versions/0/",
            "manifest_signing_service": null,
            "name": "openstack1",
            "pulp_created": "2022-01-25T13:10:45.780669Z",
            "pulp_href": "/pulp/api/v3/repositories/container/container/335e97e5-4f4f-4ede-ba1e-a4e1d07c8e87/",
            "pulp_labels": {},
            "remote": "/pulp/api/v3/remotes/container/container/699e0a8b-eb3c-4a3f-95c1-64d7fe737d4e/",
            "retain_repo_versions": null,
            "versions_href": "/pulp/api/v3/repositories/container/container/335e97e5-4f4f-4ede-ba1e-a4e1d07c8e87/versions/"
        },
        {
            "description": null,
            "latest_version_href": "/pulp/api/v3/repositories/container/container/338e2af9-f613-4f64-ad07-d5172ac50e8d/versions/0/",
            "manifest_signing_service": null,
            "name": "openstack",
            "pulp_created": "2022-01-25T13:10:36.805312Z",
            "pulp_href": "/pulp/api/v3/repositories/container/container/338e2af9-f613-4f64-ad07-d5172ac50e8d/",
            "pulp_labels": {},
            "remote": "/pulp/api/v3/remotes/container/container/699e0a8b-eb3c-4a3f-95c1-64d7fe737d4e/",
            "retain_repo_versions": null,
            "versions_href": "/pulp/api/v3/repositories/container/container/338e2af9-f613-4f64-ad07-d5172ac50e8d/versions/"
        }
    ]
}

(pulp) [vagrant@pulp3-source-fedora34 ~]$ http DELETE https://pulp3-source-fedora34.fluffy.example.com/pulp/api/v3/remotes/container/container/699e0a8b-eb3c-4a3f-95c1-64d7fe737d4e/
HTTP/1.1 202 Accepted
Access-Control-Expose-Headers: Correlation-ID
Allow: GET, PUT, PATCH, DELETE, HEAD, OPTIONS
Connection: keep-alive
Content-Length: 67
Content-Type: application/json
Correlation-ID: 3fc3cd753a51467590e9619fe5743b3e
Date: Tue, 25 Jan 2022 13:13:11 GMT
Referrer-Policy: same-origin
Server: nginx/1.20.1
Vary: Accept, Cookie
X-Content-Type-Options: nosniff
X-Frame-Options: DENY

{
    "task": "/pulp/api/v3/tasks/f19accb2-f995-4fdb-a28a-2702029fc794/"
}


(pulp) [vagrant@pulp3-source-fedora34 ~]$ http https://pulp3-source-fedora34.fluffy.example.com/pulp/api/v3/repositories/container/container/
HTTP/1.1 200 OK
Access-Control-Expose-Headers: Correlation-ID
Allow: GET, POST, HEAD, OPTIONS
Connection: keep-alive
Content-Length: 1060
Content-Type: application/json
Correlation-ID: 1d4b312dc8534b7cac94e6648dd0c870
Date: Tue, 25 Jan 2022 13:13:15 GMT
Referrer-Policy: same-origin
Server: nginx/1.20.1
Strict-Transport-Security: max-age=15768000
Vary: Accept, Cookie
X-Content-Type-Options: nosniff
X-Frame-Options: DENY

{
    "count": 2,
    "next": null,
    "previous": null,
    "results": [
        {
            "description": null,
            "latest_version_href": "/pulp/api/v3/repositories/container/container/335e97e5-4f4f-4ede-ba1e-a4e1d07c8e87/versions/0/",
            "manifest_signing_service": null,
            "name": "openstack1",
            "pulp_created": "2022-01-25T13:10:45.780669Z",
            "pulp_href": "/pulp/api/v3/repositories/container/container/335e97e5-4f4f-4ede-ba1e-a4e1d07c8e87/",
            "pulp_labels": {},
            "remote": null,
            "retain_repo_versions": null,
            "versions_href": "/pulp/api/v3/repositories/container/container/335e97e5-4f4f-4ede-ba1e-a4e1d07c8e87/versions/"
        },
        {
            "description": null,
            "latest_version_href": "/pulp/api/v3/repositories/container/container/338e2af9-f613-4f64-ad07-d5172ac50e8d/versions/0/",
            "manifest_signing_service": null,
            "name": "openstack",
            "pulp_created": "2022-01-25T13:10:36.805312Z",
            "pulp_href": "/pulp/api/v3/repositories/container/container/338e2af9-f613-4f64-ad07-d5172ac50e8d/",
            "pulp_labels": {},
            "remote": null,
            "retain_repo_versions": null,
            "versions_href": "/pulp/api/v3/repositories/container/container/338e2af9-f613-4f64-ad07-d5172ac50e8d/versions/"
        }
    ]
}

Same stands for content_guards and N distributions that use same content_guard. Once the content_guard is removed, all distributions that were using it will stop serving protected content!

Deleting objects that are used as FK with on_delete=SET_NULL can break stuff, open security holes, or just produce unexpected results for the users. It is impossible to query what distributions use a certain content guard, or what repos use a certain remote before making a decision whether it is safe or desirable to remove that object.

What do you think we should do?

should we protect objects from being deleted if they are used in other objects?
do nothing, document
other?

Please provide feedback whether on this thread or directly on this issue:

github.com/pulp/pulpcore

When an object is shared and has on_delete=SET_NULL all other objects lose reference to it

opened 02:10PM - 25 Jan 22 UTC

ipanova

We have number of instances where in our models we use on FK on_detete=SET_NULL … ``` [ipanova@puffy models]$ git grep SET_NULL exporter.py: "PulpExport", related_name="last_export", on_delete=models.SET_NULL, null=True publication.py: content_guard = models.ForeignKey(ContentGuard, null=True, on_delete=models.SET_NULL) publication.py: remote = models.ForeignKey(Remote, null=True, on_delete=models.SET_NULL) publication.py: content_guard = models.ForeignKey(ContentGuard, null=True, on_delete=models.SET_NULL) publication.py: publication = models.ForeignKey(Publication, null=True, on_delete=models.SET_NULL) publication.py: remote = models.ForeignKey(Remote, null=True, on_delete=models.SET_NULL) publication.py: Repository, null=True, on_delete=models.SET_NULL, related_name="distributions" publication.py: repository_version = models.ForeignKey(RepositoryVersion, null=True, on_delete=models.SET_NULL) repository.py: remote = models.ForeignKey("Remote", null=True, on_delete=models.SET_NULL) repository.py: base_version = models.ForeignKey("RepositoryVersion", null=True, on_delete=models.SET_NULL) task.py: worker = models.ForeignKey("Worker", null=True, related_name="tasks", on_delete=models.SET_NULL) task.py: "Task", null=True, related_name="child_tasks", on_delete=models.SET_NULL task.py: "TaskGroup", null=True, related_name="tasks", on_delete=models.SET_NULL ``` Examples: In a situation where a remote is being shared/user by number of repositories by removing the remote all repos will lose reference to the remote. Same stands for content_guards and N distributions that use same content_guard. Once the content_guard is removed, all distributions that were using it will stop serving protected content. Deleting objects that are used as FK with on_delete=SET_NULL can break stuff, open security holes, or just produce unexpected results for the users, since it is impossible to query what distributions use certain content guard, or what repos user certain remote before making a decision whether it is safe or desirable to remove that object. What would be potentials steps: 1. should we protect objects from being deleted if they are used in other objects? 2. do nothing, document 3. other? https://pulp.plan.io/issues/8305 is a special variant of this issue

x9c4 · January 25, 2022, 9:41pm

I would suggest to change all occurances to on_delete=PROTECT. As you mentioned the current behaviour can break existing remote artifacts (deleted Remote), open security holes (delete ContentGuard) or violate the principle of least surprise (almost all types).
To ease the deletion, we should in a quick followup effort provide facilities to see where a thing is being used / protected from deletion.

davidd · January 26, 2022, 2:11pm

Generally, I think using PROTECT makes sense (this is without actually digging into the code and confirming though).

To ease the deletion, we should in a quick followup effort provide facilities to see where a thing is being used / protected from deletion.

This would be great. Also, I assume that if a user tries to delete a used resource, they’ll get a friendly message?