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.

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

lightning configuration tidb.tls = false should not affect cluster certificate (PD's connection) #54172

Open
lance6716 opened this issue Jun 24, 2024 · 4 comments · May be fixed by #54211
Open
Assignees
Labels
component/lightning This issue is related to Lightning of TiDB. severity/moderate type/bug This issue is a bug.

Comments

@lance6716
Copy link
Contributor

When user only set security and not set tidb.security, TiDB connection's certificate will fallback to cluster certificate. However, TiDB connection has a separate tidb.tls to control other TLS behaviours. We should not let it affects cluster certificate, like in the case when fallack TiDB has tidb-tls = false but PD needs TLS

if d.Security == nil {
d.Security = s
}

@lance6716 lance6716 added type/bug This issue is a bug. severity/moderate labels Jun 24, 2024
@jebter jebter added the component/lightning This issue is related to Lightning of TiDB. label Jun 25, 2024
@ei-sugimoto
Copy link
Contributor

/assign

@ei-sugimoto
Copy link
Contributor

I have a question.
What behavior would you expect for the following input?

{
    [security]
    [tidb]
    tls = "false"
    [tidb.security]
    ca-path = "/path/to/ca2.pem"
}

There is an error in the test at TestAdjstSecuritySection. What should I do?

~/workspace/tidb/pkg/lightning/config$ go test
--- FAIL: TestAdjustSecuritySection (0.00s)
    config_test.go:474: 
                Error Trace:    /home/sugimoto_e/workspace/tidb/pkg/lightning/config/config_test.go:474
                Error:          Not equal: 
                                expected: ""
                                actual  : "/path/to/ca2.pem"
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -
                                +/path/to/ca2.pem
                Test:           TestAdjustSecuritySection
                Messages:       input = 
                                                                [security]
                                                                [tidb]
                                                                tls = "false"
                                                                [tidb.security]
                                                                ca-path = "/path/to/ca2.pem"

@lance6716
Copy link
Contributor Author

What behavior would you expect for the following input?

The expected behaviour is lightning doesn't use TLS to connect to TiDB.

There is an error in the test at TestAdjstSecuritySection. What should I do?

The test is a bit confusing, it checks two fields CAPath and TLSConfig. The final goal is don't use TLS when tls = false, so we need to understand how the two fields take effect when dealing with TLS. Maybe we don't need to check CAPath in tests, or CAPath must be set to empty to disable TLS so the unit test asserts it (I forget which one is true). When you write the PR you can check the logic and change necessary test code.

@ei-sugimoto
Copy link
Contributor

@lance6716
Thanks for the reply!
I have made my own modifications. Please give me a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/lightning This issue is related to Lightning of TiDB. severity/moderate type/bug This issue is a bug.
Projects
None yet
3 participants