-
Notifications
You must be signed in to change notification settings - Fork 114
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
set manager to "chef" on chef managed nodes #3840
Conversation
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
@@ -90,6 +90,10 @@ func gatherInfoForNode(in message.Compliance) (*manager.NodeMetadata, error) { | |||
tags = append(tags, &common.Kv{Key: "environment", Value: in.Report.GetEnvironment()}) | |||
} | |||
|
|||
mgrType := in.Report.GetAutomateManagerType() | |||
if in.Report.GetSourceFqdn() != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there's a value for the chef server, we know the node is managed by chef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the value is "localhost" it is not managed by a chef server it is an effortless or chef solo report.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yaa, i was going with the idea that that node is still "managed" by chef in the sense that chef has converged on the node at least once/chef-client is on the node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whaddya think? should we distinguish between chef-server managed and just chef solo/effortless?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should for now. But it looks like could when and if we want to.
@@ -127,20 +127,12 @@ func (db *DB) ProcessIncomingNode(node *manager.NodeMetadata) error { | |||
return errors.Wrap(err, "ProcessIncomingNode unable to marshal projects data") | |||
} | |||
|
|||
var mgrType string | |||
if node.ManagerId != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the db call we can now remove since we have the type
} | ||
} | ||
} | ||
} | ||
return nil | ||
} | ||
|
||
func isValidManagerType(managerType string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we had two functions basically looking at the same thing and i kept getting tripped up on it. figured it's better to just use one!
@@ -690,7 +690,7 @@ func validateNodeFilters(filters []*common.Filter) error { | |||
func isValidNodeManagerType(item string) bool { | |||
switch item { | |||
// empty string covers legacy nodes (nodes created before we started associating all manual nodes with "automate" as a manager and nodes brought over from a1) | |||
case "aws-ec2", "aws-api", "azure-api", "azure-vm", "automate", "": | |||
case "aws-ec2", "aws-api", "azure-api", "azure-vm", "automate", "gcp-api", "chef", "": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops! we didn't have gcp-api listed as valid! it has been a valid option for a long time
@@ -45,7 +45,7 @@ export class ScannerEffects { | |||
const nodesStatusFilter = { key: 'status', values: ['reachable', 'unreachable', 'unknown'] }; | |||
// ensures that we only collect nodes that belong to a manager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is stale now because there are no nodes that don't have a manager now.
if node.ManagerId != "" { | ||
mgrType, err = db.SelectStr(sqlGetManagerTypeFromId, node.ManagerId) | ||
if err != nil { | ||
logrus.Warnf("unable to find manager for node") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we still warn here if node.GetManagerType()
is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, good call ya
if !isValidManagerType(item) { | ||
return &errorutils.InvalidError{Msg: fmt.Sprintf("Invalid manager type filter: %s. manager_type must be one of the following: 'aws-ec2', 'aws-api', 'azure-api'", item)} | ||
if !isValidNodeManagerType(item) { | ||
return &errorutils.InvalidError{Msg: fmt.Sprintf("Invalid manager type filter: %s. manager_type must be one of the following: 'aws-ec2', 'aws-api', 'azure-api', 'chef', '', 'azure-vm', 'gcp-api' ", item)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the manager type can still be empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no new ones should be empty, but we need to keep this around for the old nodes. they should get updated once a new run/scan comes thru but if there are no new runs/scans for the node it will be stuck in the empty manager state
@@ -0,0 +1,38 @@ | |||
import { uuidv4 } from '../../../support/helpers'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all your helper functions/the way you wrote the tests made it SO easy to write this one up, thank you!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good. I just go hung up on after running "chef_load_compliance_nodes" and getting errors from the #3724 problem.
Signed-off-by: Victoria Jeffrey <[email protected]>
🔩 Description: What code changed, and why?
this change adds the manager type to the nodemanager process node flow, meaning
we can assign any node which is coming through ingest service the manager "chef" since we know that node is managed by chef
any node that is automate or aws-ec2 or azure-api etc will get the type on the processnode message, meaning that we dont have to look up the type of the manager based on the id (save an extra db call)
⛓️ Related Resources
#3515
👍 Definition of Done
manager type is on process node message
nodes managed by chef are labelled as such
👟 How to Build and Test the Change
rebuild compliance, ingest, nodemanager
load some nodes (chef_load_nodes, load_compliance_reports, chef_load_compliance_nodes, load_scan_jobs)
see the manager type for the nodes is correct!