Pulsar Manager Initialization and Securing of Admin Secrets

[Mortom123]

See #438 / #438 (comment) 3.PR

Also the Pulsar-Manager Admin interface is exposed on a different, non outward facing, service because of given concerns.

Motivation

Right now you need to initialize the pulsar-manager manually, this can be done as an init job.

Modifications

1. Change Admin Port exposure of Pulsar Manager to second internal service
2. Provide means to securely generate a password / use an existing one
3. Provide a job that runs on init and conditions the Pulsar Manager

Verifying this change

• Make sure that the change passes the CI checks. As of right now still running

Some things I also noted:

1. I tried all combinations of BROKER_URL and BOOKIE_URL such that BOOKIE_URL references the Bookie service and one of its ports (as given in the values). However, none of them worked. I suspect there's a naming error on side of the Pulsar Manager
2. There are some errors when starting the manager. Maybe part of this deployment is incorrect.

/pulsar-manager/startup.sh: 21: initdb: not found 
/pulsar-manager/startup.sh: 22: pg_ctl: not found 

[Mortom123]

Please have a look @lhotari, this also addresses some of the issues you mentioned regarding the admin port exposure.
Thanks

[Mortom123]

I also tried to use .Values.bookie.component with any of the ports under .Values.bookie.ports, but none worked. However, .Values.bookie.component worked with .Values.broker.ports.pulsar ???

[lhotari]

I don't see how it could work with broker's component and port. More comments below.

[lhotari]

Please address the review comments.

[lhotari]

Thanks for the contribution @Mortom123 ! This will be a great improvement to Apache Pulsar Helm chart!

[Mortom123]

@lhotari I updated with your feedback and decided to also reduce the complexity of the secret by only use a single username/password for the UI / DB. I don't see much advantage of having separate ones there.

I allowed for manual overwrite of secret values, similar to kube-prometheus-stack, but the default (empty password) is a randomly generated password.
Maybe you have an idea how can test if the login/checking of the environment of the pulsar manager works in the CI pipeline?

I removed existingSecretName, because when injecting Secrets through secondary applications, I would argue that you have to provide a name for it either way. so might as well set it to the required name.

During this I also wondered why we have such flexibility for the names for of the components. Most of the things are setup using pulsar.fullname-component.name. Why don't we fix component.name and only allow for a changing fullname through e.g. other release names?

[lhotari]

I updated with your feedback and decided to also reduce the complexity of the secret by only use a single username/password for the UI / DB. I don't see much advantage of having separate ones there.

I think having a single username/password for both UI & DB is simply a very bad practice. It's better to have separate passwords. It's not that much additional overhead to add it now and follow good practices.

I allowed for manual overwrite of secret values, similar to kube-prometheus-stack, but the default (empty password) is a randomly generated password.

That part looks really good!

Maybe you have an idea how can test if the login/checking of the environment of the pulsar manager works in the CI pipeline?

Perhaps a simple HTTP request with curl & grep would do the job? That could be added in a similar way as the Pulsar Functions tests:

pulsar-helm-chart/.ci/chart_test.sh

Lines 77 to 80 in 24b80c1

	if [[ "$(ci::helm_values_for_deployment | yq .components.functions)" == "true" ]]; then
	# test functions
	ci::test_pulsar_function
	fi

You would add this to .ci/chart_test.sh and .ci/helm.sh files. In addition there would need to be a test scenario in the GitHub Actions workflow matrix configured in https://github.com/apache/pulsar-helm-chart/blob/master/.github/workflows/pulsar-helm-chart-ci.yaml#L170 with respective values file in .ci/clusters directory.
When you run the CI jobs in your own fork (by enabling GitHub Actions) and opening a PR to your own fork, you can debug GitHub Actions by logging in by ssh. ssh access is only for your ssh public keys registered in GH, it's https://github.com/Mortom123.keys for you. There's a step in the build that will print the command for connecting with ssh. example from a build in my fork:

Once you have ssh access, you can debug things to see what problems there are in the CI environment.

I removed existingSecretName, because when injecting Secrets through secondary applications, I would argue that you have to provide a name for it either way. so might as well set it to the required name.

+1, pulsar-manager integration has been poor so I'm not concerned about the possible breaking change.

During this I also wondered why we have such flexibility for the names for of the components. Most of the things are setup using pulsar.fullname-component.name. Why don't we fix component.name and only allow for a changing fullname through e.g. other release names?

I guess this is some of the legacy baggage of the chart. As long as there's a backwards compatible way to improve things gradually, we can do that.

[lhotari]

Linting failed on this line: 1337:18 [comments] too few spaces before comment

[Mortom123]

@lhotari I added both credentials as well as CI tests. But the tests have some problems:

1. Here the pulsar manager is not ready to accept connections before being hit by the curl. I tried to fix this by always waiting for jobs to complete in the pipeline. I don't know why we don't wait when installing anyway? Maybe you have some explanation?
2. This also failed, because it takes some time for the job to be triggered (both manager + broker need to be fully ready).

There are now a couple of ways to fix this:

1. Increase wait timeout in $install_args
2. Revert the commit touching $install args and put an until loop before hitting the manager in a similar fashion as here.

I would prefer to wait for all jobs to complete during install. But what do you or @frankjkelly think?

[lhotari]

Closing and re-opening to trigger a new build

[lhotari]

Closing and re-opening once again to get a fix for the linting job in CI.

[Mortom123]

@lhotari i think the build wont work yet. because the job that initializes the manager is not run in timed / waited for, see #448 (comment)

[lhotari]

1. I don't know why we don't wait when installing anyway? Maybe you have some explanation?

I guess the reason has been that --wait didn't work before adding MetalLB in a8c7745 . Services of type: LoadBalancer require a load balancer implementation in the k8s cluster. MetalLB can be used for "kind" which is used in CI.

Another reason for not using --wait might have been that it's possible to have more logging about k8s events and pod state when the helm install command returns and the waiting loop can do whatever it wants. That's just a guess that I'm making.

[lhotari]

@Mortom123 if you'd like to debug the issue, it's best to create a PR to your own fork since that will activate ssh access to the build VM.

pulsar-helm-chart/.github/workflows/pulsar-helm-chart-ci.yaml

Line 255 in 1f20887

if: ${{ github.repository != 'apache/pulsar-helm-chart' && github.event_name == 'pull_request' }}

It requires a pull request currently and won't work for manually triggered jobs in your fork.

[lhotari]

I wonder if upgrading the image to v0.4.0 would help?

pulsar-helm-chart/charts/pulsar/values.yaml

Line 187 in 1f20887

tag: v0.3.0

https://hub.docker.com/r/apachepulsar/pulsar-manager/tags

[Mortom123]

Alright. I will try to do so. :) @lhotari

[lhotari]

@Mortom123 have you figured out how to use the ssh access to the build? that makes the feedback loop much faster when you have a shell inside a build VM and you can try out different things and debug issues directly. For example, there's k9s available to view the kind k8s cluster.

[Mortom123]

I will try debugging on my Repo for now and submit a clean PR once its done.