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

Adding the option to use a new crd modules in policies #2

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

Conversation

lieberlois
Copy link
Contributor

This PR introduces modules. A module can be created using a CRD like this:

apiVersion: bridgekeeper.maibornwolff.de/v1alpha1
kind: Module
metadata:
  name: test-module
spec:
  python: |
    def module_demo():
      print("Hello world!")

Now, this module can be used within a policy like this:

apiVersion: bridgekeeper.maibornwolff.de/v1alpha1
kind: Policy
metadata:
  name: test-policy
spec:
  audit: true
  enforce: false
  target:
    matches:
      - apiGroup: "apps"
        kind: "Deployment"
  modules:
  - test-module
  rule:
    python: |
      module_demo()
      def validate(request):
        return True

Upon evaluation, the modules are loaded and their code is prepended to the actual policy python code. In this case, you can see the "Hello, world" statement within the bridgekeeper logs:

image

Note that you can see the print statement twice. This is because the policy gets run in the validating webhook, which ensures that all references modules are present. The second print statement comes from the kubectl apply of a new deployment.

From the architecture side, this PR also attempts to generalize behavior of the PolicyStore and the PolicyEvent type using pattern matching and enums. Existing modules are stored in memory using a HashMap combined with a watcher task.

Copy link
Collaborator

@swoehrl-mw swoehrl-mw left a comment

Choose a reason for hiding this comment

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

The new CRD must be added to the ClusterRole in charts/bridgekeeper/templates/rbac.yaml.

IMO it makes more sense to put the module into the rule field in the CRD:

apiVersion: bridgekeeper.maibornwolff.de/v1alpha1
kind: Policy
metadata:
  name: test-policy
spec:
  rule:
    modules:
      - test-module
    python: |
      module_demo()
      def validate(request):
        return True

Regarding how modules are used:

I think it would be more python-idiomatic to provide the modules as python modules to be imported. This can be done with Pyo3:

Python::with_gil(|py| {
    let _module_code = PyModule::from_code(py, "def p():\n  print('abc from mymod')", "mymod.py", "mymod").unwrap();
    let rule_code = PyModule::from_code(py, "import mymod\ndef validate():\n  mymod.p()", "rule.py", "bridgekeeper").unwrap();
    let func = rule_code.getattr("validate").unwrap();
    func.call0().unwrap();
});

But one problem I see is that the python interpreter is valid for the entire bridgekeeper process and is reused. And this means any modules created are kept so another run could import a module even if it was not explicitly declared and there is no way to unload modules. We could mitigate this by giving modules random names each run and generating a import x as y statement at the top:

Python::with_gil(|py| {
    let _module_code = PyModule::from_code(py, "def p():\n  print('abc from mymod')", "mymod.py", "mymod123").unwrap();
    let rule_code = PyModule::from_code(py, "import mymod123 as mymod\ndef validate():\n  mymod.p()", "rule.py", "bridgekeeper").unwrap();
    let func = rule_code.getattr("validate").unwrap();
    func.call0().unwrap();
});

But that would preclude people doing things like from y import a.

I'm not really sure if this is worth the hassle. Opinions?

src/api.rs Outdated
let (allowed, reason) = validate_policy_admission(&admission_request);
let mut module_code = String::new();

if let Some(policy) = &admission_request.object {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be cleaner to move the code to collect the modules into the validate_policy_admission function and to make that function into a method of the PolicyEvaluator to have access to all the needed state.

}

impl ObjectReference {
pub fn to_object_reference(&self) -> KubeObjectReference {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe rename to to_k8s_object_reference to make it clearer what this method produces?

src/evaluator.rs Outdated
module_code.push_str("\n");
},
None => {
log::warn!("Could not find module '{}'", module_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think in this case it's better to directly stop the evaluation and send an error to the user.

src/policy.rs Outdated
@@ -19,33 +20,13 @@ pub struct PolicyStore {
pub policies: HashMap<String, PolicyInfo>,
}

pub type PolicyStoreRef = Arc<Mutex<PolicyStore>>;
pub type PolicyStoreRef = Arc<Mutex<dyn ObjectStore<Policy, HashMap<String, PolicyInfo>> + Send>>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no need to use the trait here.
Arc<Mutex<PolicyStore>> works as well and is faster because there is no dynamic dispatch.

src/module.rs Outdated
pub modules: HashMap<String, ModuleInfo>,
}

pub type ModuleStoreRef = Arc<Mutex<dyn ObjectStore<Module, HashMap<String, ModuleInfo>> + Send>>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no need to use the trait here.

@@ -0,0 +1,7 @@
use crate::util::types::ObjectReference;

pub trait ObjectStore<T, V> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if using a trait is actually beneficial. At the moment all code that uses either policies or modules is specific for that so we cannot really take advantage of the trait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants