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

feature: Compile source materials (configmaps, secrets) to support image policy admission controller #1418

Closed
kkavitha opened this issue Feb 7, 2022 · 7 comments
Closed

feature: Compile source materials (configmaps, secrets) to support image policy admission controller #1418

kkavitha opened this issue Feb 7, 2022 · 7 comments
Assignees
@vaikas
Labels
enhancement New feature or request

Comments

@kkavitha
Copy link
Contributor

kkavitha commented Feb 7, 2022 •
edited
Loading

In order to enable more efficient use of resources by the admission controller, compile the various sources of truth, be they secrets or configmaps, etc., into a single document (configmap) for consumption.

Outcomes:

  • A single source for admission information is established
  • Informers, etc. update admission information on resource change

@mattmoor @coyote240 @hectorj2f

See also: #1417 #1419
@kkavitha kkavitha added the enhancement New feature or request label Feb 7, 2022
This was referenced Feb 7, 2022
Closed
Closed
@mattmoor
Copy link
Member

mattmoor commented Feb 7, 2022

With the ClusterImagePolicy stuff landed, we should be able to leverage the genreconciler stuff to start to write reconcilers (some docs here).

Generally we split the setup and wiring of informer events into controller.go, e.g. https://github.com/knative-sandbox/sample-controller/blob/main/pkg/reconciler/addressableservice/controller.go. Particularly worth calling out is the use of the "tracker" here, which we will want to use for tracking secret references.

The genreconciler interface let's us implement this nice strongly typed interface here: https://github.com/knative-sandbox/sample-controller/blob/e0d660a91633d4c48d9ce12359fc1b17a2dbfc0b/pkg/reconciler/addressableservice/addressableservice.go#L50. If we start to add status to CRDs, this helps manage posting back status updates.

Unfortunately the sample-controller doesn't touch on our table testing stuff, but we do have a lot of examples of this at least. Here is one from serving: https://github.com/knative/serving/blob/0b4ef84741509d6d8b67454a09fd15849ba48b93/pkg/reconciler/domainmapping/table_test.go#L68. Here is another example from a smaller repo where it might be simpler to disentangle the parts: https://github.com/knative-sandbox/net-contour/blob/main/pkg/reconciler/contour/contour_test.go#L57

@mattmoor
Copy link
Member

mattmoor commented Feb 7, 2022

This is where the tracker "tracks" the foreign key: https://github.com/knative-sandbox/sample-controller/blob/e0d660a91633d4c48d9ce12359fc1b17a2dbfc0b/pkg/reconciler/addressableservice/addressableservice.go#L53-L58

tracker.Reference is actually intended to be a type we can embed in our API, but for instances like this controller (or secretRef) it is sometimes necessary to construct the reference with the appropriate apiVersion and kind

@mattmoor
Copy link
Member

mattmoor commented Feb 7, 2022

Since I mentioned testing error paths, the TableTest stuff let's you induce failures on particular API operations like so: https://github.com/knative-sandbox/net-contour/blob/2f7655b2da0afcff70d4fd2b347359b034e26f05/pkg/reconciler/contour/contour_test.go#L99

@hectorj2f
Copy link
Contributor

These are great references. Thanks @mattmoor.

@mattmoor mattmoor mentioned this issue Feb 24, 2022
Merged
@mattmoor
Copy link
Member

I've laid out a skeleton for the reconciler itself here: #1513

@vaikas vaikas self-assigned this Mar 7, 2022
vaikas added a commit to vaikas/cosign that referenced this issue Mar 8, 2022
@vaikas
This is the start of the necessary pieces to get sigstore#1418 and si…
831b721 
…gstore#1419 implemented

ClusterImagePolicy reconciler will now create a configmap (no secret support yet)
and update it on changes (not on deletions yet). Also put up most necessary
testing pieces so that we can start unit testing the reconciler and make sure
it updates the resulting configmap.

There's also a ConfigStore that we can then inject into the admission webhook
that I have wired in there (nop for now, but demonstrating how it could work).
Idea being that you could then for a given image ask for all the authorities that
need to be validated. You can see what that config looks like in the
/pkg/apis/config/testdata/image-policies.yaml and the accompanying tests
in /pkg/apis/config/image_policies_test
I made sure that it works with both yaml/json.

While playing with this there's some questions that came to mind, so I'll take
those to the document.

Hope is that we get enough pieces in place so that we can agree on the major
moving pieces and how they fit together and enough testing in place that
we can start sharding up the work more efficiently and in more focused areas.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
@vaikas vaikas mentioned this issue Mar 8, 2022
Merged
vaikas added a commit to vaikas/cosign that referenced this issue Mar 8, 2022
@vaikas
This is the start of the necessary pieces to get sigstore#1418 and si…
c65515f 
…gstore#1419 implemented

ClusterImagePolicy reconciler will now create a configmap (no secret support yet)
and update it on changes (not on deletions yet). Also put up most necessary
testing pieces so that we can start unit testing the reconciler and make sure
it updates the resulting configmap.

There's also a ConfigStore that we can then inject into the admission webhook
that I have wired in there (nop for now, but demonstrating how it could work).
Idea being that you could then for a given image ask for all the authorities that
need to be validated. You can see what that config looks like in the
/pkg/apis/config/testdata/image-policies.yaml and the accompanying tests
in /pkg/apis/config/image_policies_test
I made sure that it works with both yaml/json.

While playing with this there's some questions that came to mind, so I'll take
those to the document.

Hope is that we get enough pieces in place so that we can agree on the major
moving pieces and how they fit together and enough testing in place that
we can start sharding up the work more efficiently and in more focused areas.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
vaikas added a commit to vaikas/cosign that referenced this issue Mar 9, 2022
@vaikas
This is the start of the necessary pieces to get sigstore#1418 and si…
dc5408f 
…gstore#1419 implemented

ClusterImagePolicy reconciler will now create a configmap (no secret support yet)
and update it on changes (not on deletions yet). Also put up most necessary
testing pieces so that we can start unit testing the reconciler and make sure
it updates the resulting configmap.

There's also a ConfigStore that we can then inject into the admission webhook
that I have wired in there (nop for now, but demonstrating how it could work).
Idea being that you could then for a given image ask for all the authorities that
need to be validated. You can see what that config looks like in the
/pkg/apis/config/testdata/image-policies.yaml and the accompanying tests
in /pkg/apis/config/image_policies_test
I made sure that it works with both yaml/json.

While playing with this there's some questions that came to mind, so I'll take
those to the document.

Hope is that we get enough pieces in place so that we can agree on the major
moving pieces and how they fit together and enough testing in place that
we can start sharding up the work more efficiently and in more focused areas.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
vaikas added a commit to vaikas/cosign that referenced this issue Mar 9, 2022
@vaikas
This is the start of the necessary pieces to get sigstore#1418 and si…
fba0b22 
…gstore#1419 implemented

ClusterImagePolicy reconciler will now create a configmap (no secret support yet)
and update it on changes (not on deletions yet). Also put up most necessary
testing pieces so that we can start unit testing the reconciler and make sure
it updates the resulting configmap.

There's also a ConfigStore that we can then inject into the admission webhook
that I have wired in there (nop for now, but demonstrating how it could work).
Idea being that you could then for a given image ask for all the authorities that
need to be validated. You can see what that config looks like in the
/pkg/apis/config/testdata/image-policies.yaml and the accompanying tests
in /pkg/apis/config/image_policies_test
I made sure that it works with both yaml/json.

While playing with this there's some questions that came to mind, so I'll take
those to the document.

Hope is that we get enough pieces in place so that we can agree on the major
moving pieces and how they fit together and enough testing in place that
we can start sharding up the work more efficiently and in more focused areas.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
mattmoor pushed a commit that referenced this issue Mar 9, 2022
@vaikas
Start of the necessary pieces to get #1418 and #1419 implemented (#1562)
70d9b9b 
* This is the start of the necessary pieces to get #1418 and #1419 implemented

ClusterImagePolicy reconciler will now create a configmap (no secret support yet)
and update it on changes (not on deletions yet). Also put up most necessary
testing pieces so that we can start unit testing the reconciler and make sure
it updates the resulting configmap.

There's also a ConfigStore that we can then inject into the admission webhook
that I have wired in there (nop for now, but demonstrating how it could work).
Idea being that you could then for a given image ask for all the authorities that
need to be validated. You can see what that config looks like in the
/pkg/apis/config/testdata/image-policies.yaml and the accompanying tests
in /pkg/apis/config/image_policies_test
I made sure that it works with both yaml/json.

While playing with this there's some questions that came to mind, so I'll take
those to the document.

Hope is that we get enough pieces in place so that we can agree on the major
moving pieces and how they fit together and enough testing in place that
we can start sharding up the work more efficiently and in more focused areas.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* Add placeholder configmap with an example.
Rename to be consistent with the other cm. image-policies => config-image-policies

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* Address lint.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* Use namespaced sharedinformerfactory so that we have the right permissions.
Ran manual tests validating that things are working, when I remove
things from the configmap, things are patched back in (after the global
resync is triggered).

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* Check error, duh.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* Just trying to remove the files that verify-codegen is complaining.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
@vaikas
Copy link
Contributor

vaikas commented Mar 14, 2022

@mattmoor @kkavitha I think we can close this issue now? I reckon the config stuff is implemented enough to close this as well as wired into the validation (maybe not in the best place) so it all flows through there.
If there's specific tasks outstanding, reckon we can track them in more surgical issues.

@hectorj2f
Copy link
Contributor

@vaikas Yes, I feel it has been addressed.

@vaikas vaikas closed this as completed Mar 16, 2022
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this issue May 6, 2022
@vaikas @mlieberman85
Start of the necessary pieces to get sigstore#1418 and sigstore#1419 …
34499bc 
…implemented (sigstore#1562)

* This is the start of the necessary pieces to get sigstore#1418 and sigstore#1419 implemented

ClusterImagePolicy reconciler will now create a configmap (no secret support yet)
and update it on changes (not on deletions yet). Also put up most necessary
testing pieces so that we can start unit testing the reconciler and make sure
it updates the resulting configmap.

There's also a ConfigStore that we can then inject into the admission webhook
that I have wired in there (nop for now, but demonstrating how it could work).
Idea being that you could then for a given image ask for all the authorities that
need to be validated. You can see what that config looks like in the
/pkg/apis/config/testdata/image-policies.yaml and the accompanying tests
in /pkg/apis/config/image_policies_test
I made sure that it works with both yaml/json.

While playing with this there's some questions that came to mind, so I'll take
those to the document.

Hope is that we get enough pieces in place so that we can agree on the major
moving pieces and how they fit together and enough testing in place that
we can start sharding up the work more efficiently and in more focused areas.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* Add placeholder configmap with an example.
Rename to be consistent with the other cm. image-policies => config-image-policies

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* Address lint.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* Use namespaced sharedinformerfactory so that we have the right permissions.
Ran manual tests validating that things are working, when I remove
things from the configmap, things are patched back in (after the global
resync is triggered).

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* Check error, duh.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* Just trying to remove the files that verify-codegen is complaining.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vaikas vaikas

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mattmoor @hectorj2f @kkavitha @vaikas