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

fix: when there is a non-ip IPAddr in Eureka, delete it to avoid a failure in EDS #1322

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lcfang
Copy link

@lcfang lcfang commented Sep 18, 2024

Ⅰ. Describe what this PR did

当eureka中注册的服务IPAddr为域名而非ip时,会导致其他EDS下发失败,故打印waring并忽略该应用

Ⅱ. Does this pull request fix one issue?

fixes #1284
fixes #1299

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

Ⅳ. Describe how to verify it

在启动higress之前往eureka注册中心注册一个服务,ipaddr为域名而非ip,修复前查看配置下发是失败的,修复后不报错,controller中有waring日志

Ⅴ. Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented Sep 18, 2024

CLA assistant check
All committers have signed the CLA.

@lcfang
Copy link
Author

lcfang commented Sep 19, 2024

break
}
}
if shouldDelete == true {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这种 == true 是不是就不用写了。。。

Copy link
Author

Choose a reason for hiding this comment

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

这个确实不需要了。。


var (
keysToDelete []string
shouldDelete bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个变量是不是没必要放这么外面呀,留在for里面就行了

Copy link
Author

Choose a reason for hiding this comment

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

这个要在for循环外面使用,要放在外面吧

Copy link
Collaborator

Choose a reason for hiding this comment

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

我是说 shouldDelete。。。

for name, app := range apps {
shouldDelete = false
for _, instant := range app.Instances {
Copy link
Collaborator

Choose a reason for hiding this comment

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

instant?

Copy link
Author

Choose a reason for hiding this comment

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

instance,我改一下

@@ -128,14 +129,34 @@ func (c *eurekaHttpClient) getApplications(path string) (*Applications, error) {
app := rj.Response.Applications[idx]
apps[app.Name] = app
Copy link
Collaborator

Choose a reason for hiding this comment

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

看了一下,是不是直接把下面的逻辑放到这个循环里就行了。。。

Copy link
Author

Choose a reason for hiding this comment

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

对!

Copy link
Collaborator

@CH3CHO CH3CHO left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.21%. Comparing base (ef31e09) to head (1b0b844).
Report is 123 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1322      +/-   ##
==========================================
+ Coverage   35.91%   44.21%   +8.30%     
==========================================
  Files          69       76       +7     
  Lines       11576     9895    -1681     
==========================================
+ Hits         4157     4375     +218     
+ Misses       7104     5184    -1920     
- Partials      315      336      +21     

see 91 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants