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

database-chassis python http-server implementation #13345

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

anamehra
Copy link
Contributor

@anamehra anamehra commented Jan 12, 2023

Signed-off-by: anamehra anamehra@cisco.com

  Python HTTP Server service.
  This service runs in database-chassis docker based on the configuration
  provided in device/<platform>/chassisdb.conf

  Configuration parameters provided by chassisdb.conf are as follows:

  start_http_server: if "yes" or "1", http-server will be started on this
      node
  chassis_db_address   : IP address for http-server. This is the same IP used
   for the chassis db server
  http_server_port : Port to bind to, default: 8000
  http_server_dir  : HTTP server home directory path, default: /var/www/

# Example of config:
'''
start_http_server=yes
chassis_db_address=127.0.0.10
http_server_dir=/var/www/tftp
'''

Why I did it

Enable http-server as database-chassis service on Chassis supervisor to enable LCs to download image and config files from Supervisor.

How I did it

Added python3 http-server based implementation in database-chassis as a docker service run by supervisord.

How to verify it

Add device//chassisdb.conf with the following data:
start_http_server=yes
chassis_db_address=127.0.0.10
http_server_dir=/var/www/tftp

Boot with the image with the above data.

put an image file at /var/www/tftp/filename
check 'systemctl status http-service'
curl http://http-server:8000/filename -o out_file
compare file_out with /var/www/tftp/filename

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

  Python HTTP Server service.
  This service runs in database-chassis docker based on the configuration
  provided in device/<platform>/chassisdb.conf

  Configuration parameters provided by chassisdb.conf are as follows:

  start_http_server: if "yes" or "1", http-server will be started on this
      node
  chassis_db_address   : IP address for http-server. This is the same IP used
   for the chassis db server
  http_server_port : Port to bind to, default: 8000
  http_server_dir  : HTTP server home directory path, default: /var/www/

Signed-off-by: anamehra <anamehra@cisco.com>
@anamehra
Copy link
Contributor Author

@abdosi , FYI-
Thanks

abdosi
abdosi previously approved these changes Jan 19, 2023
Copy link
Contributor

@abdosi abdosi left a comment

Choose a reason for hiding this comment

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

LGTM

@abdosi
Copy link
Contributor

abdosi commented Jan 19, 2023

@arlakshm / @judyjoseph please help review.

@anamehra
Copy link
Contributor Author

@judyjoseph , @arlakshm , please let me know if you have any comments. I resolved all comments from our last meeting discussion in this PR. moved the http-server in database-chassis docker for chassis.

@anamehra anamehra requested review from judyjoseph and removed request for lguohan and arlakshm February 1, 2023 16:05
judyjoseph
judyjoseph previously approved these changes Feb 2, 2023

{% if INSTANCES %}
{% for redis_inst, redis_items in INSTANCES.items() %}
{% if redis_inst == 'redis_chassis' %}
Copy link
Collaborator

@qiluo-msft qiluo-msft Feb 4, 2023

Choose a reason for hiding this comment

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

if

