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

CIS 6.2.1 Ensure that the 'log_checkpoints' database flag for Cloud SQL PostgreSQL instance is set to 'on' #410

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

palani-ram-google-partner
Copy link
Contributor

CIS 6.2.1 Ensure that the 'log_checkpoints' database flag for Cloud SQL PostgreSQL instance is set to 'on'

@palani-ram-google-partner
Copy link
Contributor Author

@morgante Please review this PR.

@palani-ram-google-partner
Copy link
Contributor Author

@morgante Checked the CFT scorecard too for this policy. All works fine. Please review this PR.

@morgante
Copy link
Contributor

@palani-ram-google-partner Before we merge this, can you fix your previous two PRs to make sure they work in CFT scorecard?

template_name := "GCPPostgreSQLCheckpointsConstraintV1"

#1. postgresql with correct key
test_postgresql_log_checkpoints_no_violations {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test / fixture is confusingly named and I'm not sure if the logic is right. If it's meant to have no violations, you should not be expecting a violation. Please update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morgante I have fixed the naming for the correct key we are checking.

settings_databaseflags(settings) {
setdata := settings.databaseFlags[_]
setdata.name == "log_checkpoints"
setdata.value != "on"
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't you looking for cases where it is on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morgante I am looking for the case where it is ON condition. Fixed it for the corresponding key value.

@palani-ram-google-partner
Copy link
Contributor Author

@morgante Changed the required files and CFT scorecard is working fine too.Please review this PR.


#1. postgresql with correct key
test_postgresql_log_checkpoints_with_correctkey {
expected_resource_names := {"//cloudsql.googleapis.com/projects/my-test-project/instances/tf-pg-ha-62380f9c-no-violation"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you expecting this fixture to raise a violation? If so, it should not be called "with_correctkey" and you definitely should not expect a resource called "no-violation" to raise a violation. Either your naming or logic is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morgante This case(postgresql with correct key) don't raise a violation in our case. So I have removed the test case since it's not needed. Violated test cases alone is attached. Hope this is Ok. Please review it.

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Please provide a test case which doesn't violate.

@palani-ram-google-partner
Copy link
Contributor Author

@morgante Added the test case with correct naming.


#1. postgresql with correct key
test_postgresql_log_checkpoints_required_databaseflag {
expected_resource_names := {"//cloudsql.googleapis.com/projects/my-test-project/instances/tf-pg-ha-62380f9c-required"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this is still wrong.
check_test_violations_count is asserting how many violations a particular fixture (data set) should raise. According to the naming of your fixture, it should NOT be violating.

Yet it is violating, since you:
(a) Are expecting a violation (count = 1).
(b) Have the name of the supposedly correct resource in the expected_resource_names. expected_resource_names is the list of resources you expect to raise a violation. A correct resource should never be found here.

It seems like your logic and tests are likely both broken.

Please read through the full test utils and look through other tests. It's important that you get this right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants