create target config proposal

[aerosouund]

Upstream issue: kyverno/policy-reporter#436

[fjogeleit]

you should consider to use different keys per target. config would be to generic, makes it hard to describe which field is required for which target or even available for a given target type. Makes also the use of the explain command for a given target type easier.

[aerosouund]

I was thinking of this as well, the benefit of doing this was simplicity in my opinion.
if we want to be more descriptive we can use s3Config, slackConfig.. etc. and the spec can contain any single one of those keys

[fjogeleit]

Do you mean different CRDs per Target? Might be to much. As in my example you can use spec.s3, spec.slack etc. to group target specfic configuration. You can still keep global configurations like filter or secretRef directly under spec.

[aerosouund]

No, same CRD. but its spec is defined as

spec:
  # general keys
  s3: ...  # you can only have one of these
  slack: ...

and we can include the general keys separate from each individual config, i suggested this in case at any point in the future we created a struct that an array of those individual configs. in such a case each array item will be its own full config. what do you think ?

[fjogeleit]

looks better to me.

in such a case each array item will be its own full config. what do you think ?

could be a future feature but nothing I would prioritize because it makes many things just more complicated without much benefit. We should keep it simple for now.

[fjogeleit]

its not always the case that a send relates to a single report. This is only the case if the target supports batch send and the policy report only relates to a single resource. Keep in mind that policy reporter also supports different tools which have different approaches.

[fjogeleit]

You could consider to create Kubernetes Events instead.

[aerosouund]

You mean to use the IDs of k8s events as the identifier ?

[fjogeleit]

I mean you create an Kubernetes Event resource when a notification was send or failed instead of updating the status for the target.

[fjogeleit]

example

Suggested change

	apiVersion: policyreporter.io/v1alpha1
	kind: TargetConfig
	metadata:
	name: slack-target
	spec:
	targetType: slack-inline
	config:
	minimumSeverity: warning
	skipExistingOnStartup: true
	webhook: "https://hooks.slack.com/services/456..."
	filter:
	namespaces:
	include: ["team-b-*"]
	apiVersion: policyreporter.io/v1alpha1
	kind: Target
	metadata:
	name: slack-target
	spec:
	slack:
	webhook: "https://hooks.slack.com/services/456..."
	filter:
	namespaces:
	include: ["team-b-*"]
	minimumSeverity: warning
	skipExistingOnStartup: true

[aerosouund]

yes, i will commit those suggestions if we are settled on the structure in the above conversation. in general i do agree with you that it makes the crd more readable