Add cloudfoundry common client into x-pack/common #16527

blakerouse · 2020-02-24T17:50:47Z

What does this PR do?

Prerequisite to adding an input for filebeat and a module for metricbeat. Contains the required clients and code to use Cloud Foundry loggregator API to collect logs and metrics.

Worry is in the size increase of the binary for the included Cloud Foundry dependencies. Once the filebeat input is enabled in a later PR the size increase is only 3M. Increases from 119M to 122M.

Why is it important?

To support Cloud Foundry.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~I have made corresponding changes to the documentation~~
~~I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works

Author's Checklist

[ ]

How to test this PR locally

Related issues

Relates [Meta] Cloud Foundry Support #16520

Use cases

Screenshots

Logs

x-pack/libbeat/common/cloudfoundry/config.go

exekias

I did a quick pass and left some comments, nothing major

x-pack/libbeat/common/cloudfoundry/events.go

…didn't expose overriding the TLSConfig. Add comment for location of documentation for event types.

blakerouse · 2020-02-24T22:34:58Z

@exekias Ready for another look.

exekias

This is looking good, I left some minor comments. Also CI found some issues, this is ready to go once CI is happy.

exekias · 2020-02-25T10:36:51Z

x-pack/libbeat/common/cloudfoundry/config.go

+}
+
+// InitDefaults initialize the defaults for the configuration.
+func (c *Config) InitDefaults() {


Not a strong opinion, but we normally use this pattern:

beats/metricbeat/helper/config.go

Line 35 in 837279a

func defaultConfig() Config {

so you can get defaults on a single call, this way you don't risk forgetting to call InitDefaults

I think this is a new way of initializing defaults 🙂 elastic/go-ucfg#141

Awesome, I missed this change 👍

exekias · 2020-02-25T10:39:47Z

x-pack/libbeat/common/cloudfoundry/events.go

+// EventType defines the different event types that can be raised from RPLClient.
+type EventType uint
+
+// EventTypes from loggregator documented here: https://github.com/cloudfoundry/loggregator-api


thanks for adding this, really helpful! It would look like we are using V1 types?

Yes the code always converts from V2 types to V1. V2 doesn't have any more events then V1 just a different structure.

jsoriano

Good job here. I have added some comments, nothing serious, the only thing I am a bit worried is about having a leak in the cache. Let me know what you think.

x-pack/libbeat/common/cloudfoundry/cache.go

x-pack/libbeat/common/cloudfoundry/cache_test.go

x-pack/libbeat/common/cloudfoundry/config_test.go

x-pack/libbeat/common/cloudfoundry/doer.go

x-pack/libbeat/common/cloudfoundry/rlplistener.go

x-pack/libbeat/common/cloudfoundry/cache.go

jsoriano · 2020-02-25T11:08:06Z

x-pack/libbeat/common/cloudfoundry/config.go

+}
+
+// InitDefaults initialize the defaults for the configuration.
+func (c *Config) InitDefaults() {


I think this is a new way of initializing defaults 🙂 elastic/go-ucfg#141

kvch · 2020-02-25T11:26:51Z

NOTICE.txt

+THE SOFTWARE.
+
+--------------------------------------------------------------------
+Dependency: github.com/cloudfoundry/sonde-go


Looking at this dependecy, we might need to copy a few files manually to make it work. But that's not a problem.

kvch

Deps look right.

kvch · 2020-02-25T11:27:43Z

I am sorry, I just wanted to comment. But I accidentally clicked on approve.

…ead of the kubernetes ttl cache. Fix other suggestions.

blakerouse · 2020-02-25T14:37:04Z

@exekias @jsoriano Ready for another look.

exekias · 2020-02-25T16:14:31Z

LGTM once @jsoriano is 👍

jsoriano · 2020-02-25T16:15:04Z

x-pack/libbeat/common/cloudfoundry/rlplistener.go

@@ -107,9 +102,8 @@ func (c *RlpListener) Start() {

 // Stop receiving events
 func (c *RlpListener) Stop() {
-	c.log.Debugw("Stopping RLP listener.", "rlpAddress", c.rlpAddress)
+	c.log.Debugw("stopping RLP listener.", "rlpAddress", c.rlpAddress)


Logged messages can start with uppercase 🙂

Suggested change

c.log.Debugw("stopping RLP listener.", "rlpAddress", c.rlpAddress)

c.log.Debugw("Stopping RLP listener.", "rlpAddress", c.rlpAddress)

jsoriano · 2020-02-25T16:17:21Z

x-pack/libbeat/common/cloudfoundry/doer.go

+
+	jsonData := make(map[string]interface{})
+	decoder := json.NewDecoder(resp.Body)
+	err = decoder.Decode(&jsonData)


Handle this error here?

x-pack/libbeat/common/cloudfoundry/doer.go

x-pack/libbeat/common/cloudfoundry/cache.go

* Add cloudfoundry common client into x-pack/common. * Run mage fmt. * Add support for tlscommon.Config, removing the uaago depedency as it didn't expose overriding the TLSConfig. Add comment for location of documentation for event types. * Use common.Cache with addition of not updated expiration on get, instead of the kubernetes ttl cache. Fix other suggestions. * Fix cache_test and new test for not updated access time. * Handle error sooner in getAuthTokenWithExpiresIn. (cherry picked from commit f4c71b4)

Add cloudfoundry common client into x-pack/common.

d9db4d1

blakerouse added enhancement libbeat x-pack Issues and pull requests for X-Pack features. Team:Platforms Label for the Integrations - Platforms team labels Feb 24, 2020

Run mage fmt.

73d5dcf

exekias requested a review from a team February 24, 2020 21:02

exekias reviewed Feb 24, 2020

View reviewed changes

x-pack/libbeat/common/cloudfoundry/config.go Outdated Show resolved Hide resolved

exekias reviewed Feb 24, 2020

View reviewed changes

x-pack/libbeat/common/cloudfoundry/events.go Show resolved Hide resolved

Add support for tlscommon.Config, removing the uaago depedency as it …

a2a5fae

…didn't expose overriding the TLSConfig. Add comment for location of documentation for event types.

exekias reviewed Feb 25, 2020

View reviewed changes

jsoriano reviewed Feb 25, 2020

View reviewed changes

kvch reviewed Feb 25, 2020

View reviewed changes

kvch approved these changes Feb 25, 2020

View reviewed changes

blakerouse added 2 commits February 25, 2020 08:09

Use common.Cache with addition of not updated expiration on get, inst…

39a09d4

…ead of the kubernetes ttl cache. Fix other suggestions.

Fix cache_test and new test for not updated access time.

3459a54

jsoriano approved these changes Feb 25, 2020

View reviewed changes

Handle error sooner in getAuthTokenWithExpiresIn.

96406e7

blakerouse merged commit f4c71b4 into elastic:master Feb 25, 2020

blakerouse deleted the common-cloudfoundry branch February 25, 2020 18:14

blakerouse mentioned this pull request Feb 25, 2020

Cherry-pick #16527 to 7.x: Add cloudfoundry common client into x-pack/common #16575

Merged

3 tasks

blakerouse added the v7.7.0 label Feb 25, 2020

andresrc added [zube]: Done and removed [zube]: Done labels Mar 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cloudfoundry common client into x-pack/common #16527

Add cloudfoundry common client into x-pack/common #16527

blakerouse commented Feb 24, 2020 •

edited by andresrc

Loading

exekias left a comment

blakerouse commented Feb 24, 2020

exekias left a comment

exekias Feb 25, 2020

jsoriano Feb 25, 2020

exekias Feb 25, 2020

exekias Feb 25, 2020

blakerouse Feb 25, 2020

jsoriano left a comment •

edited

Loading

jsoriano Feb 25, 2020

kvch Feb 25, 2020

kvch left a comment

kvch commented Feb 25, 2020

blakerouse commented Feb 25, 2020

exekias commented Feb 25, 2020

jsoriano Feb 25, 2020

jsoriano Feb 25, 2020

	c.log.Debugw("stopping RLP listener.", "rlpAddress", c.rlpAddress)
	c.log.Debugw("Stopping RLP listener.", "rlpAddress", c.rlpAddress)

Add cloudfoundry common client into x-pack/common #16527

Add cloudfoundry common client into x-pack/common #16527

Conversation

blakerouse commented Feb 24, 2020 • edited by andresrc Loading

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

exekias left a comment

Choose a reason for hiding this comment

blakerouse commented Feb 24, 2020

exekias 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

jsoriano left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvch left a comment

Choose a reason for hiding this comment

kvch commented Feb 25, 2020

blakerouse commented Feb 25, 2020

exekias commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakerouse commented Feb 24, 2020 •

edited by andresrc

Loading

jsoriano left a comment •

edited

Loading