pivotal-cf / java-cfenv

Apache License 2.0
91 stars 56 forks source link

Rabbit MQ Port is not being set Properly #220

Closed JulieDoc closed 6 months ago

JulieDoc commented 11 months ago

Describe the bug

I am deploying my spring boot app to Cloud Foundry. I am connecting to an external Rabbit MQ instance with a cups (user provided service). When I specify a uri with "amqps" it is trying to connect to RMQ with a port of 5671, but that is not the port our RMQ is running on, and not the port in my URI.

Reproduction steps

1.Create a cups on Cloud Foundry with an ampqs URI that is not running on 5671 2.Push a spring boot 3.1.2 application and bind to the cups 3.Try to push a message to RMQ from your spring boot apps ...

Expected behavior

Should use the port provided in URI

Additional context

No response

berkkonak commented 11 months ago

We are facing the same problem. If I change protocol from amqps to amqp in uri, the port I provide (5595) is being used. However, when I change url back to amqps (with ssl:true) the port that I provide is not being used, default value (5671) is used.

Also, when I evaluate properties.put("spring.rabbitmq.port", "5595") inside process method of AmqpCfEnvProcessor before returning, connection establishes to RabbitMQ successfully. Actually, inside process method, the port I provided is available ( see: credential with key "amqp+ssl"). I wonder why this value is not being used instead of hardcoded 5671.

image

rama78 commented 9 months ago

We faced the same issue and solved it by using a pull request and adding only the following few lines of code. I believe that if you fix it by adding below, it will be useful for the rest of the community, rather than us creating a fork of it.

AmqpCfEnvProcessor.java

if (uriInfo.getScheme().equals("amqps")) { properties.put("spring.rabbitmq.ssl.enabled", "true"); //properties.put("spring.rabbitmq.port", "5671"); properties.put("spring.rabbitmq.port", uriInfo.getPort());

can you please help us!

-Rama

anthonydahanne commented 7 months ago

hello @rama78 , @berkkonak I've pushed an MR that should fix this issue; thanks @rama78 for the suggestion that I enhanced to also support the case when no port is provided at all (-1) Let us know if you could test it in your environments before it gets merged. Thanks for your patience!

rama78 commented 7 months ago

@anthonydahanne - Thanks a lot for making the changes. I tested the respective branch, and it failed. Then, I realized we added an extra line (to fetch the VHOST). I sincerely apologize for missing that in the above code snippet.

PCF CUPS URI format: amqps://temp:temp!@abc..com:8071/CPE_SWAT

We have added code like the one below:

if (uriInfo.getScheme().equals("amqps")) { properties.put("spring.rabbitmq.ssl.enabled", "true"); properties.put("spring.rabbitmq.port", uriInfo.getPort()); properties.put("spring.rabbitmq.virtualHost", uriInfo.getPath()); } else { populateAddress(cfCredentials, properties, uri); } }

Our server is rejecting the connection because it is not finding the VHOST information as we added part of the URI. can you please add the logic to fetch the VHOST from the URI like the above code snippet?

Thanks again! Rama

anthonydahanne commented 7 months ago

hum, ok, it seems consistent with some VCAP_SERVICES that was sent to me via another channel

"p.rabbitmq ": [
    {
      "binding_guid ": "some-guid ",
      "binding_name ": null,
      "credentials ": {
        "dashboard_url ": "https://dashboard.com ",
        "hostname ": "hostname.com ",
        "hostnames ": [
          "hostname.com "
        ],
        "http_api_uri ": "https://some-guid:REDACTED@myapi.com/api/ ",
        "http_api_uris ": [
          "https://some-guid:REDACTED@myapi.com/api/ "
        ],
        "password ": "REDACTED ",
        "protocols ": {
          "amqp+ssl ": {
            "host ": "hostname.com ",
            "hosts ": [
              "hostname.com "
            ],
            "password ": "REDACTED ",
            "port ": 1029,
            "ssl ": true,
            "uri ": "amqps://some-guid:REDACTED@hostname.com:1029/vhost ",
            "uris ": [
              "amqps://some-guid:REDACTED@hostname.com:1029/vhost "
            ],
            "username ": "some-guid ",
            "vhost ": "vhost "
          }
        },
        "ssl ": true,
        "uri ": "amqps://some-guid:REDACTED@hostname.com:1029/vhost ",
        "uris ": [
          "amqps://some-guid:REDACTED@hostname.com:1029/vhost "
        ],
        "username ": "some-guid ",
        "vhost ": "vhost "
      },
      "instance_guid ": "vhost ",
      "instance_name ": "instance-name ",
      "label ": "p.rabbitmq ",
      "name ": "instance-name ",
      "plan ": "good-plan ",
      "provider ": null,
      "syslog_drain_url ": null,
      "tags ": [
        "rabbitmq "
      ],
      "volume_mounts ": []
    }
]

It also seems consistent with: https://www.rabbitmq.com/uri-spec.html

Finally, you just reminded me to focus on: https://github.com/spring-cloud/spring-cloud-connectors/blob/v2.0.2.RELEASE/spring-cloud-core/src/test/java/org/springframework/cloud/service/common/AmqpServiceInfoTest.java and make sure we have something equivalent, as long as it does not break existing behavior

All good; I'll update !

rama78 commented 7 months ago

thanks, @anthonydahanne - we use the enterprise Rabbit MQ (outside PCF) with the CUPS, thanks for accepting the changes, I will look forward to the new branch and test it out when available.

Rama

anthonydahanne commented 6 months ago

OK, I think it's simpler than what I thought. Let me know @rama78 if that works out for you Thanks

anthonydahanne commented 6 months ago

Even better; I've published an online buildpack using the fixed java-cfenv: Make sure to use that in your manifest.yml

  buildpacks:
    - https://github.com/anthonydahanne/java-buildpack.git#fix-220-amqp

You'll see:

   -----> Downloading Java Cf Env 3.1.3 from https://anthonydahanne.github.io/java-cfenv/java-cfenv-all-3.1.3-SNAPSHOT.jar (0.2s)
rama78 commented 6 months ago

OK, I think it's simpler than what I thought. Let me know @rama78 if that works out for you Thanks

@anthonydahanne - Thanks a lot for the quick turnaround. I tested the branch, and it works as expected. I'm looking forward to a GA release so that we can start using it!

-Rama

rama78 commented 6 months ago

Even better; I've published an online buildpack using the fixed java-cfenv: Make sure to use that in your manifest.yml

  buildpacks:
    - https://github.com/anthonydahanne/java-buildpack.git#fix-220-amqp

You'll see:

   -----> Downloading Java Cf Env 3.1.3 from https://anthonydahanne.github.io/java-cfenv/java-cfenv-all-3.1.3-SNAPSHOT.jar (0.2s)

Indeed, great news! It may take time for me to test this buildpack as our company restricts downloads from the internet. We use offline buildpacks downloaded from Pivnet, but we could use an HTTP Proxy. I need to get access, etc

explorer-fullstack commented 6 months ago

@anthonydahanne - Even with latest JBP v4-64-0, we are still seeing rabbit mq connection errors.. we have to go down to JBP 4-59-0 to get past this error.

{"level":"ERROR","timestamp":"2023-12-18T22:38:47.378Z","loggerName":"com.rabbitmq.client.impl.SocketFrameHandler","message":"TLS connection failed: No subject alternative names matching IP address found","logType":"Normal","duid":"duid"}

This issue is seen in the JBP versions starting at least 4-62-0