Get elastic-agent-managed-daemonset.yaml from upstream 7.x instead of using a local static file #452

MichaelKatsoulis · 2021-07-29T14:16:56Z

This PR aims to close #328.

There is no need anymore for maintaining a static go file for elastic-agent-managed deployment.
Instead the upstream manifest (which creates a Daemonset) can be used. manifest

It is still in draft mode due to one dependancy.

… using a local static file

elasticmachine · 2021-07-29T14:16:58Z

Pinging @elastic/integrations (Team:Integrations)

elasticmachine · 2021-07-29T14:30:20Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2021-08-02T10:05:17.985+0000
Duration: 25 min 56 sec
Commit: cb63b32

Test stats 🧪

Test	Results
Failed	0
Passed	316
Skipped	4
Total	320

Trends 🧪

mtojek

While I was reviewing this draft, I found a potential issue. This file will be downloaded during the installation procedure and be left there forever. If the remote file changes (new commits in 7.x), the elastic-package won't pick it up.
Also, the installation procedure doesn't require internet access so far, so this would be blocking. I think we should consider an alternative approach:

Remove all local elastic-agent.yaml definitions.
Replace the logic which uses local file with one downloading from remote location every time (you can also use the cache directory in ~/.elastic-package/cache).

mtojek · 2021-07-29T14:29:04Z

internal/install/install.go

@@ -238,3 +246,20 @@ func createServiceLogsDir(elasticPackagePath *locations.LocationManager) error {
 	}
 	return nil
 }
