Add image pull secrets to Jaeger resources

[Saad-Hussain1]

Context: #1018

Adds support for k8s imagePullSecrets to Jaeger resources.

Associated PR to update documentation: jaegertracing/documentation#416

[codecov]

Codecov Report

Merging #1103 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1103      +/-   ##
==========================================
+ Coverage   88.05%   88.10%   +0.05%     
==========================================
  Files          86       86              
  Lines        5274     5297      +23     
==========================================
+ Hits         4644     4667      +23     
  Misses        466      466              
  Partials      164      164              

Impacted Files	Coverage Δ
pkg/apis/jaegertracing/v1/jaeger_types.go	100.00% <ø> (ø)
pkg/cronjob/es_index_cleaner.go	100.00% <100.00%> (ø)
pkg/cronjob/es_rollover.go	95.93% <100.00%> (+0.03%)	⬆️
pkg/cronjob/spark_dependencies.go	92.38% <100.00%> (+0.07%)	⬆️
pkg/deployment/agent.go	96.24% <100.00%> (+0.02%)	⬆️
pkg/deployment/all_in_one.go	100.00% <100.00%> (ø)
pkg/deployment/collector.go	96.81% <100.00%> (+0.02%)	⬆️
pkg/deployment/ingester.go	96.29% <100.00%> (+0.02%)	⬆️
pkg/deployment/query.go	100.00% <100.00%> (ø)
pkg/inject/sidecar.go	94.41% <100.00%> (+0.02%)	⬆️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cde42a...36bd00c. Read the comment docs.

[jpkrohling]

We really should document this, as we are changing a deployment that isn't being managed by us.

[Saad-Hussain1]

Hm now that you mention that, I wonder if we should change
dep.Spec.Template.Spec.ImagePullSecrets = util.RemoveDuplicatedImagePullSecrets(append(dep.Spec.Template.Spec.ImagePullSecrets, commonSpec.ImagePullSecrets...))
to
dep.Spec.Template.Spec.ImagePullSecrets = util.RemoveDuplicatedImagePullSecrets(append(dep.Spec.Template.Spec.ImagePullSecrets, jaeger.Spec.Agent.JaegerCommonSpec.ImagePullSecrets...))

i.e., don't use the imagePullSecrets from the commonSpec, but rather only the ones under the agent spec, so that we don't add all the imagePullSecrets for the rest of the jaeger components to a deployment which isn't managed by the jaeger-operator. We would only add an imagePullSecret to the deployment if someone specifically adds it to the agent spec, which would then be more justified since it would seem to be a more intentional decision.

---

If we do make that ^ change, do you think we should still document this?
If we should still document, do you mean adding a comment, adding a log message, or updating documentation?

[jpkrohling]

It should be a user documentation, probably under the sidecar part: https://www.jaegertracing.io/docs/1.18/operator/#auto-injecting-jaeger-agent-sidecars

[jpkrohling]

Looks good to me! The only missing bits seems to be around the auto-provisioned ES and Kafka. Do you know whether their CRs also support customizing the image pull secret?

[Saad-Hussain1]

@jpkrohling ahmm I'm not too sure actually... I'm admittedly not too familiar with them since I've only really used the agent, collector, and query.

[jpkrohling]

@Saad-Hussain1 would you be able to check that? This PR looks good to me, but it would be good to know in advance if we need to extend this feature to include ES/Kafka, or if this feature is complete.

One more thing I'd like to ask: could you please open an issue to document this new field?

[Saad-Hussain1]

@jpkrohling Alright I'll try to look into it.

Is something missing from this issue: #1018?
If I do need to open another issue, what would I need to include in it which that one ^ lacks?

[jpkrohling]

If I do need to open another issue, what would I need to include in it which that one ^ lacks?

Sorry, I meant an issue on https://github.com/jaegertracing/documentation, so that we don't forget to let users know about this behavior.

[jpkrohling]

I just realized that we might not need this after all... Given that we already support overriding the service account, and service accounts might contain image pull secrets, one could achieve the goal of this PR by just setting the appropriate service account to the component/spec. Like:

apiVersion: v1
kind: ServiceAccount
metadata:
  name: my-sa
imagePullSecrets:
- name: my-pull-secret

apiVersion: jaegertracing.io/v1
kind: "Jaeger"
metadata:
  name: "my-jaeger"
spec:
  serviceAccount: my-sa

Or am I missing something?

[Saad-Hussain1]

@jpkrohling :O I actually did not know about that, but it does seem that that works! The original response to the git issue also made me think that this wasn't supported in any way, but I didn't know that imagePullSecrets could be added to serviceAccounts. I guess majority of this PR is actually unnecessary then.

The only issue I have remaining is with the agent sidecar. It doesn't seem like serviceAccounts are supported for it, probably because the original deployment might already be using its own serviceAccount? Perhaps I can make a new PR to add imagePullSecrets to the sidecar's Deployment? Like this: #1115

[jpkrohling]

I'm closing this PR in favor of #1115. Hope you did have fun while working on this, though!