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

Agent owns its directories #3084

Merged
merged 2 commits into from
Jul 19, 2023
Merged

Conversation

michalpristas
Copy link
Contributor

@michalpristas michalpristas commented Jul 14, 2023

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

@michalpristas 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 michalpristas self-assigned this Jul 14, 2023
@michalpristas michalpristas requested a review from a team as a code owner July 14, 2023 09:59
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@mergify
Copy link
Contributor

mergify bot commented Jul 14, 2023

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
Copy link
Contributor

elasticmachine commented Jul 14, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

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!)

Copy link
Member

@pchila pchila left a comment

Choose a reason for hiding this comment

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

LGTM

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@michalpristas michalpristas Jul 14, 2023

Choose a reason for hiding this comment

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

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
Copy link
Contributor

elasticmachine commented Jul 14, 2023

🌐 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
Copy link
Contributor

/test

@jlind23
Copy link
Contributor

jlind23 commented Jul 17, 2023

buildkite test it

@jlind23
Copy link
Contributor

jlind23 commented Jul 17, 2023

/test

@111andre111
Copy link
Contributor

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

  1. Is this one still needed?
    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.
  2. Is there the --groups 0 still needed here:
    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
Copy link
Contributor Author

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
Copy link
Contributor Author

/package

@111andre111
Copy link
Contributor

@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)

@michalpristas michalpristas merged commit c79ee5f into elastic:main Jul 19, 2023
9 checks passed
michalpristas added a commit that referenced this pull request Jul 27, 2023
michalpristas added a commit that referenced this pull request Jul 28, 2023
michalpristas added a commit to michalpristas/elastic-agent that referenced this pull request Oct 10, 2023
michalpristas added a commit that referenced this pull request Oct 16, 2023

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.
mergify bot pushed a commit that referenced this pull request Oct 16, 2023
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)
michalpristas added a commit that referenced this pull request Oct 18, 2023
…#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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip bug Something isn't working Team:Elastic-Agent Label for the Agent team Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Elastic Agent docker image for azure containers runtime
6 participants