+
+// DownloadFile will download a url from a path and return the response body as a string.
+func DownloadFile(filepath string, url string) (string, error) {


nit: does it have to be exposed?

mtojek · 2021-07-29T14:30:36Z

internal/install/install.go

@@ -184,9 +187,14 @@ func writeKubernetesDeployerResources(elasticPackagePath *locations.LocationMana
 		return errors.Wrap(err, "can't read application configuration")
 	}

+	elasticAgentManagedYaml, err := DownloadFile("elastic-agent-managed-kubernetes.yaml", elasticAgentManagedYamlUrl)


I don't think that filepath is needed here.

😔 it is not

mtojek · 2021-07-29T14:31:10Z

internal/install/install.go

@@ -184,9 +187,14 @@ func writeKubernetesDeployerResources(elasticPackagePath *locations.LocationMana
 		return errors.Wrap(err, "can't read application configuration")
 	}

+	elasticAgentManagedYaml, err := DownloadFile("elastic-agent-managed-kubernetes.yaml", elasticAgentManagedYamlUrl)
+	if err != nil {
+		return errors.Wrap(err, "downloading elastic-agent-managed-kubernetes.yaml failed")


I would replace the filename with something abstract in case the filename changes.

mtojek · 2021-07-29T14:31:55Z

internal/install/install.go

+	// Get the data
+	resp, err := http.Get(url)
+	if err != nil {
+		return "", err


Please use the errors.Wrap to stick some context here.

mtojek · 2021-07-29T14:32:00Z

internal/install/install.go

+	// Convert to string
+	b, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return "", err


mtojek · 2021-07-29T14:33:54Z

internal/install/install.go

@@ -184,9 +187,14 @@ func writeKubernetesDeployerResources(elasticPackagePath *locations.LocationMana
 		return errors.Wrap(err, "can't read application configuration")
 	}

+	elasticAgentManagedYaml, err := DownloadFile("elastic-agent-managed-kubernetes.yaml", elasticAgentManagedYamlUrl)


WDYT to replace this function with less generic one like downloadElasticAgentManagedYAML()? I don't think we need generic DownloadFile function here.

ChrsMark · 2021-07-29T14:49:38Z

Great to see this happening! It will help us eliminate issues when changes are happening on Beats/Agent regarding the deployment model/roles etc.

While I was reviewing this draft, I found a potential issue. This file will be downloaded during the installation procedure and be left there forever. If the remote file changes (new commits in 7.x), the elastic-package won't pick it up.
Also, the installation procedure doesn't require internet access so far, so this would be blocking. I think we should consider an alternative approach:

Why the file will stay there forever? I would expect a "download&replace" approach to work here. Or to be more consistent before downloading the deployer can try to k delete -f any old Agents and then download the new one and apply again. In case we want to save time and bandwidth we can download first and check the diff and if there is any do the update otherwise skip the delete+deploy steps.

mtojek · 2021-07-29T15:18:10Z

Why the file will stay there forever? I would expect a "download&replace" approach to work here.

No, it's a different pattern. Logic behind EnsureInstalled runs an embedded "installer" which creates required files and directories in the ~/.elastic-package. It's executed only once per application revision (if newer application is installed, then it will replace the directory), on startup.

Or to be more consistent before downloading the deployer can try to k delete -f any old Agents and then download the new one and apply again. In case we want to save time and bandwidth we can download first and check the diff and if there is any do the update otherwise skip the delete+deploy steps.

That's why I suggested to use this approach. We can download the YAML file in runtime and cache it locally if necessary.

mtojek · 2021-07-30T11:53:36Z

internal/kubectl/kubectl_apply.go

@@ -84,6 +84,22 @@ func Apply(definitionPaths ...string) error {
 	return nil
 }

+// ApplyStdin function adds resources to the Kubernetes cluster based on provided stdin.
+func ApplyStdin(input string) error {
+	logger.Debugf("Apply Kubernetes definitions")


nit: ... from stdin.

mtojek · 2021-07-30T11:53:51Z

internal/kubectl/kubectl_apply.go

+	logger.Debugf("Apply Kubernetes definitions")
+	out, err := applyKubernetesResourcesStdin(input)
+	if err != nil {
+		return errors.Wrap(err, "can't modify Kubernetes resources (apply)")


nit: ... apply from stdin

mtojek · 2021-07-30T11:54:18Z

internal/testrunner/runners/system/servicedeployer/kubernetes.go

 	"strings"

+	"github.com/elastic/elastic-package/internal/install"


nit: please sort this line (put it together with other internal

mtojek · 2021-07-30T11:58:47Z

internal/testrunner/runners/system/servicedeployer/kubernetes.go

+	}
+	// Replace fleet url
+	elasticAgentManagedYaml = strings.ReplaceAll(elasticAgentManagedYaml,
+		"https://fleet-server:8220",


nit: I wonder if we should replace it with regexp in case somebody modifies it:

http(s){0,1}://fleet-server:(\d+)

This is a valid concern. I will change it

mtojek · 2021-07-30T12:01:38Z

test/packages/kubernetes/data_stream/pod/fields/ecs.yml

@@ -7,3 +7,7 @@
 - name: service.type
  type: keyword
  description: Service type
+- name: orchestrator.cluster.name


To enable dependency management here, you need to include build.yaml file.

So I will just add this file

You need to format it with elastic-package format:

[2021-07-30T13:08:28.902Z] Error: checking package failed: formatting the integration failed (path: /var/lib/jenkins/workspace/t-manager_elastic-package_PR-452/src/github.com/elastic/elastic-package/test/packages/kubernetes, failFast: true): walking through the integration files failed: formatting file failed (path: /var/lib/jenkins/workspace/t-manager_elastic-package_PR-452/src/github.com/elastic/elastic-package/test/packages/kubernetes/_dev/build/build.yml): file is not formatted (path: /var/lib/jenkins/workspace/t-manager_elastic-package_PR-452/src/github.com/elastic/elastic-package/test/packages/kubernetes/_dev/build/build.yml)

mtojek · 2021-08-02T07:39:20Z

internal/testrunner/runners/system/servicedeployer/kubernetes.go

+	if err != nil {
+		return "", errors.Wrap(err, "can't read application configuration")
+	}
+	elasticAgentManagedYaml, err := downloadElasticAgentManagedYAML(elasticAgentManagedYamlURL)


nit: YAML comes as bytes. Did you check if we can skip converting to string? It seems that byte-body will fit well here.

mtojek · 2021-08-02T07:40:35Z

internal/testrunner/runners/system/servicedeployer/kubernetes.go

+	if err != nil {
+		return "", errors.Wrap(err, "can't read application configuration")
+	}
+	elasticAgentManagedYaml, err := downloadElasticAgentManagedYAML(elasticAgentManagedYamlURL)


nit: please add a logger debug indicating the download from upstream

mtojek · 2021-08-02T07:42:35Z

internal/kubectl/kubectl_apply.go

+	logger.Debugf("Apply Kubernetes stdin")
+	out, err := applyKubernetesResourcesStdin(input)
+	if err != nil {
+		return errors.Wrap(err, "can't modify Kubernetes resources (apply from stdin)")


nit: I think you're right in L89, "apply stdin"

mtojek · 2021-08-02T07:43:43Z

internal/testrunner/runners/system/servicedeployer/kubernetes.go

 	}

-	err = kubectl.Apply(locationManager.KubernetesDeployerAgentYml())


nit: logger debug: downloaded N bytes

mtojek · 2021-08-02T07:44:15Z

internal/testrunner/runners/system/servicedeployer/kubernetes.go

+	defer resp.Body.Close()
+
+	// Convert to string
+	b, err := io.ReadAll(resp.Body)


is it necessary to go with string?

It is not necessary. I did that in order to use it later in Regexp ReplaceAllString.
But I can instead use ReplaceAll that takes a slice of bytes as input.

But I can instead use ReplaceAll that takes a slice of bytes as input.

Sounds good to me

mtojek · 2021-08-02T07:44:37Z

internal/testrunner/runners/system/servicedeployer/kubernetes.go

+	// Get the data
+	resp, err := http.Get(url)
+	if err != nil {
+		return "", errors.Wrapf(err, "failed to get file from url %s", url)


nit: from URL

mtojek · 2021-08-02T07:44:53Z

internal/kubectl/kubectl.go

@@ -49,3 +50,21 @@ func modifyKubernetesResources(action string, definitionPaths ...string) ([]byte
 	}
 	return output, nil
 }
+
+// applyKubernetesResourcesStdin applies a kubernetes manifest provided as stdin.


nit: a Kubernetes manifest

….com:MichaelKatsoulis/elastic-package into use_upstream_manifest_to_deploy_agent_on_k8s

mtojek · 2021-08-02T09:37:52Z

internal/testrunner/runners/system/servicedeployer/kubernetes.go

+
+// getElasticAgentYaml retrieves elastic-agent-managed.yaml from upstream and modifies the file as needed
+// to run locally.
+func getElasticAgentYaml() ([]byte, error) {


nit: ...YAML()

…fest_to_deploy_agent_on_k8s

mtojek · 2021-08-02T11:05:26Z

@MichaelKatsoulis Ship it, please.

Get elastic-agent-managed-daemonset.yaml from upstream 7.x instead of…

fba955c

… using a local static file

MichaelKatsoulis added enhancement New feature or request Team:Integrations Label for the Integrations team labels Jul 29, 2021

MichaelKatsoulis marked this pull request as draft July 29, 2021 14:17

mtojek self-requested a review July 29, 2021 14:25

mtojek reviewed Jul 29, 2021

View reviewed changes

MichaelKatsoulis added 2 commits July 29, 2021 17:56

Update due to review

6484cfb

Replace fleet url with no https

228464f

MichaelKatsoulis added 2 commits July 29, 2021 18:33

Linter suggestion

be57036

Update apply of kubernetes resources logic

ed27df4

mtojek reviewed Jul 30, 2021

View reviewed changes

MichaelKatsoulis added 2 commits July 30, 2021 16:01

Review Updates

7345e5a

Format the package

fc4134e

MichaelKatsoulis marked this pull request as ready for review July 30, 2021 14:06

Merge branch 'master' into use_upstream_manifest_to_deploy_agent_on_k8s

153bc64

mtojek reviewed Aug 2, 2021

View reviewed changes

MichaelKatsoulis added 2 commits August 2, 2021 12:29

Use []byte instead of string

32a52b9

Merge branch 'use_upstream_manifest_to_deploy_agent_on_k8s' of github…

999765f

….com:MichaelKatsoulis/elastic-package into use_upstream_manifest_to_deploy_agent_on_k8s

mtojek approved these changes Aug 2, 2021

View reviewed changes

MichaelKatsoulis added 2 commits August 2, 2021 12:50

change function name

7d44d2d

Merge remote-tracking branch 'upstream/master' into use_upstream_mani…

cb63b32

…fest_to_deploy_agent_on_k8s

MichaelKatsoulis merged commit e815623 into elastic:master Aug 2, 2021

mtojek mentioned this pull request Aug 3, 2021

Revert: Get elastic-agent-managed-daemonset.yaml from upstream 7.x in… #459

Merged

This was referenced Aug 3, 2021

Fix downloading agent manifest from upstream #461

Merged

Add support for multi node kubernetes cluster #465

Open

		"strings"

		"github.com/elastic/elastic-package/internal/install"

		}

		err = kubectl.Apply(locationManager.KubernetesDeployerAgentYml())

Get elastic-agent-managed-daemonset.yaml from upstream 7.x instead of using a local static file #452

Get elastic-agent-managed-daemonset.yaml from upstream 7.x instead of using a local static file #452

Conversation

MichaelKatsoulis commented Jul 29, 2021

elasticmachine commented Jul 29, 2021

elasticmachine commented Jul 29, 2021 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

mtojek 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

ChrsMark commented Jul 29, 2021

mtojek commented Jul 29, 2021 • edited Loading

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

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

mtojek commented Aug 2, 2021

elasticmachine commented Jul 29, 2021 •

edited

Loading

mtojek commented Jul 29, 2021 •

edited

Loading