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

catalog: compare node names case insensitively in more places #12444

Merged
merged 8 commits into from
Feb 24, 2022

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Feb 24, 2022

Many places in consul already treated node names case insensitively.
The state store indexes already do it, but there are a few places that
did a direct byte comparison which have now been corrected.

One place of particular consideration is ensureCheckIfNodeMatches
which is executed during snapshot restore (among other places). If a
node check used a slightly different casing than the casing of the node
during register then the snapshot restore here would deterministically
fail. This has been fixed.

Primary approach:

git grep -i "node.*[!=]=.*node" -- ':!*_test.go' ':!docs'
git grep -i '\[[^]]*member[^]]*\]
git grep -i '\[[^]]*\(member\|name\|node\)[^]]*\]' -- ':!*_test.go' ':!website' ':!ui' ':!agent/proxycfg/testing.go:' ':!*.md'

Many places in consul already treated node names case insensitively.
The state store indexes already do it, but there are a few places that
did a direct byte comparison which have now been corrected.

One place of particular consideration is ensureCheckIfNodeMatches
which is executed during snapshot restore (among other places). If a
node check used a slightly different casing than the casing of the node
during register then the snapshot restore here would deterministically
fail. This has been fixed.

Primary approach:

    git grep -i "node.*[!=]=.*node" -- ':!*_test.go' ':!docs'
    git grep -i '\[[^]]*member[^]]*\]
    git grep -i '\[[^]]*\(member\|name\|node\)[^]]*\]' -- ':!*_test.go' ':!website' ':!ui' ':!agent/proxycfg/testing.go:' ':!*.md'
@rboyer rboyer requested review from mkeeler and a team February 24, 2022 21:05
@rboyer rboyer self-assigned this Feb 24, 2022
@github-actions github-actions bot added the theme/cli Flags and documentation for the CLI interface label Feb 24, 2022
@vercel vercel bot temporarily deployed to Preview – consul February 24, 2022 21:07 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 24, 2022 21:07 Inactive
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

I think I found a couple more places that could use tests.

The isSerfMember in agent/consul/util.go could potentially as well but we don't test that method at all right now.

.changelog/12444.txt Outdated Show resolved Hide resolved
agent/structs/structs.go Show resolved Hide resolved
command/rtt/rtt.go Show resolved Hide resolved
Co-authored-by: Matt Keeler <mkeeler@users.noreply.github.com>
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 24, 2022 21:20 Inactive
@vercel vercel bot temporarily deployed to Preview – consul February 24, 2022 21:20 Inactive
@rboyer rboyer changed the title compare node names case insensitively in more places catalog: compare node names case insensitively in more places Feb 24, 2022
@vercel vercel bot temporarily deployed to Preview – consul February 24, 2022 21:45 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 24, 2022 21:45 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 24, 2022 21:51 Inactive
@vercel vercel bot temporarily deployed to Preview – consul February 24, 2022 21:51 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 24, 2022 22:04 Inactive
@vercel vercel bot temporarily deployed to Preview – consul February 24, 2022 22:04 Inactive
@vercel vercel bot temporarily deployed to Preview – consul February 24, 2022 22:11 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 24, 2022 22:11 Inactive
@vercel vercel bot temporarily deployed to Preview – consul February 24, 2022 22:28 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 24, 2022 22:28 Inactive
@rboyer rboyer requested a review from mkeeler February 24, 2022 22:28
@rboyer rboyer merged commit 9571464 into main Feb 24, 2022
@rboyer rboyer deleted the case-insensitive-node-names branch February 24, 2022 22:54
@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/595905.

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

🍒✅ Cherry pick of commit 9571464 onto release/1.11.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Feb 24, 2022
Many places in consul already treated node names case insensitively.
The state store indexes already do it, but there are a few places that
did a direct byte comparison which have now been corrected.

One place of particular consideration is ensureCheckIfNodeMatches
which is executed during snapshot restore (among other places). If a
node check used a slightly different casing than the casing of the node
during register then the snapshot restore here would deterministically
fail. This has been fixed.

Primary approach:

    git grep -i "node.*[!=]=.*node" -- ':!*_test.go' ':!docs'
    git grep -i '\[[^]]*member[^]]*\]
    git grep -i '\[[^]]*\(member\|name\|node\)[^]]*\]' -- ':!*_test.go' ':!website' ':!ui' ':!agent/proxycfg/testing.go:' ':!*.md'
rboyer added a commit that referenced this pull request Feb 28, 2022
Extension of #12444 but with the slightly backwards incompatible change
of having node and agent resources be case insensitive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/cli Flags and documentation for the CLI interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants