Skip to content

Conversation

@gvmw
Copy link
Contributor

@gvmw gvmw commented Nov 5, 2021

Changes

  • 🐛   Use the RabbitmqCluster namespace in cluster references, otherwise resources will fail to reconcile when RabbitMQ is running in a different namespace

When declaring a new Broker of class RabbitMQBroker, use the RabbitmqCluster namespace in RabbitmqClusterReference, otherwise the dependent objects which get declared - i.e. Exchange, Queue, Binding - and are expected to be reconciled by the https://github.com/rabbitmq/messaging-topology-operator will fail to be created.

This has more details: #454

/kind bug

Fixes #454

@knative-prow-robot knative-prow-robot added kind/enhancement do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 5, 2021
@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 5, 2021
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 5, 2021
@codecov
Copy link

codecov bot commented Nov 5, 2021

Codecov Report

Merging #492 (6b0a386) into main (c79d3aa) will decrease coverage by 0.15%.
The diff coverage is 70.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #492      +/-   ##
==========================================
- Coverage   76.13%   75.97%   -0.16%     
==========================================
  Files          46       46              
  Lines        2782     2781       -1     
==========================================
- Hits         2118     2113       -5     
- Misses        538      542       +4     
  Partials      126      126              
Impacted Files Coverage Δ
test/e2e/rabbitmqcluster.go 0.00% <ø> (ø)
pkg/reconciler/trigger/trigger.go 60.44% <44.44%> (-0.63%) ⬇️
pkg/reconciler/broker/broker.go 89.89% <100.00%> (+0.10%) ⬆️
pkg/reconciler/broker/broker_config.go 73.77% <100.00%> (+0.88%) ⬆️
pkg/reconciler/broker/resources/exchange.go 100.00% <100.00%> (ø)
pkg/reconciler/trigger/resources/binding.go 74.28% <100.00%> (+0.75%) ⬆️
pkg/reconciler/trigger/resources/queue.go 74.07% <100.00%> (+3.24%) ⬆️
pkg/adapter/message.go 88.88% <0.00%> (-3.42%) ⬇️
pkg/reconciler/broker/controller.go 97.05% <0.00%> (ø)
pkg/reconciler/trigger/controller.go 73.43% <0.00%> (ø)
... and 2 more

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 c79d3aa...6b0a386. Read the comment docs.

@gvmw
Copy link
Contributor Author

gvmw commented Nov 8, 2021

I've realised the issues with my initial approach. If resources belonging to a single namespace start getting spread, the logical namespace encapsulation gets broken and we would end up with a mess. Resources that should be close to one another aren't, which makes lifecycle events more complicated - especially deletions.

It would make a lot more sense to define the namespace property in rabbitmqClusterReference instead. This seems to be supported:

https://github.com/rabbitmq/messaging-topology-operator/blob/51f6d88b214e17b12dd31cd6b4c555c1531b3f0c/internal/cluster_reference.go#L26

I have confirmed it in the type definition too:

https://github.com/rabbitmq/messaging-topology-operator/blob/51f6d88b214e17b12dd31cd6b4c555c1531b3f0c/api/v1beta1/queue_types.go#L46-L54

Let's see what happens with this approach.

@gvmw gvmw changed the title Use the RabbitmqCluster namespace for Exchange CRDs Use RabbitmqCluster namespace in Exchange Nov 8, 2021
@gvmw gvmw changed the title Use RabbitmqCluster namespace in Exchange Use RabbitmqCluster namespace in RabbitmqClusterReference Nov 8, 2021
@knative-prow-robot knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 8, 2021
@gvmw
Copy link
Contributor Author

gvmw commented Nov 8, 2021

I think that I'm going in the wrong direction with deps. Current status:

go test -race knative.dev/eventing-rabbitmq/pkg/reconciler/broker
# knative.dev/pkg/client/injection/kube/client
vendor/knative.dev/pkg/client/injection/kube/client/client.go:1236:9: cannot use &wrapAppsV1DeploymentImpl{...} (type *wrapAppsV1DeploymentImpl) as type "k8s.io/client-go/kubernetes/typed/apps/v1".DeploymentInterface in return argument:
        *wrapAppsV1DeploymentImpl does not implement "k8s.io/client-go/kubernetes/typed/apps/v1".DeploymentInterface (missing ApplyScale method)
vendor/knative.dev/pkg/client/injection/kube/client/client.go:1253:5: cannot use (*wrapAppsV1DeploymentImpl)(nil) (type *wrapAppsV1DeploymentImpl) as type "k8s.io/client-go/kubernetes/typed/apps/v1".DeploymentInterface in assignment:
        *wrapAppsV1DeploymentImpl does not implement "k8s.io/client-go/kubernetes/typed/apps/v1".DeploymentInterface (missing ApplyScale method)
