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.

Sign up for GitHub

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

Jump to bottom

[build_templates] [hostcfgd] Keep containers hostname up to date #2924

Merged
merged 2 commits into from
Jun 6, 2019
Merged

[build_templates] [hostcfgd] Keep containers hostname up to date #2924

merged 2 commits into from
Jun 6, 2019

Conversation

msosyak
Copy link
Contributor

@msosyak msosyak commented May 20, 2019

- What I did
Implemented logic to keep containers hostname up to date

- How I did it

  • Added updateHostName function to docker_image_ctl.j2
  • Added hostname specification on container creating step
  • Added listener for hostname changes in hostcfgd which call updateHostName for all running containers if an appropriate script(generated from docker_image_ctl.j2) exist in /usr/bin

- How to verify it
Change hostname in config_db.json
Load new configuration
Check hostname in containers

- Description for the changelog
Added logic to keep containers hostname up to date

@akokhan @pyadvichuk @vsenchyshyn @qiluo-msft @jleveque @lguohan

@msftclas
Copy link

msftclas commented May 20, 2019 •
edited
Loading

CLA assistant check
All CLA requirements met.

qiluo-msft
qiluo-msft reviewed May 22, 2019
View reviewed changes
files/build_templates/docker_image_ctl.j2 Outdated
@@ -153,6 +180,7 @@ start() {
{%- endif %}
--tmpfs /tmp \
--tmpfs /var/tmp \
--hostname $HOSTNAME \
Copy link
Collaborator

@qiluo-msft qiluo-msft May 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

$HOSTNAME [](start = 19, length = 9)

Possible input injection #Closed

Copy link
Collaborator

@qiluo-msft qiluo-msft May 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Still possible. For example:
sonichost"; rm -rf /; echo "

In reply to: 286279985 [](ancestors = 286279985)

Copy link
Contributor Author

@msosyak msosyak May 23, 2019 •
edited by qiluo-msft
Loading

Choose a reason for hiding this comment

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

Bash did not process metacharacters from variables only metacharacters directly present in a script or interpreter line.
It is easy to check by the next commands:

print_args() {
while (( "$#" )); do 
  echo $1 
  shift 
done
}
X="hostName"
Y='sonic"; echo injection; echo "'
print_args --hostname "$X" --name cname
#output
>>> --hostname
hostName
--name
cname

print_args --hostname "$Y" --name cname
#output
>>> --hostname
sonic"; echo injection; echo "
--name
cname

I thought that CVE-2016-0634 can be exploited there. But sonic use bash 4.4

So, for any case added validation for allowed characters in a hostname. #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.

@qiluo-msft Could we close this conversation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice comments! Really appreciate the clarification and reference.

In reply to: 286898693 [](ancestors = 286898693)

qiluo-msft
qiluo-msft requested changes May 22, 2019
View reviewed changes
Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

As comments

@msosyak msosyak force-pushed the msosyak/container-hostname-up-to-date branch 2 times, most recently from a1ac6f2 to 07ddbb8 Compare May 22, 2019 11:31
@myronsosyak
Copy link

Review comments fixed

@msosyak msosyak force-pushed the msosyak/container-hostname-up-to-date branch 2 times, most recently from 8778f17 to 95dcf85 Compare May 23, 2019 11:36
@mykolaf
Copy link
Collaborator

mykolaf commented May 23, 2019 •
edited
Loading

Please use a standard commit message format:

[component/folder touched]: Description intent of your changes

[List of changes]

Signed-off-by: Your Name your@email.com

e.g. [image config] Keep containers hostname up to date

@msosyak msosyak force-pushed the msosyak/container-hostname-up-to-date branch from 95dcf85 to 1856544 Compare May 27, 2019 08:28
@msosyak msosyak changed the title Keep containers hostname up to date [build_templates, hostcfgd]Keep containers hostname up to date May 27, 2019
Keep containers hostname up to date
7393f80 
* Add updateHostName function to docker_image_ctl.j2
* Add hostname specification on container creating step
* Add listener for hostname changes in hostcfgd

Signed-off-by: Myron Sosyak <msosyak@barefootnetworks.com>
@msosyak msosyak force-pushed the msosyak/container-hostname-up-to-date branch from 1856544 to 5ff6ca3 Compare May 29, 2019 10:59
@msosyak msosyak force-pushed the msosyak/container-hostname-up-to-date branch from 5ff6ca3 to fbbe144 Compare May 30, 2019 08:16
@jleveque jleveque changed the title [build_templates, hostcfgd]Keep containers hostname up to date [build_templates] [hostcfgd] Keep containers hostname up to date May 30, 2019
jleveque
jleveque suggested changes May 30, 2019
View reviewed changes
Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

Please fix typo in log message per comment above.

files/image_config/hostcfgd/hostcfgd Outdated Show resolved Hide resolved
Fix comments
574f237 
Signed-off-by: Myron Sosyak <msosyak@barefootnetworks.com>
@msosyak msosyak force-pushed the msosyak/container-hostname-up-to-date branch from fbbe144 to 574f237 Compare May 31, 2019 06:24
@akokhan
Copy link
Contributor

akokhan commented Jun 4, 2019

@jleveque , @lguohan ,
as I see, all review comments are resolved now. Is there anything else that blocks us from merging this PR?

@jleveque
Copy link
Contributor

jleveque commented Jun 4, 2019

Retest this please

@jleveque jleveque merged commit 3ec95e1 into sonic-net:master Jun 6, 2019
@lguohan lguohan mentioned this pull request Jun 13, 2019
Closed
@samaity samaity mentioned this pull request Sep 25, 2019
Merged
mssonicbld added a commit that referenced this pull request Oct 14, 2023
@mssonicbld
[submodule] Update submodule sonic-swss to the latest HEAD automatica…
07827d3 
…lly (#16785)

#### Why I did it
src/sonic-swss
```
* b9313df0 - (HEAD -> master, origin/master, origin/HEAD) Reducing the severity of oper fec attribute get failure (#2924) (89 minutes ago) [Sudharsan Dhamal Gopalarathnam]
* cb98893f - Add support for SEND_TO_INGRESS port table.  (#2816) (19 hours ago) [Yilan Ji]
* 966c5bb0 - [Dash] Fix wrong table name for acl_out_table (#2911) (2 days ago) [Ze Gan]
* 35996350 - [FEC]Auto FEC initial changes (#2893) (8 days ago) [Sudharsan Dhamal Gopalarathnam]
```
#### How I did it
#### How to verify it
#### Description for the changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@myronsosyak myronsosyak myronsosyak left review comments

@jleveque jleveque jleveque approved these changes

@qiluo-msft qiluo-msft qiluo-msft approved these changes

@lguohan lguohan Awaiting requested review from lguohan lguohan is a code owner

Assignees
No one assigned
Labels
Enhancement ➕
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@msosyak @msftclas @myronsosyak @mykolaf @akokhan @jleveque @qiluo-msft