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

Fixed DO group permissions to allow for writing the connection string #61

Merged
merged 2 commits into from
May 11, 2021

Conversation

nihemstr
Copy link
Contributor

Added the code in our postisnt script to add adu to the do group so we have permissions to write the connection_string to DO in order to fix an issue with the Nested Edge scenario.

@nihemstr nihemstr requested a review from a team May 11, 2021 19:37
@nihemstr nihemstr self-assigned this May 11, 2021
# Restart deliveryoptimization-agent service to ensure that the new 'do' user's permissions take effect.
echo "Restart $ms_doclient_lite_service"
systemctl restart "$ms_doclient_lite_service"
fi
Copy link
Contributor

@Nox-MSFT Nox-MSFT May 11, 2021

Choose a reason for hiding this comment

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

Can you combine the 2 blocks and restart do-agent only once please?

(Or, just set a boolean, and only restart do-agent is needed) #Pending

Copy link
Contributor

@Nox-MSFT Nox-MSFT left a comment

Choose a reason for hiding this comment

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

🕐

# Restart deliveryoptimization-agent service to ensure that the new 'do' user's permissions take effect.
echo "Restart $ms_doclient_lite_service"
systemctl restart "$ms_doclient_lite_service"
fi
Copy link
Contributor

@JeffMill JeffMill May 11, 2021

Choose a reason for hiding this comment

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

what happens if someone builds a client that doesn't use DO? Do they not use / modify this postinst file? Is this documented?

Can we call another script from postinst and move do related stuff there? #Pending

Copy link
Contributor

Choose a reason for hiding this comment

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

We're tying DO configuration into this postinst file, which might be okay if this is all just for our reference package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our postinst script is expressly for our debian package which already lists DO as a dependency. You cannot install our package produced by our scripts without the DO dependency.

If we moved to the model you're suggesting we would have to remove that DO dependency which I don't think makes sense.

To me it is fine that we keep it here as long as we are building our package based agent to run as such. If we want to make our package downloader configurable I think we would have to change a large amount of the setup.

In fact the way these are written if DO wasn't installed it would still pass we just wouldn't setup the relationships posted here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not thrilled with this whole comment:

        # Note: DO user and group are created by deliveryoptimization-agent Debian package,
        # which is one of the dependencies declared in adu-agent control file.
        # We are assuming that both DO user and group currently exist at this point.

seems hacky to me. Not blocking, but consider tracking for future

Copy link
Contributor

@JeffMill JeffMill left a comment

Choose a reason for hiding this comment

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

🕐

@@ -143,9 +143,11 @@ case "$1" in
# which is one of the dependencies declared in adu-agent control file.
# We are assuming that both DO user and group currently exist at this point.
# Add 'do' user to 'adu' group to allow DO to write to ADU download sandbox.
echo "Add the 'do' user to the 'adu' group."
echo "Add the 'do' user to the 'adu' group and 'adu' to the 'do' group"
if getent passwd 'do' > /dev/null; then
usermod -aG 'adu' 'do'
Copy link
Contributor

@JeffMill JeffMill May 11, 2021

Choose a reason for hiding this comment

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

adu

$adu_group ?

if getent passwd 'do' > /dev/null; then
usermod -aG 'adu' 'do'
# Note: We must add the 'adu' user to the 'do' group so that we can set the connection_string for DO
usermod -aG 'do' 'adu'
Copy link
Contributor

Choose a reason for hiding this comment

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

adu

$adu_user ?

Copy link
Contributor

@JeffMill JeffMill left a comment

Choose a reason for hiding this comment

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

:shipit:

@JeffMill
Copy link
Contributor

approved with comments

Copy link
Contributor

@Nox-MSFT Nox-MSFT left a comment

Choose a reason for hiding this comment

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

:shipit:

@nihemstr nihemstr merged commit 50c3331 into release/2021-q2 May 11, 2021
jw-msft added a commit that referenced this pull request Jul 30, 2021
* Fixed DO group permissions to allow for writing the connection string (#61)

* Updated agent version to remove rc1 (#66)

Co-authored-by: Nicholas Hemstreet <62158276+nihemstr@users.noreply.github.com>
@Nox-MSFT Nox-MSFT deleted the user/nihemstr/do-adu-group-addition branch March 2, 2022 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants