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.

Sign up for GitHub

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

Jump to bottom

fix: changes to metadata.cloud.project.* fields collected for GCP #3618

Merged
merged 1 commit into from
Sep 13, 2023
Merged

fix: changes to metadata.cloud.project.* fields collected for GCP #3618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions CHANGELOG.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,28 @@ Notes:

See the <<upgrade-to-v4>> guide.


==== Unreleased

[float]
===== Breaking changes

[float]
===== Features

[float]
===== Bug fixes

[float]
===== Chores

- Changes to cloud metadata collection for Google Cloud (GCP). Most notably
the `cloud.project.id` field is now the `project-id` from
https://cloud.google.com/compute/docs/metadata/default-metadata-values#project_metadata
rather than the `numeric-project-id`. This matches the value produced by
Elastic Beats (like filebeat). {issues}3614[#3614]


[[release-notes-4.0.0]]
==== 4.0.0 - 2023/09/07

Expand Down
101 changes: 40 additions & 61 deletions lib/cloud-metadata/gcp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,13 @@
*/

'use strict';

const URL = require('url').URL;
const JSONBigInt = require('json-bigint');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer note: This is the same lib that OTel is indirectly using. They use the Google-provided "gcp-metadata" package. I didn't want to use that because it adds yet another HTTP lib that we don't particularly need (gaxios, a wrapper around node-fetch that provides an interface like axios).

The "bignumber.js" dep (used by json-bigint) adds about 1MB to the install size, I think, which is unfortunate. We could get into shenanigans like using a regex to parse out the "id". :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking the metadata docs and also the JSON fixture you added I think we can be sure that there are only two numeric values in the response: numericProjectId and id (within instance)

{
  "instance": {
    "attributes": {},
    "id": 5737554347302044216,
    ...
  },
  "project": {
    "attributes": {
      ...
    },
    "numericProjectId": 523926462582,
    "projectId": "acme-eng"
    ...
  }
}

Since it is so specific I think we can opt-in to a custom solution to capture that info. You mentioned RegExp and maybe is the way to go using capture groups with String.match. We can capture the numeric values with something similar to the snippet below and use JSON.parse for the rest

const respBody = '...' // The response text with all the metadata

const instanceIdRegExp = /"id":(\d+),/;
const projectIdRegExp = /"numericProjectId":(\d+),/;

const instanceIdMatch = respBody.match(instanceIdRegExp)
const projectIdMatch = respBody.match(projectIdRegExp)

// The match is already an String so no need to convert
const instanceId = instanceIdMatch && instanceMatch[1];
const projectId = projectIdMatch && projectIdMatch[1];

NOTE: regexes should be reviewed since they do not cover the case of being the last property of the object

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if numeric attributes can be added. E.g. could a user have a project.attributes.id: <some numeric>. I think regex matching would still be fine... we'd just assume it is the first "id":... match.

Playing with this a little bit, I added metadata with gcloud compute project-info --project "[REDACTED]" add-metadata --metadata=trentmfoo=42 and it at least defaults to setting metadata values as a string:

      {
        "key": "trentmfoo",
        "value": "42"
      }

So I think it would be fine.

@david-luna Do you have a preference either way? (Regex or using json-bigint?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

David and I discussed. The added "bignumber.js" dep is a bit of size, but still relatively low compared to a number of other deps, so we think it is better to have the clearer implementation and not get into possibly brittle regex usage just to save some install size.

const { httpRequest } = require('../http-request');

const DEFAULT_BASE_URL = new URL('/', 'http://metadata.google.internal:80');

/**
* Checks for metadata server then fetches data
*
Expand Down Expand Up @@ -44,11 +48,25 @@ function getMetadataGcp(
});

res.on('end', function (data) {
if (res.statusCode !== 200) {
logger.debug('gcp metadata: unexpected statusCode: %s', res.statusCode);
cb(
new Error(
'error fetching gcp metadata: unexpected statusCode: ' +
res.statusCode,
),
);
return;
}
// Note: We could also guard on the response having the
// 'Metadata-Flavor: Google' header as done by:
// https://github.com/googleapis/gcp-metadata/blob/v6.0.0/src/index.ts#L109-L112

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer note: Add a sanity guard on the response from the Google metadata API. If it isn't a 200, we shouldn't get into parsing it. This is my justification for dropping some of the (unnecessary, IMO) defensive coding and testing cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my justification for dropping some of the (unnecessary, IMO) defensive coding and testing cases.

I agree that we can assume safely that a 200 status comes with the proper metadata described in their docs. Also it simplifies the code so IMO this is a good change :)

let result;
try {
result = formatMetadataStringIntoObject(finalData.join(''));
} catch (err) {
logger.trace(
logger.debug(
'gcp metadata server responded, but there was an ' +
'error parsing the result: %o',
err,
Expand Down Expand Up @@ -78,76 +96,37 @@ function getMetadataGcp(
/**
* Builds metadata object
*
* Takes the response from a /computeMetadata/v1/?recursive=true
* service request and formats it into the cloud metadata object
* Convert a GCP Cloud Engine VM metadata response
* (https://cloud.google.com/compute/docs/metadata/default-metadata-values)
* to the APM intake cloud metadata object
* (https://github.com/elastic/apm/blob/main/specs/agents/metadata.md#gcp-metadata).
*
* See discussion about big int values here:
* https://github.com/googleapis/gcp-metadata#take-care-with-large-number-valued-properties
* This implementation is using the same 'json-bigint' library as 'gcp-metadata'.
*/
function formatMetadataStringIntoObject(string) {
const data = JSON.parse(string);
// cast string manipulation fields as strings "just in case"
if (data.instance) {
data.instance.machineType = String(data.instance.machineType);
data.instance.zone = String(data.instance.zone);
}
const data = JSONBigInt.parse(string);

// E.g., 'projects/513326162531/zones/us-west1-b' -> 'us-west1-b'
const az = data.instance.zone.split('/').pop();

const metadata = {
availability_zone: null,
region: null,
provider: 'gcp',
instance: {
id: null,
id: data.instance.id.toString(), // We expect this to be a BigInt.
name: data.instance.name,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer note: This function has been simplified (IMO). The instance.name had been missed before (or perhaps the APM spec changed after original impl?). The data.instance.id is the interesting BigInt case.

},
machine: {
type: null,
},
provider: null,
project: {
id: null,
name: null,
id: data.project.projectId,
},
availability_zone: az,
region: az.slice(0, az.lastIndexOf('-')), // 'us-west1-b' -> 'us-west1'
machine: {
type: data.instance.machineType.split('/').pop(),
},
};

metadata.availability_zone = null;
metadata.region = null;
if (data.instance && data.instance.zone) {
// `projects/513326162531/zones/us-west1-b` manipuated into
// `us-west1-b`, and then `us-west1`
const regionWithZone = data.instance.zone.split('/').pop();
const parts = regionWithZone.split('-');
parts.pop();
metadata.region = parts.join('-');
metadata.availability_zone = regionWithZone;
}

if (data.instance) {
metadata.instance = {
id: String(data.instance.id),
};

metadata.machine = {
type: String(data.instance.machineType.split('/').pop()),
};
} else {
metadata.instance = {
id: null,
};

metadata.machine = {
type: null,
};
}

metadata.provider = 'gcp';

if (data.project) {
metadata.project = {
id: String(data.project.numericProjectId),
name: String(data.project.projectId),
};
} else {
metadata.project = {
id: null,
name: null,
};
}
return metadata;
}

Expand Down
21 changes: 18 additions & 3 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@
"fast-stream-to-buffer": "^1.0.0",
"http-headers": "^3.0.2",
"import-in-the-middle": "1.4.2",
"json-bigint": "^1.0.0",
"lru-cache": "^10.0.1",
"measured-reporting": "^1.51.1",
"module-details-from-path": "^1.0.3",
Expand Down
123 changes: 3 additions & 120 deletions test/cloud-metadata/_fixtures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,128 +64,11 @@ module.exports = {
},
],
gcp: [
{
name: 'gcp does not crash on empty response',
response: {},
},
{
name: 'gcp unexpected string fixture',
response: {
instance: {
zone: 123456,
machineType: 123456,
},
},
},
{
name: 'default gcp fixture',
response: {
instance: {
attributes: {},
cpuPlatform: 'Intel Broadwell',
description: '',
disks: [
{
deviceName: 'username-temp-delete-me-cloud-metadata',
index: 0,
interface: 'SCSI',
mode: 'READ_WRITE',
type: 'PERSISTENT',
},
],
guestAttributes: {},
hostname:
'username-temp-delete-me-cloud-metadata.c.elastic-apm.internal',
id: 7684572792595385000,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer note: I've changed this test fixture to use an unparsed actual response on a test GCP VM. The problem with having an already parsed object, is that it accidentally included the BigInt loss of precision. That 7684572792595385000 is wrong. The actual instance.is was some 7684572792595385??? value with something other than zeros. It was just not noticed originally.

image:
'projects/debian-cloud/global/images/debian-10-buster-v20201216',
legacyEndpointAccess: {
0.1: 0,
v1beta1: 0,
},
licenses: [
{
id: '5543610867827062957',
},
],
machineType: 'projects/513326162531/machineTypes/e2-micro',
maintenanceEvent: 'NONE',
name: 'username-temp-delete-me-cloud-metadata',
networkInterfaces: [
{
accessConfigs: [
{
externalIp: '35.247.28.180',
type: 'ONE_TO_ONE_NAT',
},
],
dnsServers: ['169.254.169.254'],
forwardedIps: [],
gateway: '10.138.0.1',
ip: '10.138.0.2',
ipAliases: [],
mac: '42:01:0a:8a:00:02',
mtu: 1460,
network: 'projects/513326162531/networks/default',
subnetmask: '255.255.240.0',
targetInstanceIps: [],
},
],
preempted: 'FALSE',
remainingCpuTime: -1,
scheduling: {
automaticRestart: 'TRUE',
onHostMaintenance: 'MIGRATE',
preemptible: 'FALSE',
},
serviceAccounts: {
'513326162531-compute@developer.gserviceaccount.com': {
aliases: ['default'],
email: '513326162531-compute@developer.gserviceaccount.com',
scopes: [
'https://www.googleapis.com/auth/devstorage.read_only',
'https://www.googleapis.com/auth/logging.write',
'https://www.googleapis.com/auth/monitoring.write',
'https://www.googleapis.com/auth/servicecontrol',
'https://www.googleapis.com/auth/service.management.readonly',
'https://www.googleapis.com/auth/trace.append',
],
},
default: {
aliases: ['default'],
email: '513326162531-compute@developer.gserviceaccount.com',
scopes: [
'https://www.googleapis.com/auth/devstorage.read_only',
'https://www.googleapis.com/auth/logging.write',
'https://www.googleapis.com/auth/monitoring.write',
'https://www.googleapis.com/auth/servicecontrol',
'https://www.googleapis.com/auth/service.management.readonly',
'https://www.googleapis.com/auth/trace.append',
],
},
},
tags: ['http-server'],
virtualClock: {
driftToken: '0',
},
zone: 'projects/513326162531/zones/us-west1-b',
},
oslogin: {
authenticate: {
sessions: {},
},
},
project: {
attributes: {
'gke-smith-de35da35-secondary-ranges':
'services:default:default:gke-smith-services-de35da35,pods:default:default:gke-smith-pods-de35da35',
'serial-port-enable': '1',
'ssh-keys': '... public keys snipped ...',
},
numericProjectId: 513326162531,
projectId: 'elastic-apm',
},
},
// This is an actual response from a dev VM, edited slightly for size and privacy.
response:
'{"instance":{"attributes":{},"cpuPlatform":"Intel Broadwell","description":"","disks":[{"deviceName":"trentm-play-vm0","index":0,"interface":"SCSI","mode":"READ_WRITE","type":"PERSISTENT-BALANCED"}],"guestAttributes":{},"hostname":"trentm-play-vm0.c.acme-eng.internal","id":5737554347302044216,"image":"projects/debian-cloud/global/images/debian-11-bullseye-v20230814","licenses":[{"id":"3853522013536123851"}],"machineType":"projects/523926462582/machineTypes/e2-medium","maintenanceEvent":"NONE","name":"trentm-play-vm0","networkInterfaces":[{"accessConfigs":[{"externalIp":"33.162.212.82","type":"ONE_TO_ONE_NAT"}],"dnsServers":["169.254.169.254"],"forwardedIps":[],"gateway":"10.138.0.1","ip":"10.138.0.7","ipAliases":[],"mac":"42:01:0a:9a:0e:27","mtu":1460,"network":"projects/523926462582/networks/default","subnetmask":"255.255.240.0","targetInstanceIps":[]}],"preempted":"FALSE","remainingCpuTime":-1,"scheduling":{"automaticRestart":"TRUE","onHostMaintenance":"MIGRATE","preemptible":"FALSE"},"serviceAccounts":{"523926462582-compute@developer.gserviceaccount.com":{"aliases":["default"],"email":"523926462582-compute@developer.gserviceaccount.com","scopes":["https://www.googleapis.com/auth/devstorage.read_only","https://www.googleapis.com/auth/logging.write","https://www.googleapis.com/auth/monitoring.write","https://www.googleapis.com/auth/servicecontrol","https://www.googleapis.com/auth/service.management.readonly","https://www.googleapis.com/auth/trace.append"]},"default":{"aliases":["default"],"email":"523926462582-compute@developer.gserviceaccount.com","scopes":["https://www.googleapis.com/auth/devstorage.read_only","https://www.googleapis.com/auth/logging.write","https://www.googleapis.com/auth/monitoring.write","https://www.googleapis.com/auth/servicecontrol","https://www.googleapis.com/auth/service.management.readonly","https://www.googleapis.com/auth/trace.append"]}},"tags":[],"virtualClock":{"driftToken":"0"},"zone":"projects/523926462582/zones/us-west1-b"},"oslogin":{"authenticate":{"sessions":{}}},"project":{"attributes":{"gke-kk-dev-cluster-8264c0ad-secondary-ranges":"services:default:gke-kk-dev-cluster-subnet-8264c0ad:gke-kk-dev-cluster-services-8264c0ad,pods:default:gke-kk-dev-cluster-subnet-8264c0ad:gke-kk-dev-cluster-pods-8264c0ad","serial-port-enable":"1","ssh-keys":"[REDACTED]","sshKeys":"[REDACTED]"},"numericProjectId":523926462582,"projectId":"acme-eng"}}',
},
],
azure: [
Expand Down
8 changes: 6 additions & 2 deletions test/cloud-metadata/_lib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ function addGcpRoute(app, fixture) {
throw new Error('Metadata-Flavor: Google header required');
}

res.status(200);
res.set('Metadata-Flavor', 'Google');
res.set('Content-Type', 'application/json');
res.set('Server', 'Metadata Server for VM');
res.send(fixture.response);
});

Expand Down Expand Up @@ -205,7 +209,7 @@ function addRoutesToExpressApp(app, provider, fixture) {
function createTestServer(provider, fixtureName) {
const fixture = loadFixtureData(provider, fixtureName);
if (!fixture) {
throw new Error(`Unknown ${provider} fixtured named ${fixtureName}`);
throw new Error(`Unknown ${provider} fixture named ${fixtureName}`);
}
const app = express();
return addRoutesToExpressApp(app, provider, fixture);
Expand All @@ -223,7 +227,7 @@ function createTestServer(provider, fixtureName) {
function createSlowTestServer(provider, fixtureName) {
const fixture = loadFixtureData(provider, fixtureName);
if (!fixture) {
throw new Error(`Unknown ${provider} fixtured named ${fixtureName}`);
throw new Error(`Unknown ${provider} fixture named ${fixtureName}`);
}
const app = express();
return addSlowRoutesToExpressApp(app, provider, fixture);
Expand Down
Loading
Loading