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

Require predicate type #5713

Merged
merged 8 commits into from Dec 19, 2022
Merged

Conversation

JimBugwadia
Copy link
Member

@JimBugwadia JimBugwadia commented Dec 17, 2022

Explanation

Make attestations.predicateType required and add error handling

Related issue

Milestone of this PR

What type of PR is this

Proposed Changes

Proof Manifests

Checklist

  • I have read the contributing guidelines.
  • I have read the PR documentation guide and followed the process including adding proof manifests to this PR.
  • This is a bug fix and I have added unit tests that prove my fix is effective.
  • This is a feature and I have added CLI tests that are applicable.
  • My PR needs to be cherry picked to a specific release branch which is .
  • My PR contains new or altered behavior to Kyverno and
    • CLI support should be added and my PR doesn't contain that functionality.
    • I have added or changed the documentation myself in an existing PR and the link is:
    • I have raised an issue in kyverno/website to track the documentation update and the link is:

Further Comments

Signed-off-by: Jim Bugwadia <jim@nirmata.com>
Signed-off-by: Jim Bugwadia <jim@nirmata.com>
Signed-off-by: Jim Bugwadia <jim@nirmata.com>
Signed-off-by: Jim Bugwadia <jim@nirmata.com>
@chipzoller
Copy link
Member

@JimBugwadia does this need to be added to the forthcoming 1.8.5 patch release or is this an enhancement/feature?

@JimBugwadia
Copy link
Member Author

@JimBugwadia does this need to be added to the forthcoming 1.8.5 patch release or is this an enhancement/feature?

Its an enhancement, but would be good to pick up as it tightens the usage model.

@codecov
Copy link

codecov bot commented Dec 17, 2022

Codecov Report

Merging #5713 (6bd90f0) into main (b5625f3) will decrease coverage by 0.01%.
The diff coverage is 12.50%.

@@            Coverage Diff             @@
##             main    #5713      +/-   ##
==========================================
- Coverage   34.66%   34.64%   -0.02%     
==========================================
  Files         190      190              
  Lines       21059    21071      +12     
==========================================
  Hits         7300     7300              
- Misses      12951    12961      +10     
- Partials      808      810       +2     
Impacted Files Coverage Δ
api/kyverno/v1/image_verification_types.go 68.05% <ø> (ø)
pkg/engine/imageVerify.go 70.08% <12.50%> (-1.74%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Jim Bugwadia <jim@nirmata.com>
@JimBugwadia
Copy link
Member Author

/cherry-pick release-1.8

@@ -451,7 +463,6 @@ func (iv *imageVerifier) verifyAttestorSet(
imageVerify kyvernov1.ImageVerification,
imageInfo apiutils.ImageInfo,
path string,
predicateType string,
Copy link
Collaborator

Choose a reason for hiding this comment

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

How predicateType is used to verify attestations.attestors if we remove it here?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the prior implementation, even though the predicateType was passed to verifyAttestorSet it was not used, except when a recursive call to verifyAttestorSet was made for nested attestors.

The predicateType is used in the verifyAttestations method:

Copy link
Collaborator

Choose a reason for hiding this comment

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

even though the predicateType was passed to verifyAttestorSet it was not used,

It was used to build and verify attestors if not empty:

opts, subPath := iv.buildOptionsAndPath(a, imageVerify, image, kyvernov1.Attestation{PredicateType: predicateType})

@realshuting realshuting enabled auto-merge (squash) December 19, 2022 09:46
@realshuting realshuting merged commit 14d82cb into kyverno:main Dec 19, 2022
15 of 17 checks passed
@gcp-cherry-pick-bot
Copy link

Cherry-pick failed with Merge error 14d82cbf6df091d0b12d2d1cdce1d9c9f29f659d into temp-cherry-pick-825d9a-release-1.8

realshuting pushed a commit to realshuting/kyverno that referenced this pull request Dec 19, 2022
Signed-off-by: ShutingZhao <shuting@nirmata.com>
@realshuting realshuting added the cherry-pick-completed The PR was cherry-picked (or merged) to required release branches label Dec 19, 2022
realshuting added a commit that referenced this pull request Dec 19, 2022
* cherry-pick #5713

Signed-off-by: ShutingZhao <shuting@nirmata.com>

* fix args

Signed-off-by: ShutingZhao <shuting@nirmata.com>

Signed-off-by: ShutingZhao <shuting@nirmata.com>
Co-authored-by: Jim Bugwadia <jim@nirmata.com>
MdSahil-oss pushed a commit to MdSahil-oss/kyverno that referenced this pull request Dec 29, 2022
* fix digest and verify logic

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* allow attestations with no attestors

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* require predicateType

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* fix typo

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

Signed-off-by: Jim Bugwadia <jim@nirmata.com>
Co-authored-by: shuting <shuting@nirmata.com>
Signed-off-by: Md Sahil <Mohdssahil1@gmail.com>
@JimBugwadia JimBugwadia deleted the require_predicate_type branch January 11, 2023 00:36
MdSahil-oss pushed a commit to MdSahil-oss/kyverno that referenced this pull request Jan 11, 2023
* fix digest and verify logic

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* allow attestations with no attestors

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* require predicateType

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* fix typo

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

Signed-off-by: Jim Bugwadia <jim@nirmata.com>
Co-authored-by: shuting <shuting@nirmata.com>
Signed-off-by: MdSahil-oss <Mohdssahil1@gmail.com>
MdSahil-oss pushed a commit to MdSahil-oss/kyverno that referenced this pull request Jan 11, 2023
* fix digest and verify logic

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* allow attestations with no attestors

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* require predicateType

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* fix typo

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

Signed-off-by: Jim Bugwadia <jim@nirmata.com>
Co-authored-by: shuting <shuting@nirmata.com>
Signed-off-by: MdSahil-oss <Mohdssahil1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-completed The PR was cherry-picked (or merged) to required release branches cherry-pick-required milestone 1.8.5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants