Validate DeliverySpec in Channels & Subscriptions

[travis-minke-sap]

Fixes #5776

Proposed Changes

• 🐛 Added missing validation of DeliverySpec to Channel and Subscription.

Pre-review Checklist

• At least 80% unit test coverage
• E2E tests for any new behavior (n/a)
• Docs PR for any user-facing impact (n/a)
• Spec PR for any new API feature (n/a)
• Conformance test for any change to the spec (n/a)

I don't see any mention of what is/isn't validated in the Specs and would assume that having consistent validation of the DeliverySpec would just be expected?

Release Note

Channel and Subscription will now enforce validation of the "delivery" field.

Docs

n/a

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/messaging/v1/channel_validation.go	96.4%	96.8%	0.3
pkg/apis/messaging/v1/subscription_validation.go	97.1%	97.3%	0.2

[codecov]

Codecov Report

Merging #5777 (a62799a) into main (c39c5a6) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5777      +/-   ##
==========================================
+ Coverage   82.52%   82.54%   +0.01%     
==========================================
  Files         203      203              
  Lines        6376     6382       +6     
==========================================
+ Hits         5262     5268       +6     
  Misses        768      768              
  Partials      346      346              

Impacted Files	Coverage Δ
pkg/apis/messaging/v1/channel_validation.go	84.21% <100.00%> (+1.35%)	⬆️
pkg/apis/messaging/v1/subscription_validation.go	86.04% <100.00%> (+1.04%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c39c5a6...a62799a. Read the comment docs.

[matzew]

/lgtm
/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, travis-minke-sap

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [matzew]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[pierDipi]

/cherry-pick release-0.26

[pierDipi]

/cherry-pick release-0.25

[pierDipi]

/cherry-pick release-0.24

[knative-prow-robot]

@pierDipi: new pull request created: #5778

Details

In response to this:

/cherry-pick release-0.26

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[knative-prow-robot]

@pierDipi: new pull request created: #5779

Details

In response to this:

/cherry-pick release-0.25

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[knative-prow-robot]

@pierDipi: new pull request created: #5780

Details

In response to this:

/cherry-pick release-0.24

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[pierDipi]

@travis-minke-sap can you please give the CLA consent to the cherry-pick PRs created by the bot?

[evankanderson]

This looks like a feature addition, rather than a failure / bug / security vulnerability.

Do we have a cherrypick policy for eventing? (I don't know, just asking the question because I was surprised by the release.)

[travis-minke-sap]

Hi Evan,

I don't know the cherrypick policy and would defer to Pier as to why he initiated that. Possibly because I created the initial issue as a "bug"?

I would disagree that this is a feature... Aside from the inconsistent validation of the DeliverySpec, the enforcement of the experimental-feature flags was flat out broken for Channels / Subscriptions. I would agree it's not a security vulnerability and would also agree that it might be low-priority.

Apologies if this change is for some reason undesirable - I meant well and would appreciate any advice on how it should have been handled differently? Thanks!

[pierDipi]

This looks like a feature addition, rather than a failure / bug / security vulnerability.

IMHO, specifying an experimental feature that isn't enabled or invalid delivery parameters are bugs.
Why aren't they bugs?

Do we have a cherrypick policy for eventing? (I don't know, just asking the question because I was surprised by the release.)

No, that I'm aware of.

[evankanderson]

I agree that this was a bug, I was just wondering whether we thought it was worth recommendiing anyone who installed 0.26 to reinstall to correct this bug (which is what a patch-fix implies).

This is obviously water under the bridge for this change, my larger question/concern was whether we had documentation so that people didn't have to guess whether a PR should be cherrypicked or not.