-
Notifications
You must be signed in to change notification settings - Fork 366
feat: add unified API server with debugging capabilities #2550
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
Conversation
internal/provider/apisix/provider.go
Outdated
| syncCh: make(chan struct{}, 1), | ||
| client: cli, | ||
| // TODO: Maybe pass port/address from external configuration | ||
| adcdebugserver: common.NewADCDebugServer(cli.Store, cli.ConfigManager, 8432), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should be needed to make this port configurable. Maybe we can hardcode it to some value.
|
I don't think it should be a standalone server, rather than a part of the ingress controller API. |
Suggestion: Move the debug API into the unified Ingress Server In the current implementation, the debug HTTP server is started directly inside the
A cleaner approach is to:
srv := NewServer(":9090")
srv.Register(&addcDebugAPI{})
srv.Register(&systemAPI{})
mgr.Add(srv)
|
done |
| func (s *Server) Register(pathPrefix string, registrant provider.RegisterHandler) { | ||
| subMux := http.NewServeMux() | ||
| registrant.Register(pathPrefix, subMux) | ||
| s.mux.Handle(pathPrefix+"/", http.StripPrefix(pathPrefix, subMux)) | ||
| s.mux.HandleFunc(pathPrefix, func(w http.ResponseWriter, r *http.Request) { | ||
| http.Redirect(w, r, pathPrefix+"/", http.StatusPermanentRedirect) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The debug server currently has no authentication mechanism, making it accessible to anyone who can reach the endpoint. Should we consider incorporating certain authentication mechanisms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original requirement was to return simple UI friendly HTML. Adding authentication will mean adding another page for login. Maybe we can use cookie based authentication and add one more simple login html page.
- Check if cookie has
INGRESS-DEBUG_TOKEN. If present then serve normally. - If not present then redirect to login page. This will also be a simple HTML page with just input box to enter
debug token. Thisdebug tokenwill be set in the static configuration for ingress and can be copied from there and pasted here. User clicksloginand this value be just stored in the cookie if matches and user will be logged in.
@bzp2010 @ronething @AlinsRan What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's enough not to expose the port, just like the control-api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's enough not to expose the port, just like the control-api.
It might be a bit different. This debug server has exposed SSL/Consumer resources, but the control-api does not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be a bit different. This debug server has exposed SSL/Consumer resources, but the control-api does not.
You're right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only k8s native way to enable/disable this dynamically will be to use some existing CR for it. But that might be an overkill. I think we should opt for one of the two ways:
- Just disable this server by default and have it enabled statically only and not dynamically. It's a debug server, I don't know if dynamically enabling/disabling it is good enough use case. This is the simplest and safest.
- Just have a single login page that expects a statically defined key as recommended above.
If it's okay to enable/disable this statically then I recommend the first way is good enough or else it will be overengineering. Later if use case emerges about dynamically enabling/disabling, we can spend some thought here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's exactly what I was thinking. A simple static config.
However, I must confirm whether the restart required to enable this feature would disrupt the current "state". Specifically, can we guarantee consistent results based on unchanged configurations? We need to ensure that the restart does not corrupt any potentially existing in-memory state, thereby preventing the debugging of previously existing issues.
For example, we sometimes encounter problems that resolve after a restart, making it impossible to analyze the root cause.
Can anyone answer this question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s not possible to guarantee the preservation of “state” across a restart.
The controller’s declarative configuration state (stored in Kubernetes/etcd via CRDs) is automatically rebuilt after a restart, but any runtime in-memory state (caches, queues, goroutine stacks, etc.) will inevitably be lost.
For an ingress controller, a restart is typically the last-resort mechanism to recover state, so if you need to debug live in-memory data, it will naturally no longer be available after restarting.
At present, for security and simplicity reasons, we recommend keeping the debug API disabled by default and enabling it only via static configuration.
If there is a strong need to inspect runtime state in the future, we can revisit supporting dynamic enablement or enabling it up front with proper authentication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. added the boolean in static config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The state itself isn't critical, but it would be preferable if you could confirm it doesn't impact configuration generation.
The key point is For a given input of Kubernetes resources, always produce a fixed output. This includes how to handle conflicting resources (determine the priority), how to sort resource lists, and so on. This process already exists within AIC, such as during full synchronization at startup. We can ignore those changes triggered by reconciliation tasks.
If these could be leveraged for debugging functionality, that would be excellent.
config/samples/config.yaml
Outdated
| metrics_addr: ":8080" # The address the metrics endpoint binds to. | ||
| # The default value is ":8080". | ||
| enable_server: false # The debug API is behind this server which is disabled by default for security reasons. | ||
| server_addr: ":9092" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
127.0.0.1?
Should we emphasize the purpose of debugging on the fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
binded to localhost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose is generic. Although currently it's to debug the in memory state of adc config. Maybe we can add a list in the comment.
/debugcan be used to debug in-memory state of translated adc configs to be synced with data plane.
....later if handlers are added, we can extend the list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds an ADC debug server with REST API endpoints to provide debugging capabilities for APISIX Ingress Controller configurations. The server exposes translated ADC configurations in JSON format for inspection and troubleshooting.
- Creates a new unified API server with debugging endpoints
- Implements HTML-based navigation interface for browsing ADC configurations
- Integrates debug server as an optional component in the controller manager
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/provider/register.go | Adds RegisterHandler interface for HTTP handler registration |
| internal/provider/provider.go | Extends Provider interface to embed RegisterHandler |
| internal/provider/common/adcdebugserver.go | Implements ADCDebugProvider with HTML templates and JSON endpoints |
| internal/provider/apisix/provider.go | Implements Register method to setup debug handlers |
| internal/manager/server/server.go | Creates new HTTP server with handler registration capabilities |
| internal/manager/run.go | Integrates debug server into controller manager |
| internal/controller/config/types.go | Adds server configuration fields |
| internal/controller/config/config.go | Sets default server address in configuration |
| internal/adc/client/client.go | Initializes ADCDebugProvider with shared store and config manager |
| config/samples/config.yaml | Documents server configuration options |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| </html> | ||
| `) | ||
|
|
||
| _ = tmpl.Execute(w, struct { |
Copilot
AI
Sep 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error from template execution is being ignored. This could lead to silent failures when rendering HTML responses. Consider handling the error appropriately, either by logging it or returning an HTTP error response.
| </html> | ||
| `) | ||
|
|
||
| _ = tmpl.Execute(w, struct { |
Copilot
AI
Sep 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Template execution errors are being ignored in multiple locations. This could lead to silent failures when rendering HTML responses. Consider handling these errors appropriately, either by logging them or returning HTTP error responses.
| </html> | ||
| `) | ||
|
|
||
| _ = tmpl.Execute(w, struct { |
Copilot
AI
Sep 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Template execution errors are being ignored in multiple locations. This could lead to silent failures when rendering HTML responses. Consider handling these errors appropriately, either by logging them or returning HTTP error responses.
| </html> | ||
| `) | ||
|
|
||
| _ = tmpl.Execute(w, struct { |
Copilot
AI
Sep 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Template execution errors are being ignored in multiple locations. This could lead to silent failures when rendering HTML responses. Consider handling these errors appropriately, either by logging them or returning HTTP error responses.
| asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(svc.ID)), | ||
| }) | ||
| } | ||
| case "consumers": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using hard coding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apisix-ingress-controller/api/adc/types.go
Lines 80 to 88 in 4e1bd6e
| const ( | |
| TypeRoute = "route" | |
| TypeService = "service" | |
| TypeConsumer = "consumer" | |
| TypeSSL = "ssl" | |
| TypeGlobalRule = "global_rule" | |
| TypePluginMetadata = "plugin_metadata" | |
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaced with adctypes
…ress-controller into revolyssup/api-adc
ronething
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
Goals:
Solution:
Serverthat registers handlers on it and runs the server.Screencast.From.2025-09-12.15-01-20.mp4
What this PR does / why we need it:
Pre-submission checklist: