Added sslmode/sslrootcert for postgresql and cockroachdb

[myeung18]

#65
Enhanced PostgreSqlBindingsPropertiesProcessor to process the sslmode, sslrootcert, and options under $SERVICE_BINDING_ROOT if they present, and construct the final connection URL with these properties.

[dmikusa]

This seems reasonable. My primary concern would be with test coverage for parsing the information out. I'd like to make sure we have that covered well.

I will take a closer look and play around with the unit tests a bit. I can probably get time next week to do that, just FYI.

[myeung18]

This seems reasonable. My primary concern would be with test coverage for parsing the information out. I'd like to make sure we have that covered well.

I will take a closer look and play around with the unit tests a bit. I can probably get time next week to do that, just FYI.

Added few more test cases. Please let me know if you have further comment.

[myeung18]

@dmikusa-pivotal
Hi, is this under review? any decision to merge?

[bmozaffa]

@dmikusa-pivotal anything we can do to move this forward?

[dmikusa]

No, nothing else is needed from your end. It just needs to be reviewed & merged. There are a couple of other PRs here as well, I will try to review them all next week and get a release cut.

[dmikusa]

Can you expand on why you have the cockroachdb specifics added into the Postgres processor?

The Postgres processor is only going to fire if your binding type is postgresql. I'll be upfront, I know nothing about cockroachdb. I'm assuming there's some sort of Postgres compatibility with it given you're combining the two here, even then, I think you'd want to define your own binding type like cockroachdb. That way there isn't ambiguity in terms of what it's connecting to. There are also clearly some things that are added on top of standard Postgres, like options.

I think we could separate the two without too much fuss. We could do that with an abstract PostgreSqlLikeBindingProcessor class that has the shared implementation. Then you could have PostgreSqlBindingsPropertiesProcessor and CockroachDbBindingsPropertiesProcessor which are final and extend from the abstract class. Or if you don't want to have the abstract class/inheritance, you could possibly implement CockroachDbBindingsPropertiesProcessor as a composable wrapper around PostgreSqlBindingsPropertiesProcessor. Either way, I think you could define a custom binding type and keep the CockroachDb-specific bits in their own class.

Does that seem reasonable?

[myeung18]

@dmikusa-pivotal
I had considered the pattern you metioned when I was developing the Quarkus and Go binding client libraries.
By doing this, A new binding type specifically for CockroachDB have to be created, but I found it unnecessary, and it creates only coding complexity and confusion.

The reasons are:

• when we work with CockroachDB and PostgreSQL connection, we're dealing with only one protocol: postgresql://.
  The connection string the processor is creating is for this protocol only. It is just that each DB could support different configurations.
  CockRoachDB uses --cluser in options, as it is a distributed database. If this option is incorrectly applied a PostgreSQL DB, the DB will reject it just like it rejects any other unsupported options params. An application will know which database they are connecting to, and will provide the correct options.
  Adding a new binding type could confuse people that there were a new protocol for CockroachDB specifically.
• We don't see any CockroacbDB driver in Java/Go/Python etc, as CockroachDB uses all the exising PostgreSQL drivers for its applications. Frameworks like Quarkus and Spring use postgresql JDBC module for CockroachDB as well.
  It is just not a design to separate two DBs in protocol and driver level. Thinking this way, we will see no ambiguity on the binding type.
  So, it makes sense for us to apply the same concept to Service Binding: both databases use the same binding type.

I have implemented this approach to other binding client libraries. It is also good for Spring-Cloud-Bindings library to be consistent with them as well.
I hope all these make sense to you. Please let me know how you think.

[dmikusa]

If you don't want to create a unique binding type that's fine by me. I just wanted to confirm the intent there, thanks for confirming.

That said, I would still like to have some separation in the code. The fact that comments are required to document what's being done to me means the complexity is high enough to warrant it (don't get me wrong, I like the comments & links, those are great, I'd just like to see some separation in the code to signify the difference as well).

You don't have to implement a full separate processor, but perhaps split out buildSslModeAndOptions(..) into two methods. That method is on the large side and doing two separate things anyway. You can use the name of the second to indicate it's specific to CockroachDB, like buildCockroachDbOptions(..).

[myeung18]

If you don't want to create a unique binding type that's fine by me. I just wanted to confirm the intent there, thanks for confirming.

That said, I would still like to have some separation in the code. The fact that comments are required to document what's being done to me means the complexity is high enough to warrant it (don't get me wrong, I like the comments & links, those are great, I'd just like to see some separation in the code to signify the difference as well).

You don't have to implement a full separate processor, but perhaps split out buildSslModeAndOptions(..) into two methods. That method is on the large side and doing two separate things anyway. You can use the name of the second to indicate it's specific to CockroachDB, like buildCockroachDbOptions(..).

yes, make sense, I did a new submit for that.
In my opinion, it is optional to even mention CockroachDB in the method name at all as long as we have properly Javadoc. options is a PostgreSQL thing and a PostgreSQL application can also specify some options for its PostgreSQL database, so I just name it as buildDboptions. However, do let me know if have any concern.

[dmikusa]

In my opinion, it is optional to even mention CockroachDB in the method name at all as long as we have properly Javadoc. options is a PostgreSQL thing and a PostgreSQL application can also specify some options for its PostgreSQL database, so I just name it as buildDboptions.

Oh, ok. I was looking at the Postgres docs yesterday and didn't see options listed. That plus the comments lead me to believe it was specific to CockroachDB. I clearly just missed it, thanks for clarifying. This looks great though. I'll re-run tests & get this merged. Thanks!