Use `github.com/tinkerbell/dhcp` library: #337

jacobweinstock · 2023-08-05T04:16:18Z

Description

This is an improved DHCP library. The package layout has been updated to improve the reflection of the services that Boots runs.

test directory and docker-compose were updated to work with these changes.

The cli flags and env vars have been updated and reorganized by function (syslog, dhcp, tftp, http ipxe binaries, http ipxe scripts). This helps func main be to more understandable.

The phonehome endpoint and all /_packet were removed. These were old deprecated endponts that don't serve much purpose now.

The syslog package was updated to handle shutdown via context, so that it can be used like the other services with errgroup.

Update nix shell file to use Go 1.20.

Why is this needed

Fixes: #266
Fixes: #195

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

This is an improved DHCP library. The package layout has been updated to improve the reflection of the services that Boots runs. The `cmd/boots` package has been remove and main.go is not top level. The extra packages are necessary. References to `cmd/boots` were removed. test directory and docker-compose were updated to work with these changes. The cli flags and env vars have been updated and reorganized by function (syslog, dhcp, tftp, http ipxe binaries, http ipxe scripts). This helps func main be to more understandable. The phonehome endpoint and all `/_packet` were removed. These were old deprecated endponts that don't serve much purpose now. The syslog package was updated to handle shutdown via context, so that it can be used like the other services with `errgroup`. Update nix shell file to use Go 1.20. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

There isn't a strong enough case for the backend code to have its own package. Signed-off-by: Jacob Weinstock <[email protected]>

This enables honoring the `http-ipxe-binary-enabled` and `http-ipxe-script-enabled` if they both are disabled. Signed-off-by: Jacob Weinstock <[email protected]>

This sorts the flags in the usage output by the service name in between the brackets "[]" in the usage string. Signed-off-by: Jacob Weinstock <[email protected]>

Clarify Boots interoperability with existing DHCP servers. Update flag and env var references. Add section on the services Boots provides. Signed-off-by: Jacob Weinstock <[email protected]>

codecov · 2023-08-07T03:43:14Z

Codecov Report

Merging #337 (fd7ff9e) into main (da97000) will increase coverage by 16%.
The diff coverage is 33%.

❗ Current head fd7ff9e differs from pull request most recent head 59b6e4e. Consider uploading reports for the commit 59b6e4e to get more accurate results

@@          Coverage Diff          @@
##           main   #337     +/-   ##
=====================================
+ Coverage    13%    30%    +16%     
=====================================
  Files        18      6     -12     
  Lines      1732    426   -1306     
=====================================
- Hits        233    129    -104     
+ Misses     1482    288   -1194     
+ Partials     17      9      -8

Files Changed	Coverage Δ
cmd/boots/backend.go	`0% <0%> (ø)`
cmd/boots/main.go	`0% <0%> (ø)`
ipxe/script/auto.go	`100% <ø> (ø)`
ipxe/script/ipxe.go	`24% <30%> (ø)`
cmd/boots/env.go	`32% <50%> (+13%)`	⬆️
cmd/boots/flag.go	`88% <88%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Order by service name in the "[]" and the flag name. Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock · 2023-08-11T16:11:51Z

Hey @chrisdoherty4, apologies that this PR is so large. It's a significant library change so i was unable to avoid this churn. We can go over it together if that would help. I can do this in our open forum community meeting if that works.

This helps clarify data translation between the handler.BackendReader and script.Data. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

HandlerMapping type in the http package allows func main to not need the stdlib http package and thus allowing bhttp to be http. Signed-off-by: Jacob Weinstock <[email protected]>

Simpler use of go run makes it easier to understand and lessens the make fu needed. Signed-off-by: Jacob Weinstock <[email protected]>

Improve clarity of purpose. Signed-off-by: Jacob Weinstock <[email protected]>

The previous version was doing what goimports was already doing. Signed-off-by: Jacob Weinstock <[email protected]>

While i prefer single command main funcs to live top level, the top level has a lot of files aleady and it felt more difficult to see and understand the Go files when they lived at the top level. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

chrisdoherty4 · 2023-08-16T11:46:24Z

.golangci.yml

@@ -106,7 +106,8 @@ linters:
    - asciicheck
    - bodyclose
    - cyclop
-    - deadcode
+    # deprecated
+    # - deadcode


Do you want to just delete these? I don't think there's a reason to leave them in given we don't intend to reenable.

ah yes, fixed.

chrisdoherty4

Minimal feedback as everything seems good.

chrisdoherty4 · 2023-08-16T11:56:21Z

cmd/boots/flag.go

+}
+
+func backendFlags(c *config, fs *flag.FlagSet) {
+	fs.BoolVar(&c.backends.file.Enabled, "backend-file-enabled", false, "[backend] enable the file backend for DHCP and the HTTP iPXE script")


It seems like I could specify --backend-file-enabled and --backend-kube-enabled without any explicit error?

yes, correct. fixed now.

chrisdoherty4 · 2023-08-16T12:01:05Z

cmd/boots/flag_test.go

+  -osie-url                   [http] url where OSIE(Hook) images are located
+  -tink-server                [http] ip:port for the Tink server
+  -tink-server-tls            [http] use TLS for Tink server (default "false")
+  -trusted-proxies            [http] comma separated list of trusted proxies


IPs or CIDRs or Domains?

CIDRs. added to the description.

chrisdoherty4 · 2023-08-16T12:01:59Z

cmd/boots/flag_test.go

+  -extra-kernel-args          [http] extra set of kernel args (k=v k=v) that are appended to the kernel cmdline iPXE script
+  -http-addr                  [http] local IP and port to listen on for iPXE http script requests (default "%[1]v:80")
+  -http-ipxe-binary-enabled   [http] enable iPXE http binary server (default "true")
+  -http-ipxe-script-enabled   [http] enable iPXE http script server) (default "true")


Suggested change

-http-ipxe-script-enabled [http] enable iPXE http script server) (default "true")

-http-ipxe-script-enabled [http] enable iPXE http script server (default "true")

chrisdoherty4 · 2023-08-16T12:02:23Z

cmd/boots/flag_test.go

+  -http-addr                  [http] local IP and port to listen on for iPXE http script requests (default "%[1]v:80")
+  -http-ipxe-binary-enabled   [http] enable iPXE http binary server (default "true")
+  -http-ipxe-script-enabled   [http] enable iPXE http script server) (default "true")
+  -osie-url                   [http] url where OSIE(Hook) images are located


Suggested change

-osie-url [http] url where OSIE(Hook) images are located

-osie-url [http] URL where OSIE(Hook) images are located

chrisdoherty4 · 2023-08-16T12:02:56Z

cmd/boots/flag_test.go

+	want := fmt.Sprintf(`USAGE
+  Run Boots server for provisioning
+
+FLAGS


There are a number of acronym inconsistencies in the help. ip vs IP, http vs HTTP.

chrisdoherty4 · 2023-08-16T12:08:13Z

cmd/boots/main.go

+
+// ipxeScript is is needed to be able to translate the handler.BackendReader
+// returned data to the script.Data struct.
+type ipxeScript struct {


It seems odd for this to live here?

ah yes, updated to a better place.

This was in main package but didnt make sense being there. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

Also fixed some typos. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock added 4 commits August 4, 2023 22:14

Update cli help usage:

b100f5b

Signed-off-by: Jacob Weinstock <[email protected]>

Update linter to latest

61687d0

Signed-off-by: Jacob Weinstock <[email protected]>

Fix GitHub issue of "no space left on device":

c2d4408

Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock mentioned this pull request Aug 5, 2023

New dhcp lib #326

Closed

3 tasks

jacobweinstock marked this pull request as ready for review August 5, 2023 04:48

jacobweinstock requested a review from chrisdoherty4 August 5, 2023 04:48

jacobweinstock added 4 commits August 5, 2023 21:30

Move backend package to main:

9b6e1f3

There isn't a strong enough case for the backend code to have its own package. Signed-off-by: Jacob Weinstock <[email protected]>

Only start the HTTP server if there are handlers:

a7b9b9c

This enables honoring the `http-ipxe-binary-enabled` and `http-ipxe-script-enabled` if they both are disabled. Signed-off-by: Jacob Weinstock <[email protected]>

Sort flags in usage function:

b57ad48

This sorts the flags in the usage output by the service name in between the brackets "[]" in the usage string. Signed-off-by: Jacob Weinstock <[email protected]>

Update README:

0b13efb

Clarify Boots interoperability with existing DHCP servers. Update flag and env var references. Add section on the services Boots provides. Signed-off-by: Jacob Weinstock <[email protected]>

Update cli flag usage output:

e34caa7

Order by service name in the "[]" and the flag name. Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock added 9 commits August 13, 2023 22:17

Add code comments, other small clean up:

72d5985

This helps clarify data translation between the handler.BackendReader and script.Data. Signed-off-by: Jacob Weinstock <[email protected]>

Rename make target, remove _packet reference:

e25fbe1

Signed-off-by: Jacob Weinstock <[email protected]>

Fix typo

b6a5c90

Signed-off-by: Jacob Weinstock <[email protected]>

Rename package bhttp -> http:

8d72e19

HandlerMapping type in the http package allows func main to not need the stdlib http package and thus allowing bhttp to be http. Signed-off-by: Jacob Weinstock <[email protected]>

Get rid of tools.go and clean up makefile:

9259b29

Simpler use of go run makes it easier to understand and lessens the make fu needed. Signed-off-by: Jacob Weinstock <[email protected]>

Rename cli.go -> flag.go:

7a018b1

Improve clarity of purpose. Signed-off-by: Jacob Weinstock <[email protected]>

Simplify goimports in ci-checks.sh:

f2b1ac1

The previous version was doing what goimports was already doing. Signed-off-by: Jacob Weinstock <[email protected]>

Move main back into cmd/boots package:

002f4c0

While i prefer single command main funcs to live top level, the top level has a lot of files aleady and it felt more difficult to see and understand the Go files when they lived at the top level. Signed-off-by: Jacob Weinstock <[email protected]>

Remove deprecated golangci linters

b1f22e9

Signed-off-by: Jacob Weinstock <[email protected]>

chrisdoherty4 reviewed Aug 16, 2023

View reviewed changes

chrisdoherty4 previously approved these changes Aug 16, 2023

View reviewed changes

Move http ipxe script backend into ipxe pkg:

3ac1445

This was in main package but didnt make sense being there. Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock dismissed chrisdoherty4’s stale review via f1ec5ca August 16, 2023 13:21

jacobweinstock requested a review from chrisdoherty4 August 16, 2023 13:24

jacobweinstock added 2 commits August 16, 2023 07:39

Panic when more than one backend is enabled:

9a9ee52

Signed-off-by: Jacob Weinstock <[email protected]>

Make the flag usage descriptions be more uniform:

d911b31

Also fixed some typos. Signed-off-by: Jacob Weinstock <[email protected]>

Remove commented lines from .golangci.yml

0d80274

Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock force-pushed the use-dhcp-lib branch from 95f67ee to 0d80274 Compare August 16, 2023 13:39

Don't make errors out of plain string in panic func:

59b6e4e

Signed-off-by: Jacob Weinstock <[email protected]>

chrisdoherty4 approved these changes Aug 16, 2023

View reviewed changes

jacobweinstock added the ready-to-merge Signal to Mergify to merge the PR. label Aug 16, 2023

mergify bot merged commit d135982 into tinkerbell:main Aug 16, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `github.com/tinkerbell/dhcp` library: #337

Use `github.com/tinkerbell/dhcp` library: #337

jacobweinstock commented Aug 5, 2023 •

edited

Loading

codecov bot commented Aug 7, 2023 •

edited

Loading

jacobweinstock commented Aug 11, 2023

chrisdoherty4 Aug 16, 2023

jacobweinstock Aug 16, 2023

chrisdoherty4 left a comment

chrisdoherty4 Aug 16, 2023

jacobweinstock Aug 16, 2023

chrisdoherty4 Aug 16, 2023

jacobweinstock Aug 16, 2023

chrisdoherty4 Aug 16, 2023

jacobweinstock Aug 16, 2023

chrisdoherty4 Aug 16, 2023

jacobweinstock Aug 16, 2023

chrisdoherty4 Aug 16, 2023

jacobweinstock Aug 16, 2023

chrisdoherty4 Aug 16, 2023

jacobweinstock Aug 16, 2023

	-http-ipxe-script-enabled [http] enable iPXE http script server) (default "true")
	-http-ipxe-script-enabled [http] enable iPXE http script server (default "true")

	-osie-url [http] url where OSIE(Hook) images are located
	-osie-url [http] URL where OSIE(Hook) images are located

Use github.com/tinkerbell/dhcp library: #337

Use github.com/tinkerbell/dhcp library: #337

Conversation

jacobweinstock commented Aug 5, 2023 • edited Loading

Description

Why is this needed

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

codecov bot commented Aug 7, 2023 • edited Loading

Codecov Report

jacobweinstock commented Aug 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdoherty4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use `github.com/tinkerbell/dhcp` library: #337

Use `github.com/tinkerbell/dhcp` library: #337

jacobweinstock commented Aug 5, 2023 •

edited

Loading

codecov bot commented Aug 7, 2023 •

edited

Loading