Need 2 level indentation #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @qiluo-msft !
I will fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@@ -44,5 +44,6 @@ COPY ["files/sysctl-net.conf", "/etc/sysctl.d/"]
COPY ["critical_processes", "/etc/supervisor"]
COPY ["files/update_chassisdb_config", "/usr/local/bin/"]
COPY ["flush_unused_database", "/usr/local/bin/"]
COPY ["http-server", "/usr/local/bin/"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

http-server

Why put this service in database container? I do not see any dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial PR was to run this as host side service as a feature but in the last review in Chassis group, it was decided to use it inside database-chassis for now as it's only for internal image/data hosting for chassis modules. It runs only on Supervisor on Chassis and binds to same internal ip as redis_chassis.server.

Copy link
Contributor

Choose a reason for hiding this comment

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

@qiluo-msft yes we had discussion on this and if we want to run on host there was comment from Guohan to add this as Feature in Feature Table. But that was little deviation from current model where each feature is like docker with we can enable/disable the feature by doing container stop/start. So in chassis subgroup we discussed and thought since that will be some infrastructure change for this scenario to unblock chassis use case we thought to make it part of database-chassis docker which is specific to chassis use case. Also in future if this is needed in host we can revisit.

# Example of config:
'''
start_http_server=yes
chassis_db_address=127.0.0.10
Copy link
Collaborator

@qiluo-msft qiluo-msft Feb 4, 2023

Choose a reason for hiding this comment

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

127.0.0.10

hard-coded IP? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an example of the config. The chassis_db_address is used for redis_chassis.server and is hardcoded as of today, provided by chassisd.conf file by vendor in dir.
This http-serevr use the same IP to bind to service requests on internal midplane network.

[program: http-server]
command=python3 /usr/local/bin/http-server
priority=3
autostart=true
Copy link
Collaborator

@qiluo-msft qiluo-msft Feb 4, 2023

Choose a reason for hiding this comment

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

true

no way to disable it #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The process starts when the database chassis starts but the server will run only if start_http_server flag is set in chassisd.conf file by the vendor. If the flag is not set, the http-server won't run and this service will exit. autorestart is false.


from pathlib import Path

HTTP_DEFAULT_BIND_PORT = "8000"
Copy link
Collaborator

@qiluo-msft qiluo-msft Feb 4, 2023

Choose a reason for hiding this comment

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

8000

Do we have rules to protect this service? #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will investigate more on this and respond/implement. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

@qiluo-msft your point is to have Control plane acl (ip table rules) to explicitly permit this ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, I believe you also want to limit the access, for example by some rules on 5-tuple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abdosi , @qiluo-msft , this port is on the internal network. Do we have any rules on other internal ip/ports which I can refer to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @qiluo-msft , Abhishek and I had a discussion on this. The current Sonic implementation does not have any rule defined for the midplane subnet yet and we need to have a rule for all kinds of access in the midplane subnet. We will have to define platform subnet parameters that caclmgrd could use to define the rules for midplane access for all kinds of midplane traffic in the chassis. I will open a case and we can use a different PR to define the general subnet access rule. Let me know if that is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

@qiluo-msft on chassis setup we have additional interface eth1-midplane both on supervisor and LC for internal traffic (eg: redis and http) . We will add iptable rule to allow all traffic on that interface. That way http traffic will be protected from outside network (eth0) and will get permit only if coming from internal network. I will create separate for that

Copy link
Contributor

Choose a reason for hiding this comment

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

Created PR : sonic-net/sonic-host-services#42 . With this PR any port 8000 traffic that is not coming on eth1-midplane will get drop and thus protected from external network access.

@abdosi
Copy link
Contributor

abdosi commented Feb 22, 2023

@qiluo-msft can you please review this again.

    If the start_http_server is not set, the process exit with status 0 but it
    exits before reaching RUNNING state which happens after running for more than a sec.
    In such a scenario, supervisord restarts the process and mark state as FATAL. For exit
    with status 0, we do not want to restart the process.

    Fixed indentation
@anamehra anamehra requested a review from xumia as a code owner March 31, 2023 15:54
@liat-grozovik liat-grozovik dismissed stale reviews from judyjoseph and abdosi via 7185cf6 April 13, 2023 11:18
@gechiang
Copy link
Collaborator

@anamehra , can you resolve the conflict since the changes has been a while and require rebase/merge to resolve conflicts.
We can then ask @qiluo-msft to approve/merge.
Thanks!

@abdosi
Copy link
Contributor

abdosi commented Jul 13, 2023

@anamehra , can you resolve the conflict since the changes has been a while and require rebase/merge to resolve conflicts. We can then ask @qiluo-msft to approve/merge. Thanks!

we don;t need this for 202205. This will gets merged to master directly.

anamehra and others added 2 commits July 12, 2023 20:00
@anamehra
Copy link
Contributor Author

@anamehra , can you resolve the conflict since the changes has been a while and require rebase/merge to resolve conflicts. We can then ask @qiluo-msft to approve/merge. Thanks!

resolved. Thanks!

@arlakshm
Copy link
Contributor

Lower priority now. Will revisit in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants