google / knative-gcp

GCP event implementations to use with Knative Eventing.
https://github.com/knative/eventing
Apache License 2.0
160 stars 74 forks source link

Broker ignores spec.delivery.retry if spec.delivery.deadLetterSink is nil #2053

Closed Harwayne closed 3 years ago

Harwayne commented 3 years ago

Describe the bug Broker ignores spec.delivery.retry if spec.delivery.deadLetterSink is nil.

Expected behavior Either the Broker is rejected by the webhook as having an invalid deliverySpec, or retries is respected.

To Reproduce With the default config-br-delivery, create

apiVersion: eventing.knative.dev/v1beta1
kind: Broker
metadata:
  name: test-broker
  annotations:
    "eventing.knative.dev/broker.class": "googlecloud"
spec:
  delivery:
    retry: 33

Observe that the GCP PubSub Subscription created does not have the retry number anywhere:

{
  "name": "projects/$PROJECT/subscriptions/cre-tgr_default_hello-display_d00c77d2-7464-4213-a76a-ab63c7d4af8f",
  "topic": "projects/$PROJECT/topics/cre-tgr_default_hello-display_d00c77d2-7464-4213-a76a-ab63c7d4af8f",
  "pushConfig": {},
  "ackDeadlineSeconds": 10,
  "messageRetentionDuration": "604800s",
  "labels": {
    "resource": "triggers",
    "name": "hello-display",
    "namespace": "default"
  },
  "expirationPolicy": {
    "ttl": "2678400s"
  },
  "retryPolicy": {
    "minimumBackoff": "1s",
    "maximumBackoff": "600s"
  }
}

This is the only place that the retry value is read, and it is ignored if deadLetterSink is nil. https://github.com/google/knative-gcp/blob/87b9a08562d4019eb35522b616b67a0fbe0b9ed0/pkg/reconciler/trigger/trigger.go#L313-L321

Harwayne commented 3 years ago

Having played with the PubSub API, it seems that the API doesn't support retry number without a DLQ, so our API shouldn't allow it either.

The webhook must reject requests that (after defaulting) have a non-nil spec.delivery.retry and a nil spec.delivery.deadLetterSink.