-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: add extra_listeners #269
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments. The description is ok. I don't have a problem with a single knob that encapsulates a bunch of complex stuff, personally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a bit confused with the extra_count
and a couple of "conflicting" comments, which have probably created some misunderstanding. But I would like to be sure we are on the same page on the UX, and probably simplify the code (skipping the extra_count) which also created a bit of misunderstanding (and I don't think it is clarified in the config option description)
afe65a7
to
d7bd731
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just some non-blocking suggestions/nipticks attached
63659de
to
80fd059
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a fascinating addition to the charm
Changes Made
refactor: add extra_listeners config, deprecate certificate_extra_sans
extra_listeners
replacescertificate_extra_sans
foobar-{unit}.baz:30000
, they will get listeners likefoobar-0.baz:39092
on unit 0certificate_extra_sans
, they will get a deprecation warning in the logsextra_listeners
will be preferred, but default tocertificate_extra_sans
if unsetrefactor: move password actions to actions.py
feat: add get-listeners Action
advertised.listeners
for the unit the Action runs againstManual Validation
Attempted to verify behavior using the following set-up:
10.155.67.110 (enp5s0)
kafka/0
has IP10.58.254.246
with hostnamejuju-ecb145-0
Configured Kafka to expose multiple extra advertised.listeners:
Edited DNS on the host machine, to point the Kafka unit hostname to point to the Kafka unit IP:
Right now, this isn't resolvable.
Then, manually added an IP route on the host machine, to reach the Kafka unit IP via the
enp5s0
LXD VM network:Finally, on the host machine, attempted to create a topic on a single Kafka unit, attempting to reach
kafka-0:39292
as the bootstrap-server, which succeeded: