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.

Sign up for GitHub

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

Jump to bottom

fingerprint: update consul fingerprinter with additional attributes #10699

Merged
merged 5 commits into from
Jun 3, 2021
Merged

fingerprint: update consul fingerprinter with additional attributes #10699

merged 5 commits into from
Jun 3, 2021

Conversation

shoenig
Copy link
Member

@shoenig shoenig commented Jun 3, 2021 •
edited
Loading

0479167

    client/fingerprint/consul: refactor the consul fingerprinter to test individual attributes
    
    This PR refactors the ConsulFingerprint implementation, breaking individual attributes
    into individual functions to make testing them easier. This is in preparation for
    additional extractors about to be added. Behavior should be otherwise unchanged.
    
    It adds the attribute consul.sku, which can be used to differentiate between Consul
    OSS vs Consul ENT.

b3254f6

    client/fingerprint/consul: add new attributes to consul fingerprinter
    
    This PR adds new probes for detecting these new Consul related attributes:
    
    consul.ft.namespaces

    Consul namespaces are a Consul enterprise feature that may be disabled depending
    on the enterprise license associated with the Consul servers. Having this attribute
    available will enable Nomad to properly decide whether to query the Consul Namespace
    API.
    
    consul.connect

    Consul connect must be explicitly enabled before Connect APIs will work. Currently
    Nomad only checks for a minimum Consul version. Having this attribute available will
    enable Nomad to properly schedule Connect tasks only on nodes with a Consul agent that
    has Connect enabled.
    
    consul.grpc

    Consul connect requires the grpc port to be explicitly set before Connect APIs will work.
    Currently Nomad only checks for a minimal Consul version. Having this attribute available
    will enable Nomad to schedule Connect tasks only on nodes with a Consul agent that has
    the grpc listener enabled.

Closes #10688

@shoenig
client/fingerprint/consul: refactor the consul fingerprinter to test …
0479167 
…individual attributes

This PR refactors the ConsulFingerprint implementation, breaking individual attributes
into individual functions to make testing them easier. This is in preparation for
additional extractors about to be added. Behavior should be otherwise unchanged.

It adds the attribute consul.sku, which can be used to differentiate between Consul
OSS vs Consul ENT.
@shoenig
client/fingerprint/consul: add new attributes to consul fingerprinter
b3254f6 
This PR adds new probes for detecting these new Consul related attributes:

Consul namespaces are a Consul enterprise feature that may be disabled depending
on the enterprise license associated with the Consul servers. Having this attribute
available will enable Nomad to properly decide whether to query the Consul Namespace
API.

Consul connect must be explicitly enabled before Connect APIs will work. Currently
Nomad only checks for a minimum Consul version. Having this attribute available will
enable Nomad to properly schedule Connect tasks only on nodes with a Consul agent that
has Connect enabled.

Consul connect requires the grpc port to be explicitly set before Connect APIs will work.
Currently Nomad only checks for a minimal Consul version. Having this attribute available
will enable Nomad to schedule Connect tasks only on nodes with a Consul agent that has
the grpc listener enabled.
@shoenig shoenig marked this pull request as draft June 3, 2021 17:57
@vercel vercel bot temporarily deployed to Preview – nomad June 3, 2021 17:58 Inactive
@shoenig shoenig mentioned this pull request Jun 3, 2021
Closed
@shoenig shoenig marked this pull request as ready for review June 3, 2021 18:51
@shoenig shoenig mentioned this pull request Jun 3, 2021
Merged
tgross
tgross approved these changes Jun 3, 2021
View reviewed changes
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for splitting out the refactoring and the new work commits, too, that made it a lot easier to get my head around for review.

CHANGELOG.md Outdated Show resolved Hide resolved
client/fingerprint/consul_test.go

t.Run("grpc set", func(t *testing.T) {
s, ok := fp.grpc(consulInfo{
"DebugConfig": {"GRPCPort": 8502.0}, // JSON numbers are floats
Copy link
Member

Choose a reason for hiding this comment

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

// JSON numbers are floats

sigh 😀

client/fingerprint/consul_test.go
@@ -1,7 +1,8 @@
package fingerprint
Copy link
Member

Choose a reason for hiding this comment

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

The test refactoring you've done here is really nice 👍

client/fingerprint/consul.go
// indicate the Agent is available now
if f.lastState == consulUnavailable {
f.logger.Info("consul agent is available")
func (f *ConsulFingerprint) server(info consulInfo) (string, bool) {
Copy link
Member

Choose a reason for hiding this comment

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

I love how legible this entire ConsulFingerprint becomes with these extractor functions.

This refactoring does make me realize that if Consul were to push a backwards incompatible JSON we could potentially panic Nomad when we do the type cast. Probably something for future robustness work to worry about rather than this PR though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we're actually okay (for panics) on type changes - each of these type casts is in the context of a type check which will simply become false. The attributes will end up being removed which isn't great, of course, and I'll probably have a few choice words with Consul team 😅

@shoenig @tgross
Apply suggestions from code review
b35fde4 
Co-authored-by: Tim Gross <tgross@hashicorp.com>
@vercel vercel bot temporarily deployed to Preview – nomad June 3, 2021 20:12 Inactive
@shoenig shoenig merged commit 24e963f into main Jun 3, 2021
@shoenig shoenig deleted the f-consul-fp branch June 3, 2021 20:14
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tgross tgross tgross approved these changes

@isabeldepapel isabeldepapel Awaiting requested review from isabeldepapel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve consul fingerprinting
2 participants
@shoenig @tgross