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

proxycfg: Ensure that endpoints for explicit upstreams in other datacenters are watched in transparent mode #10391

Merged
merged 2 commits into from
Jun 15, 2021

Conversation

ndhanushkodi
Copy link
Contributor

@ndhanushkodi ndhanushkodi commented Jun 14, 2021

Co-authored-by: Freddy Vallenilla freddy@hashicorp.com

Other Notes:

  • pulled out the fmt.Sprintf("%s?dc=dc2", n.String()) into a function in the test file
  • ran consul-helm acceptance tests with this image, and the OSS tests pass.

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

It looks like CI did not run on this PR, I think you may need to re-authenticate with CircleCI to have them run.

Comment on lines 130 to 132
func (n ServiceName) DiscoveryChainID() string {
return fmt.Sprintf("%s?dc=dc2", n.String())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to only be used by tests, so would probably be better as a function in state_test.go instead of a method on ServiceName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @dnephin! I've moved it to the test file and left the hardcoding of the dc and named the function appropriately since it's the only case the function is used for. Let me know if you'd prefer a different style.

@ndhanushkodi ndhanushkodi reopened this Jun 14, 2021
@ndhanushkodi ndhanushkodi reopened this Jun 14, 2021
@ndhanushkodi ndhanushkodi reopened this Jun 14, 2021
@vercel vercel bot temporarily deployed to Preview – consul June 14, 2021 23:56 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging June 14, 2021 23:56 Inactive
@ndhanushkodi ndhanushkodi changed the title Explicit upstreams in other dcs shouldn't be deleted proxycfg: explicit upstreams in other dcs shouldn't be deleted Jun 15, 2021
@freddygv freddygv added this to the 1.10.0 milestone Jun 15, 2021
when they're not in the seen services list of explicit and implicit
upstreams within a datacenter.

Co-authored-by: Freddy Vallenilla <freddy@hashicorp.com>
@vercel vercel bot temporarily deployed to Preview – consul June 15, 2021 00:43 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging June 15, 2021 00:43 Inactive
Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Nice! I'm not familiar enough with tproxy to understand the underlying bug, but I trust you and Freddy understand it well.

The code change LGTM, but one suggestion for the changelog entry (not blocking a merge).

.changelog/10391.txt Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging June 15, 2021 17:53 Inactive
@vercel vercel bot temporarily deployed to Preview – consul June 15, 2021 17:53 Inactive
@ndhanushkodi ndhanushkodi merged commit b8b4441 into master Jun 15, 2021
@ndhanushkodi ndhanushkodi deleted the multidc-explicit branch June 15, 2021 18:00
@ndhanushkodi ndhanushkodi changed the title proxycfg: explicit upstreams in other dcs shouldn't be deleted proxycfg: Ensure that endpoints for explicit upstreams in other datacenters are watched in transparent mode Jun 15, 2021
@hc-github-team-consul-core
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/387160.

@hc-github-team-consul-core
Copy link
Contributor

🍒✅ Cherry pick of commit b8b4441 onto release/1.10.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Jun 15, 2021
…enters are watched in transparent mode (#10391)

Co-authored-by: Freddy Vallenilla <freddy@hashicorp.com>
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.

4 participants