Fix #5935: Add ArkMQ operator queue Pipe binding - #6845
Thundercloud12 wants to merge 18 commits into
Conversation
squakez
left a comment
There was a problem hiding this comment.
Nice work. However I think we need to consider the following thread: arkmq-org/arkmq-org-broker-operator#1121 - which it seems to point to a deprecation of certain resources used here. Maybe we can only use the newer approach instead and making it easier to maintain in the long term.
Also, when complete, we should add a short documentation as provided for the other bindings in docs/modules/ROOT/pages/pipes/pipes.adoc#more-advanced-examples
|
BTW, also mind #6846 - while reviewing the PR I realized the Strimzi binding which you may have used as a reference is flawed. We can avoid that problem reported there if possible. |
|
@squakez so the issues you mentioned are resolved, actually i wanted to follow kafka to the point so ended up following that doc( was recommended by llm when i said i am following kafka implementation should have atleast checked that), also after this i am planning to raise a pr to fix issue #6846. Thank you! |
|
✔️ Unit test coverage report - coverage increased from 63.8% to 63.9% (+0.1%) |
squakez
left a comment
There was a problem hiding this comment.
LGTM, though there are a few points to clarify.
There may be already somebody else working on it. Please, verify the assignment is clear before starting to avoid duplicate works and add a comment to let some maintainer assign it as well if it's free. |
|
✔️ Unit test coverage report - coverage increased from 63.8% to 64% (+0.2%) |
|
@squakez yeah at the time I checked it wasn't assigned I can see someone is working on it, I wouldn't pick that up |
|
@squakez all the changes you mentioned are reconcile that rm -rf was a leftover line during previous refactoring that part you raised about the search that was a minconception because in an activemqartemisaddress does not match underlying queue address name. But while researching for it i got to know that kubernetes semantics and camel k conventions would never allow it so i have ensure that too, also a lint ci was failing, fixes that too |
|
✔️ Unit test coverage report - coverage increased from 63.8% to 64% (+0.2%) |
|
@squakez please have a look at the updated code |
squakez
left a comment
There was a problem hiding this comment.
I think it's more or less fine. Now it's a matter to verify the proper configuration that Camel expects. I think this step requires some manual testing against an existing ArkMQ broker in order to find the proper configuration params.
|
✔️ Unit test coverage report - coverage increased from 63.8% to 64.1% (+0.3%) |
ArkMQ / ActiveMQ Artemis AMQP VerificationIssue: #5935 – Support ArkMQ in Pipe Binding ObjectiveVerify the AMQP configuration used by the ArkMQ Pipe binding against an ActiveMQ Artemis broker. VerificationTested with:
An Artemis broker was started with both AMQP acceptors:
Both were tested using: with the Camel endpoint: ResultsPort 61616: SUCCESS Port 5672: SUCCESS ConfigurationFor Camel K, the following properties were verified: quarkus.qpid-jms.url=amqp://<host>:<port>
camel.component.amqp.broker-url=amqp://<host>:<port>The Pipe binding can therefore continue using: without additional custom connection/bean configuration. ConclusionThe current ArkMQ binding configuration is compatible with ActiveMQ Artemis AMQP.
|
|
@squakez the above verification was done using docker because as you mentioned you need to check the correct config params and this does it well, is it okay or should i look towards setting up a cluster and checking? |
|
The only failing test is the new one that should prove the feature. This is what we get: As suggested during the review, let's use |
|
@squakez Updated e2e/arkmq/setup/setup.sh to pin and parameterize the ArkMQ operator version |
|
✔️ Unit test coverage report - coverage increased from 63.8% to 64.1% (+0.3%) |
|
✔️ Unit test coverage report - coverage increased from 63.8% to 64.1% (+0.3%) |
|
@squakez so it has been frustrating on me and i think on you as well i think i was lax in my work earlier i thought fixxinf this one failing test issue would solve the pipeline but it was not the case i should have been more thorough with my methods so to actually confirm i ran tests in the below manner:
only aftyer this was verified i have pushed the change if you think the testing methodology should be changed please let me know because that would avoid us to go into again this ci failing loop |
|
✔️ Unit test coverage report - coverage increased from 63.8% to 64.1% (+0.3%) |
|
The new worflow runs, but we have some issue to fix: -> we must include watch and list rbac to the resource on the queue side: which indicates that maybe we need to configure the protocol of the acceptor. I wonder if we should try to configure |
|
@squakez please check the changes, let me know for any changes! |
|
✔️ Unit test coverage report - coverage increased from 64.4% to 64.7% (+0.3%) |
|
You may need to rebase with |
- Support binding directly to ActiveMQArtemis cluster with destination/queue property
- Avoid reliance on deprecated ActiveMQArtemisAddress while keeping backward compatibility
- Update unit tests and pipes.adoc documentation
…for queue in e2e setup
…ture consistency
…ss in e2e setup
3b77bac to
bb089e9
Compare
|
✔️ Unit test coverage report - coverage increased from 64.4% to 64.7% (+0.3%) |
|
@squakez rebase with main! |
Fixes #6837
Motivation & Context
Camel K
Piperesources currently support Strimzi Kafka, Knative, and Kamelet resources as declarative endpoints. However, there was no native binding provider for messaging queues managed by the ArkMQ / ActiveMQ Artemis operator (broker.amq.io).This PR adds a minimal, queue-only
Pipebinding provider for ArkMQ operator-managed queues, following the architectural pattern of the existing Strimzi binding.Architecture & Design Decisions
Following maintainer guidance on the issue, this implementation focuses strictly on a minimal queue use case:
1. Queue-Only Resource Target
ActiveMQArtemisAddress(broker.amq.io/v1beta1).spec.routingType: multicast(topics) to ensure only queue bindings are configured.2. Automated Broker Discovery
If
brokerURLis not manually provided in the Pipe endpoint properties, the provider:ActiveMQArtemiscluster fromspec.applyToor theActiveMQArtemislabel.core,all, oropenwire), defaulting to the standard61616.3. Queue Name Resolution
The queue name is resolved using the following priority:
4. Deterministic Component URI
The provider always generates:
Any user-supplied Pipe endpoint properties are appended to the generated URI.
Key Changes
Duck Types
ActiveMQArtemisAddressandActiveMQArtemisduck types inpkg/apis/duck/arkmq/v1beta1/.Client Generation
client-genconfiguration inscript/gen_client.sh.pkg/client/arkmq/.Binding Provider
ArkMQBindingProviderinpkg/util/bindings/arkmq.go.RBAC
Added namespaced and descoped roles and bindings under
pkg/resources/config/rbac/.Grants read-only (
get,list,watch) permissions for:activemqartemisesactivemqartemisaddressesUnit Tests
Added
pkg/util/bindings/arkmq_test.gocovering:brokerURLproperty overrideAdded
pkg/controller/pipe/initialize_test.gocovering bidirectional support:Timer → ArkMQ QueueArkMQ Queue → LogDocumentation
docs/modules/ROOT/pages/pipes/pipes.adocE2E Infrastructure
e2e/arkmq/.test-arkmqMakefile target..github/workflows/arkmq.ymlEventuallyfor asynchronous assertions.How Has This Been Tested?
The following checks were run successfully:
Unit Tests
Static Analysis
go vetpasses across all packages with zero issues.Formatting
Both formatting and import checks pass successfully.