Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Edgewize 0.59 #1

Open
wants to merge 12 commits into
base: 0.59.0
Choose a base branch
from
Open

Edgewize 0.59 #1

wants to merge 12 commits into from

Conversation

zuoxuesong-worker
Copy link
Collaborator

WHY

imneov
imneov reviewed Nov 8, 2024
View reviewed changes
Copy link

@imneov imneov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我理解有个原则是：尽量不要动原来的代码

  • 尽量不动原来的代码
    • 新增的函数、结构体可以创建一个新的文件
  • 尽量改动最少的文件
    • version 这种直接改一个地方就好，不需要每个地方都硬编码
    • 其他分支判断尽量在 config 中去强制
  • 如果修改最好是新增，不要删除
    • 如果是函数调用的参数，可以重新赋值来

cmd/router-client/sub/root.go Outdated
@@ -54,7 +53,8 @@ var rootCmd = &cobra.Command{
Short: "frpc is the client of frp (https://github.com/fatedier/frp)",
RunE: func(cmd *cobra.Command, args []string) error {
if showVersion {
fmt.Println(version.Full())
//fmt.Println(version.Full())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里为啥不直接修改 version.Full 的内容，我看已经出现多次这种修改

//fmt.Println(version.Full())
fmt.Println("edgewize-msg-transport-v1")

pkg/auth/oidc.go Outdated
@@ -149,3 +153,25 @@ func (auth *OidcAuthConsumer) VerifyNewWorkConn(newWorkConnMsg *msg.NewWorkConn)

return auth.verifyPostLoginToken(newWorkConnMsg.PrivilegeKey)
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果是新增函数建议单独创建一个文件，不要直接修改源文件

pkg/auth/pass.go Outdated
@@ -24,6 +24,10 @@ var _ Verifier = &alwaysPass{}

type alwaysPass struct{}

func (p *alwaysPass) VerifyCrypto(c *msg.CryptoLogin) *msg.Login {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

新增函数，建议创建一个新文件

pkg/auth/token.go Outdated
@@ -89,3 +92,25 @@ func (auth *TokenAuthSetterVerifier) VerifyNewWorkConn(m *msg.NewWorkConn) error
}
return nil
}

func (auth *TokenAuthSetterVerifier) SetCrypto(c *msg.CryptoLogin, login *msg.Login) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

建立在新文件中创建函数

pkg/config/legacy/conversion.go Outdated
@@ -57,8 +57,10 @@ func Convert_ClientCommonConf_To_v1(conf *ClientCommonConf) *v1.ClientCommonConf
out.Transport.QUIC.KeepalivePeriod = conf.QUICKeepalivePeriod
out.Transport.QUIC.MaxIdleTimeout = conf.QUICMaxIdleTimeout
out.Transport.QUIC.MaxIncomingStreams = conf.QUICMaxIncomingStreams
out.Transport.TLS.Enable = lo.ToPtr(conf.TLSEnable)
out.Transport.TLS.DisableCustomTLSFirstByte = lo.ToPtr(conf.DisableCustomTLSFirstByte)
// 强制开启tls
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

建议在 config 创建的地方修改，不要直接硬编码

pkg/msg/msg.go Outdated
type CryptoLogin struct {
TimeStamp int64 `json:"time_stamp,omitempty"`
Auth string `json:"auth"`
Sign string `json:"sign,omitempty"`
}

// When frpc start, client send this message to login to server.
type Login struct {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是啥意思？

pkg/msg/msg.go Outdated
@@ -160,17 +170,17 @@ type StartWorkConn struct {
}

type NewVisitorConn struct {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里也不太懂

pkg/plugin/server/manager.go Outdated
if p.IsSupport(OpNewUserConn) {
m.newUserConnPlugins = append(m.newUserConnPlugins, p)
}
//if p.IsSupport(OpNewProxy) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里也是建议修改 feature

pkg/transport/tls.go Outdated
@@ -108,7 +108,6 @@ func NewClientTLSConfig(certPath, keyPath, caPath, serverName string) (*tls.Conf
if err != nil {
return nil, err
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

空行不要提交

pkg/util/net/dial.go Outdated
@@ -11,8 +11,9 @@ import (

func DialHookCustomTLSHeadByte(enableTLS bool, disableCustomTLSHeadByte bool) libnet.AfterHookFunc {
return func(ctx context.Context, c net.Conn, addr string) (context.Context, net.Conn, error) {
if enableTLS && !disableCustomTLSHeadByte {
_, err := c.Write([]byte{byte(FRPTLSHeadByte)})
if enableTLS {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

强制设定对应参数即可

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

尽量不要直接修改代码

zuoxuesong-worker
zuoxuesong-worker commented Nov 11, 2024
View reviewed changes
pkg/auth/edgewize_oidc.go
"github.com/fatedier/frp/pkg/util/util"
"strconv"
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

最初是为了保持和原生同样的风格，但是没有实际意义，已修改

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imneov imneov imneov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zuoxuesong-worker @imneov