super_cron HS en https #4345

Closed
opened 4 years ago by JLuc · 22 comments
JLuc commented 4 years ago

action_super_cron_dist appelle fsockopen avec le port 80 par défaut, si l'url issue de genererl_url_action ne spécifie pas le port (c'est à dire la plupart du temps).

sur un site en https ça échoue (éventuellement ça renvoie une 301 mais fsockopen ne les suit pas)

Faute de parvenir à proposer une PR git, voici un patch à l'ancienne qui corrige ça en testant si http ou https.

En plus, il loge les échecs de fsockopen afin de pas avoir une partie silencieuse de SPIP qui ne marche pas.
Et il teste aussi le statut renvoyé. Cette partie peut être supprimée si nécessaire.

action_super_cron_dist appelle fsockopen avec le port 80 par défaut, si l'url issue de genererl_url_action ne spécifie pas le port (c'est à dire la plupart du temps). sur un site en https ça échoue (éventuellement ça renvoie une 301 mais fsockopen ne les suit pas) Faute de parvenir à proposer une PR git, voici un patch à l'ancienne qui corrige ça en testant si http ou https. En plus, il loge les échecs de fsockopen afin de pas avoir une partie silencieuse de SPIP qui ne marche pas. Et il teste aussi le statut renvoyé. Cette partie peut être supprimée si nécessaire.
Poster

voici le diff dont la partie qui teste le statut du fsockopen a été retirée - au cas où vous préféreriez

voici le diff dont la partie qui teste le statut du fsockopen a été retirée - au cas où vous préféreriez
Poster

Sauf si c'est évident le dev qui regarde, c'est à vérifier avant commit.

Sauf si c'est évident le dev qui regarde, c'est à vérifier avant commit.
Poster

C'est pas fini, il faut aussi ajouter ssl:// à l'host

C'est pas fini, il faut aussi ajouter ssl:// à l'host
Poster

Je confirme 443 + ssl:// avant le host comme argument de fsockopen, mais pas dans le GET : comme ça le super_cron marche en https

Je confirme 443 + ssl:// avant le host comme argument de fsockopen, mais pas dans le GET : comme ça le super_cron marche en https
Poster

Et j'ai du ajouter un t=time() ou t=intval(time()/10) dans la query aussi, pour que l'url appelée ne soit pas interpelée par le varnish...

Et j'ai du ajouter un t=time() ou t=intval(time()/10) dans la query aussi, pour que l'url appelée ne soit pas interpelée par le varnish...
Poster

Voici le nouveau .diff avec tout : "supercron v2"

  • le port 443 et le préfixe ssl:// pour fsockopen si on en est https
  • les logs d'échecs de connexion et de fsockopen
  • le timestamp sur l'url pour bypasser les caches
Voici le nouveau .diff avec tout : "supercron v2" - le port 443 et le préfixe ssl:// pour fsockopen si on en est https - les logs d'échecs de connexion et de fsockopen - le timestamp sur l'url pour bypasser les caches
Poster

Pour vraiment tester le retour du fsockopen on peut aussi ajouter :

$response = fgets($fp, 79);
$statut_init = substr($response, 9, 1);
if ($statut_init != '2')
   spip_log("Erreur statut renvoyé pour $url statut renvoyé = $response...", 'erreur_super_cron'.LOG_ERREUR);
Pour vraiment tester le retour du fsockopen on peut aussi ajouter : ``` $response = fgets($fp, 79); $statut_init = substr($response, 9, 1); if ($statut_init != '2') spip_log("Erreur statut renvoyé pour $url statut renvoyé = $response...", 'erreur_super_cron'.LOG_ERREUR); ```
Poster

Et puis tant qu'à faire, changer les commentaires :

 * Cette fonction se termine tout de suite, après avoir lancée un cron asynchrone
 * Elle est inadaptée depuis un cron UNIX dans lesquels il faut directement appeler l'action cron
Et puis tant qu'à faire, changer les commentaires : <pre> * Cette fonction se termine tout de suite, après avoir lancée un cron asynchrone * Elle est inadaptée depuis un cron UNIX dans lesquels il faut directement appeler l'action cron </pre>
b_b commented 4 years ago
Owner

Le patch grossit à vue d'oeil, pourquoi ajouter une nouvelle constante _SUPER_CRON_DELAIS ?

De plus, il serait intéressant de s'inspirer de ce que fait https://core.spip.net/projects/spip/repository/entry/spip/ecrire/inc/queue.php#L630 pour la détection du port.

Et pour finir, ça serait pas mal de respecter les PSRs dans tes structures de test :)
Statut changé à En cours

Le patch grossit à vue d'oeil, pourquoi ajouter une nouvelle constante `_SUPER_CRON_DELAIS` ? De plus, il serait intéressant de s'inspirer de ce que fait https://core.spip.net/projects/spip/repository/entry/spip/ecrire/inc/queue.php#L630 pour la détection du port. Et pour finir, ça serait pas mal de respecter les PSRs dans tes structures de test :) **Statut changé à En cours**
Poster

Il y a en effet plusieurs trucs : une correction de code, une amélioration potentielle et des modifs des commentaires.

La constante _SUPER_CRON_DELAIS permet d'ajouter un timestamp sur l'appel de cron. Cela vise à ce que le php reçoive vraiment la requête au lieu qu'elle soit interceptée par un varnish "mal configuré" (comme les timestamp qu'on ajoute aux fichiers images)
Dans le cas où il y a un cache de ce type et si la valeur de _SUPER_CRON_DELAIS vaut plus que 1, cela divise d'autant la fréquence des appels au cron en cas de grosse fréquentation.
Si ça convient pas comme ça, on pourrait la garder mais ne pas diviser le timestamp avec (et alors, la renommer _SUPER_CRON_TIMESTAMP )

Autre point :
il faudrait utiliser les constantes _PORT_HTTP_STANDARD et _PORT_HTTPS_STANDARD si définies plutôt que 80 et 443,
ici et aussi ailleurs : dans la fonction queue_affichage_cron

Il y a en effet plusieurs trucs : une correction de code, une amélioration potentielle et des modifs des commentaires. La constante _SUPER_CRON_DELAIS permet d'ajouter un timestamp sur l'appel de cron. Cela vise à ce que le php reçoive vraiment la requête au lieu qu'elle soit interceptée par un varnish "mal configuré" (comme les timestamp qu'on ajoute aux fichiers images) Dans le cas où il y a un cache de ce type et si la valeur de _SUPER_CRON_DELAIS vaut plus que 1, cela divise d'autant la fréquence des appels au cron en cas de grosse fréquentation. Si ça convient pas comme ça, on pourrait la garder mais ne pas diviser le timestamp avec (et alors, la renommer _SUPER_CRON_TIMESTAMP ) Autre point : il faudrait utiliser les constantes _PORT_HTTP_STANDARD et _PORT_HTTPS_STANDARD si définies plutôt que 80 et 443, ici et aussi ailleurs : dans la fonction queue_affichage_cron
Poster

Pour produire un diff à intégrer directement il me faudrait savoir ce que je met dedans : tout ? Sinon c'est en kit :

  • le fix pour 80 / 443 via les constantes et en testant le scheme comme queue_affichage_cron ? (a priori oui !)
  • l'emploi d'un timestamp ?
  • le timestamp divisé qui introduit un délai ?
  • le remplacement des commentaires phpdoc comme indiqué plus haut ? https://core.spip.net/issues/4345#note-8
  • les logs dans les divers cas d'erreur déjà testé ?
  • le test du statut de retour du fsockopen en plus, avec log si c'est pas 2XX ?
  • la correction de queue_affichage_cron pour employer les constantes ?
Pour produire un diff à intégrer directement il me faudrait savoir ce que je met dedans : tout ? Sinon c'est en kit : - le fix pour 80 / 443 via les constantes et en testant le scheme comme queue_affichage_cron ? (a priori oui !) - l'emploi d'un timestamp ? - le timestamp divisé qui introduit un délai ? - le remplacement des commentaires phpdoc comme indiqué plus haut ? https://core.spip.net/issues/4345#note-8 - les logs dans les divers cas d'erreur déjà testé ? - le test du statut de retour du fsockopen en plus, avec log si c'est pas 2XX ? - la correction de queue_affichage_cron pour employer les constantes ?
Poster

