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

[BV-195] Run toolbox with the host user #269

Merged
merged 18 commits into from
Oct 7, 2024
Merged

Conversation

Franr
Copy link
Contributor

@Franr Franr commented May 25, 2024

What?

We now run the commands in the toolbox with the same uid/gid of the host.
That remove many needs, like changing the ownership of files generated inside the container.

Why?

  • Simplification of logic in the CLI.
  • Running containers as non-root user (securer).

References

closes #193

Before release

Review the checklist here
Integrations tests must be fixed (ref: #272)

@coveralls
Copy link
Collaborator

coveralls commented May 25, 2024

Pull Request Test Coverage Report for Build 11221535107

Details

  • 65 of 69 (94.2%) changed or added relevant lines in 5 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-9.3%) to 59.901%

Changes Missing Coverage Covered Lines Changed/Added Lines %
leverage/container.py 54 56 96.43%
leverage/modules/terraform.py 4 6 66.67%
Files with Coverage Reduction New Missed Lines %
leverage/container.py 4 60.94%
Totals Coverage Status
Change from base Build 10894199021: -9.3%
Covered Lines: 2450
Relevant Lines: 3930

💛 - Coveralls

@Franr Franr self-assigned this May 30, 2024
Makefile Outdated
@@ -1,7 +1,7 @@
.PHONY: help build
LEVERAGE_TESTING_IMAGE := binbash/leverage-cli-testing
LEVERAGE_TESTING_TAG := 2.5.0
LEVERAGE_IMAGE_TAG := 1.2.7-0.0.5
LEVERAGE_IMAGE_TAG := 1.2.7-0.1.14
Copy link
Contributor Author

Choose a reason for hiding this comment

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

0.0.5 doesn't have the groupadd binary so the local image fails to compile

this is a good case to use the minimal required version section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually we now need the last toolbox image (0.1.19) that support the custom home folder

@@ -381,61 +428,14 @@ def system_exec(self, command):
self.entrypoint = self.AWS_CLI_BINARY
return exit_code, output

def get_sso_code(self, container) -> str:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these were moved to the SSOContainer class, where they fit better


class TerraformContainer(SSOContainer):
"""Leverage container specifically tailored to run Terraform commands.
It handles authentication and some checks regarding where the command is being executed."""

TF_BINARY = "/bin/terraform"

TF_MFA_ENTRYPOINT = "/root/scripts/aws-mfa/aws-mfa-entrypoint.sh"
TF_SSO_ENTRYPOINT = "/root/scripts/aws-sso/aws-sso-entrypoint.sh"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the SSO script was unused, is part of the CLI (in python) already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were moved from the toolbox into the CLI, and are mounted into the container when we run commands.
That give us a better control and an easier way to introduce modifications over them.

@Franr Franr changed the title 195 local user [BV-195] Run toolbox with the host user Jun 1, 2024
@@ -323,23 +313,9 @@ def _init(tf, args):

tf.paths.check_for_layer_location()

with LiveContainer(tf) as container:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

copying ssh keys and changing permissions is not required anymore, given now the user of the container and the host matches (thus permissions matches!)

@Franr Franr marked this pull request as ready for review June 1, 2024 22:17
from leverage.container import TerraformContainer


class KubeCtlContainer(TerraformContainer):
"""Container specifically tailored to run kubectl commands."""

KUBECTL_CLI_BINARY = "/usr/local/bin/kubectl"
KUBECTL_CONFIG_PATH = Path("/root/.kube")
KUBECTL_CONFIG_PATH = Path("/home/leverage/.kube")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please replace 'leverage' with the constant that defines that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 👌

@angelofenoglio angelofenoglio merged commit 2992536 into master Oct 7, 2024
31 of 33 checks passed
@exequielrafaela exequielrafaela deleted the 195-local-user branch October 11, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement | Run Toolbox as host user instead of root
6 participants