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

621 allow for plain http connections #622

Merged

Conversation

JorgeAndresQuintero
Copy link
Contributor

This closes #621

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

With the env variable CONNECT_USING_PLAIN_HTTP it is possible to configure the topology operator to communicate with the RabbitMQ Cluster using plain http, even if the cluster has TLS enabled. This is will remove the limitation

Messaging Topology Operator will not attempt to connect to the RabbitmqCluster over HTTP if HTTPS is available (even if the certificate is not trusted)

documented in https://www.rabbitmq.com/kubernetes/operator/tls-topology-operator.html, and allow users to decide whether HTTP or HTTPS should be used.

Default behavior remains unchanged in that not setting the flag will cause the operator to keep trying to establish a HTTPS connection.

@vmwclabot
Copy link

@JorgeAndresQuintero, 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.

@vmwclabot
Copy link

@JorgeAndresQuintero, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

@vmwclabot
Copy link

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

@ChunyiLyu ChunyiLyu self-assigned this May 23, 2023
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.

@JorgeAndresQuintero Thanks for contributing! Please see my comments. Happy to merge once suggestions/comments are resolved. Let me know what you think :)

main.go Outdated Show resolved Hide resolved
rabbitmqclient/cluster_reference_test.go Show resolved Hide resolved
rabbitmqclient/cluster_reference_test.go Outdated Show resolved Hide resolved
@@ -178,25 +178,28 @@ func readUsernamePassword(secret *corev1.Secret) (string, string, error) {
return string(secret.Data["username"]), string(secret.Data["password"]), nil
}

func managementURI(svc *corev1.Service, tlsEnabled bool, clusterDomain string, pathPrefix string) (string, error) {
func managementURI(svc *corev1.Service, useTLSForConnection bool, clusterDomain string, pathPrefix string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function currently fails with error "failed to find management from service rmq" if useTLSForConnection is false and the rabbit cluster only has TLS listener available. It should succeed and return a https scheme URI instead of failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left the logic in this function the same, but changed how useTLSForConnection is computed in line 102. That way, its value will always be false if non-TLS listeners are disabled.

@JorgeAndresQuintero
Copy link
Contributor Author

Thanks for the review, @ChunyiLyu. I will work on it over the next couple days.

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.

Thanks for incorporating feedback. Will squash merge for clean history in main branch.

@ChunyiLyu ChunyiLyu merged commit 9bba483 into rabbitmq:main May 30, 2023
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.

Allow for plain http connections to cluster even if tls is enabled
3 participants