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

Moved the internal subscriptions delivery configuration to a config map #4832

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions config/brokers/mt-channel-broker/300-config-mtbroker-delivery.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Copyright 2020 The Knative Authors
slinkydeveloper marked this conversation as resolved.
Show resolved Hide resolved
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# https://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

apiVersion: v1
kind: ConfigMap
metadata:
name: mt-broker-internal-delivery
namespace: knative-eventing
labels:
eventing.knative.dev/release: devel
data:
_example: |
# Configure the internal channels subscription delivery spec
delivery: |
retry: 10
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ import (
"sigs.k8s.io/yaml"

corev1 "k8s.io/api/core/v1"
messagingv1 "knative.dev/eventing/pkg/apis/messaging/v1"
"knative.dev/pkg/logging"

messagingv1 "knative.dev/eventing/pkg/apis/messaging/v1"
slinkydeveloper marked this conversation as resolved.
Show resolved Hide resolved
)

type Config struct {
Expand Down
75 changes: 75 additions & 0 deletions pkg/reconciler/mtbroker/delivery_config.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
Copyright 2021 The Knative Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package mtbroker

import (
corev1 "k8s.io/api/core/v1"
"knative.dev/pkg/configmap"
"sigs.k8s.io/yaml"

eventingduck "knative.dev/eventing/pkg/apis/duck/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Don't we use duckv1 everywhere else, like corev1 above?

)

const (
InternalDeliveryConfigMapName = "mt-broker-internal-delivery"
internalDeliveryConfigMapDeliveryKey = "delivery"
Comment on lines +28 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for making the name public but the key private? Can't they both be private?

)

// NewInternalDeliveryConfigFromConfigMap parses the config map into a DeliverySpec
func NewInternalDeliveryConfigFromConfigMap(cm *corev1.ConfigMap) (*eventingduck.DeliverySpec, error) {
if cm == nil || len(cm.Data) == 0 {
return nil, nil
}
d, ok := cm.Data[internalDeliveryConfigMapDeliveryKey]
if !ok {
return nil, nil
}

var delivery eventingduck.DeliverySpec
err := yaml.Unmarshal([]byte(d), &delivery)
if err != nil {
return nil, err
}
return &delivery, nil
}

// InternalDeliveryConfigStore is a typed wrapper around configmap.Untyped store to handle our configmaps.
// +k8s:deepcopy-gen=false
Copy link
Contributor

@antoineco antoineco Feb 12, 2021

Choose a reason for hiding this comment

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

Is that annotation needed? As far as I can see we don't run deepcopy-gen on that package.

type InternalDeliveryConfigStore struct {
*configmap.UntypedStore
}

// NewInternalDeliveryConfigStore creates a new store of Configs and optionally calls functions when ConfigMaps are updated.
func NewInternalDeliveryConfigStore(logger configmap.Logger, onAfterStore ...func(name string, value interface{})) *InternalDeliveryConfigStore {
store := &InternalDeliveryConfigStore{
UntypedStore: configmap.NewUntypedStore(
"channeldefaults",
logger,
configmap.Constructors{
InternalDeliveryConfigMapName: NewInternalDeliveryConfigFromConfigMap,
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that NewInternalDeliveryConfigFromConfigMap can return nil but I don't see a check to fallback to a default when this is the case. Maybe I overlooked, just want to be sure you've ensured this can not cause a panic.

},
onAfterStore...,
),
}

return store
}

// Load creates a InternalDeliveryConfigStore from the current config state of the InternalDeliveryConfigStore.
func (s *InternalDeliveryConfigStore) Load() *eventingduck.DeliverySpec {
return s.UntypedLoad(InternalDeliveryConfigMapName).(*eventingduck.DeliverySpec)
}
64 changes: 64 additions & 0 deletions pkg/reconciler/mtbroker/delivery_config_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
Copyright 2021 The Knative Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package mtbroker

import (
"testing"

"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
"k8s.io/utils/pointer"

eventingduck "knative.dev/eventing/pkg/apis/duck/v1"

. "knative.dev/pkg/configmap/testing"
Copy link
Contributor

Choose a reason for hiding this comment

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

Wherever possible it would be nice to avoid . imports. Even something short such as cmt or cmtesting helps with readability I think.

)

func TestMtBrokerInternalConfig(t *testing.T) {
_, example := ConfigMapsFromTestFile(t, "config-mtbroker-delivery")

for _, tt := range []struct {
name string
fail bool
want *eventingduck.DeliverySpec
data *corev1.ConfigMap
}{{
name: "Nil config",
fail: false,
want: nil,
data: nil,
}, {
name: "Empty config",
fail: false,
want: nil,
data: &corev1.ConfigMap{},
}, {
name: "With values",
fail: false,
want: &eventingduck.DeliverySpec{Retry: pointer.Int32Ptr(10)},
data: example,
}} {
t.Run(tt.name, func(t *testing.T) {
testConfig, err := NewInternalDeliveryConfigFromConfigMap(tt.data)
if tt.fail != (err != nil) {
t.Fatal("Unexpected error value:", err)
}

require.Equal(t, tt.want, testConfig)
})
}
}
24 changes: 24 additions & 0 deletions pkg/reconciler/mtbroker/testdata/config-mtbroker-delivery.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Copyright 2020 The Knative Authors
slinkydeveloper marked this conversation as resolved.
Show resolved Hide resolved
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# https://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

apiVersion: v1
kind: ConfigMap
metadata:
name: mt-broker-internal-delivery
namespace: default

data:
_example: |
delivery: |
retry: 10
4 changes: 4 additions & 0 deletions pkg/reconciler/mtbroker/trigger/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ import (
brokerreconciler "knative.dev/eventing/pkg/client/injection/reconciler/eventing/v1/broker"
triggerreconciler "knative.dev/eventing/pkg/client/injection/reconciler/eventing/v1/trigger"
"knative.dev/eventing/pkg/duck"
"knative.dev/eventing/pkg/reconciler/mtbroker"

"knative.dev/pkg/client/injection/ducks/duck/v1/source"
configmapinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/configmap"
"knative.dev/pkg/configmap"
Expand Down Expand Up @@ -68,6 +70,8 @@ func NewController(

r.sourceTracker = duck.NewListableTracker(ctx, source.Get, impl.EnqueueKey, controller.GetTrackerLease(ctx))
r.uriResolver = resolver.NewURIResolver(ctx, impl.EnqueueKey)
r.internalDeliveryConfigStore = mtbroker.NewInternalDeliveryConfigStore(logger)
r.internalDeliveryConfigStore.WatchConfigs(cmw)

triggerInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))

Expand Down
14 changes: 13 additions & 1 deletion pkg/reconciler/mtbroker/trigger/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,33 @@ package mttrigger
import (
"testing"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/configmap"
. "knative.dev/pkg/reconciler/testing"
"knative.dev/pkg/system"

// Fake injection informers
_ "knative.dev/eventing/pkg/client/injection/informers/eventing/v1/broker/fake"
_ "knative.dev/eventing/pkg/client/injection/informers/eventing/v1/trigger/fake"
_ "knative.dev/eventing/pkg/client/injection/informers/messaging/v1/subscription/fake"
"knative.dev/eventing/pkg/reconciler/mtbroker"
Comment on lines 28 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

This import should be relocated, it is not a "Fake injection informer" so future readers will be confused.


_ "knative.dev/pkg/client/injection/ducks/duck/v1/source/fake"
_ "knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/fake"
)

func TestNew(t *testing.T) {
ctx, _ := SetupFakeContext(t)

c := NewController(ctx, configmap.NewStaticWatcher())
cmw := configmap.NewStaticWatcher(&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: mtbroker.InternalDeliveryConfigMapName,
Namespace: system.Namespace(),
},
Comment on lines +41 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

Good candidate for rectesting.NewConfigMap I think.

})

c := NewController(ctx, cmw)

if c == nil {
t.Fatal("Expected NewController to return a non-nil value")
Expand Down
23 changes: 14 additions & 9 deletions pkg/reconciler/mtbroker/trigger/trigger.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,23 +29,25 @@ import (
"k8s.io/client-go/dynamic"
corev1listers "k8s.io/client-go/listers/core/v1"

"knative.dev/pkg/apis"
duckv1 "knative.dev/pkg/apis/duck/v1"
"knative.dev/pkg/controller"
"knative.dev/pkg/logging"
"knative.dev/pkg/network"
pkgreconciler "knative.dev/pkg/reconciler"
"knative.dev/pkg/resolver"
"knative.dev/pkg/system"

"knative.dev/eventing/pkg/apis/eventing"
eventingv1 "knative.dev/eventing/pkg/apis/eventing/v1"
messagingv1 "knative.dev/eventing/pkg/apis/messaging/v1"
clientset "knative.dev/eventing/pkg/client/clientset/versioned"
eventinglisters "knative.dev/eventing/pkg/client/listers/eventing/v1"
messaginglisters "knative.dev/eventing/pkg/client/listers/messaging/v1"
"knative.dev/eventing/pkg/duck"
"knative.dev/eventing/pkg/reconciler/mtbroker"
"knative.dev/eventing/pkg/reconciler/mtbroker/resources"
"knative.dev/eventing/pkg/reconciler/sugar/trigger/path"
"knative.dev/pkg/apis"
duckv1 "knative.dev/pkg/apis/duck/v1"
"knative.dev/pkg/controller"
"knative.dev/pkg/logging"
"knative.dev/pkg/network"
pkgreconciler "knative.dev/pkg/reconciler"
"knative.dev/pkg/resolver"
"knative.dev/pkg/system"
)

var brokerGVK = eventingv1.SchemeGroupVersion.WithKind("Broker")
Expand All @@ -67,6 +69,9 @@ type Reconciler struct {
triggerLister eventinglisters.TriggerLister
configmapLister corev1listers.ConfigMapLister

// Internal Delivery store
internalDeliveryConfigStore *mtbroker.InternalDeliveryConfigStore

// Dynamic tracker to track Sources. In particular, it tracks the dependency between Triggers and Sources.
sourceTracker duck.ListableTracker

Expand Down Expand Up @@ -163,7 +168,7 @@ func (r *Reconciler) subscribeToBrokerChannel(ctx context.Context, b *eventingv1
Name: b.Name,
Namespace: b.Namespace,
}
expected := resources.NewSubscription(t, brokerTrigger, brokerObjRef, uri, b.Spec.Delivery)
expected := resources.NewSubscription(t, brokerTrigger, brokerObjRef, uri, r.internalDeliveryConfigStore.Load())
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm little confused why we need to add the ConfigMap here. As discussed in the spec PR, I thought the logic here would simply be (something like this):
del := t.Spec.Delivery
if del == nil {
del = b.Spec.Delivery
}
expected := resources.NewSubscription(t, brokerTrigger, brokerObjRef, uri, del)

if (t.Spec.Delivery != nil

Copy link
Contributor Author

@slinkydeveloper slinkydeveloper Feb 3, 2021

Choose a reason for hiding this comment

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

This subscription is an internal subscription between broker components, the actual dispatch to the service is done by the filter handler.

Now, with #4654 we're saying BrokerSpec.Delivery (same for TriggerSpec.Delivery) configures the trigger delivery, hence the hop between the filter_handler and the actual service. On the other hand, this subscription is an internal hop between broker components.

I've added a config map for that to allow the user to continue to modify this internal delivery spec, because in another PR i'll actually implement in mtbroker the delivery spec as stated by #4654

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look at the description of this issue: #4515


sub, err := r.subscriptionLister.Subscriptions(t.Namespace).Get(expected.Name)
// If the resource doesn't exist, we'll create it.
Expand Down
Loading