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

Consul nameresolution resolves conflicts #2489

Closed
alberthuang24 opened this issue Feb 1, 2023 · 6 comments · Fixed by #2980
Closed

Consul nameresolution resolves conflicts #2489

alberthuang24 opened this issue Feb 1, 2023 · 6 comments · Fixed by #2980
Assignees
Labels
kind/bug Something isn't working P2 size/S 1 week of work triaged/resolved
Milestone

Comments

@alberthuang24
Copy link
Contributor

alberthuang24 commented Feb 1, 2023

Consul does not automatically delete or overwrite service registrations. When services service-a and service-b exist and the following behavior occurs.
A parsing error occurs

service-a = 192.168.1.2:3500

service-b = 192.168.1.3:3500

At this point consul has the following parsing

service-a -> 192.168.1.2:3500 (health check passing)

service-b -> 192.168.1.3:3500(Health check passing)

Restart both services and reassign service ip

service-a -> 192.168.1.4:3500

service-b -> 192.168.1.2:3500

At this point consul has the following resolution

service-a -> 192.168.1.4:3500 (health check passing)
service-a -> 192.168.1.2:3500(Health check passing)

service-b -> 192.168.1.2:3500(Health check passing)
service-b -> 192.168.1.3:3500(Health check failed)

Now when we access service-a there is a chance that it will resolve to 192.168.1.2:3500 resulting in the server actually accessing service-b

Expected Behavior

Actual Behavior

Steps to Reproduce the Problem

Release Note

RELEASE NOTE: Fix consul name-resolution resolves conflicts

@alberthuang24 alberthuang24 added the kind/bug Something isn't working label Feb 1, 2023
@alberthuang24
Copy link
Contributor Author

alberthuang24 commented Feb 1, 2023

One of the solutions I can think of is

Add the appId query to check.

		cfg.Checks = []*consul.AgentServiceCheck{
			{
				Name: "Dapr Health Status",
				CheckID: fmt.Sprintf("daprHealth:%s", id),
				Interval: "15s",
				HTTP: fmt.Sprintf("http://%s/v1.0/healthz?appId=appID", net.JoinHostPort(host, httpPort)),
			},
		}

Then in sidercar if the appId exists then match the appId consistently, otherwise return 403.

This allows consul to assume the check is invalid and not find the record

alberthuang24 added a commit to TheLandGame/dapr that referenced this issue Feb 1, 2023
alberthuang24 added a commit to TheLandGame/components-contrib that referenced this issue Feb 1, 2023
alberthuang24 added a commit to TheLandGame/components-contrib that referenced this issue Feb 1, 2023
alberthuang24 added a commit to TheLandGame/dapr that referenced this issue Feb 1, 2023
alberthuang24 added a commit to TheLandGame/components-contrib that referenced this issue Feb 1, 2023
@ItalyPaleAle
Copy link
Contributor

Could we make the consul resolver delete the registration on shutdown?

@alberthuang24
Copy link
Contributor Author

This is also an idea, but it seems difficult to prevent accidental shutdowns that lead to unsuccessful deletions.
For example, if the aws spot instance expires

@ItalyPaleAle
Copy link
Contributor

ItalyPaleAle commented Feb 1, 2023

What about pairing that (de-registering on shutdown) with a check at startup to see if the old IP is registered and replace that?

@berndverst
Copy link
Member

This sounds a lot like the mDNS cache problem #2413

@berndverst berndverst added this to the v1.11 milestone Feb 1, 2023
@alberthuang24
Copy link
Contributor Author

What about pairing that (de-registering on shutdown) with a check at startup to see if the old IP is registered and replace that?

It seems difficult to do if a service has multiple instances

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working P2 size/S 1 week of work triaged/resolved
Projects
None yet
3 participants