Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop URL validation on ElasticSearch host in proxy settings #2074

Closed
shaliko opened this issue Feb 7, 2017 · 19 comments
Closed

Drop URL validation on ElasticSearch host in proxy settings #2074

shaliko opened this issue Feb 7, 2017 · 19 comments
Assignees
Milestone

Comments

@shaliko
Copy link
Contributor

shaliko commented Feb 7, 2017

For docker-compose and OpenShift/Kubernetes deploy we have a hack for able use Elasticsearch URL.

For OpenShift/Kubernetes (or cloud services) Elasticsearch can be available by host name (not valid URL) like "elasticsearch".

apinf_-_api_management_platform

Suggestion: Drop URL validation and allow user use url safe string as hostname.

Examples:

"elasticsearch"
"elasticsearch-2-4"
"elastic-search"

@shaliko
Copy link
Contributor Author

shaliko commented Feb 7, 2017

After finish that story we can drop hack from docker file https://github.com/apinf/platform/blob/develop/docker-compose.yml#L14-L15.

@shaliko shaliko changed the title Drop URL validation on elasticsearch host in proxy settings Drop URL validation on ElasticSearch host in proxy settings Feb 7, 2017
@brylie
Copy link
Contributor

brylie commented Feb 7, 2017

Some quick notes/questions,

  1. can't the internal elasticsearch url be structured to pass regex validation? E.g. http://elasticsearch.local?
    • in other words, is it the protocol in the string that is breaking things, or something else?
  2. we are currently refactoring our codebase, so that the Elasticsearch URL will be irrelevant ASAP (Use API Umbrella Admin API to get Dashboard Analytics #2036)

@shaliko
Copy link
Contributor Author

shaliko commented Feb 7, 2017

  1. Sometimes you can't use dot symbol (OpenShift).
    openshift_web_console
  2. If that in near feature, my request is not relevant.

@bajiat
Copy link
Contributor

bajiat commented Feb 7, 2017

@shaliko #2036 will not be implemented this sprint (36), I have only asked for a research on the changes required. Depending on the amount of changes or whether any current functitonality is lost, I am considering sprint 37.

@brylie
Copy link
Contributor

brylie commented Feb 7, 2017

It is also worth considering how the API Umbrella URL field will be affected by OpenShift deployment.

  • How are we deploying the API Umbrella instance related to OpenShift?
  • What kind of URL will we expect from the API Umbrella instance when deployed on OpenShift?

Issue #2036 is opened in order to remove the Elasticsearch url, which will mean we communicate directly with API Umbrella for analytics data. Regardless of when #2036 is resolved, it is our planned trajectory.

@brylie
Copy link
Contributor

brylie commented Feb 7, 2017

We can easily remove the RegEx validation from any URL field, but I just want to understand the issue and explore alternatives (in context of our development roadmap).

@brylie
Copy link
Contributor

brylie commented Feb 7, 2017

E.g. it may be possible to specify a different OpenShift hostname, which would possible resolve this issue:

@shaliko
Copy link
Contributor Author

shaliko commented Feb 7, 2017

"API Umbrella URL" because we still need access to API-Umbrella after setup for getting admin API-keys, we will need to assign regular domain name for API-Umbrella.

The issue can happen for internal services (ElasticSearch) because they should be accessible only from a private network by hostname instead of domains.

In future, if we will able get Admin API keys from "API Umbrella" without visit it from browser and do it only from Platform UI, we will have the same issue. Because "API Umbrella" will be only in private network by name like "api-umbrella".

@shaliko
Copy link
Contributor Author

shaliko commented Feb 7, 2017

@brylie Your link for the settings of setup OpenShift hostname (admin panel), not for services run on openshift (docker images like platform, api-umbrella). That not related to our issue.

@brylie
Copy link
Contributor

brylie commented Feb 7, 2017

In order for services to be exposed externally, an OpenShift Container Platform route allows you to associate a service with an externally-reachable host name.

OpenShift Core Concepts: Route Host Names

The first sentence of that section seems to be talking about services run on OpenShift.

@brylie
Copy link
Contributor

brylie commented Feb 7, 2017

I just think this issue is not urgent, although I can understand why it seems to be a high priority. Rather than take up the issue right away, I am simply recommending we wait until #2036 progresses.

@bajiat
Copy link
Contributor

bajiat commented Feb 15, 2017

Can I place this issue in icebox until we find out what will happen with #2036? The current understanding is that the data available through the admin API is really limited and would not serve our dashboard. Either we would need to enhance the admin API or we need to continue using ElasticSearch.

Or will placing this in icebox and waiting mean problems with any OpenShift experiments?

@bajiat bajiat added the icebox label Feb 16, 2017
@shaliko
Copy link
Contributor Author

shaliko commented Feb 16, 2017

I am OK with a move that issue to the icebox, fix #2036 will make not relevant that issue.

@bajiat bajiat added planning and removed icebox labels Mar 17, 2017
@marla-singer
Copy link
Contributor

@bajiat Interested

@bajiat
Copy link
Contributor

bajiat commented Mar 20, 2017

@brylie wanted have a discussion about this need in the Github issue first

@bajiat
Copy link
Contributor

bajiat commented May 2, 2017

@marla-singer Can you make the change?

@bajiat bajiat added this to the Sprint 42 milestone May 2, 2017
@marla-singer
Copy link
Contributor

@bajiat Yes, I can

@marla-singer
Copy link
Contributor

@frenchbread Now emq scheme has the regex for ElasticSearch field as well. Should it be removed?

@frenchbread
Copy link
Contributor

@marla-singer Yep, similarly to api-umbrella

@brylie brylie removed the ready label May 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants