Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Allow longer FQDN and IPv4/v6 in host field #128

Merged
merged 2 commits into from
Mar 29, 2021
Merged

Allow longer FQDN and IPv4/v6 in host field #128

merged 2 commits into from
Mar 29, 2021

Conversation

srilumpa
Copy link
Contributor

This PR should fix the following issues :

It is complementary to the PR #115 since it fixed the domain name only for the full URL field and not also for the standalone domain field.

For better code reuse, I moved the various regexes from the validateURL method out of it in order to mutualize them with the validateHost method. That way, the same regexes are used to validate both the standalone Host field and the hostname/ip in the full URL field.

Entering hostname, IPv4 or IPv6 is now valid in the standalone Host field.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@srilumpa srilumpa changed the title Allow longer FQDN and IPv4/v6 in hot field Allow longer FQDN and IPv4/v6 in host field Apr 24, 2020
@youtous
Copy link

youtous commented May 14, 2020

Thank you for fixing this!

Is your validation working with simple hostname such as a docker service name (using for inter container communication) ?

I'm using a webhook processor in a container, I would like to be able to query the webhook service using it's name: http://webhook:8080/email or with http://webhook/email.

ISSUE: #149

@Cyb3rSn0rlax
Copy link

Still having this issue with plugin 1.7.0 :
image

@srilumpa
Copy link
Contributor Author

Hi @youtous, no it won't work since the applied regexp to validate hostname keeps looking for a full FQDN.

@youtous
Copy link

youtous commented May 18, 2020

Hi @srilumpa,

Could you make it work for simples domains?
I'm pretty sure it could be useful for docker setup where services are queried by their name (in docker swarm you can't define static ip, so unless adding a startup script for updating /etc/hosts in the container, consequently we are unable to set a service url in this field).

The current workaround I use is:

POST _opendistro/_alerting/destinations
{
  "name": "smtp",
  "type": "custom_webhook",
  "custom_webhook": {
    "url": "http://alerts-smtp-forwarder:8080/email"
  }
}

But it's not GUI friendly, thank you :)

@srilumpa
Copy link
Contributor Author

I understand the issue but this would mean the validation to be much more permissive and I am not at the right place to take that design decision. If I have a go from @dbbaughe or other people from the Opendistro team, I can adapt the validation to allow short names to be set in through the GUI.

@dbbaughe
Copy link
Contributor

Hey @srilumpa, thanks for the PR.

For @youtous case, does the example already work just through the API layer (instead of Kibana)? If so it should be perfectly fine to allow it. If it doesn't then it would need to be handled also on the backend plugin too. This Kibana plugin is just a frontend layer to the Elasticsearch plugin. All the real validation will be happening there, whereas here it's just to help the user make correct choices that are known not to fail. It seems the current validation has been too restrictive though if all of this is already allowed by backend plugin.

@srilumpa
Copy link
Contributor Author

Thank you for your feedback. I will fix something less restrictive

@srilumpa
Copy link
Contributor Author

I think this last commit should do the trick

@tlfeng tlfeng force-pushed the master branch 3 times, most recently from ce430a5 to 09f93ce Compare February 6, 2021 08:44
Base automatically changed from master to main February 10, 2021 00:14
@skkosuri-amzn
Copy link

@srilumpa Apologies for the delay on this. Thanks for this PR. Tested this on 1.13 release. Targeted for next OpenDistro release.

@skkosuri-amzn
Copy link

On backend, we added "Support host deny list for Destinations" . Made sure that is IPv6 supported.

@skkosuri-amzn skkosuri-amzn merged commit 156161d into opendistro-for-elasticsearch:main Mar 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants