Targetconfig crd

[aerosouund]

• Crd definition
• Crd storage
• Informer initialization
• Testing
• Makefile cleanup

[fjogeleit]

you should move this into a controller/client struct. The resolver is only to manage dependencies and not this kind of business logic

[aerosouund]

You are right, this is me still testing out stuff. The Informer initialization task i have in the description is to mark this exact problem

[fjogeleit]

I assume you wanted to try things out, don't forget to clean everything up again.

[fjogeleit]

is there a reason to separate crd targets from config targets?

[aerosouund]

I wanted to first write out all the flows for storing crd targets separate from the existing config and once everything is figured out merge them. but no, they both can be stored in the same map as i see the map key is just a random uuid

[fjogeleit]

the test make target is missing. Please ensure to add it again and that all tests passing

[fjogeleit]

cleanup

[fjogeleit]

I don't think this is needed, you could make S3 etc. to pointers and check which property is set. You can add a schema validation that only one of this fields can be set and not multiple ones.

[fjogeleit]

as mentioned I would make S3 etc to pointers and check if they are not nil

[aerosouund]

Wouldn't this enforce priority on some level ? if the code goes: if s3 isn't nil.. else if slack isn't nil.. etc then for a config that has both (even though this is invalid) it will make it so that s3 takes precedence over whatever. with this field i can treat them all the same and only look at the field set in TargetType and also it simplifies the validation logic. if a TargetConfig is of type s3 and the s3 field is nil then its invalid. rather than having "if all the fields are not set then the config is invalid"

[fjogeleit]

You can enforce that only one field is set on the schema, so the API Server will not allow multiple types to be set.
(https://github.com/kyverno/kyverno/blob/main/api/kyverno/v1/common_types.go#L99)

Maybe @JimBugwadia or @eddycharly has an opinion on the TargetType. I think it should not be more cumbersome if needed. If the user configures the s3 and has to set targetType might confuse people.

[coveralls]

Pull Request Test Coverage Report for Build 13371519744

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 96 of 506 (18.97%) changed or added relevant lines in 14 files are covered.
• 12 unchanged lines in 4 files lost coverage.
• Overall coverage decreased (-4.0%) to 76.851%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/listener/scope_results.go	12	13	92.31%
pkg/cache/redis.go	0	3	0.0%
pkg/listener/new_result.go	3	8	37.5%
pkg/target/client.go	1	10	10.0%
pkg/target/collection.go	3	13	23.08%
pkg/cache/memory.go	0	16	0.0%
pkg/config/resolver.go	19	55	34.55%
pkg/target/factory/factory.go	31	77	40.26%
pkg/targetconfig/tc.go	0	66	0.0%
hack/main.go	0	218	0.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/kubernetes/policy_report_client.go	1	83.08%
pkg/listener/send_result.go	1	89.47%
pkg/target/discord/discord.go	4	88.89%
pkg/listener/new_result.go	6	55.45%

Totals
Change from base Build 12974201379:	-4.0%
Covered Lines:	6424
Relevant Lines:	8359

---

💛 - Coveralls

[fjogeleit]

The TargetConfig "slack-notifier" is invalid: 
* spec.slack.certificate: Required value
* spec.slack.channel: Required value
* spec.slack.headers: Required value
* spec.slack.skipTLS: Required value

They are optional and should not be required, please check also other targets to ensure that all optional values are correctly configured.

[fjogeleit]

Slack Notifications for a new resource are published twice

[fjogeleit]

Following Scenario did not work:

• Start Policy Reporter
• Apply new Slack TargetConfiguration
• Apply new Policy
• (x) Get Pushes for new violation of existing resources (got no pushes at all)

Expected:

• Get pushes with new violations only

TargetConfig:

kind: TargetConfig
apiVersion: wgpolicyk8s.io/v1alpha1
metadata:
  name: slack-notifier
spec:
  targetType: slack
  slack:
    webhook: https://hooks.slack.com/...
    certificate: ""
    channel: "kyverno"
    headers: {}
    skipTLS: false
  skipExistingOnStartup: true