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

Move messaging configuration to file and deprecate deployMessagingService flag #1041

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nasark
Copy link
Member

@nasark nasark commented Jan 26, 2024

  • move messaging config from env vars to file
  • the configs will be stored in secrets and mounted as a projected volume since the kafka user password is located in another secret created by strimzi
  • kafka user secret naming convention normalized to match downstream cr.Spec.AppName-user -> cr.Spec.AppName-kafka-admin so that upstream and downstream can both use func MessagingEnvSecret
  • moving the config to secrets allows us to check the hostname to see whether kafka resources should be deployed or not in the case of an external kafka. This allows us to fully deprecate cr.Spec.DeployMessagingService

Depends on:

@miq-bot assign @bdunne
@miq-bot add_reviewer @Fryguy

@miq-bot
Copy link
Member

miq-bot commented Jan 26, 2024

@nasark unrecognized command 'enhancement', ignoring...

Accepted commands are: add_label, add_reviewer, request_review, assign, close_issue, cross_repo_test, move_issue, remove_label, rm_label, remove_reviewer, set_milestone, unassign

@nasark
Copy link
Member Author

nasark commented Jan 26, 2024

@miq-bot add_label quinteros/yes?

@miq-bot
Copy link
Member

miq-bot commented Jan 26, 2024

Checked commits nasark/manageiq-pods@6c880dd~...21cd053 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. ⭐

Comment on lines +534 to +535
hostName := string(secret.Data["hostname"])
if hostName != "manageiq-kafka-bootstrap" {
Copy link
Member Author

Choose a reason for hiding this comment

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

I followed the same methodology we use for postgresql but in this case the hostname also depends on cr.Spec.AppName. Would this be an issue? For example if a user decides to change the default app name for some reason then this wouldn't work, however this allows us to not deploy upstream kafka in the downstream scenario

Comment on lines +56 to +62
messaging_hostname=${MESSAGING_HOSTNAME:-$messaging_hostname_file}
messaging_hostname=${messaging_hostname:-localhost}
messaging_username=${MESSAGING_USERNAME:-$messaging_username_file}
messaging_password=${MESSAGING_PASSWORD:-$messaging_password_file}
messaging_port=${MESSAGING_PORT:-$messaging_port_file}
messaging_port=${messaging_port:-9093}
messaging_sasl_mechanism=${MESSAGING_SASL_MECHANISM:-$messaging_sasl_mechanism_file}
Copy link
Member Author

Choose a reason for hiding this comment

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

Since the env vars won't be used, should they be removed from these conditional assignments? I see that the database env vars are still checked in these conditional assignments https://github.com/ManageIQ/manageiq-pods/blob/master/images/manageiq-base/container-assets/container_env#L12-L20 so I followed suite but didn't understand the reasoning

Copy link
Member

Choose a reason for hiding this comment

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

We kept them more as a convenience for developers - if you run it locally, it's just a lot easier to pass them as -e on the command line

[[ -f /run/secrets/messaging/MESSAGING_SASL_MECHANISM ]] && messaging_sasl_mechanism_file=$(cat /run/secrets/messaging/MESSAGING_SASL_MECHANISM)
[[ -f /etc/pki/ca-trust/source/anchors/root.crt ]] && messaging_ca_path=/etc/pki/ca-trust/source/anchors/root.crt
messaging_hostname=${MESSAGING_HOSTNAME:-$messaging_hostname_file}
messaging_hostname=${messaging_hostname:-localhost}
Copy link
Member

Choose a reason for hiding this comment

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

Is this right? I thought localhost didn't work?

Copy link
Member

Choose a reason for hiding this comment

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

It should probably default to the name of the service used to reach kafka

@miq-bot miq-bot added the stale label Jun 10, 2024
@miq-bot
Copy link
Member

miq-bot commented Jun 10, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

1 similar comment
@miq-bot
Copy link
Member

miq-bot commented Sep 16, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

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

Successfully merging this pull request may close these issues.

4 participants