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

Support for Configuring imagePullSecrets for Collector and Agents with CRDs #3199

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

dvdlevanon-miggo
Copy link

Description: Support for Configuring imagePullSecrets for Collector and Agents with CRDs

Link to tracking Issue(s): #3127

Testing:

  • make test
  • make e2e
  • Tested imagePullSecrets locally in minikube for both Collector and Agents

Documentation: The api.md generated and pushed.

This pull request adds the imagePullSecrets to the CRDs of both the Collector and Instrumentation. I have followed the contribution guidelines and have run make test, make lint, make fmt, and other necessary checks.

However, I am unsure about the process for updating the CRDs in the Helm charts repository. Does this update occur automatically based on the CRDs generated in this repository?

Additionally, I have the same question regarding the documentation. What is the process for updating the Helm charts and the corresponding documentation? I would appreciate any guidance on how to proceed with these updates.

@dvdlevanon-miggo dvdlevanon-miggo requested a review from a team as a code owner August 6, 2024 15:06
Copy link

linux-foundation-easycla bot commented Aug 6, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@jaronoff97
Copy link
Contributor

@dvdlevanon-miggo thank you for your PR :D Can you make sure to sign the CLA so we can run the rest of the test suite?

@jaronoff97
Copy link
Contributor

@dvdlevanon-miggo for your helm-charts question... that gets updated after the operator is released by the release manager, no need for you to worry about it :)

// +optional
// +listType=map
// +listMapKey=name
ImagePullSecrets []v1.LocalObjectReference `json:"imagePullSecrets,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be a list? It is related to a single image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The official Kubernetes Pod API uses a list for imagePullSecrets. Aligning with this standard ensures consistency and compatibility with existing Kubernetes practices, allowing for greater flexibility and future-proofing the configuration.

Comment on lines +170 to +176
// ImagePullSecrets is an optional list of references to secrets in the same namespace to use for pulling any of the images used by this PodSpec.
// If specified, these secrets will be passed to individual puller implementations for them to use.
// More info: https://kubernetes.io/docs/concepts/containers/images#specifying-imagepullsecrets-on-a-pod
// +optional
// +listType=map
// +listMapKey=name
ImagePullSecrets []v1.LocalObjectReference `json:"imagePullSecrets,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v1alpha1 is frozen at this point, we should only add this field to the v1beta1 CRD.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it

@jaronoff97
Copy link
Contributor

@dvdlevanon-miggo looks like some failing unit tests

change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
component: crds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be collector, auto-instrumentation

component: crds

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Support for Configuring imagePullSecrets for Collector and Agents with CRDs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Support for configuring imagePullSecrets for collector and auto-instrumentation images

@@ -39,6 +40,11 @@ func DaemonSet(params manifests.Params) (*appsv1.DaemonSet, error) {
return nil, err
}

imagePullSecrets := params.OtelCol.Spec.ImagePullSecrets
if params.OtelCol.Spec.ImagePullSecrets == nil {
imagePullSecrets = []v1.LocalObjectReference{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to assign it an empty list if it is null? This does not seem necessary

@pavolloffay
Copy link
Member

The CI failed

@jaronoff97
Copy link
Contributor

@dvdlevanon @dvdlevanon-miggo i'd love to get this merged, would you be able to fix the failing tests so we can get this?

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.

Support for Configuring imagePullSecrets for Collector and Agents without ServiceAccount
4 participants