Adding Porch private authenticated registries functionality

[Catalin-Stratulat-Ericsson]

Tackles #637

This PR adds the functionality for porch to use private authenticated container registries for its KPT functions in the porch packages.
It does this by mounting a docker config.json file as a secret which holds the authenticated information for the private repositories used by the user in their porch packages.
This secret information is then used as the ImagePullSecret on the KPT function pods created by the function runner at render time.

A documentation PR will soon follow which will document the configuration and i will link to it in this PR
Documentation PR #178
RBAC changes must be propagated in the catalog PR #126

[Catalin-Stratulat-Ericsson]

/retest presubmit-nephio-go-test

[nephio-prow]

@Catalin-Stratulat-Ericsson: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

• /test presubmit-nephio-go-test

Use /test all to run all jobs.

In response to this:

/retest presubmit-nephio-go-test

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.

[Catalin-Stratulat-Ericsson]

/test presubmit-nephio-go-test

[Catalin-Stratulat-Ericsson]

/test presubmit-nephio-go-test

[JamesMcDermott]

/approve

[arora-sagar]

Just a side question: should all the functions use the same image pull secret? Probably that will be the most used scenario.

But how about adding an imagePullSecret row in the kptfile? This will provide more flexibility.

    - image: gcr.io/kpt-fn/apply-replacements:v0.1.1
      configPath: apply-replacements-namespace.yaml
      imagePullSecret: XXXXX

[Catalin-Stratulat-Ericsson]

Just a side question: should all the functions use the same image pull secret? Probably that will be the most used scenario.

But how about adding an imagePullSecret row in the kptfile? This will provide more flexibility.

    - image: gcr.io/kpt-fn/apply-replacements:v0.1.1
      configPath: apply-replacements-namespace.yaml
      imagePullSecret: XXXXX

1. you are correct this would be a more flexible way however it comes with a major drawback and that is backwards compatibility.
2. if we implement this new section in the kpt file we must modify the kpt file schema which has not been altered since we took it into porch.
3. For example if currently whatever we add into the porch pkg kpt file will be rendered just fine by kpt commands e.g. kpt live init, kpt render etc, if we make fundamental changes to the porch kpt file schema for example adding a new variable "imagePullSecret" to the schema, attempting to do the kpt render will fail due to a new unknown and unaccounted for variable whilst our porch render which occurs when we push the pkg will work and render just fine.
4. This was a problem encountered by myself and Istvan before in a previous ticket about perhaps adding the render status in the kpt file which caused the exact same error and we had to go back to the drawing board for this same reason.

[arora-sagar]

Just a side question: should all the functions use the same image pull secret? Probably that will be the most used scenario.
But how about adding an imagePullSecret row in the kptfile? This will provide more flexibility.

    - image: gcr.io/kpt-fn/apply-replacements:v0.1.1
      configPath: apply-replacements-namespace.yaml
      imagePullSecret: XXXXX

1. you are correct this would be a more flexible way however it comes with a major drawback and that is backwards compatibility.
2. if we implement this new section in the kpt file we must modify the kpt file schema which has not been altered since we took it into porch.
3. For example if currently whatever we add into the porch pkg kpt file will be rendered just fine by kpt commands e.g. kpt live init, kpt render etc, if we make fundamental changes to the porch kpt file schema for example adding a new variable "imagePullSecret" to the schema, attempting to do the kpt render will fail due to a new unknown and unaccounted for variable whilst our porch render which occurs when we push the pkg will work and render just fine.
4. This was a problem encountered by myself and Istvan before in a previous ticket about perhaps adding the render status in the kpt file which caused the exact same error and we had to go back to the drawing board for this same reason.

I understand the points thanks for explaining. In this case making changes in KPT file will result in making quite big changes in all the files.

[kispaljr]

First of all thanks for this much needed feature!
A quick clarification question: do I understand correctly that the user is asked to create a secret of type kubernetes.io/dockerconfigjson that is mounted to the function-runner pods, then the function runner creates an almost identical secret (as far as I can see the only difference is the name of the secret) that is in turn specified as one of the ImagePullSecrets of the KRM function pods?
If my understanding is correct, why isn't the user simply asked to create the secret with a fixed name, or why isn't the name of the user-created secret(s) is/are passed to the function-runner (instead of mounting it)? Then we could append the user-created secret(s) directly to the ImagePullSecrets of the KRM function pods.

[Catalin-Stratulat-Ericsson]

First of all thanks for this much needed feature! A quick clarification question: do I understand correctly that the user is asked to create a secret of type kubernetes.io/dockerconfigjson that is mounted to the function-runner pods, then the function runner creates an almost identical secret (as far as I can see the only difference is the name of the secret) that is in turn specified as one of the ImagePullSecrets of the KRM function pods? If my understanding is correct, why isn't the user simply asked to create the secret with a fixed name, or why isn't the name of the user-created secret(s) is/are passed to the function-runner (instead of mounting it)? Then we could append the user-created secret(s) directly to the ImagePullSecrets of the KRM function pods.

You are correct Istvan.
Ill try and explain it in sections.

1. firstly the easy one to explain the reason for the naming differences was mainly uncertainties (as you can tell by the comment above the secret name variable in the code). in my eyes the easiest would be to have another argument passed to the fn-runner stating the name of the secret so that could be carried on.
2. The reason a user is not asked for a fixed secret name is simply that its impractical. we shouldn't use hard coded naming for resources that porch uses etc in the code never mind in the cluster in my opinion or at least try not too even if it makes things a bit more difficult to work around.
3. The reason the name is not given but mounted needs a deeper explanation. The secret information is needed in 2 places. firstly inside the code on the fn-runner such that authn which runs inside porch can authenticate for image digestion & entrypoint, secondly it must be present on the porch-fn-system namespace to mount it as an imagePullSecrets to the pod which requires it for use.
4. The simplest solution i saw when doing this was to simply ask the user to create 2 identical secrets on the 2 namespaces. but that seemed like a weird ask if i was a user of this. so i ended up going for a "give me one ill replicate it" approach. however i didnt add a variable which one could pass the secret name with because i didnt want to add too much argument bloat if i could help it.

[kispaljr]

Thank you for the detailed clarification. I haven't realized before that we need the same secret in two different namespaces.

That properly explains the copying in the code.

I think this also explains what my follow-up question would have been, namely why aren't we just adding the pull secretes to the appropriate service accounts (as explained here). But now I understand that wouldn't solve the authentication problem when the function-runner itself pulls the container in order to inspect its metadata. Can you confirm that this is true?

/lgtm

[Catalin-Stratulat-Ericsson]

Thank you for the detailed clarification. I haven't realized before that we need the same secret in two different namespaces.

That properly explains the copying in the code.

I think this also explains what my follow-up question would have been, namely why aren't we just adding the pull secretes to the appropriate service accounts as explained https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#add-imagepullsecrets-to-a-service-account. But now I understand that wouldn't solve the authentication problem when the function-runner itself pulls the container in order to inspect its metadata. Can you confirm that this is true?

/lgtm

• i believe it is true Istvan particularly in this function

  porch/func/internal/podevaluator.go

  Line 539 in 419b898

  // imageDigestAndEntrypoint gets the entrypoint of a container image by looking at its metadata.

• The comment describing it even details that it inspects the metadata to store in the cache at first use of that image.
• I will also be including a commit soon which will fix the hard coded nature of that copied secret's name to follow what the user given secrets name is using an environment variable. i will update the documentation accordingly.

[kispaljr]

/lgtm

[liamfallon]

/approve

[nephio-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Catalin-Stratulat-Ericsson, JamesMcDermott, liamfallon

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

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