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

[analyze] Add Analyzer for MySQL #3193

Merged
merged 7 commits into from
Sep 12, 2024

Conversation

abmussani
Copy link
Contributor

Description:

implemented analyzer interface for Mysql.

Checklist:

  • Tests passing (make test-community)?
  • Lint passing (make lint this requires golangci-lint)?

@abmussani abmussani requested a review from a team as a code owner August 7, 2024 13:07
@abmussani abmussani changed the title Add Analyzer for MySQL [analyze] Add Analyzer for MySQL Aug 12, 2024
pkg/analyzer/analyzers/mysql/mysql.go Outdated Show resolved Hide resolved
pkg/analyzer/analyzers/mysql/mysql.go Outdated Show resolved Hide resolved
pkg/analyzer/analyzers/mysql/mysql_test.go Outdated Show resolved Hide resolved
Comment on lines +31 to +18
//go:embed expected_output.json
var expectedOutput []byte
Copy link
Collaborator

Choose a reason for hiding this comment

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

Neat!

@mcastorina
Copy link
Collaborator

This is also missing the enumerated permissions and generated types.

@abmussani abmussani requested a review from mcastorina August 16, 2024 14:35
@abmussani
Copy link
Contributor Author

This is also missing the enumerated permissions and generated types.

Add the enumerated permission types.

@abmussani abmussani force-pushed the impl-data-model-mysql branch from 00fa5b2 to 44c2bc6 Compare August 28, 2024 15:11
for _, database := range info.Databases {
dbResource := analyzers.Resource{
Name: database.Name,
FullyQualifiedName: database.Name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the database name include the hostname? I think this needs to be a bit more unique..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included the hostname in database resources FQN.

for _, table := range *database.Tables {
tableResource := analyzers.Resource{
Name: table.Name,
FullyQualifiedName: table.Name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the fully qualified name should probably be <db name> / <table name>. It's supposed to be a unique identifier for this resource, right? Maybe it even needs the hostname too 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, I included the hostname & database name in tableresources FQN.

@abmussani abmussani force-pushed the impl-data-model-mysql branch from 44c2bc6 to e00c8dd Compare September 9, 2024 16:05
@abmussani
Copy link
Contributor Author

@mcastorina I have also simplified the test for mysql.

@mcastorina mcastorina merged commit b0318a9 into trufflesecurity:main Sep 12, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants