You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "Revolyssup (via GitHub)" <gi...@apache.org> on 2025/09/09 18:18:41 UTC
[PR] feat: add adc debug server [apisix-ingress-controller]
Revolyssup opened a new pull request, #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550
https://github.com/user-attachments/assets/775e9427-4d30-494e-bf25-26c78b8e68c0
- [ ] Bugfix
- [ ] New feature provided
- [ ] Improve performance
- [ ] Backport patches
- [ ] Documentation
- [ ] Refactor
- [ ] Chore
- [ ] CI/CD or Tests
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### Pre-submission checklist:
<!--
Please follow the requirements:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. Use "request review" to notify the reviewer once you have resolved the review
-->
- [ ] Did you explain what problem does this PR solve? Or what new features have been added?
- [ ] Have you added corresponding test cases?
- [ ] Have you modified the corresponding document?
- [ ] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix-ingress-controller#community) first**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2349382304
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
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 reboot does not corrupt any potentially existing in-memory state, thereby preventing the debugging of previously existing issues.
For instance, we sometimes encounter problems that resolve after a reboot, making it impossible to analyze the root cause.
Can anyone answer this question?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2355028897
##########
config/samples/config.yaml:
##########
@@ -18,6 +18,8 @@ leader_election:
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.
Review Comment:
I couldn't think of a better name because it's a generic server and currently has debugging related endpoints but can be extended. Do you have a better suggestion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2348356639
##########
internal/provider/apisix/provider.go:
##########
@@ -80,15 +82,20 @@ func New(updater status.Updater, readier readiness.ReadinessManager, opts ...pro
}
return &apisixProvider{
- client: cli,
- Options: o,
- translator: &translator.Translator{},
- updater: updater,
- readier: readier,
- syncCh: make(chan struct{}, 1),
+ client: cli,
+ adcDebugProvider: common.NewADCDebugProvider(cli.Store, cli.ConfigManager),
Review Comment:
Makes sense. move it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2349437477
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
The restart will rebuild the in-memory store which is the data store for this API. Given same set of configuration, theoretically it should go rebuild the same state according to me. Because the data from this in-mem store directly corresponds to data that will be sent to data plane, in case there are inconsistencies on restart this has to be a bug.
If there are problems that resolve after restart, then we should note them and debug to find the root cause. The expectation from the system should be such that it produces consistently same state for the same input. And on this expectation of the system we can go with the static config approach. I cannot think of a case where it will be expected from the system to produce different state on reruns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add adc debug server [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#issuecomment-3272853508
I don't think it should be a standalone server, rather than a part of the ingress API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "ronething (via GitHub)" <gi...@apache.org>.
ronething commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2357427186
##########
config/samples/config.yaml:
##########
@@ -18,6 +18,8 @@ leader_election:
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.
Review Comment:
i think it's okay to keep the `server` for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2355032649
##########
internal/provider/common/adcdebugserver.go:
##########
@@ -0,0 +1,341 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package common
+
+import (
+ "encoding/json"
+ "fmt"
+ "html/template"
+ "net/http"
+ "net/url"
+
+ adctypes "github.com/apache/apisix-ingress-controller/api/adc"
+ "github.com/apache/apisix-ingress-controller/internal/adc/cache"
+ "github.com/apache/apisix-ingress-controller/internal/types"
+)
+
+type ResourceInfo struct {
+ ID string
+ Name string
+ Type string
+ Link string
+}
+
+type ADCDebugProvider struct {
+ store *cache.Store
+ configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]
+ pathPrefix string
+}
+
+func newTemplate(name, body string) *template.Template {
+ return template.Must(template.New(name).
+ Funcs(template.FuncMap{"urlencode": url.QueryEscape}).
+ Parse(body))
+}
+
+func (asrv *ADCDebugProvider) SetupHandler(pathPrefix string, mux *http.ServeMux) {
+ asrv.pathPrefix = pathPrefix
+ mux.HandleFunc("/config", asrv.handleConfig)
+ mux.HandleFunc("/", asrv.handleIndex)
Review Comment:
No. The underlying data store has adc configs translated by the ingress controller we're hitting. So obviously it will show those resources only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add adc debug server [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#issuecomment-3283422930
> 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 `provider`.
While this works, it introduces several issues:
- **Tight coupling of responsibilities**: The provider’s job is to synchronize resources, not to start its own HTTP server.
- **Lack of configurability**: Port, paths, authentication, etc. are “black-boxed” inside the provider and cannot be configured or extended externally.
- **Harder to extend**: If other components later need to expose debug endpoints, you will end up with multiple scattered servers.
A cleaner approach is to:
- Start a unified debug server at the Ingress Server layer, e.g.:
```go
srv := NewServer(":9090")
srv.Register(&addcDebugAPI{})
srv.Register(&systemAPI{})
mgr.Add(srv)
```
* Let each component (including the provider) only implement a RegisterHandlers/DebugProvider interface to attach its handlers, without starting its own server.
* This way, port, authentication, middleware, etc. can be configured in one place, and the server lifecycle follows the manager.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2354581156
##########
internal/provider/common/adcdebugserver.go:
##########
@@ -0,0 +1,341 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package common
+
+import (
+ "encoding/json"
+ "fmt"
+ "html/template"
+ "net/http"
+ "net/url"
+
+ adctypes "github.com/apache/apisix-ingress-controller/api/adc"
+ "github.com/apache/apisix-ingress-controller/internal/adc/cache"
+ "github.com/apache/apisix-ingress-controller/internal/types"
+)
+
+type ResourceInfo struct {
+ ID string
+ Name string
+ Type string
+ Link string
+}
+
+type ADCDebugProvider struct {
+ store *cache.Store
+ configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]
+ pathPrefix string
+}
+
+func newTemplate(name, body string) *template.Template {
+ return template.Must(template.New(name).
+ Funcs(template.FuncMap{"urlencode": url.QueryEscape}).
+ Parse(body))
+}
+
+func (asrv *ADCDebugProvider) SetupHandler(pathPrefix string, mux *http.ServeMux) {
+ asrv.pathPrefix = pathPrefix
+ mux.HandleFunc("/config", asrv.handleConfig)
+ mux.HandleFunc("/", asrv.handleIndex)
+}
+
+func NewADCDebugProvider(store *cache.Store, configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]) *ADCDebugProvider {
+ return &ADCDebugProvider{store: store, configManager: configManager}
+}
+
+func (asrv *ADCDebugProvider) handleIndex(w http.ResponseWriter, r *http.Request) {
+ configs := asrv.configManager.List()
+ configNames := make([]string, 0, len(configs))
+ for _, cfg := range configs {
+ configNames = append(configNames, cfg.Name)
+ }
+
+ tmpl := newTemplate("index", `
+ <html>
+ <head><title>ADC Debug Server</title></head>
+ <body>
+ <h1>Configurations</h1>
+ <ul>
+ {{range .ConfigNames}}
+ <li><a href="{{$.Prefix}}/config?name={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
+ ConfigNames []string
+ Prefix string
+ }{ConfigNames: configNames, Prefix: asrv.pathPrefix})
+}
+
+func (asrv *ADCDebugProvider) handleConfig(w http.ResponseWriter, r *http.Request) {
+ configNameEncoded := r.URL.Query().Get("name")
+ if configNameEncoded == "" {
+ http.Error(w, "Config name is required", http.StatusBadRequest)
+ return
+ }
+
+ configName, err := url.QueryUnescape(configNameEncoded)
+ if err != nil {
+ http.Error(w, "Invalid config name encoding", http.StatusBadRequest)
+ return
+ }
+
+ resourceIDEncoded := r.URL.Query().Get("id")
+ resourceID := ""
+ if resourceIDEncoded != "" {
+ resourceID, err = url.QueryUnescape(resourceIDEncoded)
+ if err != nil {
+ http.Error(w, "Invalid resource ID encoding", http.StatusBadRequest)
+ return
+ }
+ }
+
+ resourceType := r.URL.Query().Get("type")
+
+ if resourceType == "" {
+ asrv.showResourceTypes(w, configName, url.QueryEscape(configName))
+ return
+ }
+
+ if resourceID == "" {
+ asrv.showResources(w, r, configName, url.QueryEscape(configName), resourceType)
+ return
+ }
+
+ asrv.showResourceDetail(w, r, configName, resourceType, resourceID)
+}
+
+func (asrv *ADCDebugProvider) showResourceTypes(w http.ResponseWriter, configName, configNameEncoded string) {
+ resourceTypes := []string{"services", "routes", "consumers", "ssls", "globalrules", "pluginmetadata"}
+
+ tmpl := newTemplate("resources", `
+ <html>
+ <head><title>Resources for {{.ConfigName}}</title></head>
+ <body>
+ <h1>Resources for {{.ConfigName}}</h1>
+ <ul>
+ {{range .ResourceTypes}}
+ <li><a href="{{$.Prefix}}/config?name={{$.ConfigNameEncoded}}&type={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
+ ConfigName string
+ ConfigNameEncoded string
+ ResourceTypes []string
+ Prefix string
+ }{
+ ConfigName: configName,
+ ConfigNameEncoded: configNameEncoded,
+ ResourceTypes: resourceTypes,
+ Prefix: asrv.pathPrefix,
+ })
+}
+
+func (asrv *ADCDebugProvider) showResources(w http.ResponseWriter, r *http.Request, configName, configNameEncoded, resourceType string) {
+ resources, err := asrv.store.GetResources(configName)
+ if err != nil {
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ return
+ }
+
+ var resourceInfos []ResourceInfo
+ switch resourceType {
+ case "services":
+ for _, svc := range resources.Services {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: svc.ID,
+ Name: svc.Name,
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(svc.ID)),
+ })
+ }
+ case "consumers":
Review Comment:
replaced with adctypes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2349382304
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
Yes, that's exactly what I was thinking. A simple static config.
However, I must confirm whether the reboot 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 reboot does not corrupt any potentially existing in-memory state, thereby preventing the debugging of previously existing issues.
For instance, we sometimes encounter problems that resolve after a reboot, making it impossible to analyze the root cause.
Can anyone answer this question?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2349382304
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
Yes, that's exactly what I was thinking. A simple static config.
However, I must confirm whether the reboot 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 reboot does not corrupt any potentially existing in-memory state, thereby preventing the debugging of previously existing issues.
For instance, we sometimes encounter problems that resolve after a reboot, making it impossible to analyze the root cause.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2349617724
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
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.
If these could be leveraged for debugging functionality, that would be excellent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2349617724
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2348161075
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
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.
1. Check if cookie has `INGRESS-DEBUG_TOKEN`. If present then server normally.
2. If not present then redirect to login page. This will also be a simple HTML page with just input box to enter `debug token`. This `debug token` will be set in the static configuration for ingress and can be copied from there and pasted here. User clicks `login` and this value be just stored in the cookie if matches and user will be logged in.
@bzp2010 @ronething @AlinsRan What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2354000481
##########
internal/provider/common/adcdebugserver.go:
##########
@@ -0,0 +1,341 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package common
+
+import (
+ "encoding/json"
+ "fmt"
+ "html/template"
+ "net/http"
+ "net/url"
+
+ adctypes "github.com/apache/apisix-ingress-controller/api/adc"
+ "github.com/apache/apisix-ingress-controller/internal/adc/cache"
+ "github.com/apache/apisix-ingress-controller/internal/types"
+)
+
+type ResourceInfo struct {
+ ID string
+ Name string
+ Type string
+ Link string
+}
+
+type ADCDebugProvider struct {
+ store *cache.Store
+ configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]
+ pathPrefix string
+}
+
+func newTemplate(name, body string) *template.Template {
+ return template.Must(template.New(name).
+ Funcs(template.FuncMap{"urlencode": url.QueryEscape}).
+ Parse(body))
+}
+
+func (asrv *ADCDebugProvider) SetupHandler(pathPrefix string, mux *http.ServeMux) {
+ asrv.pathPrefix = pathPrefix
+ mux.HandleFunc("/config", asrv.handleConfig)
+ mux.HandleFunc("/", asrv.handleIndex)
+}
+
+func NewADCDebugProvider(store *cache.Store, configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]) *ADCDebugProvider {
+ return &ADCDebugProvider{store: store, configManager: configManager}
+}
+
+func (asrv *ADCDebugProvider) handleIndex(w http.ResponseWriter, r *http.Request) {
+ configs := asrv.configManager.List()
+ configNames := make([]string, 0, len(configs))
+ for _, cfg := range configs {
+ configNames = append(configNames, cfg.Name)
+ }
+
+ tmpl := newTemplate("index", `
+ <html>
+ <head><title>ADC Debug Server</title></head>
+ <body>
+ <h1>Configurations</h1>
+ <ul>
+ {{range .ConfigNames}}
+ <li><a href="{{$.Prefix}}/config?name={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
Review Comment:
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.
##########
internal/provider/common/adcdebugserver.go:
##########
@@ -0,0 +1,341 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package common
+
+import (
+ "encoding/json"
+ "fmt"
+ "html/template"
+ "net/http"
+ "net/url"
+
+ adctypes "github.com/apache/apisix-ingress-controller/api/adc"
+ "github.com/apache/apisix-ingress-controller/internal/adc/cache"
+ "github.com/apache/apisix-ingress-controller/internal/types"
+)
+
+type ResourceInfo struct {
+ ID string
+ Name string
+ Type string
+ Link string
+}
+
+type ADCDebugProvider struct {
+ store *cache.Store
+ configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]
+ pathPrefix string
+}
+
+func newTemplate(name, body string) *template.Template {
+ return template.Must(template.New(name).
+ Funcs(template.FuncMap{"urlencode": url.QueryEscape}).
+ Parse(body))
+}
+
+func (asrv *ADCDebugProvider) SetupHandler(pathPrefix string, mux *http.ServeMux) {
+ asrv.pathPrefix = pathPrefix
+ mux.HandleFunc("/config", asrv.handleConfig)
+ mux.HandleFunc("/", asrv.handleIndex)
+}
+
+func NewADCDebugProvider(store *cache.Store, configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]) *ADCDebugProvider {
+ return &ADCDebugProvider{store: store, configManager: configManager}
+}
+
+func (asrv *ADCDebugProvider) handleIndex(w http.ResponseWriter, r *http.Request) {
+ configs := asrv.configManager.List()
+ configNames := make([]string, 0, len(configs))
+ for _, cfg := range configs {
+ configNames = append(configNames, cfg.Name)
+ }
+
+ tmpl := newTemplate("index", `
+ <html>
+ <head><title>ADC Debug Server</title></head>
+ <body>
+ <h1>Configurations</h1>
+ <ul>
+ {{range .ConfigNames}}
+ <li><a href="{{$.Prefix}}/config?name={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
+ ConfigNames []string
+ Prefix string
+ }{ConfigNames: configNames, Prefix: asrv.pathPrefix})
+}
+
+func (asrv *ADCDebugProvider) handleConfig(w http.ResponseWriter, r *http.Request) {
+ configNameEncoded := r.URL.Query().Get("name")
+ if configNameEncoded == "" {
+ http.Error(w, "Config name is required", http.StatusBadRequest)
+ return
+ }
+
+ configName, err := url.QueryUnescape(configNameEncoded)
+ if err != nil {
+ http.Error(w, "Invalid config name encoding", http.StatusBadRequest)
+ return
+ }
+
+ resourceIDEncoded := r.URL.Query().Get("id")
+ resourceID := ""
+ if resourceIDEncoded != "" {
+ resourceID, err = url.QueryUnescape(resourceIDEncoded)
+ if err != nil {
+ http.Error(w, "Invalid resource ID encoding", http.StatusBadRequest)
+ return
+ }
+ }
+
+ resourceType := r.URL.Query().Get("type")
+
+ if resourceType == "" {
+ asrv.showResourceTypes(w, configName, url.QueryEscape(configName))
+ return
+ }
+
+ if resourceID == "" {
+ asrv.showResources(w, r, configName, url.QueryEscape(configName), resourceType)
+ return
+ }
+
+ asrv.showResourceDetail(w, r, configName, resourceType, resourceID)
+}
+
+func (asrv *ADCDebugProvider) showResourceTypes(w http.ResponseWriter, configName, configNameEncoded string) {
+ resourceTypes := []string{"services", "routes", "consumers", "ssls", "globalrules", "pluginmetadata"}
+
+ tmpl := newTemplate("resources", `
+ <html>
+ <head><title>Resources for {{.ConfigName}}</title></head>
+ <body>
+ <h1>Resources for {{.ConfigName}}</h1>
+ <ul>
+ {{range .ResourceTypes}}
+ <li><a href="{{$.Prefix}}/config?name={{$.ConfigNameEncoded}}&type={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
Review Comment:
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.
##########
internal/provider/common/adcdebugserver.go:
##########
@@ -0,0 +1,341 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package common
+
+import (
+ "encoding/json"
+ "fmt"
+ "html/template"
+ "net/http"
+ "net/url"
+
+ adctypes "github.com/apache/apisix-ingress-controller/api/adc"
+ "github.com/apache/apisix-ingress-controller/internal/adc/cache"
+ "github.com/apache/apisix-ingress-controller/internal/types"
+)
+
+type ResourceInfo struct {
+ ID string
+ Name string
+ Type string
+ Link string
+}
+
+type ADCDebugProvider struct {
+ store *cache.Store
+ configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]
+ pathPrefix string
+}
+
+func newTemplate(name, body string) *template.Template {
+ return template.Must(template.New(name).
+ Funcs(template.FuncMap{"urlencode": url.QueryEscape}).
+ Parse(body))
+}
+
+func (asrv *ADCDebugProvider) SetupHandler(pathPrefix string, mux *http.ServeMux) {
+ asrv.pathPrefix = pathPrefix
+ mux.HandleFunc("/config", asrv.handleConfig)
+ mux.HandleFunc("/", asrv.handleIndex)
+}
+
+func NewADCDebugProvider(store *cache.Store, configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]) *ADCDebugProvider {
+ return &ADCDebugProvider{store: store, configManager: configManager}
+}
+
+func (asrv *ADCDebugProvider) handleIndex(w http.ResponseWriter, r *http.Request) {
+ configs := asrv.configManager.List()
+ configNames := make([]string, 0, len(configs))
+ for _, cfg := range configs {
+ configNames = append(configNames, cfg.Name)
+ }
+
+ tmpl := newTemplate("index", `
+ <html>
+ <head><title>ADC Debug Server</title></head>
+ <body>
+ <h1>Configurations</h1>
+ <ul>
+ {{range .ConfigNames}}
+ <li><a href="{{$.Prefix}}/config?name={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
+ ConfigNames []string
+ Prefix string
+ }{ConfigNames: configNames, Prefix: asrv.pathPrefix})
+}
+
+func (asrv *ADCDebugProvider) handleConfig(w http.ResponseWriter, r *http.Request) {
+ configNameEncoded := r.URL.Query().Get("name")
+ if configNameEncoded == "" {
+ http.Error(w, "Config name is required", http.StatusBadRequest)
+ return
+ }
+
+ configName, err := url.QueryUnescape(configNameEncoded)
+ if err != nil {
+ http.Error(w, "Invalid config name encoding", http.StatusBadRequest)
+ return
+ }
+
+ resourceIDEncoded := r.URL.Query().Get("id")
+ resourceID := ""
+ if resourceIDEncoded != "" {
+ resourceID, err = url.QueryUnescape(resourceIDEncoded)
+ if err != nil {
+ http.Error(w, "Invalid resource ID encoding", http.StatusBadRequest)
+ return
+ }
+ }
+
+ resourceType := r.URL.Query().Get("type")
+
+ if resourceType == "" {
+ asrv.showResourceTypes(w, configName, url.QueryEscape(configName))
+ return
+ }
+
+ if resourceID == "" {
+ asrv.showResources(w, r, configName, url.QueryEscape(configName), resourceType)
+ return
+ }
+
+ asrv.showResourceDetail(w, r, configName, resourceType, resourceID)
+}
+
+func (asrv *ADCDebugProvider) showResourceTypes(w http.ResponseWriter, configName, configNameEncoded string) {
+ resourceTypes := []string{"services", "routes", "consumers", "ssls", "globalrules", "pluginmetadata"}
+
+ tmpl := newTemplate("resources", `
+ <html>
+ <head><title>Resources for {{.ConfigName}}</title></head>
+ <body>
+ <h1>Resources for {{.ConfigName}}</h1>
+ <ul>
+ {{range .ResourceTypes}}
+ <li><a href="{{$.Prefix}}/config?name={{$.ConfigNameEncoded}}&type={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
+ ConfigName string
+ ConfigNameEncoded string
+ ResourceTypes []string
+ Prefix string
+ }{
+ ConfigName: configName,
+ ConfigNameEncoded: configNameEncoded,
+ ResourceTypes: resourceTypes,
+ Prefix: asrv.pathPrefix,
+ })
+}
+
+func (asrv *ADCDebugProvider) showResources(w http.ResponseWriter, r *http.Request, configName, configNameEncoded, resourceType string) {
+ resources, err := asrv.store.GetResources(configName)
+ if err != nil {
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ return
+ }
+
+ var resourceInfos []ResourceInfo
+ switch resourceType {
+ case "services":
+ for _, svc := range resources.Services {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: svc.ID,
+ Name: svc.Name,
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(svc.ID)),
+ })
+ }
+ case "consumers":
+ for _, consumer := range resources.Consumers {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: consumer.Username,
+ Name: consumer.Username,
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(consumer.Username)),
+ })
+ }
+ case "ssls":
+ for _, ssl := range resources.SSLs {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: ssl.ID,
+ Name: ssl.ID,
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(ssl.ID)),
+ })
+ }
+ case "globalrules":
+ for key := range resources.GlobalRules {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: key,
+ Name: key,
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(key)),
+ })
+ }
+ case "pluginmetadata":
+ if resources.PluginMetadata != nil {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: "pluginmetadata",
+ Name: "Plugin Metadata",
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), "pluginmetadata"),
+ })
+ }
+ case "routes":
+ for _, svc := range resources.Services {
+ for _, route := range svc.Routes {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: route.ID,
+ Name: route.Name,
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(route.ID)),
+ })
+ }
+ }
+ default:
+ http.NotFound(w, r)
+ return
+ }
+
+ tmpl := newTemplate("resourceList", `
+ <html>
+ <head><title>{{.ResourceType}} for {{.ConfigName}}</title></head>
+ <body>
+ <h1>{{.ResourceType}} for {{.ConfigName}}</h1>
+ <ul>
+ {{range .Resources}}
+ <li><a href="{{.Link}}">{{.Name}} ({{.ID}})</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
Review Comment:
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.
##########
internal/provider/common/adcdebugserver.go:
##########
@@ -0,0 +1,341 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package common
+
+import (
+ "encoding/json"
+ "fmt"
+ "html/template"
+ "net/http"
+ "net/url"
+
+ adctypes "github.com/apache/apisix-ingress-controller/api/adc"
+ "github.com/apache/apisix-ingress-controller/internal/adc/cache"
+ "github.com/apache/apisix-ingress-controller/internal/types"
+)
+
+type ResourceInfo struct {
+ ID string
+ Name string
+ Type string
+ Link string
+}
+
+type ADCDebugProvider struct {
+ store *cache.Store
+ configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]
+ pathPrefix string
+}
+
+func newTemplate(name, body string) *template.Template {
+ return template.Must(template.New(name).
+ Funcs(template.FuncMap{"urlencode": url.QueryEscape}).
+ Parse(body))
+}
+
+func (asrv *ADCDebugProvider) SetupHandler(pathPrefix string, mux *http.ServeMux) {
+ asrv.pathPrefix = pathPrefix
+ mux.HandleFunc("/config", asrv.handleConfig)
+ mux.HandleFunc("/", asrv.handleIndex)
+}
+
+func NewADCDebugProvider(store *cache.Store, configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]) *ADCDebugProvider {
+ return &ADCDebugProvider{store: store, configManager: configManager}
+}
+
+func (asrv *ADCDebugProvider) handleIndex(w http.ResponseWriter, r *http.Request) {
+ configs := asrv.configManager.List()
+ configNames := make([]string, 0, len(configs))
+ for _, cfg := range configs {
+ configNames = append(configNames, cfg.Name)
+ }
+
+ tmpl := newTemplate("index", `
+ <html>
+ <head><title>ADC Debug Server</title></head>
+ <body>
+ <h1>Configurations</h1>
+ <ul>
+ {{range .ConfigNames}}
+ <li><a href="{{$.Prefix}}/config?name={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
+ ConfigNames []string
+ Prefix string
+ }{ConfigNames: configNames, Prefix: asrv.pathPrefix})
+}
+
+func (asrv *ADCDebugProvider) handleConfig(w http.ResponseWriter, r *http.Request) {
+ configNameEncoded := r.URL.Query().Get("name")
+ if configNameEncoded == "" {
+ http.Error(w, "Config name is required", http.StatusBadRequest)
+ return
+ }
+
+ configName, err := url.QueryUnescape(configNameEncoded)
+ if err != nil {
+ http.Error(w, "Invalid config name encoding", http.StatusBadRequest)
+ return
+ }
+
+ resourceIDEncoded := r.URL.Query().Get("id")
+ resourceID := ""
+ if resourceIDEncoded != "" {
+ resourceID, err = url.QueryUnescape(resourceIDEncoded)
+ if err != nil {
+ http.Error(w, "Invalid resource ID encoding", http.StatusBadRequest)
+ return
+ }
+ }
+
+ resourceType := r.URL.Query().Get("type")
+
+ if resourceType == "" {
+ asrv.showResourceTypes(w, configName, url.QueryEscape(configName))
+ return
+ }
+
+ if resourceID == "" {
+ asrv.showResources(w, r, configName, url.QueryEscape(configName), resourceType)
+ return
+ }
+
+ asrv.showResourceDetail(w, r, configName, resourceType, resourceID)
+}
+
+func (asrv *ADCDebugProvider) showResourceTypes(w http.ResponseWriter, configName, configNameEncoded string) {
+ resourceTypes := []string{"services", "routes", "consumers", "ssls", "globalrules", "pluginmetadata"}
+
+ tmpl := newTemplate("resources", `
+ <html>
+ <head><title>Resources for {{.ConfigName}}</title></head>
+ <body>
+ <h1>Resources for {{.ConfigName}}</h1>
+ <ul>
+ {{range .ResourceTypes}}
+ <li><a href="{{$.Prefix}}/config?name={{$.ConfigNameEncoded}}&type={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
+ ConfigName string
+ ConfigNameEncoded string
+ ResourceTypes []string
+ Prefix string
+ }{
+ ConfigName: configName,
+ ConfigNameEncoded: configNameEncoded,
+ ResourceTypes: resourceTypes,
+ Prefix: asrv.pathPrefix,
+ })
+}
+
+func (asrv *ADCDebugProvider) showResources(w http.ResponseWriter, r *http.Request, configName, configNameEncoded, resourceType string) {
+ resources, err := asrv.store.GetResources(configName)
+ if err != nil {
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ return
+ }
+
+ var resourceInfos []ResourceInfo
+ switch resourceType {
+ case "services":
+ for _, svc := range resources.Services {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: svc.ID,
+ Name: svc.Name,
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(svc.ID)),
+ })
+ }
+ case "consumers":
+ for _, consumer := range resources.Consumers {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: consumer.Username,
+ Name: consumer.Username,
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(consumer.Username)),
+ })
+ }
+ case "ssls":
+ for _, ssl := range resources.SSLs {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: ssl.ID,
+ Name: ssl.ID,
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(ssl.ID)),
+ })
+ }
+ case "globalrules":
+ for key := range resources.GlobalRules {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: key,
+ Name: key,
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(key)),
+ })
+ }
+ case "pluginmetadata":
+ if resources.PluginMetadata != nil {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: "pluginmetadata",
+ Name: "Plugin Metadata",
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), "pluginmetadata"),
+ })
+ }
+ case "routes":
+ for _, svc := range resources.Services {
+ for _, route := range svc.Routes {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: route.ID,
+ Name: route.Name,
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(route.ID)),
+ })
+ }
+ }
+ default:
+ http.NotFound(w, r)
+ return
+ }
+
+ tmpl := newTemplate("resourceList", `
+ <html>
+ <head><title>{{.ResourceType}} for {{.ConfigName}}</title></head>
+ <body>
+ <h1>{{.ResourceType}} for {{.ConfigName}}</h1>
+ <ul>
+ {{range .Resources}}
+ <li><a href="{{.Link}}">{{.Name}} ({{.ID}})</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
+ ConfigName string
+ ResourceType string
+ Resources []ResourceInfo
+ Prefix string
+ }{
+ ConfigName: configName,
+ ResourceType: resourceType,
+ Resources: resourceInfos,
+ Prefix: asrv.pathPrefix,
+ })
+}
+
+func (asrv *ADCDebugProvider) showResourceDetail(w http.ResponseWriter, r *http.Request, configName, resourceType, resourceID string) {
+ resources, err := asrv.store.GetResources(configName)
+ if err != nil {
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ return
+ }
+
+ var resource interface{}
+ switch resourceType {
+ case "services":
+ for _, svc := range resources.Services {
+ if svc.ID == resourceID {
+ resource = svc
+ break
+ }
+ }
+ case "consumers":
+ for _, consumer := range resources.Consumers {
+ if consumer.Username == resourceID {
+ resource = consumer
+ break
+ }
+ }
+ case "ssls":
+ for _, ssl := range resources.SSLs {
+ if ssl.ID == resourceID {
+ resource = ssl
+ break
+ }
+ }
+ case "globalrules":
+ resource = resources.GlobalRules
+ case "pluginmetadata":
+ resource = resources.PluginMetadata
+ case "routes":
+ for _, svc := range resources.Services {
+ for _, route := range svc.Routes {
+ if route.ID == resourceID {
+ resource = route
+ break
+ }
+ }
+ }
+ default:
+ http.NotFound(w, r)
+ return
+ }
+
+ if resource == nil {
+ http.NotFound(w, r)
+ return
+ }
+
+ jsonData, err := json.MarshalIndent(resource, "", " ")
+ if err != nil {
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ return
+ }
+
+ tmpl := newTemplate("resourceDetail", `
+ <html>
+ <head><title>Resource Detail</title></head>
+ <body>
+ <h1>Resource Details: {{.ResourceType}}/{{.ResourceID}}</h1>
+ <pre>{{.Resource}}</pre>
+ <a href="{{.Prefix}}/config?name={{.ConfigName | urlencode}}&type={{.ResourceType}}">Back</a>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2348161075
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
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.
1. Check if cookie has `INGRESS-DEBUG_TOKEN`. If present then serve normally.
2. If not present then redirect to login page. This will also be a simple HTML page with just input box to enter `debug token`. This `debug token` will be set in the static configuration for ingress and can be copied from there and pasted here. User clicks `login` and this value be just stored in the cookie if matches and user will be logged in.
@bzp2010 @ronething @AlinsRan What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2349437477
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
The restart will rebuild the in-memory store which is the data store for this API. Given same set of configuration, theoretically it should rebuild the same state according to me. Because the data from this in-mem store directly corresponds to data that will be sent to data plane, in case there are inconsistencies on restart this has to be a bug.
If there are problems that resolve after restart, then we should note them and debug to find the root cause. The expectation from the system should be such that it produces consistently same state for the same input. And on this expectation of the system we can go with the static config approach. I cannot think of a case where it will be expected from the system to produce different state on reruns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2348917954
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
From a security standpoint, listening on 127.0.0.1 is the minimum requirement. We could disable this feature by default and enable it selectively via configuration files. If we do this, would we need some mechanism to safely enable this feature dynamically without requiring a restart?
Alternatively, if we can guarantee that the ingress controller's output remains fixed for a given input regardless of how it restarts, we could configure it to enable this feature only after a restart.
@AlinsRan @ronething @Revolyssup
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2348329500
##########
internal/provider/apisix/provider.go:
##########
@@ -80,15 +82,20 @@ func New(updater status.Updater, readier readiness.ReadinessManager, opts ...pro
}
return &apisixProvider{
- client: cli,
- Options: o,
- translator: &translator.Translator{},
- updater: updater,
- readier: readier,
- syncCh: make(chan struct{}, 1),
+ client: cli,
+ adcDebugProvider: common.NewADCDebugProvider(cli.Store, cli.ConfigManager),
Review Comment:
Why does it belong to the provider rather than the ADC client?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2349382304
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
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 reboot, making it impossible to analyze the root cause.
Can anyone answer this question?
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup merged PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2354509412
##########
internal/provider/common/adcdebugserver.go:
##########
@@ -0,0 +1,341 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package common
+
+import (
+ "encoding/json"
+ "fmt"
+ "html/template"
+ "net/http"
+ "net/url"
+
+ adctypes "github.com/apache/apisix-ingress-controller/api/adc"
+ "github.com/apache/apisix-ingress-controller/internal/adc/cache"
+ "github.com/apache/apisix-ingress-controller/internal/types"
+)
+
+type ResourceInfo struct {
+ ID string
+ Name string
+ Type string
+ Link string
+}
+
+type ADCDebugProvider struct {
+ store *cache.Store
+ configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]
+ pathPrefix string
+}
+
+func newTemplate(name, body string) *template.Template {
+ return template.Must(template.New(name).
+ Funcs(template.FuncMap{"urlencode": url.QueryEscape}).
+ Parse(body))
+}
+
+func (asrv *ADCDebugProvider) SetupHandler(pathPrefix string, mux *http.ServeMux) {
+ asrv.pathPrefix = pathPrefix
+ mux.HandleFunc("/config", asrv.handleConfig)
+ mux.HandleFunc("/", asrv.handleIndex)
+}
+
+func NewADCDebugProvider(store *cache.Store, configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]) *ADCDebugProvider {
+ return &ADCDebugProvider{store: store, configManager: configManager}
+}
+
+func (asrv *ADCDebugProvider) handleIndex(w http.ResponseWriter, r *http.Request) {
+ configs := asrv.configManager.List()
+ configNames := make([]string, 0, len(configs))
+ for _, cfg := range configs {
+ configNames = append(configNames, cfg.Name)
+ }
+
+ tmpl := newTemplate("index", `
+ <html>
+ <head><title>ADC Debug Server</title></head>
+ <body>
+ <h1>Configurations</h1>
+ <ul>
+ {{range .ConfigNames}}
+ <li><a href="{{$.Prefix}}/config?name={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
+ ConfigNames []string
+ Prefix string
+ }{ConfigNames: configNames, Prefix: asrv.pathPrefix})
+}
+
+func (asrv *ADCDebugProvider) handleConfig(w http.ResponseWriter, r *http.Request) {
+ configNameEncoded := r.URL.Query().Get("name")
+ if configNameEncoded == "" {
+ http.Error(w, "Config name is required", http.StatusBadRequest)
+ return
+ }
+
+ configName, err := url.QueryUnescape(configNameEncoded)
+ if err != nil {
+ http.Error(w, "Invalid config name encoding", http.StatusBadRequest)
+ return
+ }
+
+ resourceIDEncoded := r.URL.Query().Get("id")
+ resourceID := ""
+ if resourceIDEncoded != "" {
+ resourceID, err = url.QueryUnescape(resourceIDEncoded)
+ if err != nil {
+ http.Error(w, "Invalid resource ID encoding", http.StatusBadRequest)
+ return
+ }
+ }
+
+ resourceType := r.URL.Query().Get("type")
+
+ if resourceType == "" {
+ asrv.showResourceTypes(w, configName, url.QueryEscape(configName))
+ return
+ }
+
+ if resourceID == "" {
+ asrv.showResources(w, r, configName, url.QueryEscape(configName), resourceType)
+ return
+ }
+
+ asrv.showResourceDetail(w, r, configName, resourceType, resourceID)
+}
+
+func (asrv *ADCDebugProvider) showResourceTypes(w http.ResponseWriter, configName, configNameEncoded string) {
+ resourceTypes := []string{"services", "routes", "consumers", "ssls", "globalrules", "pluginmetadata"}
+
+ tmpl := newTemplate("resources", `
+ <html>
+ <head><title>Resources for {{.ConfigName}}</title></head>
+ <body>
+ <h1>Resources for {{.ConfigName}}</h1>
+ <ul>
+ {{range .ResourceTypes}}
+ <li><a href="{{$.Prefix}}/config?name={{$.ConfigNameEncoded}}&type={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
+ ConfigName string
+ ConfigNameEncoded string
+ ResourceTypes []string
+ Prefix string
+ }{
+ ConfigName: configName,
+ ConfigNameEncoded: configNameEncoded,
+ ResourceTypes: resourceTypes,
+ Prefix: asrv.pathPrefix,
+ })
+}
+
+func (asrv *ADCDebugProvider) showResources(w http.ResponseWriter, r *http.Request, configName, configNameEncoded, resourceType string) {
+ resources, err := asrv.store.GetResources(configName)
+ if err != nil {
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ return
+ }
+
+ var resourceInfos []ResourceInfo
+ switch resourceType {
+ case "services":
+ for _, svc := range resources.Services {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: svc.ID,
+ Name: svc.Name,
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(svc.ID)),
+ })
+ }
+ case "consumers":
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2354919264
##########
config/samples/config.yaml:
##########
@@ -18,6 +18,8 @@ leader_election:
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.
Review Comment:
The `server` may be ambiguous. For example, we also have a metrics server (and `metrics_addr`). Does this need any tuning?
##########
internal/provider/common/adcdebugserver.go:
##########
@@ -0,0 +1,341 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package common
+
+import (
+ "encoding/json"
+ "fmt"
+ "html/template"
+ "net/http"
+ "net/url"
+
+ adctypes "github.com/apache/apisix-ingress-controller/api/adc"
+ "github.com/apache/apisix-ingress-controller/internal/adc/cache"
+ "github.com/apache/apisix-ingress-controller/internal/types"
+)
+
+type ResourceInfo struct {
+ ID string
+ Name string
+ Type string
+ Link string
+}
+
+type ADCDebugProvider struct {
+ store *cache.Store
+ configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]
+ pathPrefix string
+}
+
+func newTemplate(name, body string) *template.Template {
+ return template.Must(template.New(name).
+ Funcs(template.FuncMap{"urlencode": url.QueryEscape}).
+ Parse(body))
+}
+
+func (asrv *ADCDebugProvider) SetupHandler(pathPrefix string, mux *http.ServeMux) {
+ asrv.pathPrefix = pathPrefix
+ mux.HandleFunc("/config", asrv.handleConfig)
+ mux.HandleFunc("/", asrv.handleIndex)
Review Comment:
Can those APIs ensure that the resources they output are managed by the current controller? For example, if an HTTPRoute is managed by another ingress controller, can it be listed by the current API?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2348325935
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
I think it's enough not to expose the port, just like the control-api.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2349353659
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
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:
1. 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.
2. 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "ronething (via GitHub)" <gi...@apache.org>.
ronething commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2348136746
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2354536920
##########
internal/provider/common/adcdebugserver.go:
##########
@@ -0,0 +1,341 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package common
+
+import (
+ "encoding/json"
+ "fmt"
+ "html/template"
+ "net/http"
+ "net/url"
+
+ adctypes "github.com/apache/apisix-ingress-controller/api/adc"
+ "github.com/apache/apisix-ingress-controller/internal/adc/cache"
+ "github.com/apache/apisix-ingress-controller/internal/types"
+)
+
+type ResourceInfo struct {
+ ID string
+ Name string
+ Type string
+ Link string
+}
+
+type ADCDebugProvider struct {
+ store *cache.Store
+ configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]
+ pathPrefix string
+}
+
+func newTemplate(name, body string) *template.Template {
+ return template.Must(template.New(name).
+ Funcs(template.FuncMap{"urlencode": url.QueryEscape}).
+ Parse(body))
+}
+
+func (asrv *ADCDebugProvider) SetupHandler(pathPrefix string, mux *http.ServeMux) {
+ asrv.pathPrefix = pathPrefix
+ mux.HandleFunc("/config", asrv.handleConfig)
+ mux.HandleFunc("/", asrv.handleIndex)
+}
+
+func NewADCDebugProvider(store *cache.Store, configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]) *ADCDebugProvider {
+ return &ADCDebugProvider{store: store, configManager: configManager}
+}
+
+func (asrv *ADCDebugProvider) handleIndex(w http.ResponseWriter, r *http.Request) {
+ configs := asrv.configManager.List()
+ configNames := make([]string, 0, len(configs))
+ for _, cfg := range configs {
+ configNames = append(configNames, cfg.Name)
+ }
+
+ tmpl := newTemplate("index", `
+ <html>
+ <head><title>ADC Debug Server</title></head>
+ <body>
+ <h1>Configurations</h1>
+ <ul>
+ {{range .ConfigNames}}
+ <li><a href="{{$.Prefix}}/config?name={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
+ ConfigNames []string
+ Prefix string
+ }{ConfigNames: configNames, Prefix: asrv.pathPrefix})
+}
+
+func (asrv *ADCDebugProvider) handleConfig(w http.ResponseWriter, r *http.Request) {
+ configNameEncoded := r.URL.Query().Get("name")
+ if configNameEncoded == "" {
+ http.Error(w, "Config name is required", http.StatusBadRequest)
+ return
+ }
+
+ configName, err := url.QueryUnescape(configNameEncoded)
+ if err != nil {
+ http.Error(w, "Invalid config name encoding", http.StatusBadRequest)
+ return
+ }
+
+ resourceIDEncoded := r.URL.Query().Get("id")
+ resourceID := ""
+ if resourceIDEncoded != "" {
+ resourceID, err = url.QueryUnescape(resourceIDEncoded)
+ if err != nil {
+ http.Error(w, "Invalid resource ID encoding", http.StatusBadRequest)
+ return
+ }
+ }
+
+ resourceType := r.URL.Query().Get("type")
+
+ if resourceType == "" {
+ asrv.showResourceTypes(w, configName, url.QueryEscape(configName))
+ return
+ }
+
+ if resourceID == "" {
+ asrv.showResources(w, r, configName, url.QueryEscape(configName), resourceType)
+ return
+ }
+
+ asrv.showResourceDetail(w, r, configName, resourceType, resourceID)
+}
+
+func (asrv *ADCDebugProvider) showResourceTypes(w http.ResponseWriter, configName, configNameEncoded string) {
+ resourceTypes := []string{"services", "routes", "consumers", "ssls", "globalrules", "pluginmetadata"}
+
+ tmpl := newTemplate("resources", `
+ <html>
+ <head><title>Resources for {{.ConfigName}}</title></head>
+ <body>
+ <h1>Resources for {{.ConfigName}}</h1>
+ <ul>
+ {{range .ResourceTypes}}
+ <li><a href="{{$.Prefix}}/config?name={{$.ConfigNameEncoded}}&type={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
+ ConfigName string
+ ConfigNameEncoded string
+ ResourceTypes []string
+ Prefix string
+ }{
+ ConfigName: configName,
+ ConfigNameEncoded: configNameEncoded,
+ ResourceTypes: resourceTypes,
+ Prefix: asrv.pathPrefix,
+ })
+}
+
+func (asrv *ADCDebugProvider) showResources(w http.ResponseWriter, r *http.Request, configName, configNameEncoded, resourceType string) {
+ resources, err := asrv.store.GetResources(configName)
+ if err != nil {
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ return
+ }
+
+ var resourceInfos []ResourceInfo
+ switch resourceType {
+ case "services":
+ for _, svc := range resources.Services {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: svc.ID,
+ Name: svc.Name,
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(svc.ID)),
+ })
+ }
+ case "consumers":
Review Comment:
https://github.com/apache/apisix-ingress-controller/blob/4e1bd6eb43591ae28c541bd43567955a02f6f829/api/adc/types.go#L80-L88
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "ronething (via GitHub)" <gi...@apache.org>.
ronething commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2348463030
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2348466609
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
> It might be a bit different. This debug server has exposed SSL/Consumer resources, but the control-api does not.
You're right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2349545097
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
done. added the boolean in static config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2349617724
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2349446492
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2349437477
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
The restart will rebuild the in-memory store which is the data store for this API. Given same set of configuration, theoretically it should rebuild the same state according to me. Because the data from this in-mem store directly corresponds to data that will be sent to data plane, in case there are inconsistencies on restart this has to be a bug.
If there are problems that resolve after restart, then we should note them and debug to find the root cause. The expectation from the system should be such that it produces consistently same state for the same input. And on this expectation of the system we can go with the static config approach. I cannot think of a case where it will be expected from the system to produce different state on reruns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2357415343
##########
config/samples/config.yaml:
##########
@@ -18,6 +18,8 @@ leader_election:
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.
Review Comment:
@AlinsRan @ronething Do you have any ideas? If not, let's use the `server` first and merge it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2354000817
##########
internal/provider/common/adcdebugserver.go:
##########
@@ -0,0 +1,341 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package common
+
+import (
+ "encoding/json"
+ "fmt"
+ "html/template"
+ "net/http"
+ "net/url"
+
+ adctypes "github.com/apache/apisix-ingress-controller/api/adc"
+ "github.com/apache/apisix-ingress-controller/internal/adc/cache"
+ "github.com/apache/apisix-ingress-controller/internal/types"
+)
+
+type ResourceInfo struct {
+ ID string
+ Name string
+ Type string
+ Link string
+}
+
+type ADCDebugProvider struct {
+ store *cache.Store
+ configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]
+ pathPrefix string
+}
+
+func newTemplate(name, body string) *template.Template {
+ return template.Must(template.New(name).
+ Funcs(template.FuncMap{"urlencode": url.QueryEscape}).
+ Parse(body))
+}
+
+func (asrv *ADCDebugProvider) SetupHandler(pathPrefix string, mux *http.ServeMux) {
+ asrv.pathPrefix = pathPrefix
+ mux.HandleFunc("/config", asrv.handleConfig)
+ mux.HandleFunc("/", asrv.handleIndex)
+}
+
+func NewADCDebugProvider(store *cache.Store, configManager *ConfigManager[types.NamespacedNameKind, adctypes.Config]) *ADCDebugProvider {
+ return &ADCDebugProvider{store: store, configManager: configManager}
+}
+
+func (asrv *ADCDebugProvider) handleIndex(w http.ResponseWriter, r *http.Request) {
+ configs := asrv.configManager.List()
+ configNames := make([]string, 0, len(configs))
+ for _, cfg := range configs {
+ configNames = append(configNames, cfg.Name)
+ }
+
+ tmpl := newTemplate("index", `
+ <html>
+ <head><title>ADC Debug Server</title></head>
+ <body>
+ <h1>Configurations</h1>
+ <ul>
+ {{range .ConfigNames}}
+ <li><a href="{{$.Prefix}}/config?name={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
+ ConfigNames []string
+ Prefix string
+ }{ConfigNames: configNames, Prefix: asrv.pathPrefix})
+}
+
+func (asrv *ADCDebugProvider) handleConfig(w http.ResponseWriter, r *http.Request) {
+ configNameEncoded := r.URL.Query().Get("name")
+ if configNameEncoded == "" {
+ http.Error(w, "Config name is required", http.StatusBadRequest)
+ return
+ }
+
+ configName, err := url.QueryUnescape(configNameEncoded)
+ if err != nil {
+ http.Error(w, "Invalid config name encoding", http.StatusBadRequest)
+ return
+ }
+
+ resourceIDEncoded := r.URL.Query().Get("id")
+ resourceID := ""
+ if resourceIDEncoded != "" {
+ resourceID, err = url.QueryUnescape(resourceIDEncoded)
+ if err != nil {
+ http.Error(w, "Invalid resource ID encoding", http.StatusBadRequest)
+ return
+ }
+ }
+
+ resourceType := r.URL.Query().Get("type")
+
+ if resourceType == "" {
+ asrv.showResourceTypes(w, configName, url.QueryEscape(configName))
+ return
+ }
+
+ if resourceID == "" {
+ asrv.showResources(w, r, configName, url.QueryEscape(configName), resourceType)
+ return
+ }
+
+ asrv.showResourceDetail(w, r, configName, resourceType, resourceID)
+}
+
+func (asrv *ADCDebugProvider) showResourceTypes(w http.ResponseWriter, configName, configNameEncoded string) {
+ resourceTypes := []string{"services", "routes", "consumers", "ssls", "globalrules", "pluginmetadata"}
+
+ tmpl := newTemplate("resources", `
+ <html>
+ <head><title>Resources for {{.ConfigName}}</title></head>
+ <body>
+ <h1>Resources for {{.ConfigName}}</h1>
+ <ul>
+ {{range .ResourceTypes}}
+ <li><a href="{{$.Prefix}}/config?name={{$.ConfigNameEncoded}}&type={{. | urlencode}}">{{.}}</a></li>
+ {{end}}
+ </ul>
+ </body>
+ </html>
+ `)
+
+ _ = tmpl.Execute(w, struct {
+ ConfigName string
+ ConfigNameEncoded string
+ ResourceTypes []string
+ Prefix string
+ }{
+ ConfigName: configName,
+ ConfigNameEncoded: configNameEncoded,
+ ResourceTypes: resourceTypes,
+ Prefix: asrv.pathPrefix,
+ })
+}
+
+func (asrv *ADCDebugProvider) showResources(w http.ResponseWriter, r *http.Request, configName, configNameEncoded, resourceType string) {
+ resources, err := asrv.store.GetResources(configName)
+ if err != nil {
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ return
+ }
+
+ var resourceInfos []ResourceInfo
+ switch resourceType {
+ case "services":
+ for _, svc := range resources.Services {
+ resourceInfos = append(resourceInfos, ResourceInfo{
+ ID: svc.ID,
+ Name: svc.Name,
+ Type: resourceType,
+ Link: fmt.Sprintf("%s/config?name=%s&type=%s&id=%s",
+ asrv.pathPrefix, configNameEncoded, url.QueryEscape(resourceType), url.QueryEscape(svc.ID)),
+ })
+ }
+ case "consumers":
Review Comment:
Avoid using hard coding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add adc debug server [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#issuecomment-3284537023
> > 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 `provider`. While this works, it introduces several issues:
>
> * **Tight coupling of responsibilities**: The provider’s job is to synchronize resources, not to start its own HTTP server.
> * **Lack of configurability**: Port, paths, authentication, etc. are “black-boxed” inside the provider and cannot be configured or extended externally.
> * **Harder to extend**: If other components later need to expose debug endpoints, you will end up with multiple scattered servers.
>
> A cleaner approach is to:
>
> * Start a unified api server at the Ingress Server layer, e.g.:
>
> ```go
> srv := NewServer(":9090")
> srv.Register(&addcDebugAPI{})
> srv.Register(&systemAPI{})
> mgr.Add(srv)
> ```
>
> * Let each component (including the provider) only implement a RegisterHandlers/DebugProvider interface to attach its handlers, without starting its own server.
> * This way, port, authentication, middleware, etc. can be configured in one place, and the server lifecycle follows the manager.
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add unified API server with debugging capabilities [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2348161075
##########
internal/manager/server/server.go:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package server
+
+import (
+ "context"
+ "net/http"
+ "time"
+
+ "github.com/apache/apisix-ingress-controller/internal/provider"
+)
+
+type Server struct {
+ server *http.Server
+ mux *http.ServeMux
+}
+
+func (s *Server) Start(ctx context.Context) error {
+ stop := make(chan error, 1)
+ go func() {
+ if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ stop <- err
+ }
+ close(stop)
+ }()
+ select {
+ case <-ctx.Done():
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ return s.server.Shutdown(shutdownCtx)
+ case err := <-stop:
+ return err
+ }
+}
+
+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)
+ })
+}
Review Comment:
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.
1. Check if cookie has `INGRESS-DEBUG_TOKEN`. If present then server normally.
2. If not present then redirect to login page. This will also be a simple HTML page with just input box to enter `debug token`. This `debug token` will be set in the static configuration for ingress and can be copied from there and pasted here. User clicks `login` and this value be just stored in the cookie if matches and user will be logged in.
@bzp2010 @ronething @AlinsRan What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add adc debug server [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2550:
URL: https://github.com/apache/apisix-ingress-controller/pull/2550#discussion_r2334439623
##########
internal/provider/apisix/provider.go:
##########
@@ -80,12 +81,14 @@ func New(updater status.Updater, readier readiness.ReadinessManager, opts ...pro
}
return &apisixProvider{
- client: cli,
- Options: o,
- translator: &translator.Translator{},
- updater: updater,
- readier: readier,
- syncCh: make(chan struct{}, 1),
+ client: cli,
+ // TODO: Maybe pass port/address from external configuration
+ adcdebugserver: common.NewADCDebugServer(cli.Store, cli.ConfigManager, 8432),
Review Comment:
I don't think it should be needed to make this port configurable. Maybe we can hardcode it to some value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org