Control the policy controller monitoring resources from chart, enhanc… #1687
Conversation
Signed-off-by: Cody Soyland <[email protected]> Signed-off-by: Senan Zedan (EXT-Nokia) <[email protected]>
…store#1683) Bumps [google-github-actions/auth](https://github.com/google-github-actions/auth) from 2.1.6 to 2.1.7. - [Release notes](https://github.com/google-github-actions/auth/releases) - [Changelog](https://github.com/google-github-actions/auth/blob/main/CHANGELOG.md) - [Commits](google-github-actions/auth@8254fb7...6fc4af4) --- updated-dependencies: - dependency-name: google-github-actions/auth dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Senan Zedan (EXT-Nokia) <[email protected]>
…ment for the current implmentation to hadd all the resources by default, The new change add avail to pass resourcesNames through the chart with list of resources comma sperataed for which resources to be monitored by the policy controller, the default is all resources if the flag wasn't presented in the chart Signed-off-by: Senan Zedan (EXT-Nokia) <[email protected]>
|
@vaikas - Could you please look into this? |
|
Could you add some tests, maybe here: https://github.com/sigstore/policy-controller/tree/main/test |
|
@vaikas - per your request, test added. |
That does not really test any of this new code. You'd have to add a test that launches policy controller with these new flags, here's one example where we change the policy-controller behaviour by the flags: So, create a new kustomize file that launches policy controller with say --resource-name=pods, and customize it like here: And then after the policy-controller has been started with the flags under test, you'd run your new test: It should at the bare minimum have a negative test and a positive test. So, if you're using resource-name pods, then launching a deployment with a failing config should succeed if you instead launch a pod. |
There was a problem hiding this comment.
Looking great! There's just some nits for naming, namespaces. One change that should be made it so make sure the tag->digest resolution does not actually happen. The webhook configuration is a good check, but it's easy enough to also make sure the actual resource (deployment) does not get modified.
| fi | ||
| echo '::endgroup::' | ||
|
|
||
| echo '::group:: Cleanup' |
There was a problem hiding this comment.
I'd add two more test cases, something like:
kubectl create deployment ... with container image that has a tag
Then kubectl get deployment and check that the container image in the spec has not been modified, and then kubectl get pod and check that the container image in the spec has been modified.
| - args: | ||
| - resource-name="pods" | ||
| - name: demo-custom-resource | ||
| image: TEST_IMAGE |
There was a problem hiding this comment.
The problem here is that the TEST_IMAGE as created has a full digest in it so it's harder to tell if it's been resolved to digest or not. I'd suggest that you have an image that has a tag, which would without this change be resolved to digest for a deployment, so creating a deployment with a tag here would be the simplest way to verify the functionality works. Then the resulting pods should have the digests resolved.
test/testdata/policy-controller/e2e/test-deployment-with-custom-resource.yaml
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1687 +/- ##
===========================================
- Coverage 52.92% 42.66% -10.27%
===========================================
Files 44 121 +77
Lines 3979 9010 +5031
===========================================
+ Hits 2106 3844 +1738
- Misses 1651 4811 +3160
- Partials 222 355 +133 ☔ View full report in Codecov by Sentry. |
|
Looks like some of the changes I requested, and you marked as fixed have not made it in to this PR 🤔 ? Also, looks like bunch of |
|
Hi Ville,
Fixed them locally and working in the last comment to add tests.
Updated PR will raise soon.
…On Mon, 9 Dec 2024 at 18:48 Ville Aikas ***@***.***> wrote:
Looks like some of the changes I requested, and you marked as fixed have
not made it in to this PR 🤔 ?
Also, looks like bunch of sed commands are failing, for example:
https://github.com/sigstore/policy-controller/actions/runs/12234409121/job/34140387122?pr=1687#step:14:132
—
Reply to this email directly, view it on GitHub
<#1687 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIDIUC3MMEW4UY6HLKCMUCL2EXCUBAVCNFSM6AAAAABRCWYFPSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMRYGY3TMOBYGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
apolofize for the delay, updated. |
…ment for the current implmentation to hadd all the resources by default, The new change add avail to pass resourcesNames through the chart with list of resources comma sperataed for which resources to be monitored by the policy controller, the default is all resources if the flag wasn't presented in the chart
This PR resolves #1388
Signed-off-by: Senan Zedan (EXT-Nokia) [email protected]
Summary
The new change add avail to pass resourcesNames through the chart with list of resources comma sperataed for which resources to be monitored by the policy controller, the default is all resources if the flag wasn't presented in the chart