Project

General

Profile

Anomalie #4345

super_cron HS en https

Added by jluc - almost 2 years ago. Updated 6 months ago.

Status:
Fermé
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Start date:
06/11/2019
Due date:
% Done:

100%

Resolution:
fixed
Navigateur:

Description

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.

supercron.diff View (1.18 KB) jluc -, 06/11/2019 01:05 PM

supercron_simple.diff View (1002 Bytes) jluc -, 06/11/2019 01:12 PM

supercron v2.diff View (1.41 KB) jluc -, 06/11/2019 05:23 PM

Associated revisions

Revision 24662 (diff)
Added by bruno@eliaz.fr 9 months ago

réparer le super_cron en https

super_cron, il est super, mais un peu cron cron en https :p

report de 542730b44f71bc6395aa43e80f8b0a12a22215db & fix #4345

History

#1 Updated by jluc - almost 2 years ago

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

#2 Updated by jluc - almost 2 years ago

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

#3 Updated by jluc - almost 2 years ago

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

#4 Updated by jluc - almost 2 years ago

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

#5 Updated by jluc - almost 2 years ago

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...

#6 Updated by jluc - almost 2 years ago

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

#7 Updated by jluc - almost 2 years ago

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);

#8 Updated by jluc - almost 2 years ago

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

#9 Updated by b b almost 2 years ago

  • Status changed from Nouveau to 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 :)

#10 Updated by jluc - almost 2 years ago

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

#11 Updated by jluc - almost 2 years ago

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 ?

#12 Updated by jluc - almost 2 years ago

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

#13 Updated by jluc - over 1 year ago

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

#15 Updated by jluc - about 1 year ago

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)

#16 Updated by b b 9 months ago

  • Target version set to 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 ?

#17 Updated by Anonymous 9 months ago

  • Status changed from En cours to Fermé
  • % Done changed from 0 to 100

Appliqué par commit r24662.

#18 Updated by b b 9 months ago

  • Status changed from Fermé to En cours

#19 Updated by jluc - 9 months ago

Thanks for merging.
On peut release 3.3 !

#20 Updated by b b 9 months ago

C'est pas encore mergé ;)

#21 Updated by b b 8 months ago

  • Status changed from En cours to Fermé
  • Resolution set to fixed

#22 Updated by jluc - 6 months ago

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.

Also available in: Atom PDF