Skip to content

Tech: ajout d'une option pour alerter sur des fichiers ayant disparu de notre bucket#5548

Merged
xavfernandez merged 2 commits into
masterfrom
xfernandez/check_existing_in_s3
Feb 6, 2025
Merged

Tech: ajout d'une option pour alerter sur des fichiers ayant disparu de notre bucket#5548
xavfernandez merged 2 commits into
masterfrom
xfernandez/check_existing_in_s3

Conversation

@xavfernandez

Copy link
Copy Markdown
Contributor

🤔 Pourquoi ?

Pour être alerté que l'on fournit des liens vers des objets n'existant pas.

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

  • Mettre à jour le CHANGELOG_breaking_changes.md ?
  • Ajouter l'étiquette « Bug » ?

🏝️ Comment tester ?

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

@francoisfreitag francoisfreitag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rien ne tourne avec cette option pour le moment. Le plan serait de la faire tourner en local de temps à autre ?
Que peut-on faire pour corriger, mis à part supprimer l’entrée de la table et éventuellement vérifier si des liens pointaient vers le fichier ? Si c’est le cas, autant le faire immédiatement et retirer l’entrée de base de données.

Pour la gestion des fichiers, on est toujours en attente de https://www.notion.so/gip-inclusion/Nettoyer-les-fichiers-S3-non-r-f-renc-s-dans-le-syst-me-suite-18a5f321b604819283fade43dcd46d72. Idéalement, les responsabilités seraient inversées : la DB donnera la liste des fichiers sur le S3 (et permettrait de supprimer les fichiers non référencés). Mettre des pansements en attendant me semble un peu investir à fond perdu.

J’avais le secret espoir d’augmenter la fréquence de sync data pour réduire la durée avant la détection d’un fichier malveillant téléversé. Il ne faudrait pas que cette option soit par défaut.

@xavfernandez

Copy link
Copy Markdown
Contributor Author

Cette option consommera un peu plus de RAM que sans (~200/250Mo) donc je pensais d'abord expérimenter à la main puis la rajouter au cron (soit systématiquement si on est large en RAM, soit une fois par jour, à un moment où les autres commandes ne tournent pas dans le cas contraire).

Il n'y a actuellement aucune raison que les fichiers disparaissent (:crossed_fingers: ) donc je ne pense pas qu'on ait besoin d'automatiser la gestion du cas (en tout cas je l'espère :sweat_smile: et cette option permettra de se rassurer). C'est juste la suppression du filtre can_open_file qui m'a fait me dire que ça pourrait être bien d'être alerté en cas de soucis.
Le jour où le soucis se produira on avisera...

@francoisfreitag

Copy link
Copy Markdown
Member

On supprime des fichiers à la main pour le RGPD (CVs). Il était aussi question de supprimer les fichiers inutilisés, mais là on vise 2030 🙈

@xavfernandez

Copy link
Copy Markdown
Contributor Author

On supprime des fichiers à la main pour le RGPD (CVs). Il était aussi question de supprimer les fichiers inutilisés, mais là on vise 2030 🙈

Bien vu 👍
Mais j'ai des doutes sur le fait que les gens suppriment le CV du bucket et de la base de données: raison de plus de rajouter le check.

@francoisfreitag

Copy link
Copy Markdown
Member

Plutôt que le logger.error, j’aurais nettoyé la DB et ajouté un log.info. L’action manuelle me paraît un pénible pour pas grand chose. D’un autre côté, ça pourrait nous alerter dans le cas où quelque chose d’inattendu se tramerait.

"--check-existing",
action=argparse.BooleanOptionalAction,
default=False,
help="Set to True to also check that all existing File objects reference existing bucket objects.",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
help="Set to True to also check that all existing File objects reference existing bucket objects.",
help="Check that all existing File objects reference existing bucket objects.",

known_permanent_files_nb = 0
if check_existing:
known_keys = set(File.objects.values_list("key", flat=True))
self.logger.info("Checking existing files: %d file(s) in database before sync", len(known_keys))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si on en est là, on peut se douter qu’il y aura toujours plus d’un fichier en prod.

Suggested change
self.logger.info("Checking existing files: %d file(s) in database before sync", len(known_keys))
self.logger.info("Checking existing files: %d files in database before sync", len(known_keys))

if known_keys:
# keys are present in database as File object but missing from our bucket
self.logger.error(
"Found %d database file(s) with unknown keys: %s", len(known_keys), sorted(known_keys)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Found %d database file(s) with unknown keys: %s", len(known_keys), sorted(known_keys)
"%d database files do not exist in the bucket: %s", len(known_keys), sorted(known_keys)

@xavfernandez xavfernandez force-pushed the xfernandez/check_existing_in_s3 branch from 186c992 to ff35385 Compare February 6, 2025 14:40
It will alert on existing File object in database referencing unknown
keys in the bucket
@xavfernandez xavfernandez force-pushed the xfernandez/check_existing_in_s3 branch from ff35385 to a5639b5 Compare February 6, 2025 15:02
@xavfernandez xavfernandez added this pull request to the merge queue Feb 6, 2025
Merged via the queue into master with commit 65020b6 Feb 6, 2025
@xavfernandez xavfernandez deleted the xfernandez/check_existing_in_s3 branch February 6, 2025 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants