Implement adding imagePullSecrets to Pod resources. #1671
Conversation
| } | ||
| err = r.updatePodSpecWithPullSecrets(podSpec) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
maybe we should not error if this fails to update the pod spec? Maybe that's intentional, for example, we are failing if the pod doesn't contain containers or an image but it's possible to define a MutatingAdmissionWebhook to modify the object on the fly and add the required info. While this is improbable maybe we should not assume that the manifest is malformed?
There was a problem hiding this comment.
maybe we should not error if this fails to update the pod spec?
Perhaps - yes. So it'd be pretty dangerous to hide errors like this, in that it creates very hard-to debug issues (or even realizing they exist), but...
Maybe that's intentional, for example, we are failing if the pod doesn't contain containers or an image but it's possible to define a MutatingAdmissionWebhook to modify the object on the fly and add the required info. While this is improbable maybe we should not assume that the manifest is malformed?
The MutatingAdmissionWebhook isn't intended to make an invalid document (something which can't validate against the API) into a valid document, but rather it can adjust values on an already valid document. (Though yes, if the yaml isn't validated with the API beforehand, it could indeed be used that way.) With this assumption, then your question does make me wonder whether, if a helm chart renders an invalid template, we should allow the k8s API server to report that as per normal, rather than our post renderer getting in the way.
So yes, for different reasons, I agree: the post renderer should not error if the yaml is invalid. Let me change that. I've updated so that each anomaly is logged. An error is only returned by the post renderer if the manifests cannot be parsed (or re-rendered).
Thanks!
| if !ok { | ||
| return fmt.Errorf("pod spec did not include a containers key: %+v", podSpec) | ||
| } | ||
| containers, ok := containersObject.([]interface{}) |
There was a problem hiding this comment.
there is a package in client-go that can help with this kind of conversions and parameters retrieval: https://godoc.org/github.com/contiv/client-go/pkg/apis/meta/v1/unstructured
Also, I would try to use the actual struct types of the resources we are handling instead keep using type conversions from interface{} and literal keys. It's easier to just access podSpec.Containers[0].Image rather than doing several conversions. It's even more useful when we add more complex types like a CronJob in which I don't necessarily know the full path till the image name.
There was a problem hiding this comment.
there is a package in
client-gothat can help with this kind of conversions and parameters retrieval: https://godoc.org/github.com/contiv/client-go/pkg/apis/meta/v1/unstructured
You meant to link to the one owned by kubernetes in apimachinery (rather than contiv) right? https://godoc.org/github.com/kubernetes/apimachinery/pkg/apis/meta/v1/unstructured
If Helm's API passed a JSON stream, rather than a YAML stream, I could use that nicely. But as it is, I'd either need to convert the YAML to JSON to parse into the unstructured.Unstructured, update, then marshal it out to JSON (using unstructured and then back to YAML, or explicitly convert the map[interface{}]interface{} to map[string]interface{} to be able to use unstructured.Unstructured.SetUnstructuredContent() to create the object in the first place. Let me know if I've missed something - happy to try.
Also, I would try to use the actual struct types of the resources we are handling instead keep using type conversions from
interface{}and literal keys. It's easier to just accesspodSpec.Containers[0].Imagerather than doing several conversions.
Yes, I had gone this way initially for the same reason, but realized that we'd then be making assumptions about the templated YAML that are not ours to make (ie. which client-go version we're using affects the resources that we import). IMO we should be heading the other way - ideally we would not change the rest of the YAML at all (no changes in ordering, no re-rendering except the addition of the image pull secret), but to do that is a little more complex.
It's even more useful when we add more complex types like a
CronJobin which I don't necessarily know the full path till the image name.
They're actually pretty trivial - they just have a spec.template.spec as do all the others (I left a comment linking to https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podtemplatespec-v1-core ).
| return fmt.Errorf("pod container did not contain an image reference: %+v", container) | ||
| } | ||
|
|
||
| // Ignore (dockerhub) image refs without a domain. |
There was a problem hiding this comment.
This comment makes more sense within the if block below. Also, what happens if the user wants to add an imagePullSecret for a private image in the dockerhub?
There was a problem hiding this comment.
So it turns out that for dockerhub the secret's registry domain will be the FQDN: https://index.docker.io/v1 . I'll deal with this in a followup.
| } | ||
| imageDomain := domainSplit[0] | ||
|
|
||
| secretName, ok := r.secrets[imageDomain] |
There was a problem hiding this comment.
I am not sure if we are just interested in a domain? For example, for a gcr container (gcr.io/my_project/image:tag), should we check gcr.io or gcr.io/my-project?
There was a problem hiding this comment.
Docker credentials are per server (though it turns out this can be the domain name or the fqdn of the server).
| } | ||
| }) | ||
| } | ||
| } |
Ref #1617. Implements adding imagePullSecrets to Pod resources (only - I'll update to handle other resources which also include pod templates).