Autrement présentés, le kit des possibles, c'est :

Correction des bugs sans améliorations supplémentaires :

  • le fix pour le port 80 / 443 via les constantes
  • ceci en testant le scheme comme queue_affichage_cron
  • le remplacement des commentaires phpdoc comme indiqué plus haut ? https://core.spip.net/issues/4345#note-8

Améliorations de la qualité du code, sans changement fonctionnel :

  • loger des divers cas d'erreurs testés
  • la correction de queue_affichage_cron pour employer les constantes

Améliorations fonctionnelles :

  • test du statut de retour du fsockopen en plus, avec log si c'est pas 2XX
  • emploi d'un timestamp
  • timestamp divisé qui introduit un délai
Autrement présentés, le kit des possibles, c'est : Correction des bugs sans améliorations supplémentaires : - le fix pour le port 80 / 443 via les constantes - ceci en testant le scheme comme queue_affichage_cron - le remplacement des commentaires phpdoc comme indiqué plus haut ? https://core.spip.net/issues/4345#note-8 Améliorations de la qualité du code, sans changement fonctionnel : - loger des divers cas d'erreurs testés - la correction de queue_affichage_cron pour employer les constantes Améliorations fonctionnelles : - test du statut de retour du fsockopen en plus, avec log si c'est pas 2XX - emploi d'un timestamp - timestamp divisé qui introduit un délai
Poster

Cf aussi ce qui a été fait pour cachecool : https://zone.spip.net/trac/spip-zone/changeset/120815

Cf aussi ce qui a été fait pour cachecool : https://zone.spip.net/trac/spip-zone/changeset/120815
Poster
aussi pour cache_cool : https://zone.spip.net/trac/spip-zone/changeset/120811/spip-zone
Poster

Up

Le diff "simple" ne fait que corriger le pb et me semble devoir être intégré
(il n'apporte aucune amélioration susceptible de provoquer un débat)

Up Le diff "simple" ne fait que corriger le pb et me semble devoir être intégré (il n'apporte aucune amélioration susceptible de provoquer un débat)
b_b commented 2 years ago
Owner

Amha il ne faut intégrer que le patch qui concerne l'objet du ticket, rien de plus. Veux tu proposer ça dans une PR (après avoir comparé ton patch avec ceux de cache_cool) ou je m'en occupe ?
Version cible mise à 4.0

Amha il ne faut intégrer que le patch qui concerne l'objet du ticket, rien de plus. Veux tu proposer ça dans une PR (après avoir comparé ton patch avec ceux de cache_cool) ou je m'en occupe ? **Version cible mise à 4.0**

Appliqué par commit r24662.
Statut changé à Fermé

Appliqué par commit r24662. **Statut changé à Fermé**
b_b commented 2 years ago
Owner

Statut changé à En cours

**Statut changé à En cours**
Poster

Thanks for merging.
On peut release 3.3 !

Thanks for merging. On peut release 3.3 !
b_b commented 2 years ago
Owner

C'est pas encore mergé ;)

C'est pas encore mergé ;)
b_b commented 2 years ago
Owner

Corrigé par https://git.spip.net/spip/spip/commit/65f8ba2dc8
Statut changé à Fermé

Corrigé par https://git.spip.net/spip/spip/commit/65f8ba2dc8 **Statut changé à Fermé**
Poster

Le bug des ports a été corrigé, mais je confirme la nécessité d'ajouter un timestamp dans l'url pour ne pas permettre à varnish de servir un plat froid - du moins sur ma config de serveur.

Le bug des ports a été corrigé, mais je confirme la nécessité d'ajouter un timestamp dans l'url pour ne pas permettre à varnish de servir un plat froid - du moins sur ma config de serveur.
Sign in to join this conversation.
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.