search

openshift / machine-config-operator

Apache License 2.0
245 stars 409 forks source link

OCPBUGS-9178: Track ssh authorized files #4536

Open RishabhSaini opened 2 months ago

RishabhSaini commented 2 months ago

Fixes: OCPBUGS-9178

- What I did

The Config Drift Monitor currently runs validateOnDiskState every time there’s a change in any tracked file. This triggers a check of all files and units for possible drift, even if only one file changed. Hence this change, efficiently only checks the specific file or path for config drift where the change happened using validateOnDiskStateForEvent. Only when the Config Drift Monitor detects a disk state mismatch due to the event that occurred, an update is scheduled and then the complete validateOnDiskState is run. This reduces the overhead every loop of the config drift monitor.

The SSH authorized paths have also been added.

- How to verify it

Any file event logged by fsnotify only triggers a check to the corresponding MC field

Verify SSH file content drifts by the test cases go test -v ./pkg/daemon/ -run ConfigDriftMonitor/ssh_file_content_drift go test -v ./pkg/daemon/ -run ConfigDriftMonitor/ssh_file_touch

- Description for the changelog

openshift-ci[bot] commented 2 months ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

openshift-ci[bot] commented 2 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: RishabhSaini Once this PR has been reviewed and has the lgtm label, please assign djoshy for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/openshift/machine-config-operator/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
openshift-ci-robot commented 2 months ago

@RishabhSaini: This pull request references Jira Issue OCPBUGS-9178, which is invalid:

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to [this](https://github.com/openshift/machine-config-operator/pull/4536): > > >Fixes: OCPBUGS-9178 > >**- What I did** > >The Config Drift Monitor runs validateOnDiskState(cfg, systemdPath) every time there’s a change in any tracked file. This triggers a check of all files and units for possible drift, even if only one file changed. >Hence this change, efficiently only checks the specific file or path for config drift where the change happened. > >It also simplifies adding a new file to be tracked by the Config Drift Monitor. The SSH authorized paths have been added using these methods > >**- How to verify it** > >**- Description for the changelog** > > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fmachine-config-operator). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
RishabhSaini commented 2 months ago

Thanks! @cheesesashimi for the detailed review.

Question: validateOnDiskStateForEvent currently only handles Ignition Spec 3 Configs. Are we still continuing to support Spec 2 Configs?

inesqyx commented 2 months ago

/retest-required

inesqyx commented 2 months ago

Thanks! @cheesesashimi for the detailed review.

Question: validateOnDiskStateForEvent currently only handles Ignition Spec 3 Configs. Are we still continuing to support Spec 2 Configs?

To my knowledge, I think we currently support both: https://github.com/openshift/machine-config-operator/blob/master/docs/MachineConfigDaemon.md and we have a function: convertIgnition22to34 to convert between V2 and V3 ignition config. There are more conversion tools in /pkg/controller/common/helpers.go. Please correct me if I am wrong :)

RishabhSaini commented 2 months ago

/retest-required

RishabhSaini commented 2 months ago

/retest-required

RishabhSaini commented 2 months ago

/retest-required

RishabhSaini commented 2 months ago

@cheesesashimi I am unsure whether the checks failing are due to my PR or flaky CI. Although it doesn't seem like most of them are failing due to the changes I made

RishabhSaini commented 2 months ago

/retest-required

RishabhSaini commented 1 month ago

/retest-required

RishabhSaini commented 1 month ago

/retest-required

RishabhSaini commented 1 month ago

/retest-required

RishabhSaini commented 1 month ago

/retest

openshift-ci[bot] commented 1 month ago

@RishabhSaini: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).