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

fix: loop bootstrap sequence in daemon, add run command and agent #403

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

bhoopesh369
Copy link
Contributor

@bhoopesh369 bhoopesh369 commented Jun 19, 2024

closes #396

@bhoopesh369 bhoopesh369 requested a review from a team as a code owner June 19, 2024 13:48
@glimchb
Copy link
Member

glimchb commented Jun 19, 2024

@bhoopesh369 thanks for your contribution

when I look at Ubuntu, when using Network Manager, I see a separate file per interface and not all interfaces in the same file...

See example:

root@bf2:~# ls -l /var/lib/NetworkManager/dhclient-*
-rw-r--r-- 1 root root 6981 Jun 19 20:31 /var/lib/NetworkManager/dhclient-aa93b667-6aac-3804-91e9-4958e07fdb2f-oob_net0.lease
-rw-r--r-- 1 root root 2514 Jun  7 18:52 /var/lib/NetworkManager/dhclient-oob_net0.conf

I wonder what is the right approach here ? pass the folder to scan ?

I also worried if NetworkManager vs networkd will have different formats...

WDYT ?

@glimchb
Copy link
Member

glimchb commented Jun 19, 2024

for example I found:

root@bf2:~# nmcli -f DHCP4 device show oob_net0
DHCP4.OPTION[1]:                        broadcast_address = 172.22.255.255
DHCP4.OPTION[2]:                        dad_wait_time = 0
DHCP4.OPTION[3]:                        dhcp_lease_time = 600
DHCP4.OPTION[4]:                        dhcp_message_type = 5
DHCP4.OPTION[5]:                        dhcp_server_identifier = 172.22.0.1
DHCP4.OPTION[6]:                        domain_name = lab.opiproject.org
DHCP4.OPTION[7]:                        domain_name_servers = 8.8.8.8 1.1.1.1
DHCP4.OPTION[8]:                        expiry = 1718831245
DHCP4.OPTION[9]:                        ip_address = 172.22.3.2
DHCP4.OPTION[10]:                       network_number = 172.22.0.0
DHCP4.OPTION[11]:                       next_server = 172.22.0.1
DHCP4.OPTION[12]:                       requested_broadcast_address = 1
DHCP4.OPTION[13]:                       requested_domain_name = 1
DHCP4.OPTION[14]:                       requested_domain_name_servers = 1
DHCP4.OPTION[15]:                       requested_domain_search = 1
DHCP4.OPTION[16]:                       requested_host_name = 1
DHCP4.OPTION[17]:                       requested_interface_mtu = 1
DHCP4.OPTION[18]:                       requested_ms_classless_static_routes = 1
DHCP4.OPTION[19]:                       requested_netbios_name_servers = 1
DHCP4.OPTION[20]:                       requested_netbios_scope = 1
DHCP4.OPTION[21]:                       requested_ntp_servers = 1
DHCP4.OPTION[22]:                       requested_rfc3442_classless_static_routes = 1
DHCP4.OPTION[23]:                       requested_root_path = 1
DHCP4.OPTION[24]:                       requested_routers = 1
DHCP4.OPTION[25]:                       requested_static_routes = 1
DHCP4.OPTION[26]:                       requested_subnet_mask = 1
DHCP4.OPTION[27]:                       requested_sztp_redirect_urls = 1
DHCP4.OPTION[28]:                       requested_time_offset = 1
DHCP4.OPTION[29]:                       requested_wpad = 1
DHCP4.OPTION[30]:                       routers = 172.22.0.1
DHCP4.OPTION[31]:                       subnet_mask = 255.255.0.0
DHCP4.OPTION[32]:                       sztp_redirect_urls = https://bootstrap:8080/restconf/operations/ietf-sztp-bootstrap-server:get-bootstrapping-data

@bhoopesh369
Copy link
Contributor Author

bhoopesh369 commented Jun 20, 2024

@bhoopesh369 thanks for your contribution

when I look at Ubuntu, when using Network Manager, I see a separate file per interface and not all interfaces in the same file...

See example:

root@bf2:~# ls -l /var/lib/NetworkManager/dhclient-*
-rw-r--r-- 1 root root 6981 Jun 19 20:31 /var/lib/NetworkManager/dhclient-aa93b667-6aac-3804-91e9-4958e07fdb2f-oob_net0.lease
-rw-r--r-- 1 root root 2514 Jun  7 18:52 /var/lib/NetworkManager/dhclient-oob_net0.conf

I wonder what is the right approach here ? pass the folder to scan ?

I also worried if NetworkManager vs networkd will have different formats...

WDYT ?

hmm, so the lease file format is same for NetworkManager and dhclient (but in NetworkManager one file for each interface)
but is different in networkd

i saw the lease file inside of the sztp-client-1 container, located at /var/dhclient/dhclient.leases - in this everything was present in one file.

I think we have to parse networkd lease files seperately and yea pass the folder to scan

WDYT?

@glimchb
Copy link
Member

glimchb commented Jun 20, 2024

  1. we can use nmcli to fetch leases like I showed on the above example. I saw few go packages out there to do that... I think they use D-BUS...

example:

root@bf2:~# nmcli -f DHCP4 device show oob_net0 | grep sztp_redirect_urls
DHCP4.OPTION[27]:                       requested_sztp_redirect_urls = 1
DHCP4.OPTION[32]:                       sztp_redirect_urls = https://bootstrap:8080/restconf/operations/ietf-sztp-bootstrap-server:get-bootstrapping-data
  1. if we successful, we exit
  2. if not, we can try networkctl for systemd-networkd after
  3. if we successful, we exit
  4. lastly as backup option we can parse customer provided lease file or extend this to folder....

WDYT ?

@bhoopesh369
Copy link
Contributor Author

Makes sense
Will look into it

@bhoopesh369
Copy link
Contributor Author

so i searched for nmcli wrapper packages in go
I found some, but it has very limited capabilities and can't fetch leases.

I looked for any alternates that can do this and found this: https://github.com/insomniacslk/dhcp/

I wrote a quick code to test this out.
you can find it over here: https://github.com/bhoopesh369/nmcli-test/blob/main/src/nmcli-test/dhcp/dhcp_info_new.go

Here's what the code will do under the hood:

  • It creates a new DHCP client.
  • It initiates a DHCP exchange (DISCOVER, OFFER, REQUEST, ACK sequence) with the DHCP server for the specified network interface.
  • It extracts information from the DHCP ACK message received from the server.

This means that the code is getting fresh DHCP information directly from the network, not from any stored lease files.

is this good, or WDYT

@glimchb
Copy link
Member

glimchb commented Jun 24, 2024

is this good, or WDYT

I do not think we can run our own DHCLIENT... since there is one that will be running already on every server, so we will conflict on packets... the only option is to use fetch from existing clients...

so we can

  1. open new issue to investigate nmcli in golang more fetch DHCP leases from nmcli over dbus #418
  2. open new issue to investigate networkd in golang
  3. open new issue to investigate single file vs folder for dhcp leases
  4. definitely take your code that runs single time loop to a separate action
  5. in cmd run call the function above once and exit
  6. in cmd daemon call the function in a loop till success

WDYT ?

@bhoopesh369
Copy link
Contributor Author

hmm works, i get why we cant run another dhclient
cool, so refactor the runCommandDaemon function and then raise those 3 issues right?

@glimchb
Copy link
Member

glimchb commented Jun 24, 2024

hmm works, i get why we cant run another dhclient cool, so refactor the runCommandDaemon function and then raise those 3 issues right?

yep

I opened #418

@glimchb
Copy link
Member

glimchb commented Jun 25, 2024

@bhoopesh369 I'm looking at the code right now and I think we should revert all the changes...
the network interfaces, are not applicable as we saw the separate dhcp files...

so the only thing we should do is probably

  • call performBootstrapSequence() function in a loop in daemon command
  • call performBootstrapSequence() function once in run command

Thanks for opening rest of the issues

@bhoopesh369
Copy link
Contributor Author

