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

Add labels to helm templates #2265

Merged
merged 1 commit into from
Sep 9, 2021
Merged

Add labels to helm templates #2265

merged 1 commit into from
Sep 9, 2021

Conversation

Namanl2001
Copy link
Contributor

Signed-off-by: Namanl2001 namanlakhwani@gmail.com

Related issue

Fixes #2262

Checklist

  • I have read the contributing guidelines.
  • [] I have added tests that prove my fix is effective or that my feature works.
  • [] My PR contains new or altered behavior to Kyverno and
    • [] I have added or changed the documentation myself in an existing PR and the link is:
    • [] I have raised an issue in kyverno/website to track the doc update and the link is:
    • [] I have read the PR documentation guide and followed the process including adding proof manifests to this PR.

@realshuting realshuting self-assigned this Aug 18, 2021
@realshuting
Copy link
Member

Thank you @Namanl2001 for the contribution, and sorry for the late response.

Yes we need to add those labels to all Helm resources. However we don't want to update each and every label when generating a new release, i.e. app.kubernetes.io/version.

I'm not sure if it's better to use the Helm template itself or Kustomize. It would be good if can manage labels similarly with the direct manifests so that we can patch the changes to all resources, for example, we use this kustomizaition file to patch labels:

---
apiVersion: builtin
kind: LabelTransformer
metadata:
name: labelTransformer
labels:
app.kubernetes.io/component: kyverno
app.kubernetes.io/instance: kyverno
app.kubernetes.io/managed-by: Kustomize
app.kubernetes.io/name: kyverno
app.kubernetes.io/part-of: kyverno
app.kubernetes.io/version: v1.4.2
fieldSpecs:
- path: metadata/labels
create: true
- kind: Deployment
path: spec/template/metadata/labels
create: true

@realshuting realshuting added the wip work in progress label Aug 20, 2021
@Namanl2001
Copy link
Contributor Author

Namanl2001 commented Aug 23, 2021

we don't want to update each and every label when generating a new release

Yes, you're right. As we'll have to update each file at each release and I think we can somehow skip this.

I guess simply adding ../charts/kyverno/templates under resources section here should work. WDYT? And how can we test this?

@realshuting
Copy link
Member

I guess simply adding ../charts/kyverno/templates under resources section here should work.

The Helm resources are managed in ./charts, we should not touch ./definitions.

And how can we test this?

Once you add the Kustomization related manifests, you can run kustomize build ... to generate resources manifests and then install Kyverno using the local Helm Chart.

@Namanl2001
Copy link
Contributor Author

I was exploring files and found these labels, which are being patched in each helm template e.g.: here so why do we need to kustomize them?

@realshuting
Copy link
Member

I was exploring files and found these labels, which are being patched in each helm template e.g.: here so why do we need to kustomize them?

We also need to add these labels to CRD definitions which are now managed by a separate Helm Chart.

@Namanl2001
Copy link
Contributor Author

We also need to add these labels to CRD definitions which are now managed by a separate Helm Chart.

Gotta! Tried to do the same in the latest commit. Also verified it locally. Thanks

@Namanl2001
Copy link
Contributor Author

@realshuting this PR is ready for review.

Copy link
Member

@realshuting realshuting left a comment

Choose a reason for hiding this comment

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

@Namanl2001 - I think you get the idea, but how should we patch these labels to CRD manifests? Currently /charts/kyverno-crds/templates/crds.yaml is generated from ./definitions/crds:

kustomize build ./definitions/crds > ./charts/kyverno-crds/templates/crds.yaml

Should we update this Make target as well?

resources:
- ./templates/

images:
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 set image tags?

@realshuting
Copy link
Member

@Namanl2001 - please see #2274 (comment).

@Namanl2001
Copy link
Contributor Author

please see #2274 (comment).

I think they mean to say

kustomize build ./definitions/release | kustomize cfg grep kind=CustomResourceDefinition > ./charts/kyverno-crds/templates/crds.yaml

instead of

kustomize build ./release | kustomize cfg grep kind=CustomResourceDefinition > ./charts/kyverno-crds/templates/crds.yaml

@Namanl2001
Copy link
Contributor Author

Namanl2001 commented Sep 4, 2021

kustomize build ./definitions/crds > ./charts/kyverno-crds/templates/crds.yaml

Replacing the above command in makefile with kustomize build ./definitions/release | kustomize cfg grep kind=CustomResourceDefinition > ./charts/kyverno-crds/templates/crds.yaml would be fine, right? It is working as expected and it doesn't require any changes I've done in the previous commit :)

@realshuting
Copy link
Member

please see #2274 (comment).

I think they mean to say

kustomize build ./definitions/release | kustomize cfg grep kind=CustomResourceDefinition > ./charts/kyverno-crds/templates/crds.yaml

instead of

kustomize build ./release | kustomize cfg grep kind=CustomResourceDefinition > ./charts/kyverno-crds/templates/crds.yaml

Yes, it should be ./definitions/release.

I don't think we need to add kustomization file anymore, as it is already defined in definitions/release/labels.yaml off the latest main branch.

Signed-off-by: Namanl2001 <namanlakhwani@gmail.com>
@Namanl2001 Namanl2001 reopened this Sep 9, 2021
Copy link
Member

@realshuting realshuting left a comment

Choose a reason for hiding this comment

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

Thank you @Namanl2001 !

@realshuting realshuting merged commit e6f1622 into kyverno:main Sep 9, 2021
@Namanl2001 Namanl2001 deleted the labels branch September 10, 2021 17:19
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.

Labels are missing from Helm templates
2 participants