Modularize API server (api/v1/server) #24016

joamaki · 2023-02-24T16:18:35Z

This PR modularizes the cilium-agent's REST API server served over UNIX socket (cilium.sock) to
allow API handlers to be implemented from any module.

See #23882 for further motivation and individual commit messages for details.
The adaption to cilium-agent is left as follow-up PR.

After this a handler for Cilium API can be implemented thus:

import (
  policyapi "github.com/cilium/cilium/api/v1/server/restapi/policy"
)

var exampleHandlersCell = cell.Module(
  "example-handlers",
  "Example API handlers",

  cell.Provide(exampleHandlers),
)

type handlersOut struct {
  cell.Out

  GetIPHandler policyapi.GetIPHandler
  GetPolicyHandler policyapi.GetPolicyHandler
  // ...
}

func exampleHandlers(s Some, d Dependencies) handlersOut {
  return handlersOut{
    GetIPHandler: &getIPHandler{s, d},
    GetPolicyHandler: &getPolicyHandler{s, d},
    // ...
  }
}

type getIPHandler struct { /* ... */ }

func (h *getIPHandler) Handle(params GETIPParams) middleware.Responder {
  // ...
}

api/v1/server/server.go

bimmlerd

Nice work!
Not too sure where handlers should live, having them all under daemon seems like a collection of mostly unrelated things again, but putting them closer to the modules the interface with doesn't encourage the clean separation as much.
Given they might share some handler code I'm in favour of collecting the handlers in one place, but I don't know how difficult it'll be to untangle them.

api/v1/server/server.go

api/v1/server.gotmpl

bimmlerd · 2023-02-27T09:53:17Z

daemon/cmd/api_handlers.go

+type handlersOut struct {
+	cell.Out
+
+	DaemonGetCgroupDumpMetadataHandler daemon.GetCgroupDumpMetadataHandler `optional:"true"`


side note, I think optional:"true" isn't described in the hive documentation, so had to read up on it in https://pkg.go.dev/go.uber.org/dig#hdr-Optional_Dependencies

Right yeah this needs to be documented. Though I did just shoot myself in the foot with it.. I had the endpoint handler depend on ratelimiter that I hadn't provided and it just ignored it rather than complained about it (e.g. the optionality was infectious).. so might be better that it stays undocumented for now :D

I've removed the optional from this for now.

daemon/cmd/api_handlers.go

daemon/cmd/handlers/endpoints.go

joamaki · 2023-04-05T08:36:15Z

/test

joamaki · 2023-04-05T10:49:40Z

/test-runtime

joamaki · 2023-04-11T08:48:44Z

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With native routing

Failure Output

FAIL: Error deleting resource /home/jenkins/workspace/Cilium-PR-K8s-1.16-kernel-4.19/src/github.com/cilium/cilium/test/k8s/manifests/host-policies.yaml: Cannot retrieve "cilium-g8ckm"'s policy revision: cannot get policy revision: ""

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-kernel-4.19/867/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19 so I can create one.

joestringer

Is it possible to break this down into smaller chunks? 3K LoC for a review is too big.

(First two commits LGTM, I stopped after that)

daemon/restapi/api_limits_test.go

joamaki · 2023-04-12T10:43:36Z

Is it possible to break this down into smaller chunks? 3K LoC for a review is too big.

(First two commits LGTM, I stopped after that)

There isn't that many changes. Most of the lines are regenerated code or documentation.
EDIT: On the other hand, the changes to the handlers were somewhat complicated, so let's split this up.

api/v1/health/server/server.go

joamaki · 2023-04-14T13:05:31Z

/test

joestringer

Looks innocent enough 👍 Were you able to figure out the solution to the question Andrew asked about above? I don't see it in the current copy of the code.

joamaki · 2023-04-18T08:00:01Z

Looks innocent enough +1 Were you able to figure out the solution to the question Andrew asked about above? I don't see it in the current copy of the code.

Oh no, I forgot to push! Could you please take another look?

joamaki · 2023-04-18T08:04:02Z

api/v1/server.gotmpl

@@ -646,22 +584,3 @@ func (s *Server) TLSListener() (net.Listener, error) {
 	return s.httpsServerL, nil
 }

-func handleInterrupt(once *sync.Once, s *Server) {


I've removed the signal handling as it's now done by hive in both the agent and the operator, and I checked that hubble-relay which uses the health server also does signal handling itself.

This implements server.Cell that takes in the API handlers individually rather than relying on some central place to construct "CiliumAPIAPI". Example use: import ( "github.com/cilium/cilium/api/v1/server "github.com/cilium/cilium/api/v1/server/restapi/endpoint" ) h := hive.New( endpointmanager.Cell, server.Cell, cell.Provide(newGetEndpointHandler), ) func newGetEndpointHandler(em endpointmanager.EndpointManager) endpoint.EndpointGetEndpointHandler { // Implement EndpointGetEndpointHandler with the endpoint manager. } Signed-off-by: Jussi Maki <jussi@isovalent.com>

Too many parameters in newDaemon, so just pass the daemonParams as-is. Signed-off-by: Jussi Maki <jussi@isovalent.com>

Refactor the server Stop() to respect the hook timeout and forcefully terminate active connections. Remove unused code from the template. Signed-off-by: Jussi Maki <jussi@isovalent.com>

bimmlerd

lgtm in this slimmed down form

api/v1/health/server/server.go

joamaki · 2023-04-19T06:13:05Z

/test

pippolo84

Nice! 🚀

joamaki · 2023-04-19T08:43:20Z

Few failing tests due to transient apt install failures and temp VM provisioning failure. All relevant tests pass, so marking ready to merge.

dylandreimerink

Very nice!

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 24, 2023

bimmlerd reviewed Feb 24, 2023

View reviewed changes

api/v1/server/server.go Outdated Show resolved Hide resolved

joamaki force-pushed the pr/joamaki/api-server-cell branch from b4d3485 to 7532362 Compare February 24, 2023 16:27

bimmlerd reviewed Feb 27, 2023

View reviewed changes

joamaki force-pushed the pr/joamaki/api-server-cell branch from 7532362 to 4d89ce0 Compare February 28, 2023 14:52

joamaki added the release-note/misc This PR makes changes that have no direct user impact. label Feb 28, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 28, 2023

joamaki force-pushed the pr/joamaki/api-server-cell branch 3 times, most recently from 44baaa4 to d0ff2ac Compare March 1, 2023 16:27

joamaki mentioned this pull request Mar 6, 2023

Modularization: ipcache #24178

Closed

joamaki force-pushed the pr/joamaki/api-server-cell branch 3 times, most recently from b2da204 to 8a5b57a Compare April 4, 2023 12:25

joamaki force-pushed the pr/joamaki/api-server-cell branch from 8a5b57a to 887aa16 Compare April 5, 2023 06:51

joamaki marked this pull request as ready for review April 5, 2023 12:42

joamaki requested review from a team as code owners April 5, 2023 12:42

joamaki requested a review from pippolo84 April 5, 2023 12:42

joamaki force-pushed the pr/joamaki/api-server-cell branch from 4ce78d5 to 78bd797 Compare April 11, 2023 07:08

joamaki removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 11, 2023

joestringer reviewed Apr 11, 2023

View reviewed changes

daemon/restapi/api_limits_test.go Outdated Show resolved Hide resolved

joamaki force-pushed the pr/joamaki/api-server-cell branch 2 times, most recently from 5835434 to 076aae0 Compare April 13, 2023 06:46

asauber reviewed Apr 13, 2023

View reviewed changes

api/v1/health/server/server.go Outdated Show resolved Hide resolved

joamaki requested review from joestringer, bimmlerd and asauber April 17, 2023 07:22

joestringer approved these changes Apr 18, 2023

View reviewed changes

joamaki force-pushed the pr/joamaki/api-server-cell branch from 076aae0 to dbb792f Compare April 18, 2023 07:59

joamaki commented Apr 18, 2023

View reviewed changes

joamaki added 3 commits April 18, 2023 13:46

daemon: Pass daemonParams to newDaemon

3f5f292

Too many parameters in newDaemon, so just pass the daemonParams as-is. Signed-off-by: Jussi Maki <jussi@isovalent.com>

api/v1: Implement proper shutdown and cleanup cruft

edd7eaa

Refactor the server Stop() to respect the hook timeout and forcefully terminate active connections. Remove unused code from the template. Signed-off-by: Jussi Maki <jussi@isovalent.com>

joamaki force-pushed the pr/joamaki/api-server-cell branch from dbb792f to edd7eaa Compare April 18, 2023 10:46

bimmlerd approved these changes Apr 18, 2023

View reviewed changes

api/v1/health/server/server.go Show resolved Hide resolved

joestringer approved these changes Apr 19, 2023

View reviewed changes

pippolo84 approved these changes Apr 19, 2023

View reviewed changes

joamaki added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 19, 2023

ldelossa merged commit 5ca3696 into cilium:main Apr 19, 2023
54 of 57 checks passed

dylandreimerink approved these changes Apr 19, 2023

View reviewed changes

joamaki mentioned this pull request May 22, 2023

daemon: Use API server cell and adapt handlers #25000

Merged

sayboras mentioned this pull request Jun 2, 2023

v1.13 backports 2023-06-02 #25855

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularize API server (api/v1/server) #24016

Modularize API server (api/v1/server) #24016

joamaki commented Feb 24, 2023 •

edited

bimmlerd left a comment

bimmlerd Feb 27, 2023

joamaki Feb 28, 2023

joamaki commented Apr 5, 2023

joamaki commented Apr 5, 2023

joamaki commented Apr 11, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

joestringer left a comment

joamaki commented Apr 12, 2023 •

edited

joamaki commented Apr 14, 2023

joestringer left a comment

joamaki commented Apr 18, 2023

joamaki Apr 18, 2023

bimmlerd left a comment

joamaki commented Apr 19, 2023

pippolo84 left a comment

joamaki commented Apr 19, 2023 •

edited

dylandreimerink left a comment

Modularize API server (api/v1/server) #24016

Modularize API server (api/v1/server) #24016

Conversation

joamaki commented Feb 24, 2023 • edited

bimmlerd left a comment

Choose a reason for hiding this comment

bimmlerd Feb 27, 2023

Choose a reason for hiding this comment

joamaki Feb 28, 2023

Choose a reason for hiding this comment

joamaki commented Apr 5, 2023

joamaki commented Apr 5, 2023

joamaki commented Apr 11, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

joestringer left a comment

Choose a reason for hiding this comment

joamaki commented Apr 12, 2023 • edited

joamaki commented Apr 14, 2023

joestringer left a comment

Choose a reason for hiding this comment

joamaki commented Apr 18, 2023

joamaki Apr 18, 2023

Choose a reason for hiding this comment

bimmlerd left a comment

Choose a reason for hiding this comment

joamaki commented Apr 19, 2023

pippolo84 left a comment

Choose a reason for hiding this comment

joamaki commented Apr 19, 2023 • edited

dylandreimerink left a comment

Choose a reason for hiding this comment

joamaki commented Feb 24, 2023 •

edited

joamaki commented Apr 11, 2023 •

edited by maintainer-s-little-helper bot

joamaki commented Apr 12, 2023 •

edited

joamaki commented Apr 19, 2023 •

edited