Creating TLSOptions in favor of PullOptions and PushOptions.

[ybettan]

The build object, always push to the same registry that the
kernelMapping will pull from, therefore, there is not need to
duplicate the config.

In addition that, I have also renamed some of the fields for better
self-documented code.

Here is how this field is used in the module:

• kernelMapping[].build.baseImageRegistryTLS is only meant for pulling the
  base image of the Dockerfile specified in the build.

• kernelMapping[].registryTLS is meant for specifying the TLS options for
  pulling the image we want to deploy or for pushing it in case we are building
  it in-cluster (because they will always be the same).

• moduleLoaderContainerSpec.registryTLS is just the global value of all
  kernelMapping[]'s entries.

Signed-off-by: Yoni Bettan yonibettan@gmail.com

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ybettan

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 [ybettan]

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

[qbarrand]

If we change the CRD now, we probably need to implement a conversion webhook to avoid breaking clients.
https://book.kubebuilder.io/multiversion-tutorial/tutorial.html

[codecov-commenter]

Codecov Report

Base: 71.86% // Head: 71.88% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (4d8aba7) compared to base (351def7).
Patch coverage: 80.00% of modified lines in pull request are covered.

❗ Current head 4d8aba7 differs from pull request most recent head 02978ac. Consider uploading reports for the commit 02978ac to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
+ Coverage   71.86%   71.88%   +0.02%     
==========================================
  Files          25       25              
  Lines        2417     2419       +2     
==========================================
+ Hits         1737     1739       +2     
  Misses        597      597              
  Partials       83       83              

Impacted Files	Coverage Δ
internal/module/helper.go	0.00% <0.00%> (ø)
internal/registry/registry.go	36.50% <80.00%> (ø)
controllers/module_reconciler.go	67.69% <100.00%> (ø)
internal/build/job/maker.go	92.98% <100.00%> (+0.08%)	⬆️
internal/build/job/manager.go	78.46% <100.00%> (ø)
internal/preflight/preflight.go	71.05% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ybettan]

@qbarrand @yevgeny-shnaidman Are we OK to merge it without a conversion webhook at this point?

[ybettan]

@yevgeny-shnaidman @qbarrand PTAL

[qbarrand]

Should we also define kernelMapping[].sign.baseImageRegistryTLS?

[ybettan]

Should we also define kernelMapping[].sign.baseImageRegistryTLS?

I don't think we do. baseImageRegistryTLS is only for pulling a base image while building, for pulling a pre-built image or for pushing a just-built image we are using km.registryTLS anyway.

@chr15p Am I missing something here?

[ybettan]

/hold
Wait for Github actions before merging.

[ybettan]

/unhold

[chr15p]

Should we also define kernelMapping[].sign.baseImageRegistryTLS?

I don't think we do. baseImageRegistryTLS is only for pulling a base image while building, for pulling a pre-built image or for pushing a just-built image we are using km.registryTLS anyway.

@chr15p Am I missing something here?

Signing pulls an image, signs its kmods, then pushes it back again. So it both pulls and pushes, and theoretically they could be to/from different registrys. We're currently using spec.ImageRepoSecret for the creds to do the pushing and pulling which it doesn't look like you're changing so I think we're ok.

We might want to add per sign pull/push secrets, it would probably be a good thing, but at the moment we dont do that.

[ybettan]

We might want to add per sign pull/push secrets, it would probably be a good thing, but at the moment we dont do that.

This PR isn't about secret but rather about transport (TLS) options.

We can discuss adding secrets to the sign if needed but I don't think is has the same context as this PR does.

[qbarrand]

baseImageRegistryTLS is only for pulling a base image while building

Signing does essentially the same as building, I don't see any reason why we shouldn't have the same TLS options.

[yevgeny-shnaidman]

baseImageRegistryTLS is only for pulling a base image while building

Signing does essentially the same as building, I don't see any reason why we shouldn't have the same TLS options.

Signing is accessing the same registry as building. Do you see any scenario where we execute sign without build?

[ybettan]

Signing does essentially the same as building, I don't see any reason why we shouldn't have the same TLS options.

Signing isn't pulling any "base image" as build does. It is only pulling existing images or pushing after signing, hence, using the km.registryTLS (same as build is doing when pushing).

build.baseImageRegistryTLS is only used to pull the base image withing a Dockerfile, we don't have such in signing.

[ybettan]

As agree with @qbarrand TLS options doesn't exist for signing in this PR, therefore, they will be added separately in a different PR.

[qbarrand]

/lgtm

[chr15p]

Signing is accessing the same registry as building. Do you see any scenario where we execute sign without build?

yes, if a vendor distributes a their own pre-built driver container and it needs to be signed to run on the users cluster.

and yes, sorry pull options not secrets, but signing still doesn't use them, and maybe it should, but that is a separate issue/PR