[Cosigned] Convert functions for webhookCIP from v1alpha1 #1736

DennyHoang · 2022-04-11T16:25:38Z

Signed-off-by: Denny Hoang dhoang@vmware.com

Summary

Continual improvement to webhookCIP

Less reliance on marshal/unmarshal to allow for more customization and streamlined conversion between CIP and the webhookCIP representation
KeylessRef.CACert now uses the webhookCIP representation of KeyRef
~~Logic for inlinedData from secretRef and KMS moved from reconciler file to the clusterimagepolicy_types for consolidation and removing bulk in reconciler file~~
No new tests were added at the moment because the clusterimagepolicy_test already had decent coverage for all the inlining data etc

cc: @coyote240 @hectorj2f @vaikas

codecov-commenter · 2022-04-11T16:35:28Z

Codecov Report

Merging #1736 (d161bce) into main (de85b7e) will decrease coverage by 0.56%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1736      +/-   ##
==========================================
- Coverage   29.52%   28.96%   -0.57%     
==========================================
  Files         141      141              
  Lines        8502     8394     -108     
==========================================
- Hits         2510     2431      -79     
+ Misses       5718     5697      -21     
+ Partials      274      266       -8

Impacted Files	Coverage Δ
...econciler/clusterimagepolicy/clusterimagepolicy.go	`53.73% <100.00%> (-10.27%)`	⬇️
pkg/cosign/tuf/client.go	`62.34% <0.00%> (-0.95%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de85b7e...d161bce. Read the comment docs.

hectorj2f

lgtm, so far.

hectorj2f · 2022-04-11T16:44:51Z

pkg/cosign/kubernetes/webhook/clusterimagepolicy/clusterimagepolicy_types.go


 	"github.com/sigstore/cosign/pkg/apis/cosigned/v1alpha1"
+	"github.com/sigstore/cosign/pkg/apis/utils"


nit: organize the imports as groups (sigstore ones, external, golang packages).

Going to organize it as golang packages, cosign, sigstore, k8s, and then knative. Then all alphabetical within those package groups (not based on the alias of the package)

hectorj2f · 2022-04-11T16:47:40Z

pkg/cosign/kubernetes/webhook/clusterimagepolicy/clusterimagepolicy_types.go

@@ -37,7 +50,7 @@ type Authority struct {
 	// +optional
 	Key *KeyRef `json:"key,omitempty"`
 	// +optional
-	Keyless *v1alpha1.KeylessRef `json:"keyless,omitempty"`


suggestion: I am considering whether we should create again all the resources again. As a consequence, we should create the type for ImagePattern, Sources and Identities here to be consistent.

@vaikas wdyt?

Consistency is good indeed :) So, I'd be down for recreating them here, or always use one from the CRD. I don't think I have strong feelings on one or the other. If we can import the CRD ones, seems better to only have to update one place.

I kept the other ones as imports from the CRD because we arent currently manipulating it at the moment and it is a direct 1:1 mapping.
We actually manipulate keyRefs so that is why Authorities and key[less]Refs were re-implemented as required.

When the other properties diverge, I feel that updating it then makes more sense to keep potential updates in one place as Ville mentions until that diverging.

If there are any strong feelings for one or the other, it is easy enough to replicate the structs and updating tests.

vaikas · 2022-04-12T17:56:00Z

pkg/cosign/kubernetes/webhook/clusterimagepolicy/clusterimagepolicy_types.go

@@ -84,9 +106,103 @@ func (k *KeyRef) UnmarshalJSON(data []byte) error {
 	return nil
 }

-func convertKeyDataToPublicKeys(pubKey string) ([]*ecdsa.PublicKey, error) {
+func ConvertClusterImagePolicyV1alpha1ToWebhook(ctx context.Context, in *v1alpha1.ClusterImagePolicy, rtracker tracker.Interface, secretLister corev1listers.SecretLister) (*ClusterImagePolicy, error) {


I'd rather not have Tracker / SecretLister exposed here. I'm envisioning using these functions for example, from Cosign the CLI for various reasons, so having the Tracker exposed or secretlister doesn't make sense.
I would rather have this be a three pass operation from the Reconciler:

Resolve the inline references

Convert to Webhook.

Right now I can't do the second one from CLI (or maybe from other users of this library) because I don't have secretlisters or trackers. So, I'd rather have a Convert function that only converts to inline webhook data with no side effects.

So, I guess I'd rather have a method in Reconciler that handles the CIP->CIP-With-References-Inlined as a method that can take trackers / listers as necessary.
Then, the second step would just take the CIP-With-References-Inlined and this has no side effects at all, and is re-useable from other places as a standalone method that will then allow those places to call the ValidatePolicy.

Thoughts?

If we see those functions been used somewhere else, then lets keep them where they can be reused.

, I guess I'd rather have a method in Reconciler that handles the CIP->CIP-With-References-Inlined as a method that can take trackers / listers as necessary.

I like this option.

Signed-off-by: Denny Hoang <dhoang@vmware.com>

vaikas

Thanks for cleaning up the marshal/nonmarshal stuff woot woot.

vaikas · 2022-04-12T21:16:26Z

pkg/cosign/kubernetes/webhook/clusterimagepolicy/clusterimagepolicy_types.go

-func convertKeyDataToPublicKeys(pubKey string) ([]*ecdsa.PublicKey, error) {
-	keys := []*ecdsa.PublicKey{}
+func ConvertClusterImagePolicyV1alpha1ToWebhook(in *v1alpha1.ClusterImagePolicy) *ClusterImagePolicy {
+	copyIn := in.DeepCopy()


nit, I think this is not stricly necessary because we don't modify, but since we use the images from L105 as inputs, can't think right now if that will make a difference.

Signed-off-by: Denny Hoang <dhoang@vmware.com>

DennyHoang changed the title ~~Convert functions for webhookCIP from v1alpha1~~ [Cosigned] Convert functions for webhookCIP from v1alpha1 Apr 11, 2022

DennyHoang force-pushed the convert-cip branch from 237919b to d161bce Compare April 11, 2022 16:27

hectorj2f requested review from vaikas and hectorj2f April 11, 2022 16:38

hectorj2f assigned DennyHoang Apr 11, 2022

hectorj2f added the enhancement New feature or request label Apr 11, 2022

hectorj2f reviewed Apr 11, 2022

View reviewed changes

DennyHoang force-pushed the convert-cip branch from d161bce to 0433d61 Compare April 12, 2022 14:06

DennyHoang marked this pull request as ready for review April 12, 2022 14:06

vaikas reviewed Apr 12, 2022

View reviewed changes

DennyHoang force-pushed the convert-cip branch from 0433d61 to 02ea635 Compare April 12, 2022 20:08

Create convert functions for internal CIP

c8ed111

Signed-off-by: Denny Hoang <dhoang@vmware.com>

DennyHoang force-pushed the convert-cip branch from 02ea635 to c8ed111 Compare April 12, 2022 20:09

vaikas approved these changes Apr 12, 2022

View reviewed changes

vaikas merged commit 427d83a into sigstore:main Apr 12, 2022

github-actions bot added this to the v1.8.0 milestone Apr 12, 2022

mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022

Create convert functions for internal CIP (sigstore#1736)

93d5b5c

Signed-off-by: Denny Hoang <dhoang@vmware.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cosigned] Convert functions for webhookCIP from v1alpha1 #1736

[Cosigned] Convert functions for webhookCIP from v1alpha1 #1736

DennyHoang commented Apr 11, 2022 •

edited

codecov-commenter commented Apr 11, 2022

hectorj2f left a comment

hectorj2f Apr 11, 2022 •

edited

DennyHoang Apr 12, 2022

hectorj2f Apr 11, 2022

hectorj2f Apr 11, 2022

vaikas Apr 12, 2022 •

edited

DennyHoang Apr 12, 2022 •

edited

vaikas Apr 12, 2022

hectorj2f Apr 12, 2022

hectorj2f Apr 12, 2022

vaikas left a comment

vaikas Apr 12, 2022


		"github.com/sigstore/cosign/pkg/apis/cosigned/v1alpha1"
		"github.com/sigstore/cosign/pkg/apis/utils"

[Cosigned] Convert functions for webhookCIP from v1alpha1 #1736

[Cosigned] Convert functions for webhookCIP from v1alpha1 #1736

Conversation

DennyHoang commented Apr 11, 2022 • edited

Summary

codecov-commenter commented Apr 11, 2022

Codecov Report

hectorj2f left a comment

Choose a reason for hiding this comment

hectorj2f Apr 11, 2022 • edited

Choose a reason for hiding this comment

DennyHoang Apr 12, 2022

Choose a reason for hiding this comment

hectorj2f Apr 11, 2022

Choose a reason for hiding this comment

hectorj2f Apr 11, 2022

Choose a reason for hiding this comment

vaikas Apr 12, 2022 • edited

Choose a reason for hiding this comment

DennyHoang Apr 12, 2022 • edited

Choose a reason for hiding this comment

vaikas Apr 12, 2022

Choose a reason for hiding this comment

hectorj2f Apr 12, 2022

Choose a reason for hiding this comment

hectorj2f Apr 12, 2022

Choose a reason for hiding this comment

vaikas left a comment

Choose a reason for hiding this comment

vaikas Apr 12, 2022

Choose a reason for hiding this comment

DennyHoang commented Apr 11, 2022 •

edited

hectorj2f Apr 11, 2022 •

edited

vaikas Apr 12, 2022 •

edited

DennyHoang Apr 12, 2022 •

edited