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.

Sign up for GitHub

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

Jump to bottom

chore: log permissions external command #5027

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

chore: log permissions external command #5027

wants to merge 10 commits into from
+15 −2

Conversation

chadinwork
Copy link

@chadinwork chadinwork commented Oct 23, 2024 •
edited
Loading

what

  • Add logs for when permissions external command decides user does not have permission (Example: command '/etc/scripts/authorization.sh apply repo/here ' returns 'the custom script echo goes here' such as command '/etc/scripts/authorization.sh apply repo/here ' returns 'user \"chadin\" must be a member of \"alice, bob\" to apply changes.')
  • Add logs for when permissions external command errors out (Example: Command returns exit code 1: Command '/etc/scripts/authorization.sh apply repo/here ' error 'exit status 1: running \"sh -c /etc/scripts/authorization.sh apply repo/here \": \nuser \"chadin\" must be a member of \"xx, yy\" to apply changes.\n')
  • Add logs for when user does not have permission (Example: User 'chadin' in team '[]' does not have permissions to execute the 'apply' command)

why

  • Increase transparency when permissions external command concludes that user does not have permission

tests

  • I have tested my changes by running make test and make test-all in Docker. All tests passed
  • Give me a while to run manual tests to confirm the logs are actually emitted (need just a bit more time to set up test Terraform repo) Tested and verified working!

references

@chadinwork chadinwork requested review from a team as code owners October 23, 2024 13:52
@chadinwork chadinwork requested review from chenrui333, lukemassa and X-Guardian and removed request for a team October 23, 2024 13:52
@github-actions github-actions bot added the go Pull requests that update Go code label Oct 23, 2024
@dosubot dosubot bot added the refactoring Code refactoring that doesn't add additional functionality label Oct 23, 2024
@chadinwork chadinwork force-pushed the log-permissions-external-command branch from 03b61b1 to 3219b58 Compare October 23, 2024 13:56
Chadin Anuwattanaporn added 2 commits October 23, 2024 21:57
chore: add logs from permissions external command execution
0704e5d 
Signed-off-by: Chadin Anuwattanaporn <chadin_anuwattanaporn@tech.gov.sg>
chore: add logs when user is not allowed to execute the command
45d47e0 
Signed-off-by: Chadin Anuwattanaporn <chadin_anuwattanaporn@tech.gov.sg>
@chadinwork chadinwork force-pushed the log-permissions-external-command branch from 3219b58 to 45d47e0 Compare October 23, 2024 13:58
@X-Guardian
Copy link
Contributor

Thanks for this @chadinwork. Can you use '%s' rather than %q in the log messages to use single quotes though, as the double quotes need to be escaped in the JSON logs and make it harder to read.

@X-Guardian X-Guardian added the waiting-on-response Waiting for a response from the user label Oct 23, 2024
Chadin Anuwattanaporn added 2 commits October 24, 2024 11:01
chore: update logs to use '%s'
5d1f93f 
Signed-off-by: Chadin Anuwattanaporn <chadin_anuwattanaporn@tech.gov.sg>
chore: add logs when permissions external command execution errors out
c959b14 
Signed-off-by: Chadin Anuwattanaporn <chadin_anuwattanaporn@tech.gov.sg>
@chadinwork
Copy link
Author

Sure @X-Guardian, I was following https://github.com/runatlantis/atlantis/blob/main/CONTRIBUTING.md which asked for %q.

Updated!

@X-Guardian
Copy link
Contributor

Sure @X-Guardian, I was following https://github.com/runatlantis/atlantis/blob/main/CONTRIBUTING.md which asked for %q.

Good spot. I'll get that updated.

X-Guardian
X-Guardian requested changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@X-Guardian X-Guardian left a comment

Choose a reason for hiding this comment

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

Can you also add some sample log output from these changes to the PR description, so that we can see what they look like.

server/events/command_runner.go Outdated Show resolved Hide resolved
server/events/external_team_allowlist_checker.go Outdated Show resolved Hide resolved
server/events/external_team_allowlist_checker.go Outdated Show resolved Hide resolved
chadinwork and others added 2 commits October 25, 2024 10:35
@chadinwork @X-Guardian
chore: apply suggestions from code review
c201f6f 
Co-authored-by: Simon Heather <32168619+X-Guardian@users.noreply.github.com>
Signed-off-by: C <92499464+chadinwork@users.noreply.github.com>
@chadinwork
Merge branch 'main' into log-permissions-external-command
60e87b5
@chadinwork
Copy link
Author

chadinwork commented Oct 25, 2024 •
edited
Loading

@X-Guardian I've applied suggested changes, and added examples. Let me know if anything else is needed!

FYI: I'm out of town from tomorrow onwards for one week, so in case I miss your replies before I go, I'll come back and attend to them after I return.

X-Guardian
X-Guardian reviewed Nov 2, 2024
View reviewed changes
server/events/external_team_allowlist_checker.go
return checker.checkOutputResults(out)
outputResults := checker.checkOutputResults(out)
if !outputResults {
ctx.Log.Info("command '%s' returns '%s'", cmd, out)
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 convinced that these should be Info messages. I think Debug would be better, so they only display when an Atlantis command is run with the -verbose flag . Same below.

Your examples in the description aren't useful, as they are made up. Please can you provide real world usage messages.

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 8, 2024

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.

@github-actions github-actions bot added the Stale label Dec 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@X-Guardian X-Guardian X-Guardian requested changes

@chenrui333 chenrui333 Awaiting requested review from chenrui333 chenrui333 is a code owner automatically assigned from runatlantis/maintainers

@lukemassa lukemassa Awaiting requested review from lukemassa lukemassa is a code owner automatically assigned from runatlantis/core-contributors

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
go Pull requests that update Go code refactoring Code refactoring that doesn't add additional functionality Stale waiting-on-response Waiting for a response from the user
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Log command and output of Repo and Project Permissions: External Command
2 participants
@chadinwork @X-Guardian