Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
@@ Coverage Diff @@
## master #1143 +/- ##
==========================================
+ Coverage 74.7% 74.84% +0.13%
==========================================
Files 128 128
Lines 18236 18337 +101
==========================================
+ Hits 13623 13724 +101
Misses 3829 3829
Partials 784 784 |
Current implementation (via "ubuntu" distro) is failing thusly:
|
7f3f033
to
6a10c29
Compare
Verified this disables auditd on the OS if not enabled vi api model:
|
Verified auditd is enabled via systemd when enabled via api model:
|
Validated via "ubuntu" distro that an enabled auditd + config produces a cluster that passes E2E. |
Validated "ubuntu-18.04" distro as well. |
f3709e5
to
29f6e92
Compare
Status: debug why auditd is exiting 6 after apt installation via VHD. Unable to repro using "ubuntu" distro creation flow:
|
f2c891c
to
1fbdad3
Compare
fed1eb3
to
b7451aa
Compare
validated new E2E on ubuntu distros |
else | ||
apt list --installed | grep 'auditd' | ||
if [ $? -eq 0 ]; then | ||
systemctlDisableAndStop auditd || exit $ERR_SYSTEMCTL_START_FAIL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not apt-get purge
if it's not enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just trying to avoid more apt operations at runtime for all cluster creates. Strictly speaking it's not doing anything except taking up space on the filesystem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like it's introducing more failure potential since with the existing cleanup functions we simply attempt to purge but without returning a non-zero exit code if the cleanup is unsuccessful (cleaning up an unnecessary package is arguably not critical to cluster deployment success). This breaks that pattern by saying: "return $ERR_SYSTEMCTL_START_FAIL if we can't disable and stop the service" which could fail for reason xyz.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very important that we (1) ensure that auditd isn't running and (2) that it won't start after reboot, so we definitely need this validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
75a21cb
to
8627efc
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Reason for Change:
Introduces auditd package and configuration into the VHD + cloud-init implementation. Requires an explicit
"auditDEnabled": true
profile configuration to enable. If not enabled explicitly, we disable auditd via systemd.Issue Fixed:
Fixes #1008
Requirements:
Notes: