-
Notifications
You must be signed in to change notification settings - Fork 270
feat(http): implement tls serving certs for server #739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,6 +69,13 @@ type StaticConfig struct { | |
| StsScopes []string `toml:"sts_scopes,omitempty"` | ||
| CertificateAuthority string `toml:"certificate_authority,omitempty"` | ||
| ServerURL string `toml:"server_url,omitempty"` | ||
|
|
||
| // TLS configuration for the HTTP server | ||
| // TLSCert is the path to the TLS certificate file for HTTPS | ||
| TLSCert string `toml:"tls_cert,omitempty"` | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These new TOML options should also be documented in |
||
| // TLSKey is the path to the TLS private key file for HTTPS | ||
| TLSKey string `toml:"tls_key,omitempty"` | ||
mjudeikis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // ClusterProviderStrategy is how the server finds clusters. | ||
| // If set to "kubeconfig", the clusters will be loaded from those in the kubeconfig. | ||
| // If set to "in-cluster", the server will use the in cluster config | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,6 +86,8 @@ const ( | |
| flagCertificateAuthority = "certificate-authority" | ||
| flagDisableMultiCluster = "disable-multi-cluster" | ||
| flagClusterProvider = "cluster-provider" | ||
| flagTLSCert = "tls-cert" | ||
| flagTLSKey = "tls-key" | ||
| ) | ||
|
|
||
| type MCPServerOptions struct { | ||
|
|
@@ -106,6 +108,8 @@ type MCPServerOptions struct { | |
| ServerURL string | ||
| DisableMultiCluster bool | ||
| ClusterProvider string | ||
| TLSCert string | ||
| TLSKey string | ||
|
|
||
| ConfigPath string | ||
| ConfigDir string | ||
|
|
@@ -167,6 +171,8 @@ func NewMCPServer(streams genericiooptions.IOStreams) *cobra.Command { | |
| _ = cmd.Flags().MarkHidden(flagCertificateAuthority) | ||
| cmd.Flags().BoolVar(&o.DisableMultiCluster, flagDisableMultiCluster, o.DisableMultiCluster, "Disable multi cluster tools. Optional. If true, all tools will be run against the default cluster/context.") | ||
| cmd.Flags().StringVar(&o.ClusterProvider, flagClusterProvider, o.ClusterProvider, "Cluster provider strategy to use (one of: kubeconfig, in-cluster, kcp, disabled). If not set, the server will auto-detect based on the environment.") | ||
| cmd.Flags().StringVar(&o.TLSCert, flagTLSCert, o.TLSCert, "Path to TLS certificate file for HTTPS. Must be used together with --tls-key.") | ||
| cmd.Flags().StringVar(&o.TLSKey, flagTLSKey, o.TLSKey, "Path to TLS private key file for HTTPS. Must be used together with --tls-cert.") | ||
|
|
||
| return cmd | ||
| } | ||
|
|
@@ -241,6 +247,12 @@ func (m *MCPServerOptions) loadFlags(cmd *cobra.Command) { | |
| if cmd.Flag(flagDisableMultiCluster).Changed && m.DisableMultiCluster { | ||
| m.StaticConfig.ClusterProviderStrategy = api.ClusterProviderDisabled | ||
| } | ||
| if cmd.Flag(flagTLSCert).Changed { | ||
| m.StaticConfig.TLSCert = m.TLSCert | ||
| } | ||
| if cmd.Flag(flagTLSKey).Changed { | ||
| m.StaticConfig.TLSKey = m.TLSKey | ||
| } | ||
| } | ||
|
|
||
| func (m *MCPServerOptions) initializeLogging() { | ||
|
|
@@ -296,6 +308,22 @@ func (m *MCPServerOptions) Validate() error { | |
| return fmt.Errorf("certificate-authority must be a valid file path: %w", err) | ||
| } | ||
| } | ||
| // Validate TLS configuration | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: TLS is only meaningful in HTTP mode (when if tlsCert != "" && m.StaticConfig.Port == "" {
return fmt.Errorf("--tls-cert and --tls-key require --port to be set (TLS is only supported in HTTP mode)")
}This would give users a clear early error instead of silently ignoring the TLS config in STDIO mode. |
||
| tlsCert := strings.TrimSpace(m.StaticConfig.TLSCert) | ||
| tlsKey := strings.TrimSpace(m.StaticConfig.TLSKey) | ||
| if (tlsCert != "" && tlsKey == "") || (tlsCert == "" && tlsKey != "") { | ||
| return fmt.Errorf("both --tls-cert and --tls-key must be provided together") | ||
| } | ||
| if tlsCert != "" { | ||
| if _, err := os.Stat(tlsCert); err != nil { | ||
| return fmt.Errorf("tls-cert must be a valid file path: %w", err) | ||
| } | ||
| } | ||
| if tlsKey != "" { | ||
| if _, err := os.Stat(tlsKey); err != nil { | ||
| return fmt.Errorf("tls-key must be a valid file path: %w", err) | ||
| } | ||
| } | ||
mjudeikis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return nil | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When TLS is enabled via
extraArgs, the defaultlivenessProbeandreadinessProbeinvalues.yamlusehttpGetwithport: http. These HTTP probes will fail against an HTTPS endpoint since the server won't respond to plain HTTP.Since TLS is configured through opaque
extraArgs, the chart has no way to auto-setscheme: HTTPSon the probes. Users would need to manually override probes, volumes, and volume mounts — which is error-prone.Minimal fix: Document the required probe override in the comments here (e.g., show that users must also set
livenessProbe.httpGet.scheme: HTTPS).Better approach: Add a first-class
tlssection to the chart values instead of relying onextraArgsfor TLS:When
tls.enabledis true, the chart template would automatically:--tls-certand--tls-keyargsscheme: HTTPSon health probesThis is the standard pattern used by most Helm charts (cert-manager webhook, ingress-nginx, etc.) and provides a much better UX — users just set
tls.enabled: trueandtls.secretName: my-cert.The
extraArgsvalue is still useful as a generic escape hatch for other flags.