Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Habilita rate-limit em todas as rotas da API #715

Merged
merged 2 commits into from
Sep 5, 2022

Conversation

filipedeschamps
Copy link
Owner

Após o maravilhoso PR #685 do @aprendendofelipe sugiro passarmos todas as rotas da API por dentro do rate-limit. Então atualizei os comentários para explicar a situação e fiz skip nos dois testes que quebram enquanto o Next.js não conserta esse bug: vercel/next.js#39262

Meu racional foi otimizar para proteção dos endpoints em troca da "garantia" de que conseguimos criar e atualizar conteúdos com 20k caracteres. Coloquei "garantia" entre aspas, porque infelizmente não há garantia alguma nesse caso, uma vez que o Middleware está se comportando de uma forma em development e outra forma em preview e production.

Em paralelo, caso seja necessário desproteger o ambiente de homologação para testes de carga, basta alterar o middleware em um commit temporário 🤝

@vercel
Copy link

vercel bot commented Sep 5, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
tabnews ✅ Ready (Inspect) Visit Preview Sep 5, 2022 at 10:15PM (UTC)

@filipedeschamps
Copy link
Owner Author

@aprendendofelipe veja o que acha do commit 8621f5c junto desse PR.

Fiz isso, pois enquanto eu estava navegando pela paginação, o negócio estava sofrido. O banco estava furioso demais fechando as conexões e isto atrasa demais o trabalho dos estáticos em /_next com o Link. Isso não conserta o problema de performance das paginações, isso deve ser feito em outra abordagem, mas dado que por esse PR estamos protegendo todas as rotas da API, pelo menos esse último commit vai dar mais margem de manobra para as lambdas fazerem o seu trabalho com o banco de dados. Bora "arriscar"?

@aprendendofelipe
Copy link
Collaborator

Bora!

É aquilo que já conversamos. Ficar encerrando as conexões de Lambdas acordadas causa um efeito cascata de perda de performance, pois cada request irá demorar mais para ser respondida (por necessitar estabelecer nova conexão), o que exige mais lambdas rodando paralelamente, o que aumenta a quantidade de conexões ativas, o que força novas conexões a serem encerradas e por aí vai...

@aprendendofelipe
Copy link
Collaborator

Uma sugestão para testar também é diminuir o intervalo entre as 5 tentativas de conexão ao banco.

Hoje está assim:

