feat: update codis fe / proxy with tls; topom updated #2383
Conversation
kolinfluence
changed the title
|
@AlexStocks please look into it. |
codis/cert.pem
Outdated
There was a problem hiding this comment.
It's better not to provide the public key file. Instead, you can add a generation command in the readme.
There was a problem hiding this comment.
@machinly the public key file is definitely a dummy one.
you put "filesystem" as default, surely a dummy version is fine right? ... maybe you can modify with error message generated when there's no cert / key files present after you merge this and delete them. will that do fine?
There was a problem hiding this comment.
Do not modify the default configuration
There was a problem hiding this comment.
can you only pull what is needed?
codis/key.pem
Outdated
There was a problem hiding this comment.
It's better not to provide the private key file. Instead, you can add a generation command in the readme.
codis/pkg/proxy/config.go
Outdated
| jodis_name = "" | ||
| jodis_addr = "" | ||
| jodis_name = "etcd" | ||
| jodis_addr = "127.0.0.1:2379" |
There was a problem hiding this comment.
Do not modify the default parameter values
There was a problem hiding this comment.
can you only pull what is needed?
codis/pkg/topom/config.go
Outdated
| @@ -25,8 +25,10 @@ const DefaultConfig = ` | |||
| # Set Coordinator, only accept "zookeeper" & "etcd" & "filesystem". | |||
| # for zookeeper/etcd, coorinator_auth accept "user:password" | |||
| # Quick Start | |||
| coordinator_name = "filesystem" | |||
There was a problem hiding this comment.
Do not modify the default parameter values
There was a problem hiding this comment.
can you only pull what is needed?
codis/config/proxy.toml
Outdated
| # slow command list e.g. hgetall, mset | ||
| slow_cmd_list = "" | ||
| # quick command list | ||
| quick_cmd_list = "get,set" |
There was a problem hiding this comment.
Do not modify the default configuration
| CODIS_FE_ADDR="0.0.0.0:9090" | ||
| CODIS_FE_TLS=1 | ||
| CODIS_FE_TLS_KEY="/usr/local/src/pika/codis/key.pem" |
There was a problem hiding this comment.
Can you help add the rest?
|
noted. will revert. |
|
@luky116 sorry was busy playing with lru benchmark tests update here so a bit lagged behind on pika (but it's on my radar daily). back to your question...
good? |
you need to use redis-cli with secure tls to connect put pls remember it will only use tls for proxy if it's set to true. so accordingly, we will need to put tls-something for everything else etc where we want tls to have. so maybe tls-pika = true in future when pika has tls? but proxy shld definitely have tls. |
|
@CodiumAI-Agent /review |
PR Review 🔍
Code feedback:
|
|
@CodiumAI-Agent /improve |
PR Code Suggestions ✨
|
|
will get back next week. i was wondering if i can also improve the whole net thing with gnet. the performance will be much faster. https://github.com/panjf2000/gnet what do u guys think? |
|
Warning Rate limit exceeded@kolinfluence has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 2 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce TLS support for secure communication across various components of the Codis project. This involves updates to configuration files, enhancements to proxy and main function logic, and modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main as Main.go
participant Proxy as Proxy.go
participant Config as Config.go
User->>Main: Start Codis Server
Main->>Config: Load Configuration
Main->>Proxy: Initialize Proxy with Config
alt TLS Enabled
Proxy->>Config: Check TLS Settings
Proxy->>Proxy: Load TLS Certificate and Key
Proxy->>Proxy: Create TLS Listener
else TLS Disabled
Proxy->>Proxy: Create Standard Listener
end
Proxy->>User: Server Ready
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
codis/cmd/fe/main.go (2)
7-7: Ensure proper import aliasing.Consider aliasing the
tlsimport to avoid potential name conflicts.import ( cryptoTLS "crypto/tls" // other imports )
60-60: Update usage message to reflect new TLS options.The usage message should clearly describe the new TLS options.
- --listen=ADDR set the listen address. + --listen=ADDR set the listen address. + --tls-cert=FILE set the TLS certificate file. + --tls-key=FILE set the TLS key file. + --tls=N set to 1 to enable TLS, 0 to disable.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
codis/cert.pemis excluded by!**/*.pemcodis/key.pemis excluded by!**/*.pem
Files selected for processing (10)
- codis/admin/codis-fe-admin.sh (2 hunks)
- codis/cmd/fe/main.go (3 hunks)
- codis/config/proxy.toml (1 hunks)
- codis/pkg/models/proxy.go (1 hunks)
- codis/pkg/models/store.go (1 hunks)
- codis/pkg/models/topom.go (1 hunks)
- codis/pkg/proxy/config.go (2 hunks)
- codis/pkg/proxy/proxy.go (2 hunks)
- codis/pkg/topom/config.go (2 hunks)
- codis/pkg/topom/topom.go (1 hunks)
Additional comments not posted (16)
codis/pkg/models/topom.go (1)
Line range hint
15-17:
LGTM!The
Encodefunction looks good.codis/pkg/models/proxy.go (3)
14-14: LGTM!The addition of the
ProxyTLSfield looks good.
15-15: LGTM!The addition of the
ProxyTLSCertfield looks good.
16-16: LGTM!The addition of the
ProxyTLSKeyfield looks good.codis/admin/codis-fe-admin.sh (2)
21-21: LGTM!The addition of the
CODIS_FE_TLSvariable looks good.
22-23: LGTM!The addition of the
CODIS_FE_TLS_KEYandCODIS_FE_TLS_CERTvariables looks good.codis/config/proxy.toml (3)
24-24: LGTM!The addition of the
proxy_tlsfield looks good.
25-25: LGTM!The addition of the
proxy_tls_certfield looks good.
26-26: LGTM!The addition of the
proxy_tls_keyfield looks good.codis/pkg/proxy/config.go (2)
40-42: Verify the TLS configuration defaults.The new TLS configuration options are commented out by default. Ensure that the default values are appropriate and secure.
172-174: Confirm the addition of TLS configuration fields.The new fields for TLS configuration have been added to the
Configstruct. Ensure that these fields are correctly used and validated in the code.codis/pkg/proxy/proxy.go (5)
7-7: Verify the necessity of thecrypto/tlsimport.The
crypto/tlspackage has been imported to handle TLS functionality. Ensure that it is used correctly and is necessary for the changes implemented.
107-110: Initializelanderrvariables appropriately.The
landerrvariables are declared without initialization. Ensure that they are correctly initialized before use.
111-130: Ensure proper error handling and logging for TLS setup.The TLS setup includes loading certificates and creating a TLS listener. Verify that errors are correctly handled and logged.
132-137: Ensure proper error handling and logging for non-TLS setup.The non-TLS setup includes creating a standard listener. Verify that errors are correctly handled and logged.
141-153: Confirm common setup for both TLS and non-TLS listeners.The common setup for both TLS and non-TLS listeners includes updating the proxy model and replacing unspecified IP addresses. Ensure that this setup is correctly implemented.
codis/pkg/models/store.go
Outdated
| func (s *Store) ReleaseByToken(token string) error { | ||
| b, err := s.client.Read(s.LockPath(),false) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if b != nil { | ||
| t, err := Decode(b); | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if t.Token == token { | ||
| return s.Release() | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Reduce nesting and improve readability.
The nested if statements can be reduced to improve readability.
func (s *Store) ReleaseByToken(token string) error {
b, err := s.client.Read(s.LockPath(), false)
if err != nil {
return err
}
if b == nil {
return nil
}
t, err := Decode(b)
if err != nil {
return err
}
if t.Token == token {
return s.Release()
}
return nil
}There was a problem hiding this comment.
@coderabbitai no. nesting is fine here.
There was a problem hiding this comment.
@kolinfluence Understood, I'll respect your decision to keep the nesting as is. If you need any further assistance, feel free to ask.
codis/pkg/models/topom.go
Outdated
| func Decode(b []byte) (*Topom, error) { | ||
| s := &Topom{} | ||
| if err := jsonDecode(s, b); err != nil { | ||
| return nil,err | ||
| } | ||
| return s,nil |
There was a problem hiding this comment.
Fix formatting issues.
There are minor formatting issues in the Decode function. Ensure there is a space after the comma in the return statements.
- return nil,err
+ return nil, err
- return s,nil
+ return s, nilCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func Decode(b []byte) (*Topom, error) { | |
| s := &Topom{} | |
| if err := jsonDecode(s, b); err != nil { | |
| return nil,err | |
| } | |
| return s,nil | |
| func Decode(b []byte) (*Topom, error) { | |
| s := &Topom{} | |
| if err := jsonDecode(s, b); err != nil { | |
| return nil, err | |
| } | |
| return s, nil |
codis/pkg/topom/config.go
Outdated
| sentinel_check_server_state_interval = "10s" | ||
| sentinel_check_master_failover_interval = "2s" | ||
| sentinel_master_dead_check_times = 10 | ||
| sentinel_check_offline_server_interval = "2s" |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure new configuration setting is used.
The new configuration setting sentinel_check_offline_server_interval is defined but not used anywhere in the codebase. Please ensure that this setting is utilized where necessary or remove it if it is not required.
codis/pkg/topom/config.go: Definition found, but no usage detected.
Analysis chain
Ensure new configuration setting is used.
The new configuration setting sentinel_check_offline_server_interval should be used in the code. Verify its usage or remove it if unnecessary.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the new configuration setting `sentinel_check_offline_server_interval`.
# Test: Search for the usage of the new configuration setting. Expect: At least one occurrence.
rg --type go 'sentinel_check_offline_server_interval'
Length of output: 300
Script:
#!/bin/bash
# Description: Verify the usage of the configuration setting `SentinelCheckOfflineServerInterval`.
# Test: Search for the usage of the configuration setting. Expect: At least one occurrence.
rg --type go 'SentinelCheckOfflineServerInterval'
Length of output: 224
| if n, ok := utils.ArgumentInteger(d, "--tls"); ok && n == 1 { | ||
|
|
||
| // Load server certificate and key | ||
| certFile, keyFile := utils.ArgumentMust(d, "--tls-cert"), utils.ArgumentMust(d, "--tls-key") | ||
| if certFile == "" || keyFile == "" { | ||
| log.Panic("TLS certificate or key file path must be provided") | ||
| } | ||
| cert, err := tls.LoadX509KeyPair(certFile, keyFile) | ||
| if err != nil { | ||
| log.PanicErrorf(err, "failed to load key pair: %s", err) | ||
| } | ||
|
|
||
| // Create a TLS config | ||
| tlsConfig := &tls.Config{ | ||
| Certificates: []tls.Certificate{cert}, | ||
| } | ||
|
|
||
| // Create a TLS listener | ||
| l, err = tls.Listen("tcp", listen, tlsConfig) | ||
| if err != nil { | ||
| log.PanicErrorf(err, "listen %s failed", listen) | ||
| } | ||
|
|
||
|
|
||
| }else{ | ||
|
|
||
| l, err = net.Listen("tcp", listen) | ||
| if err != nil { | ||
| log.PanicErrorf(err, "listen %s failed", listen) | ||
| } | ||
|
|
There was a problem hiding this comment.
Improve error handling and logging for TLS setup.
Ensure that all potential errors are logged appropriately and provide more context.
if n, ok := utils.ArgumentInteger(d, "--tls"); ok && n == 1 {
// Load server certificate and key
certFile, keyFile := utils.ArgumentMust(d, "--tls-cert"), utils.ArgumentMust(d, "--tls-key")
if certFile == "" || keyFile == "" {
log.Panic("TLS certificate or key file path must be provided")
}
cert, err := tls.LoadX509KeyPair(certFile, keyFile)
if err != nil {
log.PanicErrorf(err, "failed to load key pair: %s", err)
}
// Create a TLS config
tlsConfig := &tls.Config{
Certificates: []tls.Certificate{cert},
}
// Create a TLS listener
l, err = tls.Listen("tcp", listen, tlsConfig)
if err != nil {
log.PanicErrorf(err, "TLS listen on %s failed: %v", listen, err)
}
log.Warnf("TLS enabled, listening on %s", listen)
} else {
l, err = net.Listen("tcp", listen)
if err != nil {
log.PanicErrorf(err, "listen on %s failed: %v", listen, err)
}
log.Warnf("Listening on %s without TLS", listen)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if n, ok := utils.ArgumentInteger(d, "--tls"); ok && n == 1 { | |
| // Load server certificate and key | |
| certFile, keyFile := utils.ArgumentMust(d, "--tls-cert"), utils.ArgumentMust(d, "--tls-key") | |
| if certFile == "" || keyFile == "" { | |
| log.Panic("TLS certificate or key file path must be provided") | |
| } | |
| cert, err := tls.LoadX509KeyPair(certFile, keyFile) | |
| if err != nil { | |
| log.PanicErrorf(err, "failed to load key pair: %s", err) | |
| } | |
| // Create a TLS config | |
| tlsConfig := &tls.Config{ | |
| Certificates: []tls.Certificate{cert}, | |
| } | |
| // Create a TLS listener | |
| l, err = tls.Listen("tcp", listen, tlsConfig) | |
| if err != nil { | |
| log.PanicErrorf(err, "listen %s failed", listen) | |
| } | |
| }else{ | |
| l, err = net.Listen("tcp", listen) | |
| if err != nil { | |
| log.PanicErrorf(err, "listen %s failed", listen) | |
| } | |
| if n, ok := utils.ArgumentInteger(d, "--tls"); ok && n == 1 { | |
| // Load server certificate and key | |
| certFile, keyFile := utils.ArgumentMust(d, "--tls-cert"), utils.ArgumentMust(d, "--tls-key") | |
| if certFile == "" || keyFile == "" { | |
| log.Panic("TLS certificate or key file path must be provided") | |
| } | |
| cert, err := tls.LoadX509KeyPair(certFile, keyFile) | |
| if err != nil { | |
| log.PanicErrorf(err, "failed to load key pair: %s", err) | |
| } | |
| // Create a TLS config | |
| tlsConfig := &tls.Config{ | |
| Certificates: []tls.Certificate{cert}, | |
| } | |
| // Create a TLS listener | |
| l, err = tls.Listen("tcp", listen, tlsConfig) | |
| if err != nil { | |
| log.PanicErrorf(err, "TLS listen on %s failed: %v", listen, err) | |
| } | |
| log.Warnf("TLS enabled, listening on %s", listen) | |
| } else { | |
| l, err = net.Listen("tcp", listen) | |
| if err != nil { | |
| log.PanicErrorf(err, "listen on %s failed: %v", listen, err) | |
| } | |
| log.Warnf("Listening on %s without TLS", listen) | |
| } |
| err := s.store.ReleaseByToken(s.model.Token); | ||
| if err != nil { |
There was a problem hiding this comment.
Improve error handling in Close method.
Ensure that the error is logged and handled appropriately.
if s.online {
err := s.store.ReleaseByToken(s.model.Token)
if err != nil {
log.ErrorErrorf(err, "store: release lock of %s failed", s.config.ProductName)
return errors.Errorf("store: release lock of %s failed", s.config.ProductName)
}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err := s.store.ReleaseByToken(s.model.Token); | |
| if err != nil { | |
| if s.online { | |
| err := s.store.ReleaseByToken(s.model.Token) | |
| if err != nil { | |
| log.ErrorErrorf(err, "store: release lock of %s failed", s.config.ProductName) | |
| return errors.Errorf("store: release lock of %s failed", s.config.ProductName) | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- codis/pkg/models/store.go (1 hunks)
- codis/pkg/proxy/proxy.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- codis/pkg/models/store.go
- codis/pkg/proxy/proxy.go
|
can someone pull this? have more assurance to have tls connection |
|
@machinly @chejinge @AlexStocks can someone pls check why "fmt" error? i would like to contribute more code. pls pull this when possible. |
|
@machinly @chejinge @AlexStocks what's wrong with the pull? |
codis fe / proxy tls + topom update
Summary by CodeRabbit
New Features
--tls-cert,--tls-key,--tls=N).Bug Fixes
Topomstruct to handle token-based releases.Refactor
Proxystruct to include TLS-related fields.