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.

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

Vendor full aws-sdk instead of individual packages #12614

Merged
merged 23 commits into from Jul 15, 2019

Conversation

andrewvc
Copy link
Contributor

@andrewvc andrewvc commented Jun 19, 2019

This takes over from #12548 , and resolves a lot of packaging weirdness by simply importing the full AWS SDK with govendor. This also updates the version to v0.9.0 from preview.5. This is a prereq for #12401

govendor doesn't play well when attempting to update the individual packages, complaining frequently about subtree overwrites. With this change, an update is as simple as govendor fetch github.com/aws/aws-sdk-go-v2/...@VERSION.

Previously I used a bash one liner to parse the package names out of vendor.json and attempt to invoke govendor once per package. However, that hit the aforementioned problems with subtree overwrites.

I've heard that there are some concerns about this potentially increasing binary size. I've tried compiling metricbeat from on this branch. The binary was 56 megabytes. Identical to master.

@andrewvc andrewvc self-assigned this Jun 19, 2019
@andrewvc andrewvc added the Team:obs-ds-hosted-services Label for the Observability Hosted Services team label Jun 19, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/uptime

@kaiyan-sheng
Copy link
Contributor

kaiyan-sheng commented Jun 19, 2019

How did you measure the new binary metricbeat size? When I tried mage update; mage build under x-pack/metricbeat with v0.9.0 aws-sdk-go-v2, it failed building the binary becasue:

>> build: Building metricbeat
# github.com/elastic/beats/x-pack/metricbeat/module/aws
module/aws/aws.go:102:21: undefined: ec2iface.EC2API
module/aws/utils.go:29:78: undefined: cloudwatchiface.CloudWatchAPI
module/aws/utils.go:46:100: undefined: cloudwatchiface.CloudWatchAPI
module/aws/utils.go:63:79: undefined: cloudwatchiface.CloudWatchAPI
Error: running "go build -o metricbeat -ldflags -X github.com/elastic/beats/libbeat/version.buildTime=2019-06-19T16:39:55Z -X github.com/elastic/beats/libbeat/version.commit=19c14dc51fe0ba8023394b61d65475291b1ed157" failed with exit code 2

So if we decide to go with v0.9.0, then I need to fix aws module first 😬

@andrewvc
Copy link
Contributor Author

@kaiyan-sheng weird! I just used make metricbeat... hmmmmmmmmmmm let me try again with mage

@andrewvc
Copy link
Contributor Author

@kaiyan-sheng works for me

➜  metricbeat git:(unify-aws-packages) rm metricbeat
➜  metricbeat git:(unify-aws-packages) stat metricbeat
stat: cannot stat 'metricbeat': No such file or directory
➜  metricbeat git:(unify-aws-packages) mage build
>> build: Building metricbeat
➜  metricbeat git:(unify-aws-packages) stat metricbeat
  File: metricbeat
  Size: 58535408  	Blocks: 114328     IO Block: 4096   regular file
Device: 1000005h/16777221d	Inode: 8664408212  Links: 1
Access: (0755/-rwxr-xr-x)  Uid: (  501/andrewcholakian)   Gid: (   20/   staff)
Access: 2019-06-19 12:29:02.673278323 -0500
Modify: 2019-06-19 12:29:01.829555131 -0500
Change: 2019-06-19 12:29:01.830320695 -0500
 Birth: 2019-06-19 12:29:01.280176443 -0500

@andrewvc
Copy link
Contributor Author

Functionbeat is broken I should say though, I'll try and fix that

@kaiyan-sheng
Copy link
Contributor

Did you run the command under beats/x-pack/metricbeat?

@andrewvc
Copy link
Contributor Author

Ah, I see the issue was that I was using the OSS version.

@exekias
Copy link
Contributor

exekias commented Jun 21, 2019

This is great! this change will require to update Functionbeat and Metricbeat AWS module, do you plan to do that?

