fix: add condition in method ensure_kubeconfig! #1964
Conversation
barmull
requested review from
denverwilliams,
wvwatson and
agentpoyo
as code owners
barmull
force-pushed
the
config
branch
3 times, most recently
from
52f3cc1
to
fc7ed95
Compare
|
lgtm |
|
⬆️ @wavell @denverwilliams take a look and merge if you 👍🏻 |
There was a problem hiding this comment.
@barmull I tested this, seems when KUBECONFIG is not set, it's defaulting to the existing output message Could not create cnf-testsuite namespace on the Kubernetes cluster... see below where I unset KUBECONFIG:
pair@cnfdev4:~/workspace/drew/testsuite-barmull$ echo $KUBECONFIG
/home/pair/workspace/drew/blah
pair@cnfdev4:~/workspace/drew/testsuite-barmull$ unset KUBECONFIG
pair@cnfdev4:~/workspace/drew/testsuite-barmull$ echo $KUBECONFIG
pair@cnfdev4:~/workspace/drew/testsuite-barmull$ ./cnf-testsuite setup
CNF TestSuite version: HEAD-2024-04-20-204358-4cfa4202
Successfully created directories for cnf-testsuite
Global helm found. Version: v3.11.1
No Local helm version found
Global kubectl found. Version: 1.26
Global kubectl client version could not be checked for compatibility with server. (Server not configured?)
No Local kubectl version found
Global git found. Version: 2.34.1
No Local git version found
All prerequisites found.
KUBECONFIG is set as /home/pair/.kube/config.
Could not create cnf-testsuite namespace on the Kubernetes cluster
Setup complete
This is likely due to KUBECONFIG is set as /home/pair/.kube/config.
When I remove the local .kube/config for my user, the result output is this:
$ ./cnf-testsuite setup
CNF TestSuite version: HEAD-2024-04-20-204914-4cfa4202
Successfully created directories for cnf-testsuite
Global helm found. Version: v3.11.1
No Local helm version found
Global kubectl found. Version: 1.26
Global kubectl client version could not be checked for compatibility with server. (Server not configured?)
No Local kubectl version found
Global git found. Version: 2.34.1
No Local git version found
All prerequisites found.
KUBECONFIG is not set and default path /home/pair/.kube/config does not exist. Please set KUBECONFIG to an existing config file, i.e. 'export KUBECONFIG=path-to-your-kubeconfig'
src/tasks/utils/utils.cr:52:45 in 'ensure_kubeconfig!'
src/tasks/setup.cr:13:3 in '->'
lib/sam/src/sam/task.cr:54:39 in 'call'
lib/sam/src/sam/execution.cr:20:7 in 'invoke'
lib/sam/src/sam/task.cr:44:29 in 'call'
lib/sam/src/sam/execution.cr:20:7 in 'invoke'
lib/sam/src/sam.cr:35:5 in 'invoke'
lib/sam/src/sam.cr:53:7 in 'process_tasks'
src/cnf-testsuite.cr:132:3 in '__crystal_main'
/home/pair/.crenv/versions/1.6.2/share/crystal/src/crystal/main.cr:115:5 in 'main_user_code'
/home/pair/.crenv/versions/1.6.2/share/crystal/src/crystal/main.cr:101:7 in 'main'
/home/pair/.crenv/versions/1.6.2/share/crystal/src/crystal/main.cr:127:3 in 'main'
/lib/x86_64-linux-gnu/libc.so.6 in '??'
/lib/x86_64-linux-gnu/libc.so.6 in '__libc_start_main'
./cnf-testsuite in '_start'
???
KUBECONFIG is not set and default path /home/pair/.kube/config does not exist. Please set KUBECONFIG to an existing config file, i.e. 'export KUBECONFIG=path-to-your-kubeconfig'
Here are the outputs when I export KUBECONFIG to a non working config and then a working config for KUBECONFIG:
pair@cnfdev4:~/workspace/drew/testsuite-barmull$ export KUBECONFIG=/home/pair/workspace/drew/3node.kubeconfig
pair@cnfdev4:~/workspace/drew/testsuite-barmull$ ./cnf-testsuite setup
CNF TestSuite version: HEAD-2024-04-20-205225-4cfa4202
Successfully created directories for cnf-testsuite
Global helm found. Version: v3.11.1
No Local helm version found
Global kubectl found. Version: 1.26
No Local kubectl version found
Global git found. Version: 2.34.1
No Local git version found
All prerequisites found.
KUBECONFIG is already set.
cnf-testsuite namespace already exists on the Kubernetes cluster
Setup complete
pair@cnfdev4:~/workspace/drew/testsuite-barmull$ export KUBECONFIG=/home/pair/workspace/drew/blah
pair@cnfdev4:~/workspace/drew/testsuite-barmull$ ./cnf-testsuite setup
CNF TestSuite version: HEAD-2024-04-20-205248-4cfa4202
Successfully created directories for cnf-testsuite
Global helm found. Version: v3.11.1
No Local helm version found
Global kubectl found. Version: 1.26
Global kubectl client version could not be checked for compatibility with server. (Server not configured?)
No Local kubectl version found
Global git found. Version: 2.34.1
No Local git version found
All prerequisites found.
KUBECONFIG is set to /home/pair/workspace/drew/blah path and it does not exist. Please set KUBECONFIG to an existing config file, i.e. 'export KUBECONFIG=path-to-your-kubeconfig'
src/tasks/utils/utils.cr:54:5 in 'ensure_kubeconfig!'
src/tasks/setup.cr:13:3 in '->'
lib/sam/src/sam/task.cr:54:39 in 'call'
lib/sam/src/sam/execution.cr:20:7 in 'invoke'
lib/sam/src/sam/task.cr:44:29 in 'call'
lib/sam/src/sam/execution.cr:20:7 in 'invoke'
lib/sam/src/sam.cr:35:5 in 'invoke'
lib/sam/src/sam.cr:53:7 in 'process_tasks'
src/cnf-testsuite.cr:132:3 in '__crystal_main'
/home/pair/.crenv/versions/1.6.2/share/crystal/src/crystal/main.cr:115:5 in 'main_user_code'
/home/pair/.crenv/versions/1.6.2/share/crystal/src/crystal/main.cr:101:7 in 'main'
/home/pair/.crenv/versions/1.6.2/share/crystal/src/crystal/main.cr:127:3 in 'main'
/lib/x86_64-linux-gnu/libc.so.6 in '??'
/lib/x86_64-linux-gnu/libc.so.6 in '__libc_start_main'
./cnf-testsuite in '_start'
???
KUBECONFIG is set to /home/pair/workspace/drew/blah path and it does not exist. Please set KUBECONFIG to an existing config file, i.e. 'export KUBECONFIG=path-to-your-kubeconfig'
@taylor here are my findings: #1964 (review) |
|
Everything from the output is behaving expected. In this PR: #1995 is an improvement to catch one condition:
|
The output when the kubeconfig file configured is empty is too generic, it doesn't detail the exact issue. |
This error message is not part of ensure_kubeconfig method, probably there are some missing prerequisites for setup. I tried to set KUBECONFIG to existing empty file and got this output (this behavior is expected): It fails on this error message: 'Could not create cnf-testsuite namespace on the Kubernetes cluster' --> which means that cluster is not healthy. That's why I added an improvement that checks if cluster is up and running. (PR:#1995) |
|
Looks good to me accordingly to intentions and scope of this PR. |
|
@barmull @martin-mat what @agentpoyo was trying to do was do a QA to see if the binary was acting the way it was intending. He was saying the output was not as expected. Having Acceptance Crietia would help with testing. This PR references the bug #985. The stack trace is not suppressed in the instances tested which was what the bug was about. @martin-mat if the scope of the PR is not supposed to cover the bug that was referenced it should probably reference a new issue for what is in this PR. The acceptance criteria should be added for the intention of the PR. When Drew is doing QA he tries to go through things like an end user with different scenarios. Since nothing was written for the different scenarios he looked at the code and tried to guess at the intentions to think about acceptance criteria. Example of acceptance criteria, which could go into the issue and the PR should allow someone to follow the acceptance criteria and get the expected results. Common steps
I would expect Falco to successfully install when the falco setup command runs:
I would expect to PASSED for non_root_user test when running against a CNF which does not run processes as root:
I would expect to see a FAILED result for the non_root_user test, when running against CNF which has processes running as the root :
|
The stack trace after the error message should be suppressed. End users should not see crystal stack traces if cnf_setup (or setup, which I stumbled upon before even getting to cnf_setup, hence why I stopped testing because it seemed to have gotten worse as there was no stack trace in the current code when no cluster connection was found, see output below compared to my output above when running Users who previously setup the testsuite could lose their config and without this code it defaults to say it could not create the namespace. A simple update to the display error detailing to "check your kubeconfig or cluster connection" is likely sufficient enough while suppressing the stack trace output that the original bug was pointing out. This code does not suppress the stack trace from output which was the original intent of the bug. |
OK, this is a good point and stack trace should not be printed out to console output (perhaps only to an error log)
This is not scope of bug report #985. As already stated in previous comments, a new ticket was written for it (#1968) and a different pull request addresses it (#1995) |
this PR is intended to address the bug report. I think there was not obvious to me that @agentpoyo addresses by his test report displaying backtrace to the console below the error message. That should be removed, I agree. |
Yes, I was just mentioning it since this one and the other kind of go hand in hand. Regardless, this PR does not address the stack trace output and the new one created is kind of redundant since we already had checks in place to detail when a cluster is not reachable in a sense. Cheers. |
- add one more elsif condition in case variable
KUBECONFIG is not set (empty)
- change error messages
Signed-off-by: barmull <barbora.muller@tietoevry.com>
|
Did the change. It should be without back trace now. This is output when KUBECONFIG is unset and default path does not exist: This is output when KUBECONFIG is set to non-existing file: |
|
@agentpoyo please re-review. The stacktrace removed. |
|
LGTM |
|
gh actions passed |
|
Prior comments addressed. Merging. |
Description
Issues:
Refs: #985
How has this been tested:
Types of changes:
Checklist:
Documentation
Code Review
Issue