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

Logstash+Kibana config flexibility improvements #2531

Merged
merged 1 commit into from
Jan 26, 2021

Conversation

PMazarovich
Copy link
Contributor

Motivation and context

It is important to be able to connect to remote instances of Logstash or Kibana. With this changes you can simply configure any address (with or without credentials) via environment variables.
As example you can use this with AWS: for Logstash you can use Fargate, for the rest - Amazon Elasticsearch, which provide access to Kibana by URL.

How has this been tested?

It was tested manually.

Checklist

  • [ x] I submit my changes into the develop branch
# Copyright (C) 2020 Intel Corporation
#
# SPDX-License-Identifier: MIT

@azhavoro
Copy link
Contributor

azhavoro commented Dec 7, 2020

@PMazarovich, thanks for the your contribution!

@@ -63,6 +63,8 @@ services:
container_name: cvat_ui
image: cvat/ui
restart: always
environment:
CVAT_KIBANA_URL: http://localhost:8080/analytics/app/kibana
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the cvat_proxy service does not use the default CVAT_HOST or port, then Kibana will not be available at http://localhost:8080/analytics/app/kibana, so this setting must be changed consistently. From my point of view, this is not good.
Inside the cvat backend uses a reverse proxy for proxy requests in kibana, I would suggest making this django app customizable and pass CVAT_KIBANA_URL env variable into cvat instead of the current implementation. Do you see any problems with this approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not use "default" cvat_proxy service (also cvat_logstash, cvat_elasticsearch and cvat_kibana), instead all these services from AWS or using Fargate, and accessed via Application Load Balancer. So it is necessary for us to have this cvat_kibana_url inside cvat_ui container, not in cvat one.
Or did I misunderstand something and there is some additional reverse proxy logic there?

Copy link
Contributor

@azhavoro azhavoro Dec 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PMazarovich sorry for the late response
I mean your changes breaks the current approach to setting the cvat hostname with a single env variable (CVAT_HOST). For example, if I want to use anything other than localhost as the cvat hostname with the default ELK, with your changes, I have to set 2 environment variables accordingly: CVAT_HOST and CVAT_KIBANA_URL.
I just would suggest proxy all requests to kibana independently of kibana URL as it's implemented now.
Also one more issue: regardless on CVAT_KIBANA_URL setting UI displays the Analytics tab as origin URL
image

Copy link
Contributor Author

@PMazarovich PMazarovich Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our case it is impossible to leave only CVAT_HOST variable because AWS kibana is accessible through this path: /_plugin/kibana/, not this one: /analytics/app/kibana. That is why I suggest to leave approach from the PR. Of course, you can change this in header.tsx, but, as for me, it is better to create another ENV rather than changing code. As for your notice about href, I'll perform another commit this week, that solves the problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Andrey, proposed changes look complicated. IMHO a client should work only with its server and if analytics located somewhere else, the server is expected to proxy request where need. Other approaches over complicate the architecture and configuration.

@PMazarovich PMazarovich force-pushed the Logstash_Kibana_config_fix branch from d1bff15 to 669c223 Compare January 20, 2021 14:27
@PMazarovich
Copy link
Contributor Author

Changes with corrections are performed taking into account the wishes of developers and minimized in general.

@coveralls
Copy link

coveralls commented Jan 20, 2021

Coverage Status

Coverage remained the same at 70.28% when pulling c73400e on PMazarovich:Logstash_Kibana_config_fix into c62949b on openvinotoolkit:develop.

@azhavoro
Copy link
Contributor

@PMazarovich LGTM, could you please add a note to changelog.md about this change? Thanks!

@PMazarovich PMazarovich force-pushed the Logstash_Kibana_config_fix branch from 669c223 to c73400e Compare January 25, 2021 10:08
@PMazarovich PMazarovich requested a review from nmanovic as a code owner January 25, 2021 10:08
@PMazarovich
Copy link
Contributor Author

Changelog updated.

@bsekachev bsekachev merged commit dfa9c0d into cvat-ai:develop Jan 26, 2021
@PMazarovich PMazarovich mentioned this pull request Jul 26, 2021
8 tasks
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

Successfully merging this pull request may close these issues.

4 participants