Skip to content

Conversation

@blackjid
Copy link
Contributor

@blackjid blackjid commented Jul 26, 2022

This closes #421

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed
Note to contributors: remember to re-generate client set if there are any API changes

Summary Of Changes

This pr add support for the operator to look at the cluster specs for the path_prefix attribute if it is set. This way the operator can communicate with the cluster api while it can be exposed on a different path.

Additional Context

The additional_config property on the cluster spec adds configuration to for the cluster.
I understand the cluster support ini and sysctl like configurations.

  • read configuration from the clusterspec
  • make unit-tests PASS

ref:
https://www.rabbitmq.com/kubernetes/operator/using-operator.html#additional-config
https://www.rabbitmq.com/configure.html#config-file

@vmwclabot
Copy link

@blackjid, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@blackjid blackjid force-pushed the feat/use_path_prefix branch from ad03af8 to 93feff5 Compare July 26, 2022 13:56
@vmwclabot
Copy link

@blackjid, VMware has approved your signed contributor license agreement.

@coro
Copy link
Contributor

coro commented Aug 8, 2022

Hi @blackjid, is this PR still WIP? Would you like the team's feedback on it yet?

@blackjid
Copy link
Contributor Author

blackjid commented Aug 8, 2022

Hi @coro, well, I think it should be working.
There are still a few things to work on. But I think it might be a good idea to just expose it for feedback :)

@blackjid blackjid marked this pull request as ready for review August 8, 2022 14:19
@blackjid blackjid force-pushed the feat/use_path_prefix branch 3 times, most recently from 84a0e0d to e0fd050 Compare August 8, 2022 14:34
@blackjid blackjid changed the title wip Add support to read path prefix from cluster specs Aug 9, 2022
@blackjid blackjid force-pushed the feat/use_path_prefix branch from e0fd050 to bace0da Compare August 9, 2022 02:16
@blackjid
Copy link
Contributor Author

blackjid commented Aug 9, 2022

I'd love to have some feedback on the way to support both ini and sysctl config from the cluster.

For the ini syntax

Using go-ini/ini package works very nice to read the ini syntax. I think this is ok.

For the sysctl like syntax

I have been trying this, I have to approaches.

  1. I haven't found an open library to parse this files, but the specification is pretty simple so I've implemented simple parser that is working fine in the tested scenarios.

  2. I just find out that I could us the go-ini/library to parse the sysctl config, as it supports empty sections.

    Instead of doing this to read the value from a ini config

    cfg.Section("management").Key("path_prefix).String()
    

    I could do this from a sysctl config

    cfg.Section("").Key("management.path_prefix").String()
    

    The specification from the ini config is very similar to the sysctl, minus the sections.

What do you thing would be better? 1 or 2?

@coro
Copy link
Contributor

coro commented Aug 10, 2022

Ah, there may be confusion from our documentation. RabbitMQ doesn't support the true .ini config format, only the sysctl format from a library called cuttlefish. The only rules of the syntax are:

  • Everything you need to know about a single setting is on one line
  • Lines are structured Key = Value
  • Any line starting with # is a comment, and will be ignored.

(From https://github.com/basho/cuttlefish/wiki/Cuttlefish-for-Application-Users)

So in summary, you don't need to worry about .ini, only sysctl. The go-ini library you've chosen is great, and is what we use in the cluster-operator to set and parse the additional config files originally. Of your options for sysctl parsing above, I prefer the last one because it matches how the keys are set originally:

cfg.Section("").Key("management.path_prefix").String()

@blackjid blackjid force-pushed the feat/use_path_prefix branch from bace0da to 89866ae Compare August 10, 2022 13:04
@blackjid
Copy link
Contributor Author

Thanks for the feedback... that was exactly what I needed :)
I updated the PR with the latest changes, let me know if there is something else to add or change.

@blackjid blackjid force-pushed the feat/use_path_prefix branch from 89866ae to f73a8b4 Compare August 10, 2022 13:08
Copy link
Contributor

@coro coro left a comment

Choose a reason for hiding this comment

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

LGTM! Let me pass this to another member of our team for a second opinion.

Copy link
Contributor

@ChunyiLyu ChunyiLyu left a comment

Choose a reason for hiding this comment

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

Thank you for contributing!

@ChunyiLyu ChunyiLyu merged commit 08c32dd into rabbitmq:main Aug 15, 2022
@blackjid
Copy link
Contributor Author

Thank you for contributing!

No problem!, I'm really happy to do it :)

Is there any plan of release this?

@ChunyiLyu
Copy link
Contributor

@blackjid New release just cut 😊 See: https://github.com/rabbitmq/messaging-topology-operator/releases/tag/v1.8.0

@blackjid
Copy link
Contributor Author

Awesome! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

management.path_prefix is not respected by messaging-topology-operator

4 participants