Question: Auto refreshing installation tokens

[tnolet]

Question: Auto refreshing installation tokens

[tnolet]

@gr2m this is awesome, I'm testing this right now. Will update

[tnolet]

@gr2m yes, this issue seems 100% resolved since last week. I'm a bit swamped with work to create PR so I copy / pasted our production code down here.

Some notes:

• we explicitly set the TTL in Redis. We set it conservatively at 30 minutes. Not setting an explicit TTL will serve a stale token and trigger a 401 on the GitHub API.
• we load in a GitHub private key, some folks might not need this, some will.
• we added an explicit retry strategy

const config = require('config')
const { Octokit } = require('@octokit/rest')
const { createAppAuth } = require('@octokit/auth-app')
const { promisify } = require('util')
const redis = require('redis')

const RETRY_TIME = 1000 * 30
const RETRY_ATTEMPTS = 5
const TOKEN_TTL_SECONDS = 60 * 30 // half hour, GitHub tokens expires after 1 hour

function redisRetryStrategy (options) {
  if (options.error && options.error.code === 'ECONNREFUSED') {
    return new Error('The server refused the connection')
  }
  if (options.total_retry_time > RETRY_TIME) {
    return new Error('Retry time exhausted')
  }
  if (options.attempt > RETRY_ATTEMPTS) {
    return undefined
  }
  // Retry with backoff
  return Math.min(options.attempt * 100, 3000)
}

const redisClient = redis.createClient({
  url: config.redisUrl,
  retry_strategy: redisRetryStrategy
})

const get = promisify(redisClient.get).bind(redisClient)
const setex = promisify(redisClient.setex).bind(redisClient)

// We need to take care of expiring the tokens because we use our own Redis cache
const setWithTTL = (key, token) => {
  return setex(key, TOKEN_TTL_SECONDS, token)
}

const OctokitWithAppAuth = Octokit.defaults((options) => {
  return {
    authStrategy: createAppAuth,
    auth: Object.assign({
      id: config.github.githubAppId,
      privateKey: config.github.githubAppPrivateKey.replace(/\\n/g, '\n'),
      cache: { get, set: setWithTTL }
    }, options.auth)
  }
})

[gr2m]

My/our expectation was that tokens are cached (we can see this happening correctly) and refreshed automatically after they expire.

Yes, that is correct. Do you see the 401s around the 1h mark? Can you share a minimal code example that reproduces the problem?

[tnolet]

My/our expectation was that tokens are cached (we can see this happening correctly) and refreshed automatically after they expire.

Yes, that is correct. Do you see the 401s around the 1h mark? Can you share a minimal code example that reproduces the problem?

Hey @gr2m seems that way. We have some more heavy users of our service and they are hitting this issue. We are in the middle of triaging and putting in a fix. Timeline is as follows:

• We spin of jobs that interact with a specific customer's GitHub app installation.
• We created more threads / workers on our deamon to deal with throughput. Obviously, the in memory LRU cache is not shared.
• We put in Redis as a cache replacement.

const debug = require('debug')('checkly:github-service')
const config = require('config')
const moment = require('moment')
const crypto = require('crypto')
const { Octokit } = require('@octokit/rest')
const { createAppAuth } = require('@octokit/auth-app')
const { promisify } = require('util')
const redis = require('redis')

const RETRY_TIME = 1000 * 30
const RETRY_ATTEMPTS = 5

function retryStrategy (options) {
  if (options.error && options.error.code === 'ECONNREFUSED') {
    return new Error('The server refused the connection')
  }
  if (options.total_retry_time > RETRY_TIME) {
    return new Error('Retry time exhausted')
  }
  if (options.attempt > RETRY_ATTEMPTS) {
    return undefined
  }
  // retry with backoff
  return Math.min(options.attempt * 100, 3000)
}

const redisClient = redis.createClient({
  url: config.redisUrl,
  prefix: 'checkly:github:installations:',
  retry_strategy: retryStrategy
})

const get = promisify(redisClient.get).bind(redisClient)
const set = promisify(redisClient.set).bind(redisClient)

const auth = createAppAuth({
  id: config.github.githubAppId,
  privateKey: config.github.githubAppPrivateKey.replace(/\\n/g, '\n')
  cache: { get, set }
})

• the Redis cache returns the stale tokens after — I guess — 1 hour.
• the expectation is that the cache is ignorant of the refresh cycle. It just store whatever.

We are now going back to 1 worker, no Redis and the following explicit refresh:

      let { token, expiresAt } = await auth({ type: 'installation', installationId })
      debug(`Fetched token: ${token}. expires: ${expiresAt}`)

      if (moment().isAfter(expiresAt)) {
        const { token: newToken } = await auth({ type: 'installation', installationId, refresh: true })
        token = newToken
      }

      return new Octokit({ auth: `token ${token}` })

[gr2m]

Instead of using @octokit/auth-app to retrieve the token and then pass it into @octokit/rest, you can do this:

const OctokitWithAppAuth = Octokit.defaults({
  authStrategy: createAppAuth,
  id: config.github.githubAppId,
  privateKey: config.github.githubAppPrivateKey.replace(/\\n/g, '\n')
  cache: { get, set }
})

And then

