feat: add ability to restrict required-probes constraint to containers in pods selected by a service

[ctrought]

Signed-off-by: Craig Trought [email protected]

What this PR does / why we need it:
Adds a parameter to restrict enforcement of the constraint to containers in pods that are selected by a service with at least one matching targetPort. For workloads that do not accept incoming traffic in pods it's common to not have readiness probes. This will allow the user to avoid generating violations for pods that do not accept incoming traffic via services. To leverage this services must be cached. This is a non-breaking change as the original implementation without caching is still supported by omitting the new parameter.

Adds a parameter to set an optional custom violation string that is appended to the standard one. I can revert this if it's not desired for the library, but we like to tailor certain violation messages with additional context so having the ability to do that in the constraint itself provides us the flexibility to do that without needing to alter the constraint template itself.

Removes the pod name from the violation message to avoid issues with expanded objects that likely won't contain a name. Seen in open-policy-agent/gatekeeper#2485

[maxsmythe]

The Rego LGTM, haven't taken a closer look at the rest yet.

@apeabody @ekitson

Here is an example of a constraint template where referential data is only needed for certain configurations.

[JaydipGabani]

Few nits, but otherwise LGTM

[sozercan]

@ctrought thanks for the PR! Looks like there are conflicts, can you resolve when you get a chance?

[ctrought]

Thanks @sozercan , sorry for the delay.. I've resolved the merge conflicts.

[stale]

This issue/PR has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[apeabody]

Thanks for the contribution @ctrought!

@sozercan The CI is now passing

[apeabody]

Thanks for the contribution @ctrought - When you get a chance, I think there are just a few very minor open items above.

[maxsmythe]

Rego code and idea LGTM (did not look at rest of PR structure, tests, etc., relying on other reviewers there)

[sozercan]

seems like we have samples in artifacthub but not here? is that intended?

[ritazh]

To clarify a bit around the UX, with the onlyServices parameter, it seems syncing service is optional, however does this annotation make syncing service a requirement?

[maxsmythe]

@apeabody this is a good question. How do we deal with conditionally referential templates? Should we just have a separate template, instead?

[apeabody]

Creating a separate template is an option, although I believe our goal should be to minimize the overall quantity of templates.

The current release Gatekeeper doesn't enforce requires-sync-data, and unmarshaling of the annotation was only added in v3.13.0-beta.1. Depending on the roadmap, this might not be a blocker if the future plan was only to add a warning by default if the "required" sync wasn't configured in gatekeeper?

stale based on discussion

[ritazh]

To clarify a bit around the UX, with the onlyServices parameter, it seems syncing service is optional, however does this annotation make syncing service a requirement?

Per today's community call, adding the requires-sync-data annotation to an existing template is a breaking change. The decision is to create a new template K8sRequiredProbesWithServices instead of bumping major version of the existing template, since v1 is still valuable to maintain and should not be deprecated.

@ctrought Can you please update this PR based on ^ Thank you!

[stale]

This issue/PR has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.