pblittle / docker-logstash

Docker image for Logstash 1.4
https://hub.docker.com/r/pblittle/docker-logstash
MIT License
236 stars 90 forks source link

S appended to https on restarting container #72

Closed simonluijk closed 9 years ago

simonluijk commented 9 years ago

When you restart a container that exited for any reason the config is updated. If you have set ES_PROXY_PROTOCOL=https an s is appended to the protocol each time the container is restarted.

I think the replace of the protocol on this line should be updated to include the character after the http. They by failing when the protocol has already been updated. https://github.com/pblittle/docker-logstash/blob/master/1.4/base/kibana.sh#L34

https

pblittle commented 9 years ago

@simonluijk, thanks man, this is a great find. I'll try to work this in as a hotfix today. I need to write a test to verify, but I think using a something as simple as replacing http with ^http[s]? should do the trick.

If you have time, feel free to submit a PR. :smile:

simonluijk commented 9 years ago

That solution was my initial thought too, and maybe the best solution. But I think it might be best for it to only match the unmodified config. I don't have a config file to hand. If you send my an example of the config file I will have a look.

Simon

On 24/03/15 15:28, P. Barrett Little wrote:

@simonluijk https://github.com/simonluijk, thanks man, this is a great find. I'll try to work this in as a hotfix today. I need to write a test to verify, but I think using a something as simple as replacing |http| with |^http[s]?| should do the trick.

If you have time, feel free to submit a PR. :smile:

— Reply to this email directly or view it on GitHub https://github.com/pblittle/docker-logstash/issues/72#issuecomment-85523004.

pblittle commented 9 years ago

@simonluijk, I imagine you know, but to be certain, the Kibana config that is being used in this project is embedded in vendor/kibana in the Logstash package.

But I think it might be best for it to only match the unmodified config.

Given the original file is being modified, additional logic will need to be added to track modifications. This is definitely worth exploring. Maybe in a separate issue?

If you send my an example of the config file I will have a look.

For testing, you should be able to use the config file that is vendored in Logstash.

simonluijk commented 9 years ago

OK, I have done some testing. This will only match the original config, if it has been modified manually or already updated it won't have any effect. Sorry I was not able to get a PR together.

sed -e 's|\"http\:|"https:|g' test.js

pblittle commented 9 years ago

@simonluijk, cool. Thanks for the update. No problem on the PR. I'll try to get something together this afternoon.

pblittle commented 9 years ago

@simonluijk do you mind taking a look at #73 and verify the fix before I merge it in?