Skip to content

Commit

Permalink
Send better user-agent values (and got config changes) (#7309)
Browse files Browse the repository at this point in the history
* expose fetchLimitBytes/userAgent in got-config module

* export a function not a factory

* send better user-agent values

- add userAgentBase setting
- send short SHA in user agent on heroku
- set a version (tag or short SHA) in Dockefile and use
  it to report server version in UA for docker users

* add a comment explaining fileSize

Co-authored-by: repo-ranger[bot] <39074581+repo-ranger[bot]@users.noreply.github.com>
  • Loading branch information
chris48s and repo-ranger[bot] authored Nov 25, 2021
1 parent 95a439a commit 99bffd3
Show file tree
Hide file tree
Showing 22 changed files with 113 additions and 54 deletions.
2 changes: 2 additions & 0 deletions .github/workflows/build-docker-image.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,5 @@ jobs:
context: .
push: false
tags: shieldsio/shields:pr-validation
build-args: |
version=${GITHUB_SHA::7}
2 changes: 2 additions & 0 deletions .github/workflows/create-release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,5 @@ jobs:
context: .
push: true
tags: shieldsio/shields:server-${{ steps.date.outputs.date }}
build-args: |
version=server-${{ steps.date.outputs.date }}
2 changes: 2 additions & 0 deletions .github/workflows/publish-docker-next.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,5 @@ jobs:
context: .
push: true
tags: shieldsio/shields:next
build-args: |
version=${GITHUB_SHA::7}
4 changes: 4 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ RUN npm cache clean --force

# Use multi-stage build to reduce size
FROM node:16-alpine

ARG version=dev
ENV DOCKER_SHIELDS_VERSION=$version

# Run the server using production configs.
ENV NODE_ENV production

Expand Down
1 change: 1 addition & 0 deletions config/custom-environment-variables.yml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ public:
defaultCacheLengthSeconds: 'BADGE_MAX_AGE_SECONDS'

fetchLimit: 'FETCH_LIMIT'
userAgentBase: 'USER_AGENT_BASE'

requestTimeoutSeconds: 'REQUEST_TIMEOUT_SECONDS'
requestTimeoutMaxAgeSeconds: 'REQUEST_TIMEOUT_MAX_AGE_SECONDS'
Expand Down
1 change: 1 addition & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public:
handleInternalErrors: true

fetchLimit: '10MB'
userAgentBase: 'shields (self-hosted)'

requestTimeoutSeconds: 120
requestTimeoutMaxAgeSeconds: 30
Expand Down
8 changes: 3 additions & 5 deletions core/base-service/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
Deprecated,
} from './errors.js'
import { validateExample, transformExample } from './examples.js'
import { fetchFactory } from './got.js'
import { fetch } from './got.js'
import {
makeFullUrl,
assertValidRoute,
Expand Down Expand Up @@ -432,7 +432,7 @@ class BaseService {
},
serviceConfig
) {
const { cacheHeaders: cacheHeaderConfig, fetchLimitBytes } = serviceConfig
const { cacheHeaders: cacheHeaderConfig } = serviceConfig
const { regex, captureNames } = prepareRoute(this.route)
const queryParams = getQueryParamNames(this.route)

Expand All @@ -441,8 +441,6 @@ class BaseService {
ServiceClass: this,
})

const fetcher = fetchFactory(fetchLimitBytes)

