MGMT-14634: Ensure that empty manifest may not be added. #5348

paul-maidment · 2023-07-10T16:06:36Z

Presently, if a manifest has no content then it passes validation. This breaks bootkube when empty content is submitted.

This PR validates that manifest content is not empty before accepting it.

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
Submitted an empty file to the API endpoint and observed that validation now fails.
[] No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

Presently, if a manifest has no content then it passes validation. This breaks bootkube when empty content is submitted. This PR validates that manifest content is not empty before accepting it.

openshift-ci-robot · 2023-07-10T16:06:40Z

openshift-ci · 2023-07-10T16:18:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, paul-maidment

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:

~~OWNERS~~ [carbonin,paul-maidment]

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

codecov · 2023-07-10T16:23:41Z

Codecov Report

Merging #5348 (f084d55) into master (820a271) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5348      +/-   ##
==========================================
- Coverage   67.54%   67.53%   -0.01%     
==========================================
  Files         226      226              
  Lines       33278    33280       +2     
==========================================
- Hits        22476    22475       -1     
- Misses       8775     8778       +3     
  Partials     2027     2027

Impacted Files	Coverage Δ
internal/manifests/manifests.go	`72.06% <100.00%> (+0.19%)`	⬆️

... and 3 files with indirect coverage changes

omertuc · 2023-07-10T17:48:09Z

internal/manifests/manifests.go

@@ -412,6 +412,11 @@ func (m *Manifests) validateAllowedToModifyManifests(ctx context.Context, cluste
 }

 func (m *Manifests) validateUserSuppliedManifest(ctx context.Context, clusterID strfmt.UUID, manifestContent []byte, fileName string) error {
+	// If the manifest content size is zero then some of the content validations might pass when they should not
+	// We need to reject zero size content
+	if len(manifestContent) == 0 {


Can we go one step further and validate that there's (exactly? at-least?) a single parsable YAML document within the manifest file?

openshift-ci · 2023-07-10T18:15:01Z

@paul-maidment: all tests passed!

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.

…dded. (openshift#5348)" This reverts commit 10a88f5. It was found that this validation is causing issues for the UI. We have some mitigations planned for the issues, however this commit needs to be rolled back for now to prevent ongoing breakage while the issues are addressed.

…dded. (#5348)" (#5353) This reverts commit 10a88f5. It was found that this validation is causing issues for the UI. We have some mitigations planned for the issues, however this commit needs to be rolled back for now to prevent ongoing breakage while the issues are addressed.

…dded. (openshift#5348)" (openshift#5353) This reverts commit 10a88f5. It was found that this validation is causing issues for the UI. We have some mitigations planned for the issues, however this commit needs to be rolled back for now to prevent ongoing breakage while the issues are addressed.

) Presently, if a manifest has no content then it passes validation. This breaks bootkube when empty content is submitted. This PR validates that manifest content is not empty before accepting it.

…dded. (openshift#5348)" (openshift#5353) This reverts commit 10a88f5. It was found that this validation is causing issues for the UI. We have some mitigations planned for the issues, however this commit needs to be rolled back for now to prevent ongoing breakage while the issues are addressed.

) Presently, if a manifest has no content then it passes validation. This breaks bootkube when empty content is submitted. This PR validates that manifest content is not empty before accepting it.

…dded. (openshift#5348)" (openshift#5353) This reverts commit 10a88f5. It was found that this validation is causing issues for the UI. We have some mitigations planned for the issues, however this commit needs to be rolled back for now to prevent ongoing breakage while the issues are addressed.

MGMT-14634: Ensure that empty manifest may not be added.

f084d55

Presently, if a manifest has no content then it passes validation. This breaks bootkube when empty content is submitted. This PR validates that manifest content is not empty before accepting it.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 10, 2023

openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 10, 2023

openshift-ci bot requested review from carbonin and CrystalChun July 10, 2023 16:09

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2023

carbonin approved these changes Jul 10, 2023

View reviewed changes

openshift-ci bot assigned carbonin Jul 10, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2023

omertuc reviewed Jul 10, 2023

View reviewed changes

openshift-merge-robot merged commit 10a88f5 into openshift:master Jul 10, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-14634: Ensure that empty manifest may not be added. #5348

MGMT-14634: Ensure that empty manifest may not be added. #5348

paul-maidment commented Jul 10, 2023

openshift-ci-robot commented Jul 10, 2023 •

edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Jul 10, 2023

codecov bot commented Jul 10, 2023 •

edited

omertuc Jul 10, 2023

openshift-ci bot commented Jul 10, 2023

MGMT-14634: Ensure that empty manifest may not be added. #5348

MGMT-14634: Ensure that empty manifest may not be added. #5348

Conversation

paul-maidment commented Jul 10, 2023

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jul 10, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Jul 10, 2023

codecov bot commented Jul 10, 2023 • edited

Codecov Report

omertuc Jul 10, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Jul 10, 2023

openshift-ci-robot commented Jul 10, 2023 •

edited by openshift-ci bot

codecov bot commented Jul 10, 2023 •

edited