As a side note, it would probably make a lot of sense to unify how we initialize AWS client, so we all share the same parameters set, like we did with Kubernetes or Docker: https://github.com/elastic/beats/blob/master/libbeat/common/docker/client.go

@andrewvc andrewvc requested a review from a team as a code owner June 24, 2019 21:20
@andrewvc
Copy link
Contributor Author

@exekias Glad to hear it! I just pushed the updates to functionbeat/metricbeat to make them compatible. I also merged in the latest master.

I agree with unifying the initialization, but perhaps we can do that in a separate PR? The scope here is large as is.

We also will need a separate PR to take advantage of the newer version of the AWS library's support for adding a context.Context to most ops. For now I just stubbed in context.TODO(). This doesn't regress the functionality, but we should add configurable timeouts in a future PR.

@exekias
Copy link
Contributor

exekias commented Jun 25, 2019

That sounds great to me, thank you for taking the load here!

@andrewvc
Copy link
Contributor Author

@kaiyan-sheng @exekias I'm thinking I'm going to put this PR on indefinite hold and use preview5 for my ELB PR. I got pretty far here, but now I'm getting weird NPEs in functionbeat. I don't have the time to sink into this right now. Maybe @ph can take a look?

@kaiyan-sheng
Copy link
Contributor

@andrewvc Thanks for the update! I can also take a look later for the unit tests in metricbeat aws module that are still failing with v0.9.0, for example https://travis-ci.org/elastic/beats/jobs/550513776#L647

@exekias
Copy link
Contributor

exekias commented Jun 27, 2019

cc @kvch FYI

@kvch kvch self-assigned this Jun 27, 2019
@kvch
Copy link
Contributor

kvch commented Jul 2, 2019

I have tracked down the issue with Functionbeat tests. In aws.Request HTTPRequest cannot be null, so the context can be set.

 .../provider/aws/event_stack_poller_test.go         |  4 +++-
 .../provider/aws/op_cloudformation_test.go          | 21 +++++++++++++--------
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/x-pack/functionbeat/provider/aws/event_stack_poller_test.go b/x-pack/functionbeat/provider/aws/event_stack_poller_test.go
index 6f72d4737..ab1ee3725 100644
--- a/x-pack/functionbeat/provider/aws/event_stack_poller_test.go
+++ b/x-pack/functionbeat/provider/aws/event_stack_poller_test.go
@@ -5,6 +5,7 @@
 package aws

 import (
+   "net/http"
    "strconv"
    "testing"
    "time"
@@ -52,8 +53,9 @@ func (m *mockCloudFormationClient) DescribeStackEventsRequest(
            m.Index++
        }
    }()
+   httpReq, _ := http.NewRequest("", "", nil)
    return cloudformation.DescribeStackEventsRequest{
-       Request: &aws.Request{Data: &m.Responses[m.Index]},
+       Request: &aws.Request{Data: &m.Responses[m.Index], HTTPRequest: httpReq},
    }
 }

diff --git a/x-pack/functionbeat/provider/aws/op_cloudformation_test.go b/x-pack/functionbeat/provider/aws/op_cloudformation_test.go
index ddc0aec4d..04c57df68 100644
--- a/x-pack/functionbeat/provider/aws/op_cloudformation_test.go
+++ b/x-pack/functionbeat/provider/aws/op_cloudformation_test.go
@@ -6,6 +6,7 @@ package aws

 import (
    "errors"
+   "net/http"
    "testing"

    "github.com/aws/aws-sdk-go-v2/aws"
@@ -40,14 +41,15 @@ func (m *mockCloudformationStack) CreateStackRequest(
        m.onCreateStackInput(input)
    }

+   httpReq, _ := http.NewRequest("", "", nil)
    if m.err != nil {
        return cloudformation.CreateStackRequest{
-           Request: &aws.Request{Data: m.respCreateStackOutput, Error: m.err},
+           Request: &aws.Request{Data: m.respCreateStackOutput, Error: m.err, HTTPRequest: httpReq},
        }
    }

    return cloudformation.CreateStackRequest{
-       Request: &aws.Request{Data: m.respCreateStackOutput},
+       Request: &aws.Request{Data: m.respCreateStackOutput, HTTPRequest: httpReq},
    }
 }

@@ -58,14 +60,15 @@ func (m *mockCloudformationStack) DeleteStackRequest(
        m.onDeleteStackInput(input)
    }

+   httpReq, _ := http.NewRequest("", "", nil)
    if m.err != nil {
        return cloudformation.DeleteStackRequest{
-           Request: &aws.Request{Data: m.respDeleteStackOutput, Error: m.err},
+           Request: &aws.Request{Data: m.respDeleteStackOutput, Error: m.err, HTTPRequest: httpReq},
        }
    }

    return cloudformation.DeleteStackRequest{
-       Request: &aws.Request{Data: m.respDeleteStackOutput},
+       Request: &aws.Request{Data: m.respDeleteStackOutput, HTTPRequest: httpReq},
    }
 }

@@ -76,14 +79,15 @@ func (m *mockCloudformationStack) DescribeStacksRequest(
        m.onDescribeStacksInput(input)
    }

+   httpReq, _ := http.NewRequest("", "", nil)
    if m.err != nil {
        return cloudformation.DescribeStacksRequest{
-           Request: &aws.Request{Data: m.respDescribeStacksOutput, Error: m.err},
+           Request: &aws.Request{Data: m.respDescribeStacksOutput, Error: m.err, HTTPRequest: httpReq},
        }
    }

    return cloudformation.DescribeStacksRequest{
-       Request: &aws.Request{Data: m.respDescribeStacksOutput},
+       Request: &aws.Request{Data: m.respDescribeStacksOutput, HTTPRequest: httpReq},
    }
 }

 @@ -94,14 +98,15 @@ func (m *mockCloudformationStack) UpdateStackRequest(
         m.onUpdateStackInput(input)
     }

 +   httpReq, _ := http.NewRequest("", "", nil)
     if m.err != nil {
         return cloudformation.UpdateStackRequest{
 -           Request: &aws.Request{Data: m.respUpdateStackOutput, Error: m.err},
 +           Request: &aws.Request{Data: m.respUpdateStackOutput, Error: m.err, HTTPRequest: httpReq},
         }
     }

     return cloudformation.UpdateStackRequest{
 -       Request: &aws.Request{Data: m.respUpdateStackOutput},
 +       Request: &aws.Request{Data: m.respUpdateStackOutput, HTTPRequest: httpReq},
     }
  }

I've mailed the patch to you.

@andrewvc
Copy link
Contributor Author

andrewvc commented Jul 3, 2019

Incorporated @kvch 's patch (thanks!) as well as merged in master and updated the new RDS stuff. 🤞

@kaiyan-sheng
Copy link
Contributor

Sorry for the delay! I will fix the rest of the failing unit tests here!

Fix unit tests for metricbeat aws module
@andrewvc
Copy link
Contributor Author

andrewvc commented Jul 9, 2019

jenkins, retest this please

@kaiyan-sheng
Copy link
Contributor

kaiyan-sheng commented Jul 9, 2019

@andrewvc TestGetListMetricsOutput is still failing, I will push a patch to fix that. Sorry!
For the filebeat test failure, if you rebase on top of the master branch, then it should pass with #12819 merged.

@kaiyan-sheng
Copy link
Contributor

andrewvc#2 should fix the unit test.

Fix unit test TestGetListMetricsOutput
@andrewvc
Copy link
Contributor Author

jenkins, please retest this.

@andrewvc
Copy link
Contributor Author

jenkins, retest this please.

@kaiyan-sheng
Copy link
Contributor

andrewvc#3 one more please!

Fix unit tests for aws module
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

Yay the tests passed!

@andrewvc andrewvc merged commit 5c46714 into elastic:master Jul 15, 2019
@andrewvc andrewvc deleted the unify-aws-packages branch July 15, 2019 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement libbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants