cloudfoundry / eirini

Pluggable container orchestration for Cloud Foundry, and a Kubernetes backend
Apache License 2.0
115 stars 30 forks source link

Managing the app-registry-secret reference passed into scheduled workloads #96

Closed acosta11 closed 4 years ago

acosta11 commented 4 years ago

Description

We recently tried to implement secret rotation with the immutable secret pattern through kapp managed "versioned secrets" (documented here). Ultimately, the app-registry-secret reference we passed into Eirini was not updated on the app workload statefulSets on redeployment of Eirini. Do y'all have any opinions about how we should manage that secret and potentially insight into how Eirini monitors references to secrets and configMaps in the workloads it schedules?

Steps to reproduce

Deploy cf-for-k8s with a google service account granted GCS storage admin (the permissions required to use GCR as an app registry) configured as the app-registry-secret passed to Eirini, updated to 'app-registry-secret-ver-1' with kapp's versioned annotation. (Note that to avoid the use kapp in this reproduction flow, we could just directly update the Eirini config to point at a new secret reference explicitly)

What was expected to happen

Eirini updated its reference to the app-registry-secret and all references to that secret in workloads it had scheduled.

What actually happened

Only the secret reference in Eirini was updated, so new app workloads got the correct reference, but references in existing workloads remained stale.

Potential fix (optional)

In a comment on a related cf-for-k8s issue, we considered the exclusion of the imagePullSecret from the statefulSet definition of scheduled workloads, instead relying on the inheritance of a serviceAccount imagePullSecret in the namespace. This assumes any namespace into which we schedule workloads has configured its default serviceAccount appropriately.

Thanks, Andrew

cf-gitbot commented 4 years ago

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/172950503

The labels on this github issue will be updated when the story is started.

julz commented 4 years ago

Hi @acosta11 - sorry for slow response, we discussed this in IPM and have spun out and prioritised a story to watch the ConfigMap and update secrets.

gdankov commented 4 years ago

Hi @acosta11,

We started implementing a custom controller that watches changes to our ConfigMap and copies the secret over to the app namespace if the secret name was updated. This works fine if there's only one eirini ConfigMap, but that, as we found out, is not the case with cf-for-k8s -- kapp creates a new CM every time there's a change. This means that the CM is never effectively updated which causes issues with our approach.

We were wondering what's the use case of CM versioning in cf-for-k8s. Would you be open to not versioning the Eirini CM?

BR, Georgi & @georgethebeatle

acosta11 commented 4 years ago

Hi Georgi,

Sorry about the delay here and thanks for the follow-up.

Intuitively, I think we want to continue using versioned ConfigMaps. We originally versioned ConfigMaps and Secrets to force Pod rolls, which forces reinitialization of the runtime config for applications that aren't written to watch the files to which their configuration is mounted in a Kubernetes pod context and enables transparent, fast feedback on the update of those resources and their volume mounts.

If Eirini can provide guarantees around the consumption of updated ConfigMaps at runtime without a pod roll, I think we would consider and exception to that general pattern.

That said, we've also considered the original question of whether imagePullSecrets in general need to follow this pattern because the pod/node will update those on demand as they are needed. So we've stopped versioning the app-registry-secret and haven't run into any issues yet. So we could potentially avoid this issue by defining the contract to make that secret name immutable?

Thanks, Andrew

gcapizzi commented 4 years ago

Hi @acosta11!

So we could potentially avoid this issue by defining the contract to make that secret name immutable?

Yes, we think this would work. Keeping the ConfigMaps versioned while maintaining the imagePullSecret immutable is probably the best way to go. Closing!

acosta11 commented 4 years ago

Yes, we think this would work. Keeping the ConfigMaps versioned while maintaining the imagePullSecret immutable is probably the best way to go. Closing!

Sounds good, thanks for thinking through this problem with us and spiking out the config map watcher!

Thanks, Andrew