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

[ntp] Fix ntp.conf template to allow setting of source port in CONFIG_DB #7586

Merged
merged 2 commits into from
May 23, 2021

Conversation

alexrallen
Copy link
Contributor

Why I did it

Currently, there is a bug in the ntp.conf jinja2 template where it will ignore the src_intf directive in CONFIG_DB if there are multiple IP addresses associated with an interface. This code change fixes that bug and allows the template to select the correct source interface for NTP.

How I did it

I did this by modifying the macro in ntp.conf.j2 which determines if there is an ip address associated with an interface to set a state variable when it detects a valid interface entry in CONFIG_DB instead of outputting "true" directly (which could result in multiple "trues" outputted for interfaces with multiple valid IP addresses).

How to verify it

  1. Add two ipv4 addresses to an interface in SONiC

  2. Add the following configuration to config_db.json

{
"NTP": {
    "global": {
        "src_intf": "Ethernet1"
        }
    }
}

Replace Ethernet1 with the interface name of the one you assigned the IP addresses to.

  1. Run sudo config reload -y

  2. Open /etc/ntp.conf and verify that the following line exists

...
interface listen Ethernet1
...

The interface specified should be the one set in the previous steps.

Description for the changelog

[ntp] Fix ntp.conf template to allow setting of source port in CONFIG_DB

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

Light-Gray-German-Angora-Rabbit_Hidden-Springs-Farm_Shutterstock-e1612565280404

@alexrallen alexrallen requested a review from lguohan as a code owner May 12, 2021 03:38
@lguohan
Copy link
Collaborator

lguohan commented May 13, 2021

can you add test in src/sonic-config-engine/tests to test the template?

@lguohan lguohan requested a review from yxieca May 13, 2021 15:01
yxieca
yxieca previously approved these changes May 13, 2021
@alexrallen
Copy link
Contributor Author

@lguohan To verify that we are on the same page I assume you are referring to a new test here (https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-config-engine/tests/test_j2files.py) that would validate the output of ntp.conf correct?

@alexrallen
Copy link
Contributor Author

I would use https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-config-engine/tests/test_j2files.py#L291 as a template, it seems to be a similar test to the one I would want to perform (i.e. provide a minimal config_db file and check that the desired template output matches a ground truth)

One question would be that there are many branches within this template file. Is our desire to test all of these branches? That may be out of the scope of this pull request. I would propose just adding a test for the case of multiple valid IP addresses for a given Ethernet interface.

@lguohan
Copy link
Collaborator

lguohan commented May 14, 2021

i am ok with your proposal.

@alexrallen
Copy link
Contributor Author

@lguohan @yxieca Added requested unit tests, please take a look when you can.

@yxieca yxieca merged commit da7533a into sonic-net:master May 23, 2021
yxieca pushed a commit that referenced this pull request May 27, 2021
…_DB (#7586)

Why I did it
Currently, there is a bug in the ntp.conf jinja2 template where it will ignore the src_intf directive in CONFIG_DB if there are multiple IP addresses associated with an interface. This code change fixes that bug and allows the template to select the correct source interface for NTP.

How I did it
I did this by modifying the macro in ntp.conf.j2 which determines if there is an ip address associated with an interface to set a state variable when it detects a valid interface entry in CONFIG_DB instead of outputting "true" directly (which could result in multiple "trues" outputted for interfaces with multiple valid IP addresses).

How to verify it
Add two ipv4 addresses to an interface in SONiC

Add the following configuration to config_db.json

{
"NTP": {
    "global": {
        "src_intf": "Ethernet1"
        }
    }
}
Replace Ethernet1 with the interface name of the one you assigned the IP addresses to.

Run sudo config reload -y

Open /etc/ntp.conf and verify that the following line exists

...
interface listen Ethernet1
...
The interface specified should be the one set in the previous steps.

Description for the changelog
[ntp] Fix ntp.conf template to allow setting of source port in CONFIG_DB
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
…_DB (sonic-net#7586)

Why I did it
Currently, there is a bug in the ntp.conf jinja2 template where it will ignore the src_intf directive in CONFIG_DB if there are multiple IP addresses associated with an interface. This code change fixes that bug and allows the template to select the correct source interface for NTP.

How I did it
I did this by modifying the macro in ntp.conf.j2 which determines if there is an ip address associated with an interface to set a state variable when it detects a valid interface entry in CONFIG_DB instead of outputting "true" directly (which could result in multiple "trues" outputted for interfaces with multiple valid IP addresses).

How to verify it
Add two ipv4 addresses to an interface in SONiC

Add the following configuration to config_db.json

{
"NTP": {
    "global": {
        "src_intf": "Ethernet1"
        }
    }
}
Replace Ethernet1 with the interface name of the one you assigned the IP addresses to.

Run sudo config reload -y

Open /etc/ntp.conf and verify that the following line exists

...
interface listen Ethernet1
...
The interface specified should be the one set in the previous steps.

Description for the changelog
[ntp] Fix ntp.conf template to allow setting of source port in CONFIG_DB
alexrallen added a commit to alexrallen/sonic-buildimage that referenced this pull request Sep 14, 2021
…_DB (sonic-net#7586)

Why I did it
Currently, there is a bug in the ntp.conf jinja2 template where it will ignore the src_intf directive in CONFIG_DB if there are multiple IP addresses associated with an interface. This code change fixes that bug and allows the template to select the correct source interface for NTP.

How I did it
I did this by modifying the macro in ntp.conf.j2 which determines if there is an ip address associated with an interface to set a state variable when it detects a valid interface entry in CONFIG_DB instead of outputting "true" directly (which could result in multiple "trues" outputted for interfaces with multiple valid IP addresses).

How to verify it
Add two ipv4 addresses to an interface in SONiC

Add the following configuration to config_db.json

{
"NTP": {
    "global": {
        "src_intf": "Ethernet1"
        }
    }
}
Replace Ethernet1 with the interface name of the one you assigned the IP addresses to.

Run sudo config reload -y

Open /etc/ntp.conf and verify that the following line exists

...
interface listen Ethernet1
...
The interface specified should be the one set in the previous steps.

Description for the changelog
[ntp] Fix ntp.conf template to allow setting of source port in CONFIG_DB
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants