Open Tabrnicus opened 8 months ago
@181192 please take a look - seems this one requires some additional attention.
CC: @abhilashjoseph as the author of the PR which broke this functionality.
Hi everyone 👋 any news this in the meantime?
I just bumped into same issue trying pull container images from a third-party registry using imagePullSecrets
.
Would be super happy for any pointers, directions, or workarround ideas.
Note: Make sure to check out known issues (https://akv2k8s.io/troubleshooting/known-issues/) before submitting
Note: Tangentially related to #495, through #631.
Components and versions
[ ]
Controller, version:[X] Env-Injector (webhook), version:x.x.x
(docker image tag)1.6.0
(docker image tag) [ ]OtherDescribe the bug
A regression in version 1.6.0 of the Env-Injector prevents the use of
imagePullSecrets
as valid registry authentication when usingauthType="azureCloudConfig"
. More specifically, the logic present in the current 1.6.0 version (covered below in detail) assumes that both the Key Vault and container registry must be using MSIs as the authentication method. My use case involves utilizing a third-party container registry, and not an ACR. As a result, I cannot set up any form of MSI to our container registry, which is why we useimagePullSecrets
. My understanding from reading through the documentation and code from 1.5.0 and below is that this use case should be supported.After investigating, I've found that the changes introduced by #631 break the previous logic that allowed
imagePullSecrets
. The functiongetContainerRegistryRemoteOptions()
inregistry.go
(a) usesauthType
to decide how the container registry authentication should be handled, and (b) assumes that an ACR will be used whenauthType="azureCloudConfig"
: https://github.com/SparebankenVest/azure-key-vault-to-kubernetes/blob/647b7156685907bb1c5257c9e0596f4eab355165/pkg/docker/registry/registry.go#L123-L182For context, the previous approach used a
getImageConfig()
function whose only branch includesimagePullSecrets
(for clarity, this same logic is preserved in thedefault
case statement in 1.6.0): https://github.com/SparebankenVest/azure-key-vault-to-kubernetes/blob/7141e5f8cf03d482a51ef3c4a4ae945e9761b14e/pkg/docker/registry/registry.go#L112-L158This seems to be a restrictive approach, for several reasons:
authType
seems to be documented as an option to specify key vault authentication only. I haven't seen this option mention container registries (Chart README.md, Chartenv-injector-deployment.yaml
). Aside from the issues being discussed here, this alone is cause for potential confusion.authType="environment"
) with ACR MSI authentication. The newly added code branch inregistry.go
will never be taken becauseauthType
does not equalazureCloudConfig
.To Reproduce
1.26.10
Free
Disabled
Local accounts with Kubernetes RBAC
Azure CNI
,Calico
admin-password
mytoken
_repositories_push_metadata_write
akv2k8s
at application version1.6.0
(Helm chart version2.6.0
)azure-key-vault-env-injection
annotationdefault
, for ease.AzureKeyVaultSecret
in that namespaceadmin-password@azurekeyvault
Notice that Pods are not deploying. Describe the ReplicaSet to show the unauthorized message:
Expected behavior
Based on the above replication steps, I would expect to see the
env-injector
use theimagePullSecrets
in the deployment specification to pull the image from the registry. Instead, it tries to pull it using invalid/anonymous credentials.As a point of comparison, installing
akv2k8s
at application version1.5.0
(chart version2.5.0
) and reinstalling the deployment will show the intended behavior. Of course, the container is nonfunctional (it crash loops because it's default Alpine), which is beside the point.Logs
Full
ReplicaSet
describe log:env-injector
logs at time of failure:Note the two new (as of #631)
INFO
logs near the bottom, as it helps to track code execution.using registry...
shows execution is ingetContainerRegistryRemoteOptions()
, before theswitch
statementusing cloudConfig for registry authentication...
shows execution is in the first branch of theswitch
statementAdditional context
In terms of potential direction for resolution, I'd probably be most in favor of the following:
azureCloudConfig
); andimagePullSecrets
, regardless of the option's value)imagePullSecrets
, to avoid making this a breaking change. Otherwise, the breaking change should be communicated.