Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate trigger filters are immutable #468

Merged
merged 1 commit into from
Oct 28, 2021

Conversation

benmoss
Copy link
Contributor

@benmoss benmoss commented Oct 20, 2021

Changes

  • 🎁 Add a validating webhook for triggers that will enforce that spec.filters are immutable

/kind enhancement

Fixes #338

Release Note

:page_facing_up: Trigger filters are now enforced to be immutable, since the underlying RMQ binding is immutable

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 20, 2021
@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 20, 2021
@benmoss
Copy link
Contributor Author

benmoss commented Oct 20, 2021

/hold

I want to see if we can actually swap the binding instead of restricting users

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 20, 2021
@codecov
Copy link

codecov bot commented Oct 20, 2021

Codecov Report

Merging #468 (6c0b1e0) into main (226facb) will decrease coverage by 0.17%.
The diff coverage is 56.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #468      +/-   ##
==========================================
- Coverage   75.99%   75.82%   -0.18%     
==========================================
  Files          44       45       +1     
  Lines        2691     2713      +22     
==========================================
+ Hits         2045     2057      +12     
- Misses        525      533       +8     
- Partials      121      123       +2     
Impacted Files Coverage Δ
pkg/apis/eventing/v1/trigger_validation.go 54.54% <54.54%> (ø)
pkg/apis/eventing/v1/broker_validation.go 81.48% <66.66%> (ø)

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 226facb...6c0b1e0. Read the comment docs.

@benmoss
Copy link
Contributor Author

benmoss commented Oct 25, 2021

/hold cancel

I think it's too much work to tackle right now, we can add immutability and change it later if we can figure out how to enable it

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2021
@benmoss
Copy link
Contributor Author

benmoss commented Oct 25, 2021

ready for review

/assign @knative-sandbox/eventing-rabbitmq-approvers

@xtreme-sameer-vohra
Copy link
Contributor

thanks @benmoss
I'll take a look at this

@xtreme-sameer-vohra
Copy link
Contributor

xtreme-sameer-vohra commented Oct 28, 2021

Test case - Update trigger and change filter

Before Change

% kn trigger create some-trigger --broker default --sink ksvc:event-display --filter source=some-source --filter type=some-type
Trigger 'some-trigger' successfully created in namespace 'default'.

% kn trigger update some-trigger --filter bar=foo
Trigger 'some-trigger' updated in namespace 'default'.

But things are not happy

% kn trigger list
NAME           BROKER    SINK                 AGE   CONDITIONS   READY   REASON
some-trigger   default   ksvc:event-display   29s   4 OK / 6     False   BindingFailure : admission webhook "vbinding.kb.io" denied the request: Binding.rabbitmq.com "t.default.some-trigger.853a4d0d-4a81-4836-88ff-cd0270365df0" is invalid: spec.arguments: Invalid value: runtime.RawExtension{Raw:[]uint8{0x7b, 0x22, 0x62, 0x61, 0x72, 0x22, 0x3a, 0x22, 0x66, 0x6f, 0x6f, 0x22, 0x2c, 0x22, 0x73, 0x6f, 0x75, 0x72, 0x63, 0x65, 0x22, 0x3a, 0x22, 0x73, 0x6f, 0x6d, 0x65, 0x2d, 0x73, 0x6f, 0x75, 0x72, 0x63, 0x65, 0x22, 0x2c, 0x22, 0x74, 0x79, 0x70, 0x65, 0x22, 0x3a, 0x22, 0x73, 0x6f, 0x6d, 0x65, 0x2d, 0x74, 0x79, 0x70, 0x65, 0x22, 0x2c, 0x22, 0x78, 0x2d, 0x6b, 0x6e, 0x61, 0x74, 0x69, 0x76, 0x65, 0x2d, 0x74, 0x72, 0x69, 0x67, 0x67, 0x65, 0x72, 0x22, 0x3a, 0x22, 0x73, 0x6f, 0x6d, 0x65, 0x2d, 0x74, 0x72, 0x69, 0x67, 0x67, 0x65, 0x72, 0x22, 0x2c, 0x22, 0x78, 0x2d, 0x6d, 0x61, 0x74, 0x63, 0x68, 0x22, 0x3a, 0x22, 0x61, 0x6c, 0x6c, 0x22, 0x7d}, Object:runtime.Object(nil)}: arguments cannot be updated


% kn trigger describe some-trigger
Name:         some-trigger
Namespace:    default
Labels:       eventing.knative.dev/broker=default
Annotations:  eventing.knative.dev/creator=kubernetes-admin, eventing.knative.dev/lastModifier=ku ...
Age:          34s
Broker:       default
Filter:
  bar:        foo
  source:     some-source
  type:       some-type

Sink:
  Name:      event-display
  Resource:  Service (serving.knative.dev/v1)

Conditions:
  OK TYPE                      AGE REASON
  !! Ready                     28s BindingFailure
  ++ BrokerReady               34s
  ++ DeadLetterSinkResolved    34s DeadLetterSinkNotConfigured
  !! DependencyReady           28s BindingFailure
  ++ SubscriberResolved        34s
  ++ SubscriptionReady         34s

With Change

% kn trigger create some-trigger --broker default --sink ksvc:event-display --filter source=some-source --filter type=some-type
Trigger 'some-trigger' successfully created in namespace 'default'.

% kn trigger update some-trigger --filter bar=foo
Error: admission webhook "validation.webhook.rabbitmq.eventing.knative.dev" denied the request: validation callback failed: Immutable fields changed (-old +new): filter, spec
{*v1.TriggerFilter}.Attributes["bar"]:
	+: "foo"

return err
}
err := b.Validate(ctx)
err := t.Validate(ctx)
if err == nil {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: is this if check required 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2021
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benmoss, xtreme-sameer-vohra

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [benmoss,xtreme-sameer-vohra]

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

@knative-prow-robot knative-prow-robot merged commit 7fed7af into knative-extensions:main Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. kind/enhancement lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add validation webhook for Trigger.
3 participants