Skip to content
This repository has been archived by the owner on Jul 25, 2022. It is now read-only.

Issue 326 sanitize logs go #425

Merged
merged 14 commits into from
Nov 16, 2020
Merged

Issue 326 sanitize logs go #425

merged 14 commits into from
Nov 16, 2020

Conversation

andrei-panov
Copy link
Contributor

@andrei-panov andrei-panov commented Nov 3, 2020

What this PR does / why we need it:
replace bash call with kubectl

Which issue(s) this PR fixes:
Fixes #326

Special notes for your reviewer:
It should be possible to follow commits to make the review. I left some methods and tests which will be deleted after review.
At the moment I can't test if it's really getting the logs because I'm missing some permission.

Release note:

@andrei-panov andrei-panov requested a review from a team as a code owner November 3, 2020 15:06
@gardener-robot gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Nov 3, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 3, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 3, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 3, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 3, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 3, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 3, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 3, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 3, 2020
@gardener-robot gardener-robot removed the needs/review Needs review label Nov 3, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 3, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 3, 2020
pkg/cmd/logs.go Outdated Show resolved Hide resolved
@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Nov 9, 2020
@gardener-robot gardener-robot added size/l Size of pull request is large (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else and removed size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Nov 10, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 10, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 10, 2020
@andrei-panov
Copy link
Contributor Author

Here I want to put some arguments about why another package is for good (subcmd/kubectl)

  1. It reduces the scope.
    Once we agreed to have all the functionality in this package than when you open the kubectl.go file you will be confident that you working only with this limited scope. You even allowed to introduce private methods not accessible outside the package. We have some private methods currently but it doesn't work since it's the same package everywhere.
  2. reduce spaghetti code
    As an example: KUBECONFIG variable setted up and readed from many different places. If you decide to collect all the logs from different places (garden, seed, shoot) in parallel, it becomes impossible because it's out of control when the variable will be updated. Get one by one is the only option for now.
  3. smaller package is more testable
    Smaller package scope gives you more confidence during refactoring.
  4. extension functions
    It's not directly related to a separate package, but once we move all the functionality related to kubectl to a dedicated package we can refactor the functions to extension functions over some structure. It will reduce the number of arguments which we pass here and there... Same way like go-client does.
    As an example, we can have KUBECONFIG structure and then call config.getLogs() or config.getNS() and execute it against different kubeconfigs in parallel. It might be treated as syntax sugar but it's present in go-land so there no reason not to use it.

I believe that more or less the same arguments might be applied to all other subcommands which we call.
So please treat it as a proposal in general.

PS:

Running Suite: Kubectl Suite
============================
Ran 2 of 2 Specs in 0.000 seconds
SUCCESS! -- 2 Passed | 0 Failed | 0 Pending | 0 Skipped
PASS
coverage: 100.0% of statements

you see... 100% for the whole suite :) It's not the goal... but who knows... what will convince you :)

@dansible
Copy link
Contributor

Hi @andrei-panov, I totally agree regarding reducing the scope of changes and making it easier to test and extend features. The issue is that these changes are outside of the scope of this PR/issue, which is for sanitizing the inputs to the logs subcommand. The repackaging work is a big refactor that should be tracked in its own issue and handled with smaller PRs that focus on one subcommand at a time. If this is something that you'd like to work on, I would encourage you to open an issue to track this either just for the kubectl subcommand or a larger umbrella issue that describes the proposed outcome for all packages.

Regarding the logs all target, the PR for that is here: #437

@gardener-robot gardener-robot added size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) and removed size/l Size of pull request is large (see gardener-robot robot/bots/size.py) labels Nov 12, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 12, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 12, 2020
@andrei-panov
Copy link
Contributor Author

@dansible Please review it

@andrei-panov andrei-panov merged commit 22930a4 into master Nov 16, 2020
@andrei-panov andrei-panov deleted the issue-326-sanitize-logs-go branch November 16, 2020 14:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs/changes Needs (more) changes needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/second-opinion Needs second review by someone else size/m Size of pull request is medium (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sanitize user input in logs.go
7 participants