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

Port cibadmin gatherer #149

Merged
merged 4 commits into from
Dec 16, 2022
Merged

Port cibadmin gatherer #149

merged 4 commits into from
Dec 16, 2022

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Dec 2, 2022

cibadmin gatherer. It tranfsorms the xml output of cibadmin command and it converts it to a map, so we can access all the fields.
As xml->json transformation cannot be done without having real xml struct domain data, I needed to transform some well done values always to list (primitive, master, etc). These values are lists, but the mxj cannot know that, if we only have 1 entry. That's why we convert them.

Usage:

./trento-agent facts gather --gatherer cibadmin --argument cib.configuration.resources.master.primitive.instance_attributes.nvpair.0.value

...

INFO[2022-12-02 12:31:53] Gathered fact for "cibadmin" with argument "cib.configuration.resources.master.0.primitive.0.instance_attributes.nvpair.0.value": 
INFO[2022-12-02 12:31:53] {
  "Name": "cib.configuration.resources.master.0.primitive.0.instance_attributes.nvpair.0.value",
  "CheckID": "",
  "Value": {
    "Value": "PRD"
  },
  "Error": null
}

@arbulu89 arbulu89 force-pushed the port-cibadmin-gatherer branch from 7a275da to 719f4a7 Compare December 2, 2022 12:34
@fabriziosestito fabriziosestito marked this pull request as ready for review December 7, 2022 07:39
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Looks good to me.

The only doubt that I have is that current checks are using cibadmin -Q --xpath WHATEVER rather than cibadmin --query --local.
Maybe they are the same, or we can achieve the same result, and I am missing something, though 😅

pkg/factsengine/entities/fact_value_test.go Outdated Show resolved Hide resolved
Comment on lines +13 to +24
// nolint:gochecknoglobals
var (
CibAdminCommandError = entities.FactGatheringError{
Type: "cibadmin-command-error",
Message: "error running cibadmin command",
}

CibAdminDecodingError = entities.FactGatheringError{
Type: "cibadmin-decoding-error",
Message: "error decoding cibadmin output",
}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

@CDimonaco are you aware of some other way to declare this stuff both cleanly and without pissing off the linter? I don't like globals, but I have to say I like what @arbulu89 did here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In any case, is not something I made up here. We have been using this way in all the gatherers since we started the porting

@arbulu89
Copy link
Contributor Author

@nelsonkopliku

The only doubt that I have is that current checks are using cibadmin -Q --xpath WHATEVER rather than cibadmin --query --local.

This is because we won't use xpath anymore. xpath is used to get specific values from a xml, but now, as the code converts the xml to a map, we can access it with the normal dot access way. xpath has some limitations to get complex values and it would be not usable in rhai.
I will document it in the gatherers docs.

About the local, is is a common thing to do when you are simply querying things in the local node.

@arbulu89 arbulu89 force-pushed the port-cibadmin-gatherer branch from 719f4a7 to 11c5bff Compare December 15, 2022 08:35
@arbulu89 arbulu89 force-pushed the port-cibadmin-gatherer branch from e0e82c7 to ae041b6 Compare December 16, 2022 13:11
@arbulu89
Copy link
Contributor Author

arbulu89 commented Dec 16, 2022

Hey @fabriziosestito ,
I have done the NewFactValue more robust now. Based on: https://github.com/protocolbuffers/protobuf-go/blob/6875c3d7242d1a3db910ce8a504f124cb840c23a/types/known/structpb/struct.pb.go#L316

You can actually put many entries in the same case line

@arbulu89
Copy link
Contributor Author

arbulu89 commented Dec 16, 2022

As an improvement of the casting to string the next code would work:

        case bool:
		return &FactValueBool{Value: value}, nil
	case int:
		return &FactValueInt{Value: value}, nil
	case int32:
		return &FactValueInt{Value: int(value)}, nil
	case int64:
		return &FactValueInt{Value: int(value)}, nil
	case uint:
		return &FactValueInt{Value: int(value)}, nil
	case uint32:
		return &FactValueInt{Value: int(value)}, nil
	case uint64:
		return &FactValueInt{Value: int(value)}, nil
	case float32:
		return &FactValueFloat{Value: float64(value)}, nil
	case float64:
		return &FactValueFloat{Value: value}, nil
	case string:
		return ParseStringToFactValue(value), nil

But in the current scenario I prefer a bit less efficient code which is easier to follow.
We can change it the future if we need more customized conversion

@arbulu89 arbulu89 merged commit 1b38255 into main Dec 16, 2022
@arbulu89 arbulu89 deleted the port-cibadmin-gatherer branch December 16, 2022 15:00
@arbulu89 arbulu89 added the enhancement New feature or request label Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants