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.

Sign up for GitHub

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

Jump to bottom

Validate Agent Name by Format and Binary #119

Merged
merged 6 commits into from
Jan 10, 2024
Merged

Validate Agent Name by Format and Binary #119

merged 6 commits into from
Jan 10, 2024

Conversation

razo7
Copy link
Member

@razo7 razo7 commented Jan 4, 2024 •
edited by openshift-ci bot
Loading

Agent field in far or farTemplate has not been verified.

  1. We add format validation by Kubebuilder (should have fence_ prefix)
  2. We add Webhook validation for whether a fence agent file exists with this agent name (under /usr/sbin directory)

ECOPROJECT-1753

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 4, 2024

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 OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 4, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: razo7

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Jan 4, 2024
@razo7
Copy link
Member Author

razo7 commented Jan 4, 2024

/test 4.14-openshift-e2e
/test 4.15-openshift-e2e

@razo7
Copy link
Member Author

razo7 commented Jan 5, 2024

/test 4.14-openshift-e2e

@razo7
Copy link
Member Author

razo7 commented Jan 7, 2024

/test 4.14-openshift-e2e
/test 4.15-openshift-e2e

@razo7
Copy link
Member Author

razo7 commented Jan 7, 2024

/test 4.14-openshift-e2e

@razo7
Validate agent name by format
c8d61b0 
Use kubebuilder validation to validate fence agent format - has fence_ prefix
@razo7
Initalize webhook for far CR
99cff8a 
Run 'operator-sdk create webhook --group fence-agents-remediation --version v1alpha1 --kind FenceAgentsRemediation --programmatic-validation'
@razo7
Implement agent name validation for far in Webhook
e1e735e 
Use Webhook to verify agent name match an agent under /usr/sbin directory
@razo7 razo7 force-pushed the validate-agent-name-webhook branch from 0e32fc7 to a1d1e2d Compare January 7, 2024 15:39
@razo7
Copy link
Member Author

razo7 commented Jan 7, 2024

/test 4.14-openshift-e2e

@razo7
Initalize and implement agent name validation for far template in Web…
15391cb 
…hook

Use Webhook to verify agent name match an agent under /usr/sbin directory
@razo7 razo7 force-pushed the validate-agent-name-webhook branch from a1d1e2d to 15391cb Compare January 8, 2024 10:00
@razo7
Copy link
Member Author

razo7 commented Jan 8, 2024

/test 4.14-openshift-e2e

@razo7
Mock agent validation in Webhook
27bddde 
Create new interface AgentValidator with new struct validateAgentExistence to mock os.Stat function, since it is not needed on in unit tests, but still want to use it in production code
@razo7
Copy link
Member Author

razo7 commented Jan 9, 2024

/test 4.14-openshift-e2e

@razo7 razo7 mentioned this pull request Jan 9, 2024
Open
clobrano
clobrano requested changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@clobrano clobrano left a comment

Choose a reason for hiding this comment

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

I left some comments

api/v1alpha1/fenceagentsremediation_webhook_test.go Outdated Show resolved Hide resolved
pkg/validation/validation.go Outdated Show resolved Hide resolved
api/v1alpha1/fenceagentsremediationtemplate_webhook_test.go Outdated Show resolved Hide resolved
@clobrano
Copy link
Contributor

clobrano commented Jan 9, 2024 •
edited
Loading

Sorry, somehow my editor pushed the same review twice

slintes
slintes requested changes Jan 9, 2024
View reviewed changes
Copy link
Member

@slintes slintes left a comment

Choose a reason for hiding this comment

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

I found it a bit complicated to read the changes.
IMHO "separation of concerns" can be improved, by not using anything about files in the webhook, and not using anything about webhooks in the validator.

I wasn't sure if the result is that much better, so I gave it a try. Code might clarify my comments :D Feel free to pick those changes where you agree that it's better.

slintes@e87ead5

api/v1alpha1/fenceagentsremediationtemplate_webhook.go Outdated Show resolved Hide resolved
api/v1alpha1/fenceagentsremediationtemplate_webhook.go Outdated Show resolved Hide resolved
pkg/validation/validation.go Outdated Show resolved Hide resolved
pkg/validation/validation.go Outdated Show resolved Hide resolved
pkg/validation/validation.go Outdated Show resolved Hide resolved
pkg/validation/validation.go Outdated Show resolved Hide resolved
@razo7 razo7 mentioned this pull request Jan 10, 2024
Closed
@razo7
Better code separation between function and packages
06c01f1 
Use explicit error messages to find, use os.stat by default in far webhook, and dummy validation in webhook pacakage
@razo7
Copy link
Member Author

razo7 commented Jan 10, 2024

/test 4.14-openshift-e2e

@slintes
Copy link
Member

slintes commented Jan 10, 2024

/lgtm

@slintes slintes marked this pull request as ready for review January 10, 2024 09:10
@razo7
Copy link
Member Author

razo7 commented Jan 10, 2024

/retest

@openshift-merge-bot openshift-merge-bot bot merged commit c097980 into medik8s:main Jan 10, 2024
18 checks passed
@razo7 razo7 mentioned this pull request Jan 10, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clobrano clobrano Awaiting requested review from clobrano

@slintes slintes Awaiting requested review from slintes

Assignees

@slintes slintes

@clobrano clobrano

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@razo7 @clobrano @slintes @openshift-merge-robot