first draft of configmap/secret injection via volumes enhancement

[bparees]

No description provided.

[bparees]

/assign @adambkaplan
/cc @smarterclayton

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees

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

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

[smarterclayton]

Hrm - I kind of expected you to say “we should not create a solution for mounting multiple volume types in different ways, so while we aren’t solving for that we expect to expose an api compatible with the future”.

Ie I expect mounting secrets for pods and builds to be the same api experience.

[bparees]

well that ship already sailed to some degree (in terms of build-pod volume mounts. not in terms of buildah-mounting). We are always going to have the existing secret/configmap input api which controls build-pod mounts.

we may also have the api you seem to be asking for, which would be a secondary way to drive build-pod mounts.

but users don't care about build-pod mounts since that has no real impact on experience. they care about whether we are doing buildah-mounts, or injecting the content in a different way, since that determines what ultimately ends up in the output image.

in any case your suggestion has benefits in terms of pushing us towards the "support PVs" direction, though at the short term cost of being a larger implementation for the immediate goal of treating secrets as a buildah-mount.

[smarterclayton]

I don’t like this. I would want to mount volumes in the future design. Mounting volumes and injecting the current set of secrets are already different (in terms of use case)

[bparees]

hm. So you're looking for an api which is "volume as an input" and then that volume can potentially come from a configmap, secret, PV, etc.

And then after we mounted that content into the build-pod, we'd wire it through as a buildah-mount.

It can be done, i think it's substantially more work than what this augmentation would require (define+implement an entire new api for mounting content into the build-pod, which this enhancement doesn't need since all that logic already exists for secret/configmap input api).

and of course we'll have to constrain the initial implementation to only allow secret+configmap "volume-types".

but i will take a pass at it

[smarterclayton]

I think volume mounts aren't part of "source" - they are part of "build execution environment" (which admittedly we don't have well defined). I think source is about assembly of the input, but execution environment is more about the pod.

[bparees]

reasonable. unfortunately today the closest thing we have to something that's part of the execution environment is our "env var" injection, which is defined on a per-strategy basis.

i'd prefer not to do this on a per-strategy basis, though we certainly could.

if we did it top-level it probably looks like:

apiVersion: v1
items:
- apiVersion: build.openshift.io/v1
  kind: BuildConfig
  metadata:
    name: mybuild
    namespace: p1
  spec:
    source:
      secrets:
      - secret: 
          name: myOtherSecret
        destinationDir: /tmp/creds
      - configMaps:
        configMap:
          name: myOtherConfigMap
        destinationDir: /tmp/config
    executionContext:
      volumes:
      - type: Secret
        reference:
          name: mySecretName
        destinationDir: /etc/subscriptions
      - type: ConfigMap
        reference: 
          name: myConfigMapName
        destinationDir: /tmp/config

if we bolted it into strategy we'd have:

apiVersion: v1
items:
- apiVersion: build.openshift.io/v1
  kind: BuildConfig
  metadata:
    name: mybuild
    namespace: p1
  spec:
    source:
      git:
        uri: https://github.com/openshift/nodejs-ex
      secrets:
      - secret: 
          name: myOtherSecret
        destinationDir: /tmp/creds
      - configMaps:
        configMap:
          name: myOtherConfigMap
        destinationDir: /tmp/config
    strategy:
      type: Source
      sourceStrategy:
        env:
        - name: foo
          value: bar
        volumes:
        - type: Secret
          reference:
            name: mySecretName
          destinationDir: /etc/subscriptions
        - type: ConfigMap
          reference: 
            name: myConfigMapName
          destinationDir: /tmp/config

[bparees]

@smarterclayton @adambkaplan i've added a new commit with a revised version of the design based on @smarterclayton's feedback

[bparees]

@smarterclayton i've revised the api to more or less just use pod Volume structs, ptal.

[smarterclayton]

I think this LGTM for now.

[smarterclayton]

I think this is pretty close, let's get final round of feedback?

[bparees]

@adambkaplan @gabemontero you want to take a pass?

[gabemontero]

OK @bparees some thoughts occurred to me given recent efforts elsewhere that did not occur to me when you first drafted this.

Certainly not "staunch" suggestions if you want to keep things as is, but I'm curious on your take all the same.

[gabemontero]

Do we still want to cite this given where #214 is headed re: CSI and mounting content via admission yada yada

If that proposal manifests, the openshift/builder image presumably just needs to look in the well understood location to seed buildah and the build API / controller sugar this proposal gives us.

[gabemontero]

Granted you can solve entitlements with this but at this point we probably won't.

[adambkaplan]

There are other use cases where credentials can otherwise leak, like private Artifactory creds for npm/maven downloads.

[bparees]

technically it could still be used for this use case until the entitlement feature lands (depending which of these lands first) and also might be used to "override" the entitlements that are otherwise being injected.

that said, i'll revise the wording to make it more generically about providing a way to inject credentials that are not published w/ the output image.

[bparees]

pushed

[gabemontero]

I wonder if there will be both some API reuse as well as perhaps some level of pod template manipulation with this that could be shared with build v2 if it was factored in the right way (both api and repo housing code)

How interested are you @bparees in citing that as a goal above?

[bparees]

i'd say "not very". If we decide we got this api perfect, then i'm ok w/ builds v2 just cloning/copying it as it's just not that complicated/bulky. I'd rather not try to abstract it in hopes of reusing it, especially in case we ultimately conclude it's "close" but not perfect by the time we get to the builds v2 api.

In any case the only new structs this is actually defining is the VolumeMount struct (simplified version of the k8s VolumeMount). So that actually will be a standalone struct that could in theory be reused elsewhere if desired.

[adambkaplan]

Reading over volumes support in Tekton, what they provide appears to be a thin vaneer over the kube volume API. We need to be clear that the mount path is the destination as seen inside the build, not the path that we mount into the container that runs buildah.

[bparees]

We need to be clear that the mount path is the destination as seen inside the build, not the path that we mount into the container that runs buildah.

yes good point. the build-controller will autogen/pick the pod mount path. the VolumeMount will determine the mapping of those pod mount paths, to the buildah args.

[bparees]

i was trying to make that clear with // Path within the build environment at which the volume should be mounted.

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[openshift-bot]

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

[bparees]

/lifecycle frozen
/remove-lifecycle rotten

[adambkaplan]

Reviving this EP - supporting only Secrets and ConfigMaps initially is a perfect starting point.

For getting entilements/content access certs into builds, we should also add support for the csi volume type. That way users could try out the projected resource CSI driver in builds in a decoupled fashion.

[adambkaplan]

Perhaps instead of using the core k8s Volume object, we create our own object that is a subset of Volume with only the volume types we support.

[adambkaplan]

/close

Replaced by #733

[openshift-ci-robot]

@adambkaplan: Closed this PR.

In response to this:

/close

Replaced by #733

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.