async function tryToGetNewClientFromPool() {
  const clientFromPool = await retry(newClientFromPool, {
    retries: 5,
    minTimeout: 1000,
    factor: 2,
  });
...

Por causa do factor: 2, do parâmetro randomize: true (default) e também do connectionTimeoutMillis: 1000, os 60s de limite da lambda podem não ser suficientes para ocorrerem essas 5 retentativas de conexão.

Sugiro testar o seguinte:

const configurations = {
 ...
 connectionTimeoutMillis: 500
async function tryToGetNewClientFromPool() {
  const clientFromPool = await retry(newClientFromPool, {
    retries: 5,
    minTimeout: 150,
    maxTimeout: 1000,
    factor: 2,
  });
...

Com isso é esperado que todas as 5 tentativas ocorram randomicante em no máximo 6,4s. Passado esse tempo sem sucesso na conexão, então a request irá retornar o erro de timeout de conexão com o banco de dados, ao invés de ocorrer o timeout da lambda e a request ficar sem resposta.

Isso só vai ocorrer quando o banco não estiver aceitando conexões, então a liberação mais rápida da lambda irá evitar que novas lambdas subam paralelamente e afoguem ainda mais o banco de dados.

@filipedeschamps
Copy link
Owner Author

Show @aprendendofelipe 😍 apliquei isso junto do commit 6383b2b

@filipedeschamps
Copy link
Owner Author

Hmm... bizarro, ta dando erro de timeout:

{"name":"ServiceError","message":"Connection terminated due to connection timeout",...

image

URL fixa para testar: https://tabnews-2d31js0xn-tabnews.vercel.app/

@filipedeschamps
Copy link
Owner Author

Mudei um pouco os valores: 9227826

Agora está indo: https://tabnews-d73obab3r-tabnews.vercel.app/

E seria muito legal a Vercel para Preview conseguir subir as lambdas em uma região e para Production em outra. É muito ruim trabalhar em Preview, muita distância entre o banco e lambda que ele chora:

image

Mas ta consumindo bem a quantidade de conexões, e isso é ótimo 👍

@filipedeschamps filipedeschamps merged commit 4c13b5a into main Sep 5, 2022
@filipedeschamps filipedeschamps deleted the rate-limit-all-api-routes branch September 5, 2022 22:22
@filipedeschamps
Copy link
Owner Author

Merged! Let's goooooo! Vou ficar acompanhando aqui 👍

@filipedeschamps
Copy link
Owner Author

Em produção ta usando mais conexões que antes, excelente!

image

@filipedeschamps
Copy link
Owner Author

Tava pensando aqui @aprendendofelipe se não seria uma boa ideia habilitar o rate limit em tudo... e por tudo eu digo páginas estáticas e /_next. Como estamos com revalidate de 1, atacar distribuidamente todas as páginas estáticas seria relativamente a mesma coisa que atacar a API.

@aprendendofelipe
Copy link
Collaborator

Tava pensando aqui @aprendendofelipe se não seria uma boa ideia habilitar o rate limit em tudo... e por tudo eu digo páginas estáticas e /_next. Como estamos com revalidate de 1, atacar distribuidamente todas as páginas estáticas seria relativamente a mesma coisa que atacar a API.

Vamos pensar aqui...

  1. Essa versão de rate-limit não evita um ataque distribuído com identificador vinculado ao IP vs Endpoint:
    identifier: limitKey === 'general' ? `${ip}` : `${ip}:${method}:${path}`
    Teria que bolar algo diferente.

  2. Um ataque para as páginas estáticas é bem pior se for direcionado para endereços que tenham estrutura válida, mas que não existam, pois aí não iria existir cache e o Next vai fazer uma consulta ao banco para cada request para tentar montar cada página inválida requisitada. Nesse caso não importa o valor do revalidate. Um dos limitantes nesse ataque seria a própria Vercel, pelo limite de 1000 lambdas simultâneas (ou 950 da Edge). Mas muito antes disso o banco já estaria sem conexões disponíveis. Só que com a implementação feita aqui, com a mudança no retry, as lambdas vão retornar erro rapidamente ao invés de estourar o timeout de 60s tentando se conectar ao banco. Então a grande diferença entre a resposta estar vindo da Edge ou das Lambdas é basicamente o tempo que cada request irá levar para retornar um erro, mas de qualquer jeito o atacante tem que ser barrado pela Vercel, ou o sistema vai ficar funcionando somente com o que existir em cache.

  3. Outra questão é o custo disso. Em conta de padaria daria uma request para o Upstash a cada request para o qualquer parte do TabNews. Tem que fazer as contas pra ver se esse custo não paga uma solução mais robusta. Provavelmente sim.


Sobre aquele monte de timeout que deu, os 500ms para o connectionTimeoutMillis não foi suficiente pela distância entre o banco de homologação e as lambdas 😩

Já a mudança no maxTimeout para 5000ms é o mesmo que deixar sem limite, pois na última tentativa de retry, com o randomize o timeout vai ser de algo entre 2400 e 4800ms. Mas tudo bem, isso só vai fazer as lambdas demorarem um pouco mais nas últimas tentativas de se conectar ao banco. Então se o banco não estiver respondendo, a lambda vai responder a request com um erro em até 15s. Vamos analisar o comportamento dessa forma.

E seria muito legal a Vercel para Preview conseguir subir as lambdas em uma região e para Production em outra.

Você diz deixar isso configurado definitivamente, né? Pra não precisar trocar a região no vercel.json de gru para iad1?

@aprendendofelipe
Copy link
Collaborator

Pensando no gap que existe entre o limite de conexões do banco e a quantidade de lambdas que podem ser executas ao mesmo tempo, que tal a gente usar um pool de verdade?

Por exemplo, colocar o PgBouncer em uma instância na AWS na mesma rede que o Postgres para proteger o banco e aumentar a quantidade de lambdas que podem ficar conectadas ao mesmo tempo se revezando nas conexões do PgBouncer com o Postgres.

Considerando o fluxo atual, acredito que o custo seria equivalente ao de colocar o UpStash na frente de tudo, mas é uma solução mais robusta e que escala bem melhor. Algumas vantagens do PgBouncer:

  1. Apesar do custo equivalente inicialmente, a solução com o PgBouncer acaba saindo mais barato se o volume de acessos subir.
  2. A princípio não exige nenhuma grande alteração no sistema atual, pois basta apontar o PgBouncer para o Postgres e apontar a aplicação para o PgBouncer como se fosse o banco. Mas com o tempo podemos ir alterando para outras configurações melhores.
  3. Não tem nem comparação com o pool executando nas lambdas, pois no caso de uma rajada, diversas lambdas frescas sobem e elas vão ficar tentando se conectar com o banco e causar ainda mais lentidão. Já com o PgBouncer muito mais lambdas podem enviar as consultas e aguardar em uma fila caso o banco esteja ocupado.
  4. Existem diversas formar de escalar se o sistema crescer rapidamente ou mesmo se precisarmos garantir uma capacidade maior somente em datas específicas.

Segue uma fonte com informações relacionadas:

https://www.revenuecat.com/blog/pgbouncer-on-aws-ecs/

@filipedeschamps
Copy link
Owner Author

  1. Essa versão de rate-limit não evita um ataque distribuído com identificador vinculado ao IP vs Endpoint:
    identifier: limitKey === 'general' ? `${ip}` : `${ip}:${method}:${path}`
    Teria que bolar algo diferente.

Para esse caso não seria IP vs Endpoint e deveria só IP mesmo para cair no limite global general. Então a pessoa conseguiria gerar simultâneamente a quantidade de estáticos que está no limite global, por IP. Mas concordo que não seja a melhor solução 🤝

Você diz deixar isso configurado definitivamente, né? Pra não precisar trocar a região no vercel.json de gru para iad1?

Isso! Para termos o comportamento de latência bem próximo ao de Produção 😍

Por exemplo, colocar o PgBouncer em uma instância na AWS na mesma rede que o Postgres para proteger o banco e aumentar a quantidade de lambdas que podem ficar conectadas ao mesmo tempo se revezando nas conexões do PgBouncer com o Postgres.

Curto total a ideia!!! No artigo que você apontou, destaco isso:

we’re able to pool 1200 connections from our application servers down to about 100 connections to our RDS cluster.

Muito massa 😍 lembro que no começo do TabNews a gente tinha escolhido o Digital Ocean porque eles oferecem a instalação do PgBouncer com "1 click".

De quaquer forma, eu trataria a proteção por ataque separadamente do banco por causa de um detalhe: processamento.

A nossa instância hoje é muito fraca nesse quesito e deixar o atacante chegar no pool, depois no banco e ter que pegar o estado do limite lá vai constantemente consumir todo o processamento do banco eu imagino.

Talvez seja interessante alguma outra peça receber primeiro essa carga (no caso hoje o Upstash, mas pode ser qualquer outra coisa) e ela decidir ou não repassar a request em diante.

@aprendendofelipe
Copy link
Collaborator

Muito massa 😍 lembro que no começo do TabNews a gente tinha escolhido o Digital Ocean porque eles oferecem a instalação do PgBouncer com "1 click".

Massa e fácil demais! Pena esse limite de 20 conexões!

> However, these PgBouncer pool connections are currently limited to 20 per database.

E isso que você destacou 🚀🚀🚀

we’re able to pool 1200 connections from our application servers down to about 100 connections to our RDS cluster.

Estarei bastante ocupado nessa semana, mas depois posso criar uma instância na AWS para fazermos testes.

De quaquer forma, eu trataria a proteção por ataque separadamente do banco por causa de um detalhe: processamento.

Sem dúvidas o pool não é solução para todos os problemas e definitivamente não é solução para um ataque. Mas é algo que pode nos fazer suportar um volume de acesso esporádico acima do comum (como aqui #735) sem precisar lidar com isso como se fosse um ataque. E talvez dê para deixar para a Vercel lidar com um ataque de verdade que esteja explorando a geração de páginas estáticas.

Já o rate-limit fica principalmente para lidar com abusos do serviço e menos preocupado em proteger o banco. Até porque o rate-limit precisa ser bem mais limitante do que a capacidade do sistema.

A nossa instância hoje é muito fraca nesse quesito e deixar o atacante chegar no pool, depois no banco e ter que pegar o estado do limite lá vai constantemente consumir todo o processamento do banco eu imagino.

Talvez seja interessante alguma outra peça receber primeiro essa carga (no caso hoje o Upstash, mas pode ser qualquer outra coisa) e ela decidir ou não repassar a request em diante.

Usar a Edge e o Upstash como rate-limit (ou qualquer outra limitação) para as páginas estáticas me parece com um rabo balançando o cachorro... hehe... Mas sim, não há dúvidas que funcionaria até certo ponto! Mas me parece que cria mais problemas do que soluções.

Por exemplo, estaremos criando alguns limites de acessos simultâneos para as páginas estáticas (1000 por segundo do UpStash e 950 simultâneos da Edge). Na prática esse limite seria ainda menor, pois seria dividido com a API. E em caso de ataque que atinja esse limite, nenhum usuário conseguirá acessar nem mesmo as páginas estáticas. E se criar alguma forma de transbordo para quando atingir esses limites o acesso acontecer sem passar pelo rate-limit, aí o atacante pode explorar isso e não existirá rate-limit nenhum.

Falando especificamente de um ataque por tentativa de geração de páginas estáticas inválidas. Uma maneira de lidar com isso pode ser criando no PostgreSQL um usuário diferente para ser utilizado pelas lambdas responsáveis pela API e outro para as lambdas que geram os conteúdos estáticos. Com isso dá pra definir rolconnlimit diferentes para esses usuários, impedindo que qualquer um deles monopolizem o banco. Mesmo assim iria usar parte do processamento do banco só pra ficar respondendo ao atacante que as páginas não existem, mas isso não travaria o banco porque podemos deixar liberado para essa função um número de conexões que a gente sabe que o banco aguenta e que ainda deixe sobrar processamento para a API.

Mas também podemos fazer qualquer outro limite entre as lambdas e o banco, mas que não limite de forma alguma o acesso aos estáticos já gerados em cache na Vercel.

@aprendendofelipe
Copy link
Collaborator

E seria muito legal a Vercel para Preview conseguir subir as lambdas em uma região e para Production em outra.

@filipedeschamps, temos a alternativa de migrar de funções sem servidor para funções de borda (beta). Só precisamos testar se todos os endpoints podem ser migrados, pois a Edge Runtime tem algumas limitações como tempo de CPU e de execução e as APIs aceitas.

Também é bom considerarmos que as funções de borda ainda estão em beta e, por isso, ainda não tem custos.

A princípio as funções de borda rodam mais próximo do client, mas podemos configurar regiões específicas, então dá pra fazer serem executadas na região do banco de dados, independentemente de onde está o client. E podemos utilizar diferentes regiões para Preview e Production, de acordo com as variáveis de ambiente.

Se achar interessante eu abro um PR.

@filipedeschamps
Copy link
Owner Author

Que massa! Não sabia que dava para configurar a região pelo código, isso com a variável de ambiente para decidir onde rodar seria sensacional!

Mas pelo que entendo a Edge não deixará usar o pg e só consegue fazer chamadas por http.

O que talvez eu faria, que não tem nada a ver com o assunto aqui, é entender como fazer para aceitar html como o GitHub faz, mas sem conseguir reconstruir os elementos gráficos da interface, como aconteceu aqui mas que por algum motivo não está mais funcionando, não entendi porque. Será que o @gabrielsozinho alterou o body?

@aprendendofelipe
Copy link
Collaborator

Mas pelo que entendo a Edge não deixará usar o pg

Verdade! Esqueci desse grande detalhe.

entender como fazer para aceitar html como o GitHub faz

Não sei se entendi. Você diz aceitar menos do que é aceito hoje, ou seja, ficar mais restrito?

por algum motivo não está mais funcionando

Acho que parou de funcionar quando atualizou as dependências

@filipedeschamps
Copy link
Owner Author

Não sei se entendi. Você diz aceitar menos do que é aceito hoje, ou seja, ficar mais restrito?

Isso! Na verdade, aceitar tudo, porém mostrar filtrado.

Acho que parou de funcionar quando atualizou as dependências

Puts, é verdade, olha que louco:

Antes: https://tabnews-hip4st1lb-tabnews.vercel.app/gsozinhos/teste
Depois: https://tabnews-171266cgp-tabnews.vercel.app/gsozinhos/teste

E é realmente muito louco, porque foi atualizado essas dependências:

https://github.com/filipedeschamps/tabnews.com.br/pull/773/files

@aprendendofelipe
Copy link
Collaborator

Eu acho que só parou de funcionar pq mudaram os nomes das classes que o @gabrielsozinho aproveitou. O que deve ter ocorrido por causa da atualização do styled-components

@gabrielsozinho
Copy link
Contributor

Eu acho que só parou de funcionar pq mudaram os nomes das classes que o @gabrielsozinho aproveitou. O que deve ter ocorrido por causa da atualização do styled-components

Foi o nome das classes que mudou mesmo. Atualizei só a parte de "Responder" com a nova classe e voltou a funcionar na nova versão, ao mesmo tempo que parou de funcionar com a antiga.

demonstração do botão "responder" falso na nova versão

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants