-
Notifications
You must be signed in to change notification settings - Fork 325
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
Fixes bug #1787 non-k8s: KubeArmor panics when not-enabled policy type is received #1789
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @itsCheithanya!
Minor changes to address... Looking good otherwise.
Signed-off-by: Cheithanya <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the changes.
Please see the comments inline @itsCheithanya
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you build the code after adding this change?
Just try running make build
once and you'll see come changes in policy.pb.go
in the protobuf directory. Please add that as well.
And then you can use the generated constant in pb.PolicyStatus_NotEnabled
in policy.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh okay my bad missed that will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, @itsCheithanya once we merge this PR we'll need to update the proto
package in kubearmor-client so that it can understand this error code.
For now it's fine, let's do it after we address all the changes and merge this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itsCheithanya you have to build the client after updating the github.com/kubearmor/KubeArmor/protobuf
dependecy in karmor client.
res := new(pb.Response) | ||
if !p.HostPolicyEnabled { | ||
res.Status = pb.PolicyStatus_Invalid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res.Status = pb.PolicyStatus_Invalid | |
res.Status = pb.PolicyStatus_NotEnabled |
This fixes the bug that KubeArmor panics when not-enabled policy type is being received
Reproduce the issue :
Modify make run target in KubeArmor's Makefile to something like below such that host policy is disabled:
Once KubeArmor is running, send the below host policy by running
karmor vm policy --gRPC=:32767 add <path-to-policy>
:You'll see that KubeArmor doesn't panic :
Fixes #1787
If the changes in this PR are manually verified, list down the scenarios covered::
Additional information for reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs
Checklist:
<type>(<scope>): <subject>