Fetch metadata from GCE VM Metadata Server #837

LujieDuan · 2022-09-08T19:28:13Z

Description

This change introduces a new package in confgenerator called resourcedetector to detect metadata of the environment Ops Agent is running in. The detected metadata can then be used by receivers/processors. Right now we only have a detector for GCE environment; the detector is not currently used by any components; this will be used by the Prometheus receivers.

Related issue

b/244434005

How has this been tested?

This has been tested with unit tests and manual tests on GCE.
Integration tests for this is TBD.

Checklist:

Unit tests
- Unit tests do not apply.
- Unit tests have been added/modified and passed for this PR.
Integration tests
- Integration tests do not apply.
- Integration tests have been added/modified and passed for this PR.
Documentation
- This PR introduces no user visible changes.
- This PR introduces user visible changes and the corresponding documentation change has been made.
Minor version bump
- This PR introduces no new features.
- This PR introduces new features, and there is a separate PR to bump the minor version since the last release already.
- This PR bumps the version.

ridwanmsharif

Overall this looks pretty solid. There are a few transformations and nuances here around differences from how the gce detector in prometheus collects things (Compute engine API) and how we do (metadata server). We should add an integration test in addition to the unit tests in this PR to make sure the metadata attributes look like what we expect for the prometheus use case.

ridwanmsharif · 2022-09-09T15:01:44Z

confgenerator/resourcedetector/gce_detector.go

+)
+
+// Keep synced with the Prometheus repo
+const (


I would suggest keeping this detector generic and leave all the translation to the prometheus style labels to the prometheus receiver. This detector is useful for other components too. One such example is: b/219991181 | P3 | Allow LogName to be used in processors

ridwanmsharif · 2022-09-09T15:04:54Z

confgenerator/resourcedetector/gce_detector.go

+	then  func(GCEDataProvider, string) (string, string, error)
+}
+
+// List of single value labels (non-nested)


labels feels like a prometheus specific thing to me. It can get confusing especially since the metadata has its own labels too. Naming it instead to something like metadata or attributes might make it clearer.

ridwanmsharif · 2022-09-09T15:06:29Z

confgenerator/resourcedetector/gce_detector.go

+	gceLabelInstanceID:   GCEDataProvider.getInstanceID,
+	gceLabelInstanceName: GCEDataProvider.getInstanceName,
+	gceLabelTags:         GCEDataProvider.getTags,
+	gceLabelLabel:        GCEDataProvider.getLabel,


Is this a single label or nested? I thought it was a nested one but could be wrong.

ridwanmsharif · 2022-09-09T15:09:24Z

confgenerator/resourcedetector/gce_detector.go

+// The data provider interface for GCE environment
+// Implementation of this provider can use either the metadata server on VM,
+// or the cloud API
+type GCEDataProvider interface {


We should add some comments here around what is available in the detector now and what is not

ridwanmsharif · 2022-09-09T15:11:15Z

confgenerator/resourcedetector/detector.go

+
+type LabelValue string
+
+type Metalabels map[LabelName]LabelValue


The contract we expose should probably be more clear - instead of having map[string]string it could be a struct that has fields for all the attributes. This way it is part of the public interface what fields are available as part of the detector, instead of maintaining another set of key labels.

ridwanmsharif · 2022-09-09T15:12:31Z

confgenerator/resourcedetector/gce_metadata_provider.go

+// Also add `nic` to the name following the Prometheus's SD convention
+func (gmp *GCEMetadataProvider) getInterfaceIPv4(name string) (string, string, error) {
+	interface_name := strings.TrimSuffix(name, "/")
+	ipv4, err := gmp.client.Get(fmt.Sprintf("instance/network-interfaces/%s/ip", interface_name))


These are very interesting mutations we have to do. I'd leave it out of the detector package and add it when the prometheus receiver uses it. Only it cares about the Prometheus SD's conventions.

LujieDuan · 2022-09-12T15:47:02Z

confgenerator/resourcedetector/detector.go

+
+// EmptyDetector that returns an empty detector without any attributes
+// for non-recognized environments
+type EmptyDetector struct {


Not sure if it would be better to call this UnrecognizedPlatformDetector or something similar.

+1 for UnrecognizedPlatformDetector.

ridwanmsharif

This is looking very good! Left a couple comments

ridwanmsharif · 2022-09-12T17:39:23Z

confgenerator/resourcedetector/detector.go

+
+// EmptyDetector that returns an empty detector without any attributes
+// for non-recognized environments
+type EmptyDetector struct {


+1 for UnrecognizedPlatformDetector.

ridwanmsharif · 2022-09-12T17:49:48Z

confgenerator/resourcedetector/gce_detector.go

+
+// Nested attribute takes two steps: first to get the list of keys,
+// and then use the key to get individual values.
+type nestedAttribute struct {


This struct and interface seems pretty confusing to me. It feels simpler to have the provider do both the fetching of the keys and then aggregate the values rather than having the detector work with this interface.

In other words, if the provider provided getMetadata, getLabels and getLabels methods that each returned a map[string]string, the GCE detector wouldn't need to do any of this nesting and unnesting logic right? That would be contained in the implementation of the getMetadata, getLabels and getLabels methods. Let me know what you think of this approach.

ridwanmsharif · 2022-09-12T17:54:45Z

confgenerator/resourcedetector/gce_detector_test.go

+	}
+}
+
+func TestGettingDetectorOnGCE(t *testing.T) {


This feels like it belongs in the integration_test folder right?

Also you will be able to test a lot more of the fields there since we have the VM information when the integration test sets it up:

ops-agent/integration_test/gce/gce_testing.go

Lines 267 to 282 in e7aa72d

type VM struct {

Name string

Project string

Network string

Platform string

Zone string

MachineType string

ID int64

// The IP address to ssh/WinRM to. This is the external IP address, unless

// USE_INTERNAL_IP is set to 'true'. See comment on extractIPAddress() for

// rationale.

IPAddress string

// WindowsCredentials is only populated for Windows VMs.

WindowsCredentials *WindowsCredentials

AlreadyDeleted bool

}

See this as an example of what is used when setting up the environment for an integration test:

ops-agent/integration_test/ops_agent_test.go

Line 346 in e7aa72d

ctx, logger, vm := agents.CommonSetup(t, platform)

I don't know what the best way is to integration test this. Maybe the easiest way is to have a small go command line runner that hits your interface and dumps the metadata into a json file. The integration test can then read that from the VM and compare it to what it expects. Just a thought, figured its worth sharing. Looking forward to whatever you end up going with.

ridwanmsharif

LGTM. I'd get another reviewer however since we were pretty involved in this

confgenerator/resourcedetector/gce_metadata_provider.go

ridwanmsharif · 2022-09-15T21:48:14Z

confgenerator/resourcedetector/gce_detector.go

+}
+
+// GCEDetector implements the Detector interface and provide attributes of the VM when on GCE
+type GCEDetector struct {


[nit] naming feels a bit off to me. GCEDetector.Project sounds like the project of the detector and not the metadata of the VM that is being detected. I'm struggling to come up with a better name/interface that works for both GCE and non-GCE resource detection however. Maybe we can just do the simple thing and rename this and Detector to Resource and the method GetDetector to GetResource. And we can add a comment that in GCE VM land the resource is the VM metadata.

It is a nit pick tho. Feel free to ignore.

I think this is a good idea - I have changed the name to Resource.

ridwanmsharif · 2022-09-15T21:58:17Z

confgenerator/resourcedetector/detector.go

+	gcp_metadata "cloud.google.com/go/compute/metadata"
+)
+
+type Detector interface {


Also can we add some comments documenting how to use the interface. Its hard to tell from the interface, how it is meant to be used.

Added comments to what the interface means and how to cast to get an instance of the underlying type such as GCEResource.

ridwanmsharif · 2022-09-15T22:02:23Z

confgenerator/resourcedetector/detector.go

+}
+
+// Get a Detector for the current environment, which can be used
+// to get all the attributes


We should also add what it needs to be casted to (for example GCEDetector)

ridwanmsharif

Wooo LGTM!

ridwanmsharif reviewed Sep 9, 2022

View reviewed changes

LujieDuan force-pushed the lujieduan-prometheus-metadata branch from 5482025 to 3a79286 Compare September 12, 2022 15:24

LujieDuan commented Sep 12, 2022

View reviewed changes

LujieDuan requested a review from ridwanmsharif September 12, 2022 16:53

LujieDuan marked this pull request as ready for review September 12, 2022 16:53

ridwanmsharif reviewed Sep 12, 2022

View reviewed changes

LujieDuan force-pushed the lujieduan-prometheus-metadata branch from 1696c5b to 939a738 Compare September 15, 2022 19:21

LujieDuan added the kokoro:force-run Forces kokoro to run integration tests on a CL label Sep 15, 2022

stackdriver-instrumentation-release removed the kokoro:force-run Forces kokoro to run integration tests on a CL label Sep 15, 2022

LujieDuan changed the title ~~WIP: Fetch meta labels for Prometheus from GCE VM Metadata Server~~ Fetch meta labels for Prometheus from GCE VM Metadata Server Sep 15, 2022

LujieDuan requested review from a team and ridwanmsharif September 15, 2022 21:24

ridwanmsharif approved these changes Sep 15, 2022

View reviewed changes

ridwanmsharif reviewed Sep 15, 2022

View reviewed changes

LujieDuan requested a review from ridwanmsharif September 15, 2022 23:05

LujieDuan force-pushed the lujieduan-prometheus-metadata branch from bd09687 to b179df4 Compare September 16, 2022 13:59

ridwanmsharif approved these changes Sep 16, 2022

View reviewed changes

LujieDuan added the kokoro:force-run Forces kokoro to run integration tests on a CL label Sep 16, 2022

stackdriver-instrumentation-release removed the kokoro:force-run Forces kokoro to run integration tests on a CL label Sep 16, 2022

LujieDuan changed the title ~~Fetch meta labels for Prometheus from GCE VM Metadata Server~~ Fetch metadata from GCE VM Metadata Server Sep 16, 2022

LujieDuan requested a review from braydonk September 16, 2022 15:31

LujieDuan added 3 commits September 19, 2022 10:01

Add GCE meta labels detector

19230db

Rename detector to resource for better clarity

f8b1356

Added documentation for the resource interface

c38b059

LujieDuan force-pushed the lujieduan-prometheus-metadata branch from b179df4 to c38b059 Compare September 19, 2022 14:01

LujieDuan merged commit 6272ea2 into master Sep 19, 2022

franciscovalentecastro pushed a commit that referenced this pull request Oct 13, 2022

Fetch metadata from GCE VM Metadata Server (#837)

a692660

igorpeshansky deleted the lujieduan-prometheus-metadata branch July 10, 2023 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch metadata from GCE VM Metadata Server #837

Fetch metadata from GCE VM Metadata Server #837

LujieDuan commented Sep 8, 2022 •

edited

ridwanmsharif left a comment

ridwanmsharif Sep 9, 2022

ridwanmsharif Sep 9, 2022

ridwanmsharif Sep 9, 2022

ridwanmsharif Sep 9, 2022

ridwanmsharif Sep 9, 2022

ridwanmsharif Sep 9, 2022

LujieDuan Sep 12, 2022

ridwanmsharif Sep 12, 2022

ridwanmsharif left a comment

ridwanmsharif Sep 12, 2022

ridwanmsharif Sep 12, 2022

ridwanmsharif Sep 12, 2022

ridwanmsharif Sep 13, 2022 •

edited

ridwanmsharif left a comment

ridwanmsharif Sep 15, 2022

LujieDuan Sep 15, 2022

ridwanmsharif Sep 15, 2022

LujieDuan Sep 15, 2022

ridwanmsharif Sep 15, 2022

ridwanmsharif left a comment


		type LabelValue string

		type Metalabels map[LabelName]LabelValue

	type VM struct {
	Name string
	Project string
	Network string
	Platform string
	Zone string
	MachineType string
	ID int64
	// The IP address to ssh/WinRM to. This is the external IP address, unless
	// USE_INTERNAL_IP is set to 'true'. See comment on extractIPAddress() for
	// rationale.
	IPAddress string
	// WindowsCredentials is only populated for Windows VMs.
	WindowsCredentials *WindowsCredentials
	AlreadyDeleted bool
	}

Fetch metadata from GCE VM Metadata Server #837

Fetch metadata from GCE VM Metadata Server #837

Conversation

LujieDuan commented Sep 8, 2022 • edited

Description

Related issue

How has this been tested?

Checklist:

ridwanmsharif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ridwanmsharif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ridwanmsharif Sep 13, 2022 • edited

Choose a reason for hiding this comment

ridwanmsharif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ridwanmsharif left a comment

Choose a reason for hiding this comment

LujieDuan commented Sep 8, 2022 •

edited

ridwanmsharif Sep 13, 2022 •

edited