Setting server_address to something running behind reverse proxy seem to NOT work since 5.1.0 #164
Comments
petermicuch
changed the title
|
BTW - in browser I can reach HAPI without issues, so reverse proxy is working as expected. |
|
There has been a change in the server address. I would start here :) |
|
Thanks @psavva That is what I already noticed and changed in reverse proxy and that was not it. |
|
Please provide your configuration file @petermicuch |
|
@jvitrifork what configuration do you mean? For hapi, I took whatever is in the github without modifications. Only file I modified was pom.xml to add support for MSSql. |
|
I dont think this is a HAPI issue.
|
|
@jvitrifork it certainly is change of behavior at least, since the same address we have used with former HAPI version, without issues. It was never complaining. Isn't there just new functionality implemented, that actually tries to access "self" link or does any kind of validation? I am able to ping domain name used for HAPI in our case and I will also try to do that from inside container once I get it out of constant failed state. |
|
well ... the error |
|
@jvitrifork all understood, I can also read the error and understand its meaning (btw it is a bit different when I run in AKS). What I am trying to point out is, that there must be new checks or something introduced in 5.1.0 as this was working fine before. So I was more expecting answer from involved developer, that "yeah we've added few tweaks in there and there". Anyhow it seems I will have to dig into the code myself to find out what exactly was changed that causes this incompatobility between 5.0.2 and 5.1.0 (no major version update included). Thank you. |
|
Well ... I more or less rewrote this project to its current version (5.1.0) and up - converting it to a more 'spring boot native' version. Whether I've introduced something or it was introduced in the core project I can't say. |
|
@jvitrifork don't get me wrong, I appreciate your attempts to help. It would be great to know if there are any tests that run on something else than localhost, because I guess there the error must have been visible as well. I will try to investigate when I free myself. Hapi is definitelly great fhir implementation. |
|
FWIW this does look boot related to me.. See the context in the stack trace:
If you look at the specific boot code there *InetAddressFormatter:44) it looks like that class wants a raw host name, not a fully qualified URL. I don't know enough about boot to have any idea what would be calling that though or why. |
|
The |
|
@vladonemo you nailed it. That totally makes sense. Why didn't I think of that. Of course, its right. Previously there was a property called server_address (in 'global' scope) and that is as you state now moved with the hapi.fhir. prefix. - and, at the same time, there is the spring variable server.address which of course now exists. Well spotted! |
|
btw @petermicuch that would have been pretty clear given the config file / environment variables |
|
@jvitrifork you wanted config file and that was as I stated same as you have it in git. Nevertheless I pulled in Vlado here today as I am not that much into Java. Even last time he was helping with PR to read ENV variables and override configuration values. Thanks to both of you, I will try it out. |
|
@petermicuch - If I'm not mistaken, there's no server.address mentioned in the default config file in github - https://github.com/hapifhir/hapi-fhir-jpaserver-starter/blob/master/src/main/resources/application.yaml |
|
Hi @jvitrifork. That is exactly the point, I am migrating from version, where Let me paste here my old configuration (hapi.properties file was taken as is in git) and my new configuration. It still does not work for me and I am unsure how to actually set 5.0.2: My original environment variables from my helm chart template for DSTU3 fhir version using image with hapi-properties file are: 5.2.0: My current not-working environment (not sure how to map properties): I got rid of the error with server.address being wrong. But I got another error and I do not think that the Here is the error, sorry for long call stack, I did not want to leave anything out. Please also note that some time ago, there was PR to make loading from environment variables easy due to usage of K8S/docker and also issue that I think finally got solved by that in acceptable way. But right now mapping of the old to new environment is an issue. |
|
@jvitrifork my bad. I now found that I need to define for postgres something in addition which was not there before: So postgres is now working for me. I am trying out the |
|
If that turns out to work, PR to include that information in the README would be greatly appreciated :) |
|
Hi @jvitrifork , @jamesagnew, so setting It only shows correctly after I set This is really a bit confusing and I am not sure about consequences. What would happen if I ran server with Here is my current important part of settings that work fine for reference (just in case someone hits the same problem): Since the original problem I opened this issue for seems to be not present, I will only create PR as requested by @jamesagnew to improve documentation and then close this issue. However I am not sure how to improve documentation of the "version" problem, since there I need more info. |
|
|
I still have reservations about using the profiles on this way.
Ideally, we should move the Fhir version out of the spring profile and into
a config item in the application.yaml file, not utilizing the
spring.profiles.active.
The spring.profiles should really be reserved to use separate configs based
on the profile, such as different regions using different db connections,
hostnames, etc...
It would make life a lot easier for developers and deployers in the long
run.
…On Mon, Nov 23, 2020, 21:36 Jens Kristian Villadsen < ***@***.***> wrote:
1. There has not been created documentation going from the previous
format to the new yaml based format. It now seems evident to me that it
would be nice to have such documentation. It was not a priority for me as
my thought was that this project mostly (at least to me) is a component
that works as a substitute for real systems and a quick prototype thingy as
well as an example implementation.
2. hapi.fhir.fhir_version looks like it is a remnant from the old
setup. It is a variable that is not read at any point in time. As such, it
has no effect assigning it a value. This is confusing and it should be
removed
3. The FHIR version is assigned only by using the
spring.profiles.active
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#164 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALDFJRVR42HW2NEHZ56JTLSRK2S3ANCNFSM4TPSWPSA>
.
|
|
I don't have strong feelings on this one - |
|
@jvitrifork I have to agree with @psavva that cleanup is required and perhaps also a bit of consistency should be kept, not moving things around if not logically required. But you might have your reasons why this was moved, and that is fine, as long as there is easy migration path or perhaps message warning users that property is replaced by new one and will not be available in next version. There is one more confusion with regards to Postgres usage and As to the note about substitute for real systems: To be honest, this is one of best fhir implementations, if not the best one if you want to experiment with FHIR. I am seriously thinking to contribute more, but I am not Java person really. So maybe more in the area of ease of deployment (i.e. helm charts). I appreciate effort you are putting into this. Also discussed with @vladonemo few months back, why there is no spring used yet and after some time, here you've done that :-). |
|
I'll also be very happy to contribute Kubernetes deployment examples.
All ready have it deployed on my staging server in Kubernetes...
…On Mon, Nov 23, 2020, 22:00 petermicuch ***@***.***> wrote:
@jvitrifork <https://github.com/jvitrifork> I have to agree with @psavva
<https://github.com/psavva> that cleanup is required and perhaps also a
bit of consistency should be kept, not moving things around if not
logically required. But you might have your reasons why this was moved, and
that is fine, as long as there is easy migration path or perhaps message
warning users that property is replaced by new one and will not be
available in next version.
There is one more confusion with regards to Postgres usage and
hibernate.dialect. Previously this was the only thing needed to tell hapi
what driver to use. Today, without specifying
spring.datasource.driverClassName you will get exception that *Access to
DialectResolutionInfo cannot be null when 'hibernate.dialect' not set*,
even though you actually provide spring.jpa.properties.hibernate.dialect.
Now I wonder which one is needed and will only find out by experimenting.
So I guess there are places in the documentation worth to revisit.
As to the note about substitute for real systems: To be honest, this is
one of best fhir implementations, if not the best one if you want to
experiment with FHIR. I am seriously thinking to contribute more, but I am
not Java person really. So maybe more in the area of ease of deployment
(i.e. helm charts).
I appreciate effort you are putting into this. Also discussed with
@vladonemo <https://github.com/vladonemo> few months back, why there is
no spring used yet and after some time, here you've done that :-).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#164 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALDFJV72T6F54BNV56WIX3SRK5MXANCNFSM4TPSWPSA>
.
|
|
That is news to me that the |
|
Yes - The new SearchBuilder module no longer uses Hibernate, but instead generates search SQL expressions "by hand". As a result it needs to know which flavour of SQL to generate (more or less entirely because of the fact that Oracle doesn't have a boolean datatype... grrrr/wtf). We use the hibernate dialect property to figure that out. This is the class I added to handle that: 8850e64#diff-cb79a8377bcfe7818c44d17e1f6fa8b24b945f910d30e4c4e32cc6e0af27d9d3 |
|
I would like to change that class to ie. the following:
|
Merged
|
@jamesagnew here you go. I'll proceed updating the documentation (at least the current doc so that it is on par with fhir_version use instead of the spring profile) if you'll have a look at the PR. |
|
@jvitrifork I am not sure now, shall I keep or is this not needed with latest commit anymore? |
|
@jvitrifork I have tested with latest changes locally, but it seems Environment of container: Output in logs: I hope I did everything correctly. I pulled latest changes, I build docker image without using cache and I see all new ENV variables that I have added to be sure I am picking correct docker image. |
|
|
|
Silly me. You are right @jvitrifork even @vladonemo was mentioning that. I thought it will work fine, since it worked yesterday before latest commit. Thanks. |
|
Feel free to close this issue @petermicuch |
|
@jkiddo , sure I am closing, original problem is I think solved. BTW: It is very cumbersome to set properties as they are defined now in application.yaml via environement variables. Especially things like |
|
@petermicuch I had a look at it and I've made #177 - which names indices instead of having them unnamed. That enables what you wish for: Being able to eg. do @jamesagnew take a look at what you think. I've also changed some defaults in the config file. |
|
@jvitrifork let me create another issue where we can discuss that. I was planning to do that today. I have more points to relaxed binding and naming of the variables. I think for arrays read from environment variables, we should use different approach that what we do have currently. I will also provide specific test scenario. But basically named is better then using 0 etc. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I was using HAPI server version 5.0.2, connected to postgres DB and running in k8s cluster. Everything was running just fine. I now switched to HAPI server version 5.1.0 and I am getting errors on resolving server_address. Below is error from server running on Docker Desktop for Windows, but the same issue I can see in Azure Kubernetes Services.
Provided subpath /fhir-dstu3 resolves by reverse proxy and is proxied to HAPI server service and /fhir subpath. As said, this all worked fine on 5.0.2, but not with 5.1.0. Was there any change done recently in the area of server_address handling?
The text was updated successfully, but these errors were encountered: