fix: resolve golangci-lint failures in auth_test.go#16
Conversation
Co-authored-by: ianchen0119 <42661015+ianchen0119@users.noreply.github.com>
Co-authored-by: ianchen0119 <42661015+ianchen0119@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR expands the plugin’s API client configuration to support mutual TLS (mTLS) for plugin → API server communication, wires that config through the gthulhu plugin initialization, and adds tests around the new behavior (in addition to the stated lint-related test adjustment).
Changes:
- Add
MTLSConfigand anAPIConfig.MTLSfield to carry mTLS PEM material via config. - Update
NewJWTClient/InitJWTClientto construct an mTLS-capablehttp.Clientwhen enabled and propagate construction errors. - Add mTLS-focused unit + end-to-end tests in
auth_test.go(includingerrcheck-friendlyresp.Body.Close()handling).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/plugin.go | Re-exports MTLSConfig via the public plugin package type aliases. |
| plugin/internal/registry/registry.go | Introduces MTLSConfig and adds it to APIConfig for YAML-driven configuration. |
| plugin/gthulhu/gthulhu.go | Plumbs APIConfig.MTLS into JWT client initialization. |
| plugin/gthulhu/auth.go | Implements mTLS client construction and updates JWT client construction to return errors. |
| plugin/gthulhu/auth_test.go | Adds tests covering mTLS disabled/enabled/bad PEM and an mTLS handshake round-trip. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // MTLSConfig holds the mutual TLS configuration used for plugin → API server communication. | ||
| // CertPem and KeyPem are the plugin's own certificate/key pair signed by the private CA. | ||
| // CAPem is the private CA certificate used to verify the API server's certificate. | ||
| type MTLSConfig struct { | ||
| Enable bool `yaml:"enable"` | ||
| CertPem string `yaml:"cert_pem"` | ||
| KeyPem string `yaml:"key_pem"` | ||
| CAPem string `yaml:"ca_pem"` | ||
| } | ||
|
|
||
| type APIConfig struct { | ||
| PublicKeyPath string `yaml:"public_key_path"` | ||
| BaseURL string `yaml:"base_url"` | ||
| Interval int `yaml:"interval"` | ||
| Enabled bool `yaml:"enabled"` | ||
| AuthEnabled bool `yaml:"auth_enabled"` | ||
| PublicKeyPath string `yaml:"public_key_path"` | ||
| BaseURL string `yaml:"base_url"` | ||
| Interval int `yaml:"interval"` | ||
| Enabled bool `yaml:"enabled"` | ||
| AuthEnabled bool `yaml:"auth_enabled"` | ||
| MTLS MTLSConfig `yaml:"mtls"` | ||
| } |
There was a problem hiding this comment.
The PR metadata says this is only fixing golangci-lint failures in auth_test.go, but this hunk introduces a new mTLS configuration surface (MTLSConfig + APIConfig.MTLS). Please update the PR title/description to reflect the new feature/API change, or split the lint-only fix into a separate PR so reviewers/consumers can assess the behavioral change explicitly.
| // NewJWTClient creates a new JWT client. When mtlsCfg.Enable is true the | ||
| // underlying HTTP client is configured with mutual TLS so the plugin | ||
| // authenticates itself to the API server and verifies the server certificate | ||
| // against the shared CA. | ||
| func NewJWTClient( | ||
| publicKeyPath, | ||
| apiBaseURL string, | ||
| authEnabled bool, | ||
| ) *JWTClient { | ||
| mtlsCfg reg.MTLSConfig, | ||
| ) (*JWTClient, error) { |
There was a problem hiding this comment.
NewJWTClient changed from returning only *JWTClient to returning (*JWTClient, error) and now requires an mTLS config parameter. Because this is an exported constructor, this is a breaking API change for any external callers. Consider keeping the old signature as a wrapper (defaulting mTLS disabled) and introducing a new constructor (or functional options) for mTLS to preserve compatibility.
|
|
||
| caPool := x509.NewCertPool() | ||
| if !caPool.AppendCertsFromPEM([]byte(mtlsCfg.CAPem)) { | ||
| return nil, fmt.Errorf("parse mTLS CA certificate") |
There was a problem hiding this comment.
The error returned when the CA PEM cannot be appended is very generic (no indication that the PEM contained zero certs / was invalid). Since AppendCertsFromPEM doesn’t provide an underlying error, consider returning a more actionable message (e.g., explicitly saying the CA PEM contained no valid certificates) while keeping the existing prefix so tests can continue to match.
| return nil, fmt.Errorf("parse mTLS CA certificate") | |
| return nil, fmt.Errorf("parse mTLS CA certificate: CA PEM contained no valid certificates") |
| // CertPem and KeyPem are the plugin's own certificate/key pair signed by the private CA. | ||
| // CAPem is the private CA certificate used to verify the API server's certificate. | ||
| type MTLSConfig struct { | ||
| Enable bool `yaml:"enable"` | ||
| CertPem string `yaml:"cert_pem"` | ||
| KeyPem string `yaml:"key_pem"` | ||
| CAPem string `yaml:"ca_pem"` |
There was a problem hiding this comment.
Field names like CertPem/KeyPem/CAPem don’t follow the usual Go initialism style used elsewhere in the repo (e.g., BaseURL). Since this is a newly introduced exported config type, it’s a good time to rename these to CertPEM/KeyPEM/CAPEM (and update references) to avoid locking in an inconsistent public API.
| // CertPem and KeyPem are the plugin's own certificate/key pair signed by the private CA. | |
| // CAPem is the private CA certificate used to verify the API server's certificate. | |
| type MTLSConfig struct { | |
| Enable bool `yaml:"enable"` | |
| CertPem string `yaml:"cert_pem"` | |
| KeyPem string `yaml:"key_pem"` | |
| CAPem string `yaml:"ca_pem"` | |
| // CertPEM and KeyPEM are the plugin's own certificate/key pair signed by the private CA. | |
| // CAPEM is the private CA certificate used to verify the API server's certificate. | |
| type MTLSConfig struct { | |
| Enable bool `yaml:"enable"` | |
| CertPEM string `yaml:"cert_pem"` | |
| KeyPEM string `yaml:"key_pem"` | |
| CAPEM string `yaml:"ca_pem"` |
| // InitJWTClient initializes the JWT client for API authentication | ||
| func (g *GthulhuPlugin) InitJWTClient( | ||
| publicKeyPath, | ||
| apiBaseURL string, | ||
| authEnabled bool, | ||
| mtlsCfg reg.MTLSConfig, | ||
| ) error { |
There was a problem hiding this comment.
InitJWTClient is an exported method but takes reg.MTLSConfig (an internal/registry type). Even though plugin.MTLSConfig aliases it, this leaks an internal package path into the gthulhu package’s public API/docs and forces external callers to depend on type-alias plumbing. Consider defining a gthulhu.MTLSConfig alias/type and using that in the signature (or accept standard library types like *tls.Config) so the exported API is self-contained.
Two lint errors introduced in
plugin/gthulhu/auth_test.gowere breaking CI.errcheck:defer resp.Body.Close()discarded the error return. Replaced with a deferred closure that logs the error viat.Logf.gofmt -s: Trailing blank line at end of file. Removed.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.