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

feat: add option to add additional custom pod labels #991

Merged

Conversation

patrick-vonsteht
Copy link
Contributor

What this PR does / why we need it:
This PR adds the option to configure additional custom pod labels for all pods created by the controller deployment.

This makes it possible to use the controller in environments that require the use of certain labels on all pods, e.g. for governance reasons. One concrete example would be the requirement to label each pod with the cost center that pays for it.

The PR adds an additionalPodLabels parameter, which is a pattern that is used by many helm charts to solve this issue.

To configure the additional labels, you need to do the following:
(A) For the helm chart deployment, add to your values.yaml:

deploy:
  additionalPodLabels:
    foo: bar
runtimeConfig:
  manager:
    additionalPodLabels:
      foo: bar

(B) For the manifest deployment, edit your manifest:

  • Add the label to the eraser-controller-manager pod template
  • Add to the contents of the eraser-manager-config config map:
manager:
  additionalPodLabels:
    foo: bar

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
None. This PR proposes this feature.

Special notes for your reviewer:
Thank you for looking at this. I'm looking forward to your feedback and hope we can add this feature to the controller.

@pmengelbert
Copy link
Contributor

@patrick-vonsteht Thanks for the PR! I'll take a look today

Copy link
Contributor

@pmengelbert pmengelbert left a comment

Choose a reason for hiding this comment

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

You will also need to run make manifests. This will generate the staging manifest and helm chart under manifest_staging.

api/v1alpha1/eraserconfig_types.go Outdated Show resolved Hide resolved
@patrick-vonsteht patrick-vonsteht force-pushed the feat/add-additional-pod-labels branch from bec7129 to 1056651 Compare February 26, 2024 16:25
@ashnamehrotra
Copy link
Contributor

@patrick-vonsteht the api changes lgtm, thanks! we will also need to change the controller code to set the labels if they are present in the config (https://github.com/eraser-dev/eraser/blob/main/controllers/imagecollector/imagecollector_controller.go#L326) before creating the ImageJob.

@patrick-vonsteht
Copy link
Contributor Author

@ashnamehrotra thank you for the hint, I'll take a look at that.

@patrick-vonsteht patrick-vonsteht force-pushed the feat/add-additional-pod-labels branch from 9737251 to ff46160 Compare March 25, 2024 13:56
@patrick-vonsteht
Copy link
Contributor Author

@ashnamehrotra I have extended the code to add the additional labels also to the ImageJob PodTemplateSpec.

@patrick-vonsteht
Copy link
Contributor Author

Looks like all the tests are successful, except the vulnerability scan. But the vulnerabilities are unrelated to my changes, because I didn't change any dependencies. How should I proceed with this?

jobTemplate := corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: labels,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

since we are also setting this in the imagejob_controller we can get rid of the functionality here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed it.

jobTemplate := corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: labels,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, we don't need to set it here since its done in the imagejob_controller already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed it.

@@ -97,6 +98,7 @@ deploy:
tag: "v1.4.0-beta.0"
additionalArgs: []
priorityClassName: ""
additionalPodLabels: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need additionalPodLabels here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal is to be able to set additionalPodLabels on every pod. So we have to handle two parts:
1.) The pod labels for the eraser-controller-manager pod. We define these in deploy.additionalPodLabels, because they are used during the deployment of the controller.
2.) The pod labels for the pods that the controller creates at runtime. We define these in runtimeConfig.manager.additionalPodLabels, because they are used during runtime of the controller.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, thanks for clarifying!

@ashnamehrotra
Copy link
Contributor

Looks like all the tests are successful, except the vulnerability scan. But the vulnerabilities are unrelated to my changes, because I didn't change any dependencies. How should I proceed with this?

I can put up a separate PR to update that vulnerability.

Patrick von Steht added 5 commits March 27, 2024 09:12
@patrick-vonsteht patrick-vonsteht force-pushed the feat/add-additional-pod-labels branch from 5cde3b4 to 5208553 Compare March 27, 2024 08:12
@sozercan
Copy link
Member

sozercan commented Mar 27, 2024

fixable vulns should already be addressed with #1002. trivy didn't had a release at the time with a fix so we can't fix that yet. vuln check is meant to be informative, and not blocking so we can ignore that for this pr specifically.

Copy link
Contributor

@ashnamehrotra ashnamehrotra left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Contributor

@pmengelbert pmengelbert left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ashnamehrotra ashnamehrotra merged commit bb740ac into eraser-dev:main Mar 28, 2024
178 of 179 checks passed
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.

4 participants