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

Add ExtractPodSpecFromObject #37

Merged
merged 4 commits into from
Feb 23, 2023

Conversation

KhaledEmaraDev
Copy link
Contributor

Description

New method for retrieving a PodSpec given you pass a high level object. Objects supported are: Deployment, ReplicaSet, StatefulSet, DaemonSet, ReplicationController, Job, CronJob, Pod.

This simplifies the evaluation of high level objects. A policy author just need to call this new method and evaluate the PodSpec returned. With this change pod-privileged-policy will reject high level objects (e.g Deployments) instead of allowing them and later rejects the pods

This is to map the Rust feature according to this issue.

Fix #36

Test

Additional Information

Tradeoff

Potential improvement

Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. This LGTM, I just have left an open question. Let's see what the other @kubewarden/kubewarden-developers think about it

testing/helpers.go Outdated Show resolved Hide resolved
Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

It looks good, but I would put the new ExtractPodSpecFromObject() in kubewarden.go, and not in testing/helpers.go, as this function is a normal lib function that will be consumed by policies.

I would like to have some unit tests too, analogous to https://github.com/kubewarden/policy-sdk-rust/blob/main/src/lib.rs#L397-L674. Would you be up for the task?

@KhaledEmaraDev
Copy link
Contributor Author

Thanks for this PR!

It looks good, but I would put the new ExtractPodSpecFromObject() in kubewarden.go, and not in testing/helpers.go, as this function is a normal lib function that will be consumed by policies.

I would like to have some unit tests too, analogous to https://github.com/kubewarden/policy-sdk-rust/blob/main/src/lib.rs#L397-L674. Would you be up for the task?

Yes, I'm up for it. I'll start working on it.

Signed-off-by: Khaled Emara <mail@KhaledEmara.dev>
Signed-off-by: Khaled Emara <mail@KhaledEmara.dev>
@KhaledEmaraDev
Copy link
Contributor Author

@flavio @viccuad I have implemented the tests as agreed and moved the function to the correct place.

CC: @kubewarden/kubewarden-developers

Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

On top of the comments I left, I think there's a lot of repetition inside of the kubewarden_test.go, maybe you could make everything more readable using a table driven approach (this is the 1st link I found about the topic)

kubewarden.go Outdated Show resolved Hide resolved
kubewarden_test.go Outdated Show resolved Hide resolved
kubewarden_test.go Outdated Show resolved Hide resolved
kubewarden_test.go Outdated Show resolved Hide resolved
kubewarden_test.go Outdated Show resolved Hide resolved
kubewarden_test.go Outdated Show resolved Hide resolved
kubewarden_test.go Outdated Show resolved Hide resolved
kubewarden_test.go Outdated Show resolved Hide resolved
kubewarden_test.go Outdated Show resolved Hide resolved
kubewarden_test.go Outdated Show resolved Hide resolved
Signed-off-by: Khaled Emara <mail@KhaledEmara.dev>
@KhaledEmaraDev
Copy link
Contributor Author

@flavio Resolved all comments and ran the tests successfully.

CC: @kubewarden/kubewarden-developers

@KhaledEmaraDev KhaledEmaraDev requested review from flavio and viccuad and removed request for flavio and viccuad February 23, 2023 12:05
kubewarden.go Outdated Show resolved Hide resolved
Signed-off-by: Víctor Cuadrado Juan <2196685+viccuad@users.noreply.github.com>
Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

LGTM! Many thanks for tackling this and contributing to the SDK, merging!

@viccuad viccuad merged commit e568011 into kubewarden:main Feb 23, 2023
@viccuad
Copy link
Member

viccuad commented Feb 24, 2023

This is now available on https://github.com/kubewarden/policy-sdk-go/releases/tag/v0.2.4.

@KhaledEmaraDev KhaledEmaraDev deleted the extract-pod-spec-from-object branch February 24, 2023 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants