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

Support mixed case consul service tags on consul storage engine #6483

Merged
merged 12 commits into from
Jul 25, 2023

Conversation

ncabatoff
Copy link
Collaborator

When support for service tags was added, the only way we had to parse
and dedup a list of strings also forced them to be lowercase. Now there's
another helper func that doesn't smash the case so use that instead.

Fixes #6427.

@calvn
Copy link
Contributor

calvn commented Mar 28, 2019

Are there any backward-compat implications in here? strutil. ParseDedupAndSortStrings would not dedup foo and Foo and will show as two separate service tags after this change instead of just foo.

This might be ok, since the "old" tag is still present (in addition to the new one that would no longer be dedupped), just something worth noting since it's a behavioral change.

@jefferai
Copy link
Member

When support for service tags was added, the only way we had to parse
and dedup a list of strings also forced them to be lowercase.

I don't think that's the root of the problem, I think it's that when it was coded (by a Consul/TFE dev who contributed this to us) they assumed service tags would always be lowercase.

@jefferai jefferai added this to the 1.2 milestone May 23, 2019
@chrishoffman chrishoffman modified the milestones: 1.2, 1.4 Oct 23, 2019
@catsby
Copy link
Contributor

catsby commented Nov 22, 2019

Are there any backward-compat implications in here? strutil. ParseDedupAndSortStrings would not dedup foo and Foo and will show as two separate service tags after this change instead of just foo.

Is that a mis-configuration that we are incorrectly de-duping into a single tag today?

@tyrannosaurus-becks
Copy link
Contributor

tyrannosaurus-becks commented Feb 11, 2020

In the ldaputil, there's a field called "case_sensitive_names". I wonder if we could do something here to make this backwards compatible but support both use cases. Perhaps it could be called something like "case_sensitive_service_tags" and default to true.

@pbernal pbernal modified the milestones: 1.4, 1.5 Feb 20, 2020
@pbernal pbernal modified the milestones: 1.5, 1.6 Jun 26, 2020
@vishalnayak vishalnayak modified the milestones: 1.6, 1.6.1 Oct 26, 2020
@ncabatoff ncabatoff modified the milestones: 1.6.1, 1.6.2 Dec 14, 2020
@mladlow mladlow removed this from the 1.6.2 milestone Jan 20, 2021
@VioletHynes VioletHynes added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jul 6, 2023
@banks banks mentioned this pull request Jul 14, 2023
@banks
Copy link
Member

banks commented Jul 14, 2023

I've asked a question on the issue to see if this is still something pressing enough to address (at least the original case folks were running into with Traeffik may no longer be an issue according to their docs at least).

We can decide if we want to merge this anyway - if it's not actively impacting anyone any more we could leave that, but I do think it's better to not impose that restriction on case here in general.

Personally I feel like it would be sufficient to just make this change and note it in the upgrade docs. My feeling (which is 100% pure wild speculation) is that it's probably unlikely enough to cause a major issue for users relying on their supplying upper case tags and having them converted to lower 🤷 . It certainly feels disproportionate to go through a lot more bother of making it backwards compatible not to mention the long-term added complexity of supporting optional modes.

My two cents anyway!

@banks
Copy link
Member

banks commented Jul 21, 2023

Seems from the issue this is still a blocker for Correct Traefik routing at least. I think we just need to note the behaviour change in upgrade docs and merge.

Anyone disagree?

and dedup a list of strings also forced them to be lowercase.  Now there's
another helper func that doesn't smash the case so use that instead.
@peteski22 peteski22 requested a review from a team July 25, 2023 14:04
@peteski22 peteski22 requested a review from a team as a code owner July 25, 2023 14:05
@peteski22 peteski22 requested a review from a team July 25, 2023 14:05
@peteski22 peteski22 requested a review from a team as a code owner July 25, 2023 14:05
@peteski22 peteski22 requested a review from a team July 25, 2023 14:05
@github-actions
Copy link

CI Results:
All Go tests succeeded! ✅

@peteski22 peteski22 force-pushed the allow-uppercase-consul-service-tags branch from f0b71da to 777dc08 Compare July 25, 2023 15:06
@hashicorp hashicorp deleted a comment from digital-content-events bot Jul 25, 2023
changelog/6483.txt Outdated Show resolved Hide resolved
Co-authored-by: Nick Cabatoff <ncabatoff@hashicorp.com>
@peteski22 peteski22 enabled auto-merge (squash) July 25, 2023 20:10
@peteski22 peteski22 merged commit 64b50ad into main Jul 25, 2023
@peteski22 peteski22 deleted the allow-uppercase-consul-service-tags branch July 25, 2023 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed service-discovery/consul
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storage consul: upper case letters in service_tags are lowered