Cool, I was thinking the same too
I will update it now

@glimchb
Copy link
Member

glimchb commented Jun 25, 2024

@bhoopesh369 looks great, please fix test

2024-06-25T18:48:48.5852000Z === RUN   TestAgent_RunCommand/TestAgent_RunCommand
2024-06-25T18:48:48.5852378Z 2024/06/25 18:48:47 runCommand started
2024-06-25T18:48:48.5852920Z 2024/06/25 18:48:47 [INFO] Starting the Request to get On-boarding Information.
2024-06-25T18:48:48.5853521Z 2024/06/25 18:48:47 [DEBUG] Sending to: https://localhost:8443
2024-06-25T18:48:48.5854772Z 2024/06/25 18:48:47 [DEBUG] Sending input: {"ietf-sztp-bootstrap-server:input":{"hw-model":"Virtual Machine","os-name":"Ubuntu 22.04.4 LTS","os-version":"22.04","nonce":""}}
2024-06-25T18:48:48.5856001Z 2024/06/25 18:48:47 Error doing the request Post "https://localhost:8443": dial tcp [::1]:8443: connect: connection refused
2024-06-25T18:48:48.5856931Z 2024/06/25 18:48:47 [ERROR]  Post "https://localhost:8443": dial tcp [::1]:8443: connect: connection refused
2024-06-25T18:48:48.5857994Z 2024/06/25 18:48:47 Error in performBootstrapSequence inside runCommand:  Post "https://localhost:8443": dial tcp [::1]:8443: connect: connection refused
2024-06-25T18:48:48.5859410Z     run_test.go:64: RunCommand() error = Post "https://localhost:8443": dial tcp [::1]:8443: connect: connection refused, wantErr false
2024-06-25T18:48:48.5860162Z --- FAIL: TestAgent_RunCommand (0.00s)
2024-06-25T18:48:48.5860680Z     --- FAIL: TestAgent_RunCommand/TestAgent_RunCommand (0.00s)

@bhoopesh369
Copy link
Contributor Author

bhoopesh369 commented Jun 25, 2024

yep looking into it

@bhoopesh369
Copy link
Contributor Author

bhoopesh369 commented Jun 25, 2024

a mock test server is not configured inside the test, that's why it failed,

I think the test for RunCommand() is redundant because the same is done by tests for each func inside of bootstrap sequence.

think that is why a test is not there for the daemon command

WDYT?

@glimchb
Copy link
Member

glimchb commented Jun 25, 2024

a mock test server is not configured inside the test, that's why it failed,

I think the test for RunCommand() is redundant because the same is done by tests for each func inside of bootstrap sequence.

think that is why a test is not there for the daemon command

WDYT?

ok, you can remove this test now in this PR

and open an issue to add back both tests properly, since we do want 100% code coverage..
.
see https://app.codecov.io/gh/opiproject/sztp/blob/main/sztp-agent%2Fpkg%2Fsecureagent%2Fdaemon.go

@glimchb
Copy link
Member

glimchb commented Jun 25, 2024

Also consider invoking run in this compose file for end2end CI, just add new agent5 entry:

sztp/docker-compose.yml

Lines 220 to 227 in 126f501

agent1:
<<: *agent
command: ['/opi-sztp-agent', 'daemon',
'--dhcp-lease-file', '/var/lib/dhclient/dhclient.leases',
'--bootstrap-trust-anchor-cert', '/certs/opi.pem',
'--device-end-entity-cert', '/certs/first_my_cert.pem',
'--device-private-key', '/certs/first_private_key.pem',
'--serial-number', 'first-serial-number']

@bhoopesh369 bhoopesh369 changed the title feat(agent): loop over DHCP servers and interfaces fix: loop bootstrap sequence in daemon, add run command and agent Jun 26, 2024
Copy link
Member

@glimchb glimchb left a comment

Choose a reason for hiding this comment

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

great work!

@glimchb glimchb merged commit 4f3ea7a into opiproject:main Jun 26, 2024
15 checks passed
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.

agent: loop over DHCP servers and interfaces
2 participants