Agent owns its directories #3084

michalpristas · 2023-07-14T09:59:35Z

What does this PR do?

This changes owner of agent directories. Tested locally and on azure container instances, seems to work.

Why is it important?

Containers running on Azure Container Instances refuse to read config.

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
I have added an entry in ./changelog/fragments using the changelog tool
I have added an integration test or an E2E test

How to test

build docker image
push to observability-ci container repo
create Azure (portal.azure.com login using SSO) Container Instance using published repo (you need to specify Other-Private docker registry with login and pass capture from docker-auth.elastic.co)
see it runs

Closes: #82

elasticmachine · 2023-07-14T09:59:39Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

elasticmachine · 2023-07-14T09:59:39Z

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

mergify · 2023-07-14T10:00:10Z

This pull request does not have a backport label. Could you fix it @michalpristas? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

elasticmachine · 2023-07-14T10:05:51Z

💚 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: 2023-07-18T09:05:48.476+0000
Duration: 70 min 58 sec

Test stats 🧪

Test	Results
Failed	0
Passed	5975
Skipped	27
Total	6002

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages.
run integration tests : Run the Elastic Agent Integration tests.
run end-to-end tests : Generate the packages and run the E2E Tests.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

pchila

LGTM

pchila · 2023-07-14T10:08:36Z

dev-tools/packaging/templates/docker/Dockerfile.elastic-agent.tmpl

+RUN groupadd --gid 1000 {{ .BeatName }} && \
+    useradd -M --uid 1000 --gid 1000 --groups 0 --home {{ $beatHome }} {{ .user }} && \
+    chown -R {{ .user }}:{{ .user }} {{ $beatHome }} && \
+    true


Why do you need a && true at the end ? 🤔

convention & readability, easier to add other commands later without messing with && \ at the end
easier to combine with if statements used in a template

elasticmachine · 2023-07-14T10:59:50Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	98.718% (`77/78`)	👍
Files	67.416% (`180/267`)	👍
Classes	66.397% (`328/494`)	👍
Methods	53.89% (`1039/1928`)	👍
Lines	40.181% (`11916/29656`)	👍 0.081
Conditionals	100.0% (`0/0`)	💚

pierrehilbert · 2023-07-14T13:53:49Z

/test

jlind23 · 2023-07-17T06:29:21Z

buildkite test it

jlind23 · 2023-07-17T06:29:26Z

/test

111andre111 · 2023-07-17T12:14:53Z

@michalpristas
I tested now with the changes und Docker Desktop as well as Azure and it was working.
Some household questions:

Is this one still needed?

elastic-agent/dev-tools/packaging/templates/docker/Dockerfile.elastic-agent.tmpl

Line 29 in bbede6e

find {{ $beatHome }}/data/elastic-agent-{{ commit_short }}/components -name "*.yml*" -type f -exec chown root:root {} \; && \

Given we later do nevertheless a chown I assume that particular step is not needed.
Is there the --groups 0 still needed here:

elastic-agent/dev-tools/packaging/templates/docker/Dockerfile.elastic-agent.tmpl

Line 147 in bbede6e

useradd -M --uid 1000 --gid 1000 --groups 0 --home {{ $beatHome }} {{ .user }} && \

Given we don't have root group nevertheless inherited from that command like mentioned in the original issue, is there any need to add this user to supplementary group root?

michalpristas · 2023-07-18T08:55:33Z

first one is needed because beats have a check to see that only root can change the config, some security measure.

second one is needed because without it npm install and check for synthetics fails. i tried to work around it but i was not successful. there's an npm install command with || option to check /root/something and without this npm install fails and this check fails as well because /root

michalpristas · 2023-07-18T09:05:34Z

/package

111andre111 · 2023-07-18T11:25:05Z

@michalpristas
Thanks for these backgrounds. These male semse.
Now the /root this is probably happening because of npm i -g
https://stackoverflow.com/questions/18088372/how-to-npm-install-global-not-as-root
The location could be probably changed by npm config set prefix but I don't want to mess around too much. Thanks for the explanation.
Just to raise attention I still found this issue once agent is working:
#82 (comment)

This reverts commit c79ee5f.

)" This reverts commit 1bf9027.

What this change is introducing on top of bringing back work introduced in #3084 is change of ordrer for some operations. Changing owner of a file, discards capabilities set. This becomes a problem with heartbeat as it needs setuid and netraw capabilities to perform properly. So setting capabilities was moved after chown.

What this change is introducing on top of bringing back work introduced in #3084 is change of ordrer for some operations. Changing owner of a file, discards capabilities set. This becomes a problem with heartbeat as it needs setuid and netraw capabilities to perform properly. So setting capabilities was moved after chown. (cherry picked from commit fa357a8)

…#3614) What this change is introducing on top of bringing back work introduced in #3084 is change of ordrer for some operations. Changing owner of a file, discards capabilities set. This becomes a problem with heartbeat as it needs setuid and netraw capabilities to perform properly. So setting capabilities was moved after chown. (cherry picked from commit fa357a8) Co-authored-by: Michal Pristas <[email protected]>

Agent owns its directories

46976af

michalpristas added bug Something isn't working Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team Team:Elastic-Agent Label for the Agent team labels Jul 14, 2023

michalpristas self-assigned this Jul 14, 2023

michalpristas requested a review from a team as a code owner July 14, 2023 09:59

michalpristas requested review from blakerouse and pchila July 14, 2023 09:59

mergify bot added the backport-skip label Jul 14, 2023

fragment

bbede6e

pchila approved these changes Jul 14, 2023

View reviewed changes

michalpristas merged commit c79ee5f into elastic:main Jul 19, 2023
9 checks passed

cmacknz mentioned this pull request Jul 27, 2023

Update Elastic Agent docker image for azure containers runtime #82

Closed

michalpristas added a commit that referenced this pull request Jul 27, 2023

Revert "Agent owns its directories (#3084)"

84e2cfa

This reverts commit c79ee5f.

michalpristas mentioned this pull request Jul 27, 2023

Revert "Agent owns its directories" #3139

Merged

michalpristas added a commit that referenced this pull request Jul 28, 2023

Revert "Agent owns its directories (#3084)" (#3139)

1bf9027

This reverts commit c79ee5f.

michalpristas added a commit to michalpristas/elastic-agent that referenced this pull request Oct 10, 2023

Revert "Revert "Agent owns its directories (elastic#3084)" (elastic#3139

cab171c

)" This reverts commit 1bf9027.

michalpristas mentioned this pull request Oct 10, 2023

[Fix] Agent incapable of running on Azure Container Instances #3576

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Agent owns its directories #3084

Agent owns its directories #3084

michalpristas commented Jul 14, 2023 •

edited

Loading

elasticmachine commented Jul 14, 2023

elasticmachine commented Jul 14, 2023

mergify bot commented Jul 14, 2023

elasticmachine commented Jul 14, 2023 •

edited

Loading

Build stats

Test stats 🧪

pchila left a comment

pchila Jul 14, 2023

michalpristas Jul 14, 2023 •

edited

Loading

elasticmachine commented Jul 14, 2023 •

edited

Loading

pierrehilbert commented Jul 14, 2023

jlind23 commented Jul 17, 2023

jlind23 commented Jul 17, 2023

111andre111 commented Jul 17, 2023

michalpristas commented Jul 18, 2023

michalpristas commented Jul 18, 2023

111andre111 commented Jul 18, 2023

Agent owns its directories #3084

Agent owns its directories #3084

Conversation

michalpristas commented Jul 14, 2023 • edited Loading

What does this PR do?

Why is it important?

Checklist

How to test

elasticmachine commented Jul 14, 2023

elasticmachine commented Jul 14, 2023

mergify bot commented Jul 14, 2023

elasticmachine commented Jul 14, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

pchila left a comment

Choose a reason for hiding this comment

pchila Jul 14, 2023

Choose a reason for hiding this comment

michalpristas Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Jul 14, 2023 • edited Loading

🌐 Coverage report

pierrehilbert commented Jul 14, 2023

jlind23 commented Jul 17, 2023

jlind23 commented Jul 17, 2023

111andre111 commented Jul 17, 2023

michalpristas commented Jul 18, 2023

michalpristas commented Jul 18, 2023

111andre111 commented Jul 18, 2023

michalpristas commented Jul 14, 2023 •

edited

Loading

elasticmachine commented Jul 14, 2023 •

edited

Loading

michalpristas Jul 14, 2023 •

edited

Loading

elasticmachine commented Jul 14, 2023 •

edited

Loading