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

feat: add TLS version annotation support for per-rule configuration #1592

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

yunmaoQu
Copy link

Ⅰ. Describe what this PR did

support for setting TLS version annotations (tls-min-version and tls-max-version) for per-rule configurations in Higress. This allows users to override global TLS settings for specific ingress rules.

Ⅱ. Does this pull request fix one issue?

fix #1586

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Verified functionality with unit tests (TestTLSConfig) and integration tests.
Manually tested with custom ingress rules.

Ⅴ. Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented Dec 13, 2024

CLA assistant check
All committers have signed the CLA.

@@ -138,6 +138,10 @@ type AnnotationHandler interface {
RouteHandler
TrafficPolicyHandler
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is suggested to extend this implementation, rather than creating a separate file.
https://github.com/alibaba/higress/blob/main/pkg/ingress/kube/annotations/downstreamtls.go

Copy link
Author

Choose a reason for hiding this comment

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

ok,get it,i will get it a try ,thanks for your review.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for contributing

Comment on lines +33 to +34
annotationMinTLSVersion = "tls-min-version"
annotationMaxTLSVersion = "tls-max-version"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
annotationMinTLSVersion = "tls-min-version"
annotationMaxTLSVersion = "tls-max-version"
annotationMinTLSVersion = "tls-min-protocol-version"
annotationMaxTLSVersion = "tls-max-protocol-version"

case "TLSv1_3":
return networking.ServerTLSSettings_TLSV1_3
default:
return networking.ServerTLSSettings_TLS_AUTO
Copy link
Collaborator

Choose a reason for hiding this comment

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

version填写错误建议直接报错,明确的错误,好过模糊的缺省行为

Copy link
Author

Choose a reason for hiding this comment

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

agree

Comment on lines +169 to +175
case "TLSv1_0":
return networking.ServerTLSSettings_TLSV1_0
case "TLSv1_1":
return networking.ServerTLSSettings_TLSV1_1
case "TLSv1_2":
return networking.ServerTLSSettings_TLSV1_2
case "TLSv1_3":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
case "TLSv1_0":
return networking.ServerTLSSettings_TLSV1_0
case "TLSv1_1":
return networking.ServerTLSSettings_TLSV1_1
case "TLSv1_2":
return networking.ServerTLSSettings_TLSV1_2
case "TLSv1_3":
case "TLSv1.0":
return networking.ServerTLSSettings_TLSV1_0
case "TLSv1.1":
return networking.ServerTLSSettings_TLSV1_1
case "TLSv1.2":
return networking.ServerTLSSettings_TLSV1_2
case "TLSv1.3":


ruleName := getRuleName(server)

// 优先使用规则级别的TLS版本设置
Copy link
Collaborator

Choose a reason for hiding this comment

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

注释麻烦统一用英文,这样项目整体风格保持一致

@@ -52,6 +58,8 @@ func (d downstreamTLS) Parse(annotations Annotations, config *Ingress, _ *Global

downstreamTLSConfig := &DownstreamTLSConfig{
Mode: networking.ServerTLSSettings_SIMPLE,
RuleMinVersion: make(map[string]string),
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个规则的使用场景,看上去是用于对不同的 server 进行区别配置,是考虑到一个 ingress 规则下有多个域名的场景吗?
但是 server 的 name 现在应该是一个空值,是否用域名更好一些?
或者也可以考虑先不支持这个,等有用户需求再扩展,还是你们有这样的需求?

@johnlanni
Copy link
Collaborator

image
提交记录里有非github id,可以rebase再force push一下

@yunmaoQu yunmaoQu force-pushed the feature/tls-annotation branch from c25f53e to b500c32 Compare December 25, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

希望新增针对规则单独注解设置TLS版本功能
7 participants