vendor/knative.dev/pkg/client/injection/kube/client/client.go:1400:5: cannot use (*wrapAppsV1ReplicaSetImpl)(nil) (type *wrapAppsV1ReplicaSetImpl) as type "k8s.io/client-go/kubernetes/typed/apps/v1".ReplicaSetInterface in assignment:
        *wrapAppsV1ReplicaSetImpl does not implement "k8s.io/client-go/kubernetes/typed/apps/v1".ReplicaSetInterface (missing ApplyScale method)
vendor/knative.dev/pkg/client/injection/kube/client/client.go:1547:5: cannot use (*wrapAppsV1StatefulSetImpl)(nil) (type *wrapAppsV1StatefulSetImpl) as type "k8s.io/client-go/kubernetes/typed/apps/v1".StatefulSetInterface in assignment:
        *wrapAppsV1StatefulSetImpl does not implement "k8s.io/client-go/kubernetes/typed/apps/v1".StatefulSetInterface (missing ApplyScale method)
vendor/knative.dev/pkg/client/injection/kube/client/client.go:2697:5: cannot use (*wrapAppsV1beta2StatefulSetImpl)(nil) (type *wrapAppsV1beta2StatefulSetImpl) as type "k8s.io/client-go/kubernetes/typed/apps/v1beta2".StatefulSetInterface in assignment:
        *wrapAppsV1beta2StatefulSetImpl does not implement "k8s.io/client-go/kubernetes/typed/apps/v1beta2".StatefulSetInterface (missing ApplyScale method)
vendor/knative.dev/pkg/client/injection/kube/client/client.go:6049:5: cannot use (*wrapCoreV1PodImpl)(nil) (type *wrapCoreV1PodImpl) as type "k8s.io/client-go/kubernetes/typed/core/v1".PodInterface in assignment:
        *wrapCoreV1PodImpl does not implement "k8s.io/client-go/kubernetes/typed/core/v1".PodInterface (missing EvictV1 method)
vendor/knative.dev/pkg/client/injection/kube/client/client.go:6170:112: undefined: "k8s.io/api/core/v1".EphemeralContainers
vendor/knative.dev/pkg/client/injection/kube/client/client.go:6174:90: undefined: "k8s.io/api/core/v1".EphemeralContainers
vendor/knative.dev/pkg/client/injection/kube/client/client.go:7808:5: cannot use (*wrapExtensionsV1beta1DeploymentImpl)(nil) (type *wrapExtensionsV1beta1DeploymentImpl) as type "k8s.io/client-go/kubernetes/typed/extensions/v1beta1".DeploymentInterface in assignment:
        *wrapExtensionsV1beta1DeploymentImpl does not implement "k8s.io/client-go/kubernetes/typed/extensions/v1beta1".DeploymentInterface (missing ApplyScale method)
vendor/knative.dev/pkg/client/injection/kube/client/client.go:8368:5: cannot use (*wrapExtensionsV1beta1ReplicaSetImpl)(nil) (type *wrapExtensionsV1beta1ReplicaSetImpl) as type "k8s.io/client-go/kubernetes/typed/extensions/v1beta1".ReplicaSetInterface in assignment:
        *wrapExtensionsV1beta1ReplicaSetImpl does not implement "k8s.io/client-go/kubernetes/typed/extensions/v1beta1".ReplicaSetInterface (missing ApplyScale method)
vendor/knative.dev/pkg/client/injection/kube/client/client.go:1236:9: too many errors
FAIL    knative.dev/eventing-rabbitmq/pkg/reconciler/broker [build failed]

After 9db1369, I think the update to k8s.io/code-generator, specifically 9db1369#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6L31-R32 broke the k8s.io/client-go (missing testing.FakeClient). I'm checking if any of the two topology operators depend on a newer client-go, and how to solve this.

@gvmw
Copy link
Contributor Author

gvmw commented Nov 8, 2021

This is the commit that bumped k8s.io/code-generator to 0.22.0: rabbitmq/messaging-topology-operator@2765a03 . It may be unrelated, but it's the first dependency that introduces 0.22 in this mix, and I'm assuming that makes everything bump.

I was able to lock it to a specific version by running go get k8s.io/[email protected], but that has also downgraded the messaging-topology-operator. I think that I'm missing a trick here, will ask for help.

@gvmw
Copy link
Contributor Author

gvmw commented Nov 8, 2021

This was my starting point (no dep updates): 0b22ee9

I ran the following:

go get -d github.com/rabbitmq/[email protected]
go get: upgraded github.com/form3tech-oss/jwt-go v3.2.2+incompatible => v3.2.3+incompatible
go get: upgraded github.com/fsnotify/fsnotify v1.4.9 => v1.5.1
go get: upgraded github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b => v0.0.0-20210429001901-424d2337a529
go get: upgraded github.com/rabbitmq/cluster-operator v1.8.2 => v1.10.0
go get: upgraded github.com/sirupsen/logrus v1.7.0 => v1.8.1
go get: upgraded go.uber.org/multierr v1.6.0 => v1.7.0
go get: upgraded gopkg.in/ini.v1 v1.62.0 => v1.63.2
go get: upgraded k8s.io/kube-openapi v0.0.0-20210305001622-591a79e4bda7 => v0.0.0-20210421082810-95288971da7e

go get -d github.com/rabbitmq/[email protected]
go get: upgraded github.com/michaelklishin/rabbit-hole/v2 v2.10.0 => v2.11.0
go get: upgraded github.com/rabbitmq/messaging-topology-operator v0.11.0 => v1.2.1
go get: upgraded k8s.io/code-generator v0.21.4 => v0.22.2
go get: upgraded k8s.io/klog/v2 v2.8.0 => v2.9.0

./hack/update-deps.sh

./hack/update-codegen.sh

Everything worked fine, and then I ran the tests:

go test -race ./...
# knative.dev/eventing-rabbitmq/pkg/client/clientset/versioned/fake
pkg/client/clientset/versioned/fake/clientset_generated.go:78:4: undefined: testing.FakeClient
# knative.dev/eventing-rabbitmq/third_party/pkg/client/clientset/versioned/fake
third_party/pkg/client/clientset/versioned/fake/clientset_generated.go:78:4: undefined: testing.FakeClient
?       knative.dev/eventing-rabbitmq/cmd/controller/broker     [no test files]
?       knative.dev/eventing-rabbitmq/cmd/controller/brokerstandalone   [no test files]
?       knative.dev/eventing-rabbitmq/cmd/controller/source     [no test files]
?       knative.dev/eventing-rabbitmq/cmd/dispatcher    [no test files]
?       knative.dev/eventing-rabbitmq/cmd/failer        [no test files]
?       knative.dev/eventing-rabbitmq/cmd/ingress       [no test files]
?       knative.dev/eventing-rabbitmq/cmd/receive_adapter       [no test files]
?       knative.dev/eventing-rabbitmq/cmd/webhook/broker        [no test files]
?       knative.dev/eventing-rabbitmq/cmd/webhook/source        [no test files]
ok      knative.dev/eventing-rabbitmq/pkg/adapter       (cached)
?       knative.dev/eventing-rabbitmq/pkg/amqp  [no test files]
ok      knative.dev/eventing-rabbitmq/pkg/apis/duck/v1beta1     (cached)
ok      knative.dev/eventing-rabbitmq/pkg/apis/eventing/v1      (cached)
?       knative.dev/eventing-rabbitmq/pkg/apis/sources  [no test files]
ok      knative.dev/eventing-rabbitmq/pkg/apis/sources/v1alpha1 (cached)
?       knative.dev/eventing-rabbitmq/pkg/client/clientset/versioned    [no test files]
ok      knative.dev/eventing-rabbitmq/pkg/dispatcher    (cached)
ok      knative.dev/eventing-rabbitmq/pkg/rabbitmqnaming        (cached)
FAIL    knative.dev/eventing-rabbitmq/pkg/reconciler/broker [build failed]
ok      knative.dev/eventing-rabbitmq/pkg/reconciler/broker/resources   0.057s
FAIL    knative.dev/eventing-rabbitmq/pkg/reconciler/brokerstandalone [build failed]
ok      knative.dev/eventing-rabbitmq/pkg/reconciler/brokerstandalone/resources (cached)
ok      knative.dev/eventing-rabbitmq/pkg/reconciler/source/resources   (cached)
FAIL    knative.dev/eventing-rabbitmq/pkg/reconciler/trigger [build failed]
ok      knative.dev/eventing-rabbitmq/pkg/reconciler/trigger/resources  0.071s
FAIL    knative.dev/eventing-rabbitmq/pkg/reconciler/triggerstandalone [build failed]
ok      knative.dev/eventing-rabbitmq/pkg/reconciler/triggerstandalone/resources        (cached)
ok      knative.dev/eventing-rabbitmq/test/e2e  (cached) [no tests to run]

If I downgrade k8s.io/code-generator:

