phenixblue / imageswap-webhook

Image Swap Mutating Admission Webhook for Kubernetes
Apache License 2.0
154 stars 53 forks source link

add ability to set imagePullSecrets #64

Closed saraforeman closed 2 years ago

saraforeman commented 2 years ago

What type of PR is this? /kind feature

What this PR does / why we need it:

If the imageswap_pull_secret_source_name is set, the kubeconfig is loaded in init_imageswap() and creates a Kubernetes CoreV1Api client. It makes a copy of the secret in the k8s object passed in, then reads in the source secret in the imageswap-system namespace to generate a new secret in the target namespace from it’s data. This generated secret is then created or updated in the target namespace.

Added Kubernetes to the Pipfile for the imageswap image and updated the Pipfile.lock accordingly.

Added a new ClusterRole, imageswap-write-secrets in deploy/install.yaml to give get and update permissions to imageswap-shared-pull-secrets and create permissions for secrets. Also added a new ClusterRoleBinding, imageswap-write-secrets-crb for the imageswap-write-secrets ClusterRole.

Added add_image_pull_secrets method to add imagePullSecrets to a k8s object. This method is used in mutate() to add imagePullSecrets (if applicable) to a pod or a pod template. The method takes in 2 arguments, the modified_spec and the namespace of the target object. There is a side-effect in this method of creating/updating the secret imageswap-shared-pull-secrets in whichever namespace was targeted.

Added 2 new environment variables:

IMAGESWAP_PULL_SECRET_DESTINATION_NAME: name of secret to be created in the target namespace (set to imageswap-shared-pull-secrets so that it will line up with the permissions write/read/delete permission granted on just that secret in other namespaces) IMAGESWAP_PULL_SECRET_SOURCE_NAME: name of secret in imageswap-system namespace to copy (the secret stored at this value will need to be pre-populated by users)

Which issue(s) this PR fixes:

Fixes #21

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

IMAGESWAP_PULL_SECRET_SOURCE_NAME is a feature flag that defaults to off, therefore it is backwards
compatible and transparent to the user.

Additional documentation e.g., usage docs, etc.:


I apologize for not going through the standard procedure of discussion on issue #21, but there was an odd
policy in place on me. I can continue any discussion here and am happy to receive feedback and perform 
continued improvements.

The logic of creating/updating the pullSecrets in a given namespace on mutation of a pod will handle
the majority of cases, since any new pods get the most recent copy of the credentials. However, if a
pod were to get rescheduled to a new node (for some reason) and the credentials that were originally
synced into the namespace have since expired, then a ImagePullBackOff could result. This could be
fixed with an additional watcher on the secrets stored at `IMAGESWAP_PULL_SECRET_SOURCE_NAME`. However
for now it could also be fix by users updating the `IMAGESWAP_PULL_SECRET_SOURCE_NAME` in the
`imageswap-system` and applying the same update to any `imageswap-shared-pull-secrets` across all
namespaces.
phenixblue commented 2 years ago

Hello, I am a bit busy at the moment, so it will take me some time before I can review this. There are some trade-offs around security with this as I mentioned in #21, so I'm still not sure it's something we want to merge into the project, but worst case, you can start a fork if it's something your organization needs.

I'll review with the other maintainers when possible and get back to you.

phenixblue commented 2 years ago

Sorry for the delay on this. After consulting with the other maintainer, we're not ready to move in this direction with cluster scoped read/write access to secrets for ImageSwap.

I think tools like https://github.com/titansoft-pte-ltd/imagepullsecret-patcher may work in tandem with ImageSwap to solve for your scenario.