camp.route(
regex,
handleRequest(cacheHeaderConfig, {
Expand All @@ -453,7 +451,7 @@ class BaseService {
const namedParams = namedParamsForMatch(captureNames, match, this)
const serviceData = await this.invoke(
{
requestFetcher: fetcher,
requestFetcher: fetch,
githubApiProvider,
librariesIoApiProvider,
metricHelper,
Expand Down
26 changes: 26 additions & 0 deletions core/base-service/got-config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import bytes from 'bytes'
import configModule from 'config'
import Joi from 'joi'
import { fileSize } from '../../services/validators.js'

const schema = Joi.object({
fetchLimit: fileSize,
userAgentBase: Joi.string().required(),
}).required()
const config = configModule.util.toObject()
const publicConfig = Joi.attempt(config.public, schema, { allowUnknown: true })

const fetchLimitBytes = bytes(publicConfig.fetchLimit)

function getUserAgent(userAgentBase = publicConfig.userAgentBase) {
let version = 'dev'
if (process.env.DOCKER_SHIELDS_VERSION) {
version = process.env.DOCKER_SHIELDS_VERSION
}
if (process.env.HEROKU_SLUG_COMMIT) {
version = process.env.HEROKU_SLUG_COMMIT.substring(0, 7)
}
return `${userAgentBase}/${version}`
}

export { fetchLimitBytes, getUserAgent }
27 changes: 27 additions & 0 deletions core/base-service/got-config.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { expect } from 'chai'
import { getUserAgent } from './got-config.js'

describe('getUserAgent function', function () {
afterEach(function () {
delete process.env.HEROKU_SLUG_COMMIT
delete process.env.DOCKER_SHIELDS_VERSION
})

it('uses the default userAgentBase', function () {
expect(getUserAgent()).to.equal('shields (self-hosted)/dev')
})

it('applies custom userAgentBase', function () {
expect(getUserAgent('custom')).to.equal('custom/dev')
})

it('uses short commit SHA from HEROKU_SLUG_COMMIT if available', function () {
process.env.HEROKU_SLUG_COMMIT = '92090bd44742a5fac03bcb117002088fc7485834'
expect(getUserAgent('custom')).to.equal('custom/92090bd')
})

it('uses short commit SHA from DOCKER_SHIELDS_VERSION if available', function () {
process.env.DOCKER_SHIELDS_VERSION = 'server-2021-11-22'
expect(getUserAgent('custom')).to.equal('custom/server-2021-11-22')
})
})
13 changes: 9 additions & 4 deletions core/base-service/got.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import got from 'got'
import { Inaccessible, InvalidResponse } from './errors.js'
import {
fetchLimitBytes as fetchLimitBytesDefault,
getUserAgent,
} from './got-config.js'

const userAgent = 'Shields.io/2003a'
const userAgent = getUserAgent()

async function sendRequest(gotWrapper, url, options) {
const gotOptions = Object.assign({}, options)
Expand All @@ -22,8 +26,7 @@ async function sendRequest(gotWrapper, url, options) {
}
}

const TEN_MB = 10485760
function fetchFactory(fetchLimitBytes = TEN_MB) {
function _fetchFactory(fetchLimitBytes = fetchLimitBytesDefault) {
const gotWithLimit = got.extend({
handlers: [
(options, next) => {
Expand Down Expand Up @@ -52,4 +55,6 @@ function fetchFactory(fetchLimitBytes = TEN_MB) {
return sendRequest.bind(sendRequest, gotWithLimit)
}

export { fetchFactory, userAgent }
const fetch = _fetchFactory()

export { fetch, _fetchFactory }
14 changes: 7 additions & 7 deletions core/base-service/got.spec.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { expect } from 'chai'
import nock from 'nock'
import { fetchFactory } from './got.js'
import { _fetchFactory } from './got.js'
import { Inaccessible, InvalidResponse } from './errors.js'

describe('got wrapper', function () {
Expand All @@ -9,7 +9,7 @@ describe('got wrapper', function () {
.get('/foo/bar')
.once()
.reply(200, 'x'.repeat(100))
const sendRequest = fetchFactory(100)
const sendRequest = _fetchFactory(100)
const { res } = await sendRequest('https://www.google.com/foo/bar')
expect(res.statusCode).to.equal(200)
})
Expand All @@ -19,15 +19,15 @@ describe('got wrapper', function () {
.get('/foo/bar')
.once()
.reply(200, 'x'.repeat(101))
const sendRequest = fetchFactory(100)
const sendRequest = _fetchFactory(100)
return expect(
sendRequest('https://www.google.com/foo/bar')
).to.be.rejectedWith(InvalidResponse, 'Maximum response size exceeded')
})

it('should throw an Inaccessible error if the request throws a (non-HTTP) error', async function () {
nock('https://www.google.com').get('/foo/bar').replyWithError('oh no')
const sendRequest = fetchFactory(1024)
const sendRequest = _fetchFactory(1024)
return expect(
sendRequest('https://www.google.com/foo/bar')
).to.be.rejectedWith(Inaccessible, 'oh no')
Expand All @@ -36,7 +36,7 @@ describe('got wrapper', function () {
it('should throw an Inaccessible error if the host can not be accessed', async function () {
this.timeout(5000)
nock.disableNetConnect()
const sendRequest = fetchFactory(1024)
const sendRequest = _fetchFactory(1024)
return expect(
sendRequest('https://www.google.com/foo/bar')
).to.be.rejectedWith(
Expand All @@ -49,14 +49,14 @@ describe('got wrapper', function () {
nock('https://www.google.com', {
reqheaders: {
'user-agent': function (agent) {
return agent.startsWith('Shields.io')
return agent.startsWith('shields (self-hosted)')
},
},
})
.get('/foo/bar')
.once()
.reply(200)
const sendRequest = fetchFactory(1024)
const sendRequest = _fetchFactory(1024)
await sendRequest('https://www.google.com/foo/bar')
})

Expand Down
5 changes: 2 additions & 3 deletions core/legacy/regular-update.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@
*/

import { InvalidResponse } from '../base-service/errors.js'
import { fetchFactory } from '../../core/base-service/got.js'
import { fetch } from '../../core/base-service/got.js'
import checkErrorResponse from '../../core/base-service/check-error-response.js'
const fetcher = fetchFactory()

// Map from URL to { timestamp: last fetch time, data: data }.
let regularUpdateCache = Object.create(null)
Expand All @@ -28,7 +27,7 @@ async function regularUpdate({
json = true,
scraper = buffer => buffer,
options = {},
requestFetcher = fetcher,
requestFetcher = fetch,
}) {
const timestamp = Date.now()
const cached = regularUpdateCache[url]
Expand Down
7 changes: 3 additions & 4 deletions core/server/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import path from 'path'
import url, { fileURLToPath } from 'url'
import { bootstrap } from 'global-agent'
import cloudflareMiddleware from 'cloudflare-middleware'
import bytes from 'bytes'
import Camp from '@shields_io/camp'
import originalJoi from 'joi'
import makeBadge from '../../badge-maker/lib/make-badge.js'
Expand All @@ -18,7 +17,7 @@ import { makeSend } from '../base-service/legacy-result-sender.js'
import { handleRequest } from '../base-service/legacy-request-handler.js'
import { clearRegularUpdateCache } from '../legacy/regular-update.js'
import { rasterRedirectUrl } from '../badge-urls/make-badge-url.js'
import { nonNegativeInteger } from '../../services/validators.js'
import { fileSize, nonNegativeInteger } from '../../services/validators.js'
import log from './log.js'
import PrometheusMetrics from './prometheus-metrics.js'
import InfluxMetrics from './influx-metrics.js'
Expand Down Expand Up @@ -143,7 +142,8 @@ const publicConfigSchema = Joi.object({
}).required(),
cacheHeaders: { defaultCacheLengthSeconds: nonNegativeInteger },
handleInternalErrors: Joi.boolean().required(),
fetchLimit: Joi.string().regex(/^[0-9]+(b|kb|mb|gb|tb)$/i),
fetchLimit: fileSize,
userAgentBase: Joi.string().required(),
requestTimeoutSeconds: nonNegativeInteger,
requestTimeoutMaxAgeSeconds: nonNegativeInteger,
documentRoot: Joi.string().default(
Expand Down Expand Up @@ -433,7 +433,6 @@ class Server {
{
handleInternalErrors: config.public.handleInternalErrors,
cacheHeaders: config.public.cacheHeaders,
fetchLimitBytes: bytes(config.public.fetchLimit),
rasterUrl: config.public.rasterUrl,
private: config.private,
public: config.public,
Expand Down
9 changes: 2 additions & 7 deletions services/github/auth/acceptor.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import queryString from 'query-string'
import { fetchFactory } from '../../../core/base-service/got.js'
import { fetch } from '../../../core/base-service/got.js'
import log from '../../../core/server/log.js'

const requestFetcher = fetchFactory()

function setRoutes({ server, authHelper, onTokenAccepted }) {
const baseUrl = process.env.GATSBY_BASE_URL || 'https://img.shields.io'

Expand Down Expand Up @@ -48,10 +46,7 @@ function setRoutes({ server, authHelper, onTokenAccepted }) {

let resp
try {
resp = await requestFetcher(
'https://github.com/login/oauth/access_token',
options
)
resp = await fetch('https://github.com/login/oauth/access_token', options)
} catch (e) {
return end('The connection to GitHub failed.')
}
Expand Down
11 changes: 3 additions & 8 deletions services/github/github-api-provider.integration.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import { expect } from 'chai'
import config from 'config'
import { fetchFactory } from '../../core/base-service/got.js'
import { fetch } from '../../core/base-service/got.js'
import GithubApiProvider from './github-api-provider.js'
const requestFetcher = fetchFactory()

describe('Github API provider', function () {
const baseUrl = process.env.GITHUB_URL || 'https://api.github.com'
Expand Down Expand Up @@ -31,11 +30,7 @@ describe('Github API provider', function () {
it('should be able to run 10 requests', async function () {
this.timeout('20s')
for (let i = 0; i < 10; ++i) {
await githubApiProvider.fetch(
requestFetcher,
'/repos/rust-lang/rust',
{}
)
await githubApiProvider.fetch(fetch, '/repos/rust-lang/rust', {})
}
})
})
Expand All @@ -54,7 +49,7 @@ describe('Github API provider', function () {
const headers = []
async function performOneRequest() {
const { res } = await githubApiProvider.fetch(
requestFetcher,
fetch,
'/repos/rust-lang/rust',
{}
)
Expand Down
4 changes: 3 additions & 1 deletion services/github/github-api-provider.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import Joi from 'joi'
import log from '../../core/server/log.js'
import { TokenPool } from '../../core/token-pooling/token-pool.js'
import { userAgent } from '../../core/base-service/got.js'
import { getUserAgent } from '../../core/base-service/got-config.js'
import { nonNegativeInteger } from '../validators.js'
import { ImproperlyConfigured } from '../index.js'

const userAgent = getUserAgent()

const headerSchema = Joi.object({
'x-ratelimit-limit': nonNegativeInteger,
'x-ratelimit-remaining': nonNegativeInteger,
Expand Down
2 changes: 1 addition & 1 deletion services/github/github-auth-service.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ describe('GithubAuthV3Service', function () {
'https://github-api.example.com/repos/badges/shields/check-runs',
{
headers: {
'User-Agent': 'Shields.io/2003a',
'User-Agent': 'shields (self-hosted)/dev',
Accept: 'application/vnd.github.antiope-preview+json',
Authorization: 'token undefined',
},
Expand Down
4 changes: 3 additions & 1 deletion services/librariesio/librariesio-api-provider.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { ImproperlyConfigured } from '../index.js'
import log from '../../core/server/log.js'
import { TokenPool } from '../../core/token-pooling/token-pool.js'
import { userAgent } from '../../core/base-service/got.js'
import { getUserAgent } from '../../core/base-service/got-config.js'

const userAgent = getUserAgent()

// Provides an interface to the Libraries.io API.
export default class LibrariesIoApiProvider {
Expand Down
7 changes: 2 additions & 5 deletions services/php-version.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* using the algorithm followed by Composer (see
* https://getcomposer.org/doc/04-schema.md#version).
*/
import { fetchFactory } from '../core/base-service/got.js'
import { fetch } from '../core/base-service/got.js'
import { regularUpdate } from '../core/legacy/regular-update.js'
import { listCompare } from './version.js'
import { omitv } from './text-formatters.js'
Expand Down Expand Up @@ -232,10 +232,7 @@ async function getPhpReleases(githubApiProvider) {
.map(tag => tag.ref.match(/^refs\/tags\/php-(\d+\.\d+)\.\d+$/)[1])
)
),
requestFetcher: githubApiProvider.fetch.bind(
githubApiProvider,
fetchFactory()
),
requestFetcher: githubApiProvider.fetch.bind(githubApiProvider, fetch),
})
}

Expand Down
Loading

0 comments on commit 99bffd3

Please sign in to comment.