const octokit = new OctokitWithAppAuth({auth: { installationId })

That way the creation and refresh will directly hook into the request lifecyle and you don't need to worry about it. The problem with new Octokit({ auth: token ${token} }) is that the token you pass it is static. And if you use the octokit instance for several requests, it might still be valid in the beginning but expire before all requests are done. That will not happen if you use the authStrategy option.

[tnolet]

@gr2m We will try that. We also had some reports from some developer SaaS-es that use the GitHub API heavily that the 401 errors are "kinda known" and should just be retried. Even with fresh tokens.

[gr2m]

Hmm first time I hear of it, at least in that context.

What happens sometimes are 401s shortly after you create the installation access tokens. This is caused by a racing condition when you send a GET/HEAD request right after creating a new installation access token. We account for this case now, see #85

[tnolet]

@gr2m o wow. That 100% confirms the anecdata I got from one of our larger customers (you can figure out which one by visiting our landing page @ checklyhq.com). I will follow your hints and see how we can integrate this with Redis as a store, as we do no need the extra runners due to the longer run times of our jobs.

[gr2m]

I'd love to share an example using a redis backend in the README.md. I’m sure many folks would be interested in that

[tnolet]

@gr2m we will share!

Still hitting a snag with your suggested solution. We are on:

    "@octokit/auth-app": "^2.4.8",
    "@octokit/rest": "^18.0.1",

We do:

// imports etc.

const OctokitWithAppAuth = Octokit.defaults({
  authStrategy: createAppAuth,
  id: config.github.githubAppId,
  privateKey: config.github.githubAppPrivateKey.replace(/\\n/g, '\n'),
  cache: { get, set }
})

function  getInstallationClient (installationId) {
    return new OctokitWithAppAuth({ auth: { installationId } })
  }

this hits a JWT error

Error: secretOrPrivateKey must have a value

[gr2m]

id and privateKey are auth.* options. Try

const OctokitWithAppAuth = Octokit.defaults({
  authStrategy: createAppAuth,
  auth: {
    id: config.github.githubAppId,
    privateKey: config.github.githubAppPrivateKey.replace(/\\n/g, '\n')
  },
  cache: { get, set }
})

[tnolet]

@gr2m found that one, still hitting this error. I think we are 99% there...

in a nut shell:

const OctokitWithAppAuth = Octokit.defaults({
  authStrategy: createAppAuth,
  auth: {
    id: config.github.githubAppId,
    privateKey: config.github.githubAppPrivateKey.replace(/\\n/g, '\n')
  },
  cache: { get, set }
})


  async function createCheckRunQueued (options) {
    const installationId = <some id>
    const octokit = new OctokitWithAppAuth({ auth: { installationId } })
    return octokit.checks.create({
      repo: options.repo,
      owner: options.owner,
      name: options.name,
      head_sha: options.sha,
      status: 'queued',
      started_at: moment().toISOString()
    })
  }

returns the Error: secretOrPrivateKey must have a value

[gr2m]

I'm sorry, my fault. I thought I implemented deep merging of options with defaults, but I ended up implementing a callback syntax instead, because deep merging has all kinds of nasty edge cases and would noticeably increase the bundle sice, so I decided to leave it up to the user by implementing a callback API for Octokit.defaults((options) => /* ... */) instead, see octokit/core.js#108

I've implemented an example here:
https://runkit.com/gr2m/octokit-auth-app-js-117/1.0.0

It's documented in the @octokit/core README, but not the @octokit/rest documentation yet:
https://github.com/octokit/core.js#defaults

[tnolet]

@gr2m thanks, that totally solves the secretOrPrivateKey issue.

Interestingly, I'm now back at the tokens not being cached. Even without Redis.
Is the LRU cache created on each call to new Octokit() or at the require level?

[gr2m]

each new Octokit() ... which I agree is not ideal in that case. But if you have an external cache store then it should work, that's what it's for

[tnolet]

@gr2m ok cool, was just playing around with it. I think the cache property should also be inside the auth object, e.g. this works and returns cached tokens using Redis.

const OctokitWithAppAuth = Octokit.defaults((options) => {
  return {
    authStrategy: createAppAuth,
    auth: Object.assign({
      id: config.github.githubAppId,
      privateKey: config.github.githubAppPrivateKey.replace(/\\n/g, '\n'),
      cache: { get, set }
    }, options.auth)
  }
})

The crux is, does it refresh the tokens when they are are stale when used in this contruction

    return new OctokitWithAppAuth({ auth: { type: 'installation', installationId } })

I think we'll find out tomorrow (getting late here in Berlin...).

My hat is of to your support @gr2m. Absolutely super. 💕

[gr2m]

I think the cache property should also be inside the auth object

Yes 🤦 sorry I missed that

Gute Nacht! 🐻🇩🇪

[gr2m]

When I investigated your problem, I've found a bug when using an external cache and the auth.hook API that Octokit is utilizing internally. Make sure to update @octokit/auth-app to v2.4.9 before continuing to test

[gr2m]

Hey @tnolet is the issue resolved for you?

Also would really, really appreciate a pull request that adds a usage example with a Redis-backed cache. I could use that right now for the upcoming upcoming probot release (probot/probot#1274). In Probot we already support a Redis backend for throttling requests in a cluster deployment, we could just re-use the same redis configuration for a Redis backed installation access token caching

[gr2m]

This is great, thank you!