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.

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

add ability to set imagePullSecrets #64

Closed

Conversation

saraforeman
Copy link

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.

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)

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
Copy link
Owner

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
Copy link
Owner

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.

@phenixblue phenixblue closed this Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend the same concept for imagePullSecrets as well
2 participants