Add TLS support for Go Feature Server#6229
Conversation
Signed-off-by: Shuchu Han <shuchu.han@gmail.com>
Signed-off-by: Shuchu Han <shuchu.han@gmail.com>
Signed-off-by: Shuchu Han <shuchu.han@gmail.com>
There was a problem hiding this comment.
🟡 ServerStarter interface not updated with StartHttpsServer, breaking the established abstraction pattern
The ServerStarter interface at go/main.go:40-43 defines StartHttpServer and StartGrpcServer but the new StartHttpsServer method was not added to it. The method was added to RealServerStarter at go/main.go:51-53 but not to the interface. This breaks the established pattern where the interface enumerates all server start methods, and means MockServerStarter in go/main_test.go:13 cannot be used to test the HTTPS code path. While the concrete type is used in main() so the code runs correctly, the interface contract is incomplete.
(Refers to lines 40-43)
Was this helpful? React with 👍 or 👎 to provide feedback.
| if certFile == "" || keyFile == "" { | ||
| return fmt.Errorf("TLS_CERT_FILE and TLS_KEY_FILE must be set") | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Error message and comment incorrectly reference environment variables instead of CLI flags
The comment at go/main.go:343 says "Requires TLS_CERT_FILE and TLS_KEY_FILE env vars" and the error at go/main.go:346 says "TLS_CERT_FILE and TLS_KEY_FILE must be set", but the actual parameters come from CLI flags --tls-cert-file and --tls-key-file (defined at go/main.go:80-81). This will confuse users who see the error: they'll look for environment variables to set rather than the correct CLI flags.
| if certFile == "" || keyFile == "" { | |
| return fmt.Errorf("TLS_CERT_FILE and TLS_KEY_FILE must be set") | |
| } | |
| // StartHttpsServer starts HTTP server with TLS. Requires --tls-cert-file and --tls-key-file flags. | |
| func StartHttpsServer(fs *feast.FeatureStore, host string, port int, metricsPort int, certFile string, keyFile string, writeLoggedFeaturesCallback logging.OfflineStoreWriteCallback, loggingOpts *logging.LoggingOptions) error { | |
| if certFile == "" || keyFile == "" { | |
| return fmt.Errorf("--tls-cert-file and --tls-key-file must be provided") |
Was this helpful? React with 👍 or 👎 to provide feedback.
Signed-off-by: Shuchu Han <shuchu.han@gmail.com>
What this PR does / why we need it:
Add TLS support for users who have enterprise security requirement
Which issue(s) this PR fixes:
Fix #6095
Checks
git commit -s)Testing Strategy
Misc