-
Notifications
You must be signed in to change notification settings - Fork 685
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
OCP4: Add rule to check ACS sensor deployed #11675
Conversation
🤖 A k8s content image for this PR is available at: Click here to see how to deploy itIf you alread have Compliance Operator deployed: Otherwise deploy the content and operator together by checking out ComplianceAsCode/compliance-operator and: |
1a4b1b8
to
5105016
Compare
/test |
@Vincent056: The
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test e2e-aws-ocp4-pci-dss |
@@ -0,0 +1,2 @@ | |||
--- | |||
default_result: FAIL |
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 goes back to @yuumasato's comment, but now that we have a manual remediation, we should be testing that it works in the second scan, right?
|
||
ocil_clause: 'ACS Sensor is not deployed' | ||
|
||
{{% set jqfilter = '[ .items[] | select(.metadata.name == "sensor" and .metadata.labels["app.kubernetes.io/name"] == "stackrox") | .status.availableReplicas]' %}} |
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.
Looks reasonable so long as it's safe to rely on sensor
as the deployment name and stackrox
as the label.
@dashrews78 does this look ok to 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.
on the surface looks OK. There may be another caveat though. Just because sensor is deployed doesn't necessarily mean it is connected to a central. But that may be a more advanced workload type check.
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.
do you know where I can check so we know it is being connected to the central?
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.
Luis could answer this, I've sent him this conversation.
My gut says it is sufficient to look into the Deployment status. A non-connecting sensor eventually will go to CrashLoopBackoff.
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 ROX_PREVENT_SENSOR_RESTART_ON_DISCONNECT
is enabled (which it should be by default), sensor should not go to CrashLoopBackoff
. The only way to know for sure if sensor is connected with central is by checking logs, metrics, or central's UI.
I'm lacking some context here but I would say knowing if sensor is connected is not necessary because even if sensor is not connected to central, if sensor is deployed, it should flush all the resources to central upon reconnection.
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.
thanks for the input, I wonder if there is a way we can find it out through any of the kube api resources? metrics might not be easily accessed by CO at the moment.
I don't think this tested anything new from the patch since we're not including the new rule in a profile, yet. |
Add rule acs_sensor_exists, so we can check that ACS sensor is being deployed on the cluster
@Vincent056: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Code Climate has analyzed commit 397a3e6 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 59.3% (0.0% change). View more on Code Climate. |
Verification failed with 4.16.0-0.nightly-2024-03-31-180021 + compliance-operator.v1.4.0 + rhacs-operator.v4.4.0
Observation 1:
Observation 2: |
Verification passed with 4.16.0-0.nightly-2024-04-01-213440 + compliance-operator.v1.4.0 + rhacs-operator.v4.4.0
|
/unhold |
/lgtm |
runtime violations based on RHACS Cloud Service policies. In addition, Sensor is | ||
responsible for all cluster interactions, such as applying network policies, | ||
initiating reprocessing of RHACS Cloud Service policies, and interacting with | ||
the Admission controller. |
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.
One minor note is that we can probably add a link to either stackrox or ACS documentation as a way for reader to find more information.
cce@ocp4: CCE-86171-6 | ||
|
||
references: | ||
pcidss: Req-6.3.2,Req-11.3.1.1,Req-11.5.1.1 |
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'll need to incorporate versions here before we release PCI-DSS v4.0.0.
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.
/lgtm
We can cleanup the documentation reference in a follow up. It also makes sense to do the versioning for the profile references separately from this change.
I believe all @yuumasato's feedback was resolved. |
We've incorporated Yuuma's feedback into the latest PR.
Description:
Add rule acs_sensor_exists, so we can check that ACS sensor is being deployed on the cluster