kaito icon indicating copy to clipboard operation
kaito copied to clipboard

chore: enable https for metrics port

Open MartinForReal opened this issue 8 months ago • 4 comments

Reason for Change:

chore: enable https for metrics port Requirements

  • [ ] added unit tests and e2e tests (if applicable).

Issue Fixed:

Notes for Reviewers:

MartinForReal avatar Apr 27 '25 10:04 MartinForReal

Title

(Describe updated until commit https://github.com/kaito-project/kaito/commit/a77c70032e980ea063c1968b7fe465f7af4de4f5)

Enable HTTPS for Metrics Port with Secure Serving Options


Description

  • Enabled HTTPS for metrics port with secure serving options.

  • Added flags for enabling HTTP/2 and specifying certificate paths.

  • Updated kustomization.yaml to include metrics service and patches.

  • Added new RBAC configurations for metrics authentication and authorization.


Changes walkthrough 📝

Relevant files
Enhancement
2 files
main.go
Added HTTPS support and secure metrics options                     
+82/-5   
main.go
Added HTTPS support and secure metrics options                     
+82/-5   
Dependencies
2 files
go.sum
Updated dependencies checksums                                                     
+20/-0   
go.mod
Added new dependencies                                                                     
+15/-0   
Configuration changes
8 files
kustomization.yaml
Added metrics service and patches                                               
+198/-108
ragengine.yaml
Added new deployment configuration                                             
+110/-0 
Makefile
Added kubebuilder and kustomize installation targets         
+18/-0   
cert_metrics_manager_patch.yaml
Added patch for metrics server certificates                           
+30/-0   
workspace.yaml
Updated deployment configuration                                                 
+9/-1     
allow-metrics-traffic.yaml
Added network policy for metrics traffic                                 
+27/-0   
kustomization.yaml
Updated RBAC configurations for metrics                                   
+9/-7     
kustomization.yaml
Added network policy resource                                                       
+2/-0     
Additional files
13 files
deployment.yaml +1/-1     
service.yaml +1/-1     
values.yaml +2/-0     
manager_auth_proxy_patch.yaml +0/-39   
manager_metrics_patch.yaml +4/-0     
metrics_service.yaml +3/-6     
kustomization.yaml +2/-2     
auth_proxy_client_clusterrole.yaml +0/-16   
auth_proxy_role.yaml +0/-24   
auth_proxy_role_binding.yaml +0/-19   
metrics_auth_role.yaml +17/-0   
metrics_auth_role_binding.yaml +12/-0   
metrics_reader_role.yaml +9/-0     

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • kaito-pr-agent[bot] avatar Apr 27 '25 10:04 kaito-pr-agent[bot]

    PR Reviewer Guide 🔍

    (Review updated until commit https://github.com/kaito-project/kaito/commit/a77c70032e980ea063c1968b7fe465f7af4de4f5)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Configuration Complexity

    The addition of multiple flags and configuration options for metrics and HTTP/2 settings increases the complexity of the configuration. Ensure that the documentation is updated to reflect these changes and that the default values are appropriate for most use cases.

    var metricsCertPath, metricsCertName, metricsCertKey string
    var enableLeaderElection bool
    var enableWebhook bool
    var probeAddr string
    var featureGates string
    var secureMetrics bool
    var enableHTTP2 bool
    var tlsOpts []func(*tls.Config)
    flag.StringVar(&metricsAddr, "metrics-bind-address", "8080", "The address the metrics endpoint binds to. "+
    	"Use :8443 for HTTPS or :8080 for HTTP")
    flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
    flag.BoolVar(&enableLeaderElection, "leader-elect", false,
    	"Enable leader election for controller manager. "+
    		"Enabling this will ensure there is only one active controller manager.")
    flag.BoolVar(&secureMetrics, "metrics-secure", false,
    	"If set, the metrics endpoint is served securely via HTTPS. Use --metrics-secure=false to use HTTP instead.")
    flag.StringVar(&metricsCertPath, "metrics-cert-path", "",
    	"The directory that contains the metrics server certificate.")
    flag.StringVar(&metricsCertName, "metrics-cert-name", "tls.crt", "The name of the metrics server certificate file.")
    flag.StringVar(&metricsCertKey, "metrics-cert-key", "tls.key", "The name of the metrics server key file.")
    flag.BoolVar(&enableWebhook, "webhook", true,
    	"Enable webhook for controller manager. Default is true.")
    flag.StringVar(&featureGates, "feature-gates", "vLLM=true", "Enable Kaito feature gates. Default,	vLLM=true.")
    flag.BoolVar(&enableHTTP2, "enable-http2", false,
    	"If set, HTTP/2 will be enabled for the metrics and webhook servers")
    opts := zap.Options{
    
    CertManager Integration

    The commented-out sections for cert-manager integration suggest that this is a work in progress. Ensure that the cert-manager integration is thoroughly tested and documented before enabling it by default.

    #- ../certmanager
    # [PROMETHEUS] To enable prometheus monitor, uncomment all sections with 'PROMETHEUS'.
    #- ../prometheus
    # [METRICS] Expose the controller manager metrics service.
    - metrics_service.yaml
    # [NETWORK POLICY] Protect the /metrics endpoint and Webhook Server with NetworkPolicy.
    # Only Pod(s) running a namespace labeled with 'metrics: enabled' will be able to gather the metrics.
    # Only CR(s) which requires webhooks and are applied on namespaces labeled with 'webhooks: enabled' will
    # be able to communicate with the Webhook Server.
    #- ../network-policy
    
    # Uncomment the patches line if you enable Metrics
    patches:
    # [METRICS] The following patch will enable the metrics endpoint using HTTPS and the port :8443.
    # More info: https://book.kubebuilder.io/reference/metrics
    - path: manager_metrics_patch.yaml
      target:
        kind: Deployment
    
    # Uncomment the patches line if you enable Metrics and CertManager
    # [METRICS-WITH-CERTS] To enable metrics protected with certManager, uncomment the following line.
    # This patch will protect the metrics with certManager self-signed certs.
    #- path: cert_metrics_manager_patch.yaml
    #  target:
    #    kind: Deployment
    
    # [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in
    # crd/kustomization.yaml
    #- path: manager_webhook_patch.yaml
    #  target:
    #    kind: Deployment
    
    # [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER' prefix.
    # Uncomment the following replacements to add the cert-manager CA injection annotations
    #replacements:
    # - source: # Uncomment the following block to enable certificates for metrics
    #     kind: Service
    #     version: v1
    #     name: controller-manager-metrics-service
    #     fieldPath: metadata.name
    #   targets:
    #     - select:
    #         kind: Certificate
    #         group: cert-manager.io
    #         version: v1
    #         name: metrics-certs
    #       fieldPaths:
    #         - spec.dnsNames.0
    #         - spec.dnsNames.1
    #       options:
    #         delimiter: '.'
    #         index: 0
    #         create: true
    #     - select: # Uncomment the following to set the Service name for TLS config in Prometheus ServiceMonitor
    #         kind: ServiceMonitor
    #         group: monitoring.coreos.com
    #         version: v1
    #         name: controller-manager-metrics-monitor
    #       fieldPaths:
    #         - spec.endpoints.0.tlsConfig.serverName
    #       options:
    #         delimiter: '.'
    #         index: 0
    #         create: true
    #
    # - source:
    #     kind: Service
    #     version: v1
    #     name: controller-manager-metrics-service
    #     fieldPath: metadata.namespace
    #   targets:
    #     - select:
    #         kind: Certificate
    #         group: cert-manager.io
    #         version: v1
    #         name: metrics-certs
    #       fieldPaths:
    #         - spec.dnsNames.0
    #         - spec.dnsNames.1
    #       options:
    #         delimiter: '.'
    #         index: 1
    #         create: true
    #     - select: # Uncomment the following to set the Service namespace for TLS in Prometheus ServiceMonitor
    #         kind: ServiceMonitor
    #         group: monitoring.coreos.com
    #         version: v1
    #         name: controller-manager-metrics-monitor
    #       fieldPaths:
    #         - spec.endpoints.0.tlsConfig.serverName
    #       options:
    #         delimiter: '.'
    #         index: 1
    #         create: true
    #
    # - source: # Uncomment the following block if you have any webhook
    #     kind: Service
    #     version: v1
    #     name: webhook-service
    #     fieldPath: .metadata.name # Name of the service
    #   targets:
    #     - select:
    #         kind: Certificate
    #         group: cert-manager.io
    #         version: v1
    #         name: serving-cert
    #       fieldPaths:
    #         - .spec.dnsNames.0
    #         - .spec.dnsNames.1
    #       options:
    #         delimiter: '.'
    #         index: 0
    #         create: true
    # - source:
    #     kind: Service
    #     version: v1
    #     name: webhook-service
    #     fieldPath: .metadata.namespace # Namespace of the service
    #   targets:
    #     - select:
    #         kind: Certificate
    #         group: cert-manager.io
    #         version: v1
    #         name: serving-cert
    #       fieldPaths:
    #         - .spec.dnsNames.0
    #         - .spec.dnsNames.1
    #       options:
    #         delimiter: '.'
    #         index: 1
    #         create: true
    #
    # - source: # Uncomment the following block if you have a ValidatingWebhook (--programmatic-validation)
    #     kind: Certificate
    #     group: cert-manager.io
    #     version: v1
    #     name: serving-cert # This name should match the one in certificate.yaml
    #     fieldPath: .metadata.namespace # Namespace of the certificate CR
    #   targets:
    #     - select:
    #         kind: ValidatingWebhookConfiguration
    #       fieldPaths:
    #         - .metadata.annotations.[cert-manager.io/inject-ca-from]
    #       options:
    #         delimiter: '/'
    #         index: 0
    #         create: true
    # - source:
    #     kind: Certificate
    #     group: cert-manager.io
    #     version: v1
    #     name: serving-cert
    #     fieldPath: .metadata.name
    #   targets:
    #     - select:
    #         kind: ValidatingWebhookConfiguration
    #       fieldPaths:
    #         - .metadata.annotations.[cert-manager.io/inject-ca-from]
    #       options:
    #         delimiter: '/'
    #         index: 1
    #         create: true
    #
    
    Unnecessary Dependencies

    Several new dependencies are added to go.mod without clear usage in the provided diff. Verify that these dependencies are necessary and that they do not introduce unnecessary bloat or security risks.

    require (
    	contrib.go.opencensus.io/exporter/ocagent v0.7.1-0.20200907061046-05415f1de66d // indirect
    	contrib.go.opencensus.io/exporter/prometheus v0.4.2 // indirect
    	github.com/antlr4-go/antlr/v4 v4.13.0 // indirect
    	github.com/beorn7/perks v1.0.1 // indirect
    	github.com/blang/semver/v4 v4.0.0 // indirect
    	github.com/blendle/zapdriver v1.3.1 // indirect
    	github.com/cenkalti/backoff/v4 v4.3.0 // indirect
    	github.com/census-instrumentation/opencensus-proto v0.4.1 // indirect
    	github.com/cespare/xxhash/v2 v2.3.0 // indirect
    	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
    	github.com/emicklei/go-restful/v3 v3.12.1 // indirect
    	github.com/evanphx/json-patch v5.9.0+incompatible // indirect
    	github.com/evanphx/json-patch/v5 v5.9.0 // indirect
    	github.com/felixge/httpsnoop v1.0.4 // indirect
    	github.com/fsnotify/fsnotify v1.7.0 // indirect
    	github.com/go-kit/log v0.2.1 // indirect
    	github.com/go-logfmt/logfmt v0.6.0 // indirect
    	github.com/go-logr/stdr v1.2.2 // indirect
    	github.com/go-logr/zapr v1.3.0 // indirect
    	github.com/go-openapi/jsonpointer v0.21.0 // indirect
    	github.com/go-openapi/jsonreference v0.21.0 // indirect
    	github.com/go-openapi/swag v0.23.0 // indirect
    	github.com/go-task/slim-sprig/v3 v3.0.0 // indirect
    	github.com/gobuffalo/flect v1.0.2 // indirect
    	github.com/gogo/protobuf v1.3.2 // indirect
    	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
    	github.com/golang/protobuf v1.5.4 // indirect
    	github.com/google/cel-go v0.20.1 // indirect
    	github.com/google/gnostic-models v0.6.8 // indirect
    	github.com/google/go-cmp v0.6.0 // indirect
    	github.com/google/gofuzz v1.2.0 // indirect
    	github.com/google/pprof v0.0.0-20241210010833-40e02aabc2ad // indirect
    	github.com/google/uuid v1.6.0 // indirect
    	github.com/gorilla/websocket v1.5.1 // indirect
    	github.com/grpc-ecosystem/grpc-gateway/v2 v2.21.0 // indirect
    	github.com/hashicorp/golang-lru v1.0.2 // indirect
    	github.com/imdario/mergo v0.3.16 // indirect
    	github.com/inconshreveable/mousetrap v1.1.0 // indirect
    	github.com/josharian/intern v1.0.0 // indirect
    	github.com/json-iterator/go v1.1.12 // indirect
    	github.com/kelseyhightower/envconfig v1.4.0 // indirect
    	github.com/klauspost/compress v1.17.9 // indirect
    	github.com/mailru/easyjson v0.7.7 // indirect
    	github.com/mitchellh/hashstructure/v2 v2.0.2 // indirect
    	github.com/moby/spdystream v0.4.0 // indirect
    	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
    	github.com/modern-go/reflect2 v1.0.2 // indirect
    	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
    	github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f // indirect
    	github.com/opencontainers/go-digest v1.0.0 // indirect
    	github.com/patrickmn/go-cache v2.1.0+incompatible // indirect
    	github.com/pkg/errors v0.9.1 // indirect
    	github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
    	github.com/prometheus/client_model v0.6.1 // indirect
    	github.com/prometheus/common v0.55.0 // indirect
    	github.com/prometheus/procfs v0.15.1 // indirect
    	github.com/prometheus/statsd_exporter v0.24.0 // indirect
    	github.com/robfig/cron/v3 v3.0.1 // indirect
    	github.com/spf13/cobra v1.8.1 // indirect
    	github.com/spf13/pflag v1.0.5 // indirect
    	github.com/stoewer/go-strcase v1.2.0 // indirect
    	github.com/stretchr/objx v0.5.2 // indirect
    	go.opencensus.io v0.24.0 // indirect
    	go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.53.0 // indirect
    	go.opentelemetry.io/otel v1.28.0 // indirect
    	go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.28.0 // indirect
    	go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.27.0 // indirect
    	go.opentelemetry.io/otel/metric v1.28.0 // indirect
    	go.opentelemetry.io/otel/sdk v1.28.0 // indirect
    	go.opentelemetry.io/otel/trace v1.28.0 // indirect
    	go.opentelemetry.io/proto/otlp v1.3.1 // indirect
    	go.uber.org/automaxprocs v1.5.3 // indirect
    	go.uber.org/multierr v1.11.0 // indirect
    	go.uber.org/zap v1.27.0 // indirect
    	golang.org/x/exp v0.0.0-20240808152545-0cdaa3abc0fa // indirect
    	golang.org/x/net v0.36.0 // indirect
    	golang.org/x/oauth2 v0.22.0 // indirect
    	golang.org/x/sync v0.11.0 // indirect
    	golang.org/x/sys v0.30.0 // indirect
    	golang.org/x/term v0.29.0 // indirect
    	golang.org/x/text v0.22.0 // indirect
    	golang.org/x/time v0.6.0 // indirect
    	golang.org/x/tools v0.28.0 // indirect
    	gomodules.xyz/jsonpatch/v2 v2.4.0 // indirect
    	google.golang.org/api v0.183.0 // indirect
    	google.golang.org/genproto/googleapis/api v0.0.0-20240808171019-573a1156607a // indirect
    	google.golang.org/genproto/googleapis/rpc v0.0.0-20240808171019-573a1156607a // indirect
    	google.golang.org/grpc v1.65.0 // indirect
    	google.golang.org/protobuf v1.36.1 // indirect
    	gopkg.in/inf.v0 v0.9.1 // indirect
    	gopkg.in/yaml.v3 v3.0.1 // indirect
    	k8s.io/apiextensions-apiserver v0.30.3 // indirect
    	k8s.io/apiserver v0.30.1 // indirect
    	k8s.io/cloud-provider v0.30.3 // indirect
    	k8s.io/csi-translation-lib v0.30.3 // indirect
    	k8s.io/kube-openapi v0.0.0-20240808142205-8e686545bdb8 // indirect
    	k8s.io/pod-security-admission v0.0.0 // indirect
    	sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.30.3 // indirect
    	sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
    

    kaito-pr-agent[bot] avatar Apr 27 '25 10:04 kaito-pr-agent[bot]

    PR Code Suggestions ✨

    Latest suggestions up to a77c700 Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Replace placeholder name

    Use a meaningful name instead of temp.

    config/default/metrics_service.yaml [6]

    -app.kubernetes.io/name: temp
    +app.kubernetes.io/name: controller-manager-metrics-service
    
    Suggestion importance[1-10]: 8

    __

    Why: Using a meaningful name improves the clarity and maintainability of the configuration.

    Medium
    Align metrics port

    Ensure the metrics port is consistent across configurations.

    charts/kaito/ragengine/values.yaml [20]

     metrics:
    -  port: 8080
    +  port: 8443
    
    Suggestion importance[1-10]: 7

    __

    Why: Ensuring consistency in the metrics port across different configurations helps avoid confusion and potential misconfigurations.

    Medium
    Add colon to default port

    Ensure that the port number includes a colon (:) when specifying the default value
    to maintain consistency with typical URL formats.

    cmd/ragengine/main.go [77]

    -flag.StringVar(&metricsAddr, "metrics-bind-address", "8080", "The address the metrics endpoint binds to. "+
    +flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metrics endpoint binds to. "+
       "Use :8443 for HTTPS or :8080 for HTTP")
    
    Suggestion importance[1-10]: 3

    __

    Why: The suggestion is correct but offers a minor improvement. Including a colon in the default port number is consistent with typical URL formats, but it doesn't fix a bug or significantly enhance functionality.

    Low
    Use specific image tag

    Verify that workspacecontroller:latest is the correct image tag and repository for
    your deployment. Consider using a specific version tag instead of latest for better
    reproducibility and stability.

    config/manager/ragengine.yaml [1]

    -image: workspacecontroller:latest
    +image: workspacecontroller:v1.0.0
    
    Suggestion importance[1-10]: 2

    __

    Why: The suggestion asks to verify and potentially change the image tag, which is good practice for reproducibility and stability. However, it is not actionable without additional context about the correct image tag, so the score is reduced.

    Low

    Previous suggestions

    Suggestions up to commit a77c700
    CategorySuggestion                                                                                                                                    Impact
    General
    Replace placeholder name

    Use a meaningful name instead of temp.

    config/default/metrics_service.yaml [6]

    -app.kubernetes.io/name: temp
    +app.kubernetes.io/name: controller-manager-metrics-service
    
    Suggestion importance[1-10]: 8

    __

    Why: Using a meaningful name improves the clarity and maintainability of the configuration.

    Medium
    Align metrics port

    Ensure the metrics port is consistent across configurations.

    charts/kaito/ragengine/values.yaml [20]

     metrics:
    -  port: 8080
    +  port: 8443
    
    Suggestion importance[1-10]: 7

    __

    Why: Ensuring consistency in the metrics port across configurations helps avoid potential misconfigurations and confusion.

    Medium
    Validate image tag

    Verify that workspacecontroller:latest is the correct image tag for the deployment.

    config/manager/ragengine.yaml [1]

    +image: workspacecontroller:latest
     
    -
    
    Suggestion importance[1-10]: 2

    __

    Why: The suggestion is not actionable as it only asks the user to verify the image tag, which is already correct in the PR.

    Low
    Possible issue
    Correct default address format

    Ensure that the default value includes the colon to specify the protocol correctly.

    cmd/ragengine/main.go [77-78]

    -flag.StringVar(&metricsAddr, "metrics-bind-address", "8080", "The address the metrics endpoint binds to. "+
    +flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metrics endpoint binds to. "+
       "Use :8443 for HTTPS or :8080 for HTTP")
    
    Suggestion importance[1-10]: 3

    __

    Why: The suggestion is correct but only offers a minor improvement. The existing code already specifies the default address format correctly with a colon.

    Low

    kaito-pr-agent[bot] avatar Apr 27 '25 10:04 kaito-pr-agent[bot]

    /hold

    Fei-Guo avatar Apr 27 '25 17:04 Fei-Guo