go get -d k8s.io/[email protected]
go get: downgraded github.com/rabbitmq/messaging-topology-operator v1.2.1 => v0.11.0
go get: downgraded k8s.io/code-generator v0.22.2 => v0.21.4

./hack/update-deps.sh

./hack/update-codegen.sh

go test -race ./...
go test -race ./...
# knative.dev/eventing-rabbitmq/pkg/reconciler/broker/resources
pkg/reconciler/broker/resources/exchange.go:66:5: unknown field 'Namespace' in struct literal of type v1beta1.RabbitmqClusterReference
# knative.dev/eventing-rabbitmq/pkg/reconciler/broker/resources [knative.dev/eventing-rabbitmq/pkg/reconciler/broker/resources.test]
pkg/reconciler/broker/resources/exchange.go:66:5: unknown field 'Namespace' in struct literal of type v1beta1.RabbitmqClusterReference
ok      knative.dev/eventing-rabbitmq/pkg/adapter       (cached)
ok      knative.dev/eventing-rabbitmq/pkg/apis/duck/v1beta1     (cached)
ok      knative.dev/eventing-rabbitmq/pkg/apis/eventing/v1      (cached)
ok      knative.dev/eventing-rabbitmq/pkg/apis/sources/v1alpha1 (cached)
ok      knative.dev/eventing-rabbitmq/pkg/dispatcher    (cached)
ok      knative.dev/eventing-rabbitmq/pkg/rabbitmqnaming        (cached)
FAIL    knative.dev/eventing-rabbitmq/pkg/reconciler/broker [build failed]
FAIL    knative.dev/eventing-rabbitmq/pkg/reconciler/broker/resources [build failed]
ok      knative.dev/eventing-rabbitmq/pkg/reconciler/brokerstandalone   3.363s
ok      knative.dev/eventing-rabbitmq/pkg/reconciler/brokerstandalone/resources (cached)
ok      knative.dev/eventing-rabbitmq/pkg/reconciler/source/resources   (cached)
FAIL    knative.dev/eventing-rabbitmq/pkg/reconciler/trigger [build failed]
ok      knative.dev/eventing-rabbitmq/pkg/reconciler/trigger/resources  0.105s
FAIL    knative.dev/eventing-rabbitmq/pkg/reconciler/triggerstandalone [build failed]
ok      knative.dev/eventing-rabbitmq/pkg/reconciler/triggerstandalone/resources        (cached)
ok      knative.dev/eventing-rabbitmq/test/e2e  (cached) [no tests to run]
root@w:~/github.com/knative-sandbox/eventing-rabbitmq.bare/add-support-for-

The commit that I have pushed just before this comment should reproduce the same issue.

How would you recommend solving this @salaboy?

gvmw added a commit to gvmw/eventing-rabbitmq that referenced this pull request Nov 8, 2021
This has the rest of the context: knative-extensions#492 (comment)

Signed-off-by: Gerhard Lazu <[email protected]>
@gvmw
Copy link
Contributor Author

gvmw commented Nov 10, 2021

I've made progress with this, there is a change that needs to happen in messaging-topology-operator: rabbitmq/messaging-topology-operator#256

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2021
gvmw added a commit to rabbitmq/messaging-topology-operator that referenced this pull request Nov 10, 2021
This was introduced part of
#87, and in
the latest eventing-rabbitmq changes it became obvious that the versions
started to drift.

This discussion explains the problem best:
#256

This holds the rest of the context:
knative-extensions/eventing-rabbitmq#492 (comment)

This change lines up all versions, and the local unit & integration
tests pass. My next step is to check if CI remains green, and if the
change actually works in the context of eventing-rabbitmq, which is how
this started.

Signed-off-by: Gerhard Lazu <[email protected]>
gvmw added a commit to gvmw/eventing-rabbitmq that referenced this pull request Nov 10, 2021
This has the rest of the context: knative-extensions#492 (comment)

Signed-off-by: Gerhard Lazu <[email protected]>
@gvmw gvmw force-pushed the add-support-for-single-rabbitmq-instance branch 4 times, most recently from 3076724 to 9a5a8bc Compare November 10, 2021 18:28
@gvmw
Copy link
Contributor Author

gvmw commented Nov 10, 2021

How do I fix the tide error @benmoss?

The conflict is showing me an old diff, I have since force pushed a few times and everything applies cleanly. If you run a git diff local, it will look different to the tide output above 🤷🏼‍♂️

@gvmw gvmw force-pushed the add-support-for-single-rabbitmq-instance branch from 9a5a8bc to ec13c71 Compare November 11, 2021 09:43
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2021
@gvmw gvmw force-pushed the add-support-for-single-rabbitmq-instance branch from ec13c71 to 2d58c34 Compare November 11, 2021 10:53
@gvmw
Copy link
Contributor Author

gvmw commented Nov 11, 2021

I am leaving a note for my future self. In case I come across this failure in the future:

panic: /ko-app/broker
  flag redefined: kubeconfig

goroutine 1 [running]:
flag.(*FlagSet).Var(0xc000090720,
  {0x1eaddf0, 0xc0001b4570}, {0x1c48cde, 0xa}, {0x1c9322f, 0x36})
    flag/flag.go:879
  +0x2f4
flag.(*FlagSet).StringVar(...)
    flag/flag.go:762
knative.dev/pkg/environment.(*ClientConfig).InitFlags(0xc0001b4540,
  0xc000090720)
    knative.dev/pkg@v0.0.0-20211109100843-91d1932616a7/environment/client_config.go:45
  +0x134
knative.dev/pkg/injection.ParseAndGetRESTConfigOrDie()
    knative.dev/pkg@v0.0.0-20211109100843-91d1932616a7/injection/config.go:32
  +0x36
knative.dev/pkg/injection/sharedmain.MainWithContext({0x1eca468,
  0xc000362030}, {0x1c5ef4e, 0x1a}, {0xc0006bff60, 0x2, 0x2})
    knative.dev/pkg@v0.0.0-20211109100843-91d1932616a7/injection/sharedmain/main.go:125
  +0x78
knative.dev/pkg/injection/sharedmain.Main({0x1c5ef4e, 0x1a}, {0xc0006bff60,
  0x2, 0x2})
    knative.dev/pkg@v0.0.0-20211109100843-91d1932616a7/injection/sharedmain/main.go:105
  +0x8f
main.main()
    knative.dev/eventing-rabbitmq/cmd/controller/broker/main.go:29
  +0x4e

Remember that it is related to dependencies being updated, and ./hack/update-codegen.sh not running.
The fix is to run it, then commit and push all the changes.

@gvmw gvmw force-pushed the add-support-for-single-rabbitmq-instance branch from 8f10675 to 804e8e5 Compare November 11, 2021 19:23
@gvmw gvmw force-pushed the add-support-for-single-rabbitmq-instance branch from 34c430a to 0efe550 Compare November 18, 2021 13:37
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2021
./hack/update-deps.sh FTW!

Signed-off-by: Gerhard Lazu <[email protected]>
@gvmw gvmw requested a review from benmoss November 18, 2021 13:53
@benmoss
Copy link
Contributor

benmoss commented Nov 18, 2021

I'm using this topology and seeing this error on the exchange:

Resource is not allowed to reference defined cluster reference. Check the namespace of the resource is allowed as part of the cluster's `rabbitmq.com/topology-allowed-namespaces` annotation

@benmoss
Copy link
Contributor

benmoss commented Nov 18, 2021

Ah, I see #454 (comment) references this annotation, its required for cross-namespace references. Adding the annotation and it seems to work.

@benmoss
Copy link
Contributor

benmoss commented Nov 18, 2021

So this is still WIP, waiting on the e2e test, right?

@gvmw
Copy link
Contributor Author

gvmw commented Nov 19, 2021

So this is still WIP, waiting on the e2e test, right?

Yes, that's right. Making slow progress, but getting there. I will commit what I have today (most of it is the Makefile which belongs in a different PR I think). Based on how today goes, it may help to pair-up as right now progress feels too slow and it's all down to learning and figuring out how the entire e2e setup fits together.

@gvmw
Copy link
Contributor Author

gvmw commented Nov 19, 2021

So this is still WIP, waiting on the e2e test, right?

I just remembered that we may want to wait for a messaging-topology release that has the feature which we depend on: rabbitmq/messaging-topology-operator#257

WDYT @benmoss ?

@benmoss
Copy link
Contributor

benmoss commented Nov 19, 2021

For rabbitmq/messaging-topology-operator#257 I think it's fine to just pin our dependency to the commit as you have done, the released version is fine for users to install

gvmw added a commit to gvmw/eventing-rabbitmq that referenced this pull request Nov 22, 2021
For example, running end-to-end tests requires setting up dependencies,
a local KiND cluster, installing operators, waiting for components to be
ready, and then invoking `go test` with many flags. The entire flow is
captured in .github/workflows/kind-e2e.yaml, so how do we run it
locally? I think that there are some scripts which try to set everything
up, but they are not discoverable, pretty sure they diverged from the CI
workflow by now, and are not as composable as the Make equivalent.

So how does it work? Type make and follow along:

    build                       Build binaries with e2e tags
    dep-update                  Update any dependency
    e2e-setup                   Setup environment for e2e testing
    env                         Configure shell env - eval "$(make env)" OR rm .env && make .env && source .env
    k9s                         Terminal ncurses UI for K8s
    test-e2e-broker             Run Broker end-to-end tests
    test-e2e-publish            Run TestKoPublish end-to-end tests
    test-e2e-source             Run Source end-to-end tests
    test-unit                   Run unit tests

To run end-to-end tests, which is what I had to do in the context of
knative-extensions#492 (this PR
is based on that one) I now only need to run the following:

    # This only needs to run once:
    make e2e-setup

    make test-e2e

    # I can run just a subset of tests with e.g.:
    make test-e2e-broker

This iteration:
- only support Linux & Darwin systems (does not support Windows)
- only supports x84_64 (does not support latest Macs with ARM silicon)
- defaults to the minimum supported K8s version (adding support for
  multiple K8s versions was out of scope)

This was only tested on Linux Ubuntu 20.04.3 LTS, and should be tested
on Darwin on Intel. Supporting Darwin on ARM should not be difficult,
but as I don't have access to one, I cannot test.

Signed-off-by: Gerhard Lazu <[email protected]>
@gvmw
Copy link
Contributor Author

gvmw commented Nov 24, 2021

This is now ready for review. Everything that we need is now in, including end-to-end tests.

gvmw added a commit to gvmw/eventing-rabbitmq that referenced this pull request Nov 24, 2021
For example, running end-to-end tests requires setting up dependencies,
a local KiND cluster, installing operators, waiting for components to be
ready, and then invoking `go test` with many flags. The entire flow is
captured in .github/workflows/kind-e2e.yaml, so how do we run it
locally? I think that there are some scripts which try to set everything
up, but they are not discoverable, pretty sure they diverged from the CI
workflow by now, and are not as composable as the Make equivalent.

So how does it work? Type make and follow along:

    build                       Build binaries with e2e tags
    dep-update                  Update any dependency
    e2e-setup                   Setup environment for e2e testing
    env                         Configure shell env - eval "$(make env)" OR rm .env && make .env && source .env
    k9s                         Terminal ncurses UI for K8s
    test-e2e-broker             Run Broker end-to-end tests
    test-e2e-publish            Run TestKoPublish end-to-end tests
    test-e2e-source             Run Source end-to-end tests
    test-unit                   Run unit tests

To run end-to-end tests, which is what I had to do in the context of
knative-extensions#492 (this PR
is based on that one) I now only need to run the following:

    # This only needs to run once:
    make e2e-setup

    make test-e2e

    # I can run just a subset of tests with e.g.:
    make test-e2e-broker

This iteration:
- only support Linux & Darwin systems (does not support Windows)
- only supports x84_64 (does not support latest Macs with ARM silicon)
- defaults to the minimum supported K8s version (adding support for
  multiple K8s versions was out of scope)

This was only tested on Linux Ubuntu 20.04.3 LTS, and should be tested
on Darwin on Intel. Supporting Darwin on ARM should not be difficult,
but as I don't have access to one, I cannot test.

Signed-off-by: Gerhard Lazu <[email protected]>
@gvmw
Copy link
Contributor Author

gvmw commented Nov 25, 2021

After this is merged, remember to update https://github.com/knative/docs/blob/ad6446431ea4de30bcbf94e8a36bdf43df11943d/docs/eventing/broker/rabbitmq-broker/README.md?plain=1#L92

With the following:

      annotations:
        # This enables a single RabbitMQ cluster to service multiple Brokers
        rabbitmq.com/topology-allowed-namespaces: "*"

@gvmw gvmw mentioned this pull request Nov 29, 2021
@benmoss
Copy link
Contributor

benmoss commented Nov 29, 2021

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benmoss, gvmw

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:

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 29, 2021
@knative-prow-robot knative-prow-robot merged commit e275182 into knative-extensions:main Nov 29, 2021
@gvmw gvmw deleted the add-support-for-single-rabbitmq-instance branch November 30, 2021 12:55
@gvmw
Copy link
Contributor Author

gvmw commented Nov 30, 2021

@embano1 I just checked and nightly builds have picked this change. Let us know if gcr.io/knative-nightly/knative.dev/eventing-rabbitmq/cmd/controller/broker@sha256:91907b32bb4f1a1e883b87efdeca9f5c7513dfe38b958086a47283ab65566401 image works for you:

kubectl apply -f https://storage.googleapis.com/knative-nightly/eventing-rabbitmq/latest/rabbitmq-broker.yaml

gvmw added a commit to gvmw/eventing-rabbitmq that referenced this pull request Nov 30, 2021
For example, running end-to-end tests requires setting up dependencies,
a local KiND cluster, installing operators, waiting for components to be
ready, and then invoking `go test` with many flags. The entire flow is
captured in .github/workflows/kind-e2e.yaml, so how do we run it
locally? I think that there are some scripts which try to set everything
up, but they are not discoverable, pretty sure they diverged from the CI
workflow by now, and are not as composable as the Make equivalent.

So how does it work? Type make and follow along:

    build                       Build binaries with e2e tags
    dep-update                  Update any dependency
    e2e-setup                   Setup environment for e2e testing
    env                         Configure shell env - eval "$(make env)" OR rm .env && make .env && source .env
    k9s                         Terminal ncurses UI for K8s
    test-e2e-broker             Run Broker end-to-end tests
    test-e2e-publish            Run TestKoPublish end-to-end tests
    test-e2e-source             Run Source end-to-end tests
    test-unit                   Run unit tests

To run end-to-end tests, which is what I had to do in the context of
knative-extensions#492 (this PR
is based on that one) I now only need to run the following:

    # This only needs to run once:
    make e2e-setup

    make test-e2e

    # I can run just a subset of tests with e.g.:
    make test-e2e-broker

This iteration:
- only support Linux & Darwin systems (does not support Windows)
- only supports x84_64 (does not support latest Macs with ARM silicon)
- defaults to the minimum supported K8s version (adding support for
  multiple K8s versions was out of scope)

This was only tested on Linux Ubuntu 20.04.3 LTS, and should be tested
on Darwin on Intel. Supporting Darwin on ARM should not be difficult,
but as I don't have access to one, I cannot test.

Signed-off-by: Gerhard Lazu <[email protected]>
@gvmw
Copy link
Contributor Author

gvmw commented Dec 1, 2021

Had confirmation this morning from @embano1 that #492 (comment) must go in as soon as knative/docs#4518 gets merged.

@gvmw
Copy link
Contributor Author

gvmw commented Dec 1, 2021

This should help: knative/docs#4532

knative-prow-robot pushed a commit that referenced this pull request Dec 16, 2021
* Add Makefile for running complicated tasks with a single command

For example, running end-to-end tests requires setting up dependencies,
a local KiND cluster, installing operators, waiting for components to be
ready, and then invoking `go test` with many flags. The entire flow is
captured in .github/workflows/kind-e2e.yaml, so how do we run it
locally? I think that there are some scripts which try to set everything
up, but they are not discoverable, pretty sure they diverged from the CI
workflow by now, and are not as composable as the Make equivalent.

So how does it work? Type make and follow along:

    build                       Build binaries with e2e tags
    dep-update                  Update any dependency
    e2e-setup                   Setup environment for e2e testing
    env                         Configure shell env - eval "$(make env)" OR rm .env && make .env && source .env
    k9s                         Terminal ncurses UI for K8s
    test-e2e-broker             Run Broker end-to-end tests
    test-e2e-publish            Run TestKoPublish end-to-end tests
    test-e2e-source             Run Source end-to-end tests
    test-unit                   Run unit tests

To run end-to-end tests, which is what I had to do in the context of
#492 (this PR
is based on that one) I now only need to run the following:

    # This only needs to run once:
    make e2e-setup

    make test-e2e

    # I can run just a subset of tests with e.g.:
    make test-e2e-broker

This iteration:
- only support Linux & Darwin systems (does not support Windows)
- only supports x84_64 (does not support latest Macs with ARM silicon)
- defaults to the minimum supported K8s version (adding support for
  multiple K8s versions was out of scope)

This was only tested on Linux Ubuntu 20.04.3 LTS, and should be tested
on Darwin on Intel. Supporting Darwin on ARM should not be difficult,
but as I don't have access to one, I cannot test.

Signed-off-by: Gerhard Lazu <[email protected]>

* Tell git to ingore .env and .config paths

We do not want to version-control these

Signed-off-by: Gerhard Lazu <[email protected]>

* Add KiND cluster config

Used in `make kind-cluster`

Signed-off-by: Gerhard Lazu <[email protected]>

* Install gh cli

This comes in handy for checking out PRs, working with Actions, etc.

Signed-off-by: Gerhard Lazu <[email protected]>

* Add descriptions for install-* targets

Allow versions to be changed just-in-time

Signed-off-by: Gerhard Lazu <[email protected]>

* Bump k9s to latest

Signed-off-by: Gerhard Lazu <[email protected]>

* Install kn CLI

After reading the first 6 chapters of Knative in Action, it became
obvious to me that this CLI is essential for working with Knative.

Signed-off-by: Gerhard Lazu <[email protected]>

* Move KO_URL after KO_BIN_DIR

On Windows on WSL with make 4.3 this fails. Not sure why it worked on
Linux with make 4.2.1 as it's clearly wrong. This fixes it.

Signed-off-by: Gerhard Lazu <[email protected]>

* Use make variables that expand only once

Variables declared with := get interpreted a single time while those
with = get evaluated every time they are invoked. We want just once
invocation.

Signed-off-by: Gerhard Lazu <[email protected]>

* Install serving part of the e2e setup

This is the easiest way to setup Sinks (they only need to be
addressable).

Signed-off-by: Gerhard Lazu <[email protected]>

* Install Kourier for ingress part of Knative Serving

Signed-off-by: Gerhard Lazu <[email protected]>

* Respect KUBECONFIG from env & do not hard-code to kind

Enables running against any K8s, helpful when testing against GKE.

Signed-off-by: Gerhard Lazu <[email protected]>

* Bump cert-manager to 1.5.4

This is the version that RabbitMQ messaging-topology-operator v1.2.1 was
tested against.

Related to #370 (comment)

Signed-off-by: Gerhard Lazu <[email protected]>

* Fix missing OPEN

Great spot @benmoss!

Signed-off-by: Gerhard Lazu <[email protected]>

* Make test-e2e depend on KUBECONFIG, no more roundabouts

@benmoss made an excellent point in this comment, and I think that this
change address it.

https://github.com/knative-sandbox/eventing-rabbitmq/pull/525/files#r759457509

Signed-off-by: Gerhard Lazu <[email protected]>

* Remove kind setup scripts

These are now replaced by the Makefile, for context see
#525

Signed-off-by: Gerhard Lazu <[email protected]>

* Use the existing context for the default install

Great suggestion @benmoss!

Signed-off-by: Gerhard Lazu <[email protected]>

* Make install & test-e2e behaviour is more intuitive

install takes care of everything, and test-e2e also runs it - they are
self-contained targets.

The other test-e2e-* targets make assumptions that an install has run
before. The idea is that running a subset of e2e tests should be as fast
as possible, while running the entire e2e should be complete.

Signed-off-by: Gerhard Lazu <[email protected]>

* Make the default help target more helpful for newcomers

That .env is always a gotcha... direnv doesn't make it better.

Signed-off-by: Gerhard Lazu <[email protected]>

* Rename build to test-compilation

I like it @benmoss!

https://github.com/knative-sandbox/eventing-rabbitmq/pull/525/files#r765968362

Signed-off-by: Gerhard Lazu <[email protected]>

* List test-unit-uncached in help

This is something that @ikvmw was mentioning yesterday. Making this
option visibile should help with WTF moments that originate in a stale cache.

Signed-off-by: Gerhard Lazu <[email protected]>

* Use Makefile in kind conformance standalone GitHub Action

The CI will continuously validate that everything is wired up correctly,
and devs using this locally will have the confidence that it just works.

Signed-off-by: Gerhard Lazu <[email protected]>

* Fix envsubst variables in kind.yaml

Signed-off-by: Gerhard Lazu <[email protected]>

* Do not link ko to gcloud

This is only necessary if pushing images to gcr.io, which is what *some*
of us do locally, but not everyone.

Signed-off-by: Gerhard Lazu <[email protected]>

* Uppercase all instances of KiND & K8s

Otherwise weird things happen, like KiND_CLUSTER_NAME vs
KIND_CLUSTER_NAME.

Signed-off-by: Gerhard Lazu <[email protected]>

* Export updated PATH so that all targets work correctly

Signed-off-by: Gerhard Lazu <[email protected]>

* Bump gh, k9s, kn & Knative versions to latest

Signed-off-by: Gerhard Lazu <[email protected]>

* Use make targets in kind e2e test GitHub Action

Signed-off-by: Gerhard Lazu <[email protected]>

* Install standalone broker for standalone conformance tests

Thanks @benmoss for spotting it!
#525 (comment)

Signed-off-by: Gerhard Lazu <[email protected]>

* Separate standalone conformance tests from the regular ones

Signed-off-by: Gerhard Lazu <[email protected]>

* Make test-conformance targets leaner

So that they are less likely to fail in GitHub Actions.

Signed-off-by: Gerhard Lazu <[email protected]>

* Convert standard conformance tests to use make targets

Signed-off-by: Gerhard Lazu <[email protected]>
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/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple Knative Brokers in different namespaces that connect and use a single RabbitMQ cluster instance

3 participants