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.

Sign up for GitHub

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

[RS-2297] Run dashboard-installer on operator update #3759

Closed
wants to merge 6 commits into from
Closed

[RS-2297] Run dashboard-installer on operator update #3759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5947b1e
Create new dashboard-installer job on upgrade
Dean-Coakley Jan 29, 2025
84b6f1e
Fix comment typo
Dean-Coakley Jan 29, 2025
0272721
Deal with long dev image names
Dean-Coakley Jan 29, 2025
de29769
Fix up tests for dyn name
Dean-Coakley Feb 5, 2025
baf8ba0
Update selector so policy is static
Dean-Coakley Feb 5, 2025
a0f1ae7
Make podName dyn and containerName static
Dean-Coakley Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/controller/logstorage/dashboards/dashboards_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2024 Tigera, Inc. All rights reserved.
// Copyright (c) 2024-2025 Tigera, Inc. All rights reserved.

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -151,7 +151,7 @@ func Add(mgr manager.Manager, opts options.AddOptions) error {
// Check if something modifies resources this controller creates.
err = c.WatchObject(&batchv1.Job{ObjectMeta: metav1.ObjectMeta{
Namespace: helper.InstallNamespace(),
Name: dashboards.Name,
Name: dashboards.GetJobName(),
}}, &handler.EnqueueRequestForObject{})
if err != nil {
return fmt.Errorf("log-storage-dashboards-controller failed to watch installer job: %v", err)
Expand Down
8 changes: 4 additions & 4 deletions pkg/controller/logstorage/dashboards/dashboards_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2024 Tigera, Inc. All rights reserved.
// Copyright (c) 2024-2025 Tigera, Inc. All rights reserved.

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -207,7 +207,7 @@ var _ = Describe("LogStorage Dashboards controller", func() {
dashboardJob := batchv1.Job{
TypeMeta: metav1.TypeMeta{Kind: "Job", APIVersion: "batch/v1"},
ObjectMeta: metav1.ObjectMeta{
Name: dashboards.Name,
Name: dashboards.GetJobName(),
Namespace: render.ElasticsearchNamespace,
},
}
Expand All @@ -231,7 +231,7 @@ var _ = Describe("LogStorage Dashboards controller", func() {
dashboardJob := batchv1.Job{
TypeMeta: metav1.TypeMeta{Kind: "Job", APIVersion: "batch/v1"},
ObjectMeta: metav1.ObjectMeta{
Name: dashboards.Name,
Name: dashboards.GetJobName(),
Namespace: render.ElasticsearchNamespace,
},
}
Expand All @@ -257,7 +257,7 @@ var _ = Describe("LogStorage Dashboards controller", func() {
dashboardJob := batchv1.Job{
TypeMeta: metav1.TypeMeta{Kind: "Job", APIVersion: "batch/v1"},
ObjectMeta: metav1.ObjectMeta{
Name: dashboards.Name,
Name: dashboards.GetJobName(),
Namespace: render.ElasticsearchNamespace,
},
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/render/common/networkpolicy/networkpolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2022-2024 Tigera, Inc. All rights reserved.
// Copyright (c) 2022-2025 Tigera, Inc. All rights reserved.

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -211,7 +211,7 @@ var KubeAPIServerServiceSelectorEntityRule = v3.EntityRule{
// Helper creates a helper for building network policies for multi-tenant capable components.
// It takes two arguments:
// - mt: true if running in multi-tenant mode, false otherwise.
// - ns: The tenant's namespce.
// - ns: The tenant's namespace.
func Helper(mt bool, ns string) *NetworkPolicyHelper {
return &NetworkPolicyHelper{
multiTenant: mt,
Expand Down
23 changes: 18 additions & 5 deletions pkg/render/logstorage/dashboards/dashboards.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2024 Tigera, Inc. All rights reserved.
// Copyright (c) 2024-2025 Tigera, Inc. All rights reserved.

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -41,6 +41,7 @@ import (
"github.com/tigera/operator/pkg/render/logstorage"
"github.com/tigera/operator/pkg/render/logstorage/kibana"
"github.com/tigera/operator/pkg/tls/certificatemanagement"
"github.com/tigera/operator/version"
)

var (
Expand All @@ -49,6 +50,18 @@ var (
PolicyName = networkpolicy.TigeraComponentPolicyPrefix + Name
)

// GetJobName makes a unique job name per operator version.
// For dev images it takes the first 32 chars.
func GetJobName() string {
operatorVersion := strings.ReplaceAll(version.VERSION, ".", "-")
Copy link
Member

Choose a reason for hiding this comment

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

If we're switching to this, we should stop exporting the Name variable so that nobody accidentally uses the wrong value.

Copy link
Member

Choose a reason for hiding this comment

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

If Name isn't actually used except for within this function, we should probably just scope it to this function to be extra sure (i.e., remove the package level variable)


if len(operatorVersion) >= 32 {
operatorVersion = operatorVersion[:32]
Copy link
Member

Choose a reason for hiding this comment

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

What is "32" in this context? Why do we need to truncate? I think we should have a comment explaining why we do this - it's not obvious!

Copy link
Contributor Author

@Dean-Coakley Dean-Coakley Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think without truncation the job name ended up being 80+ characters and I think Job names are limited to 63.

Good idea to explain in a comment.

Maybe I can also include an example in the comment, for example:
v1.36.10-1.dev-281-g9558bdac82be-2025-01-28-master-treachery is truncated to be v1.36.10-1.dev-281-g9558bdac82be

}

return fmt.Sprintf("dashboards-installer-%s", operatorVersion)
}

func Dashboards(c *Config) render.Component {
return &dashboards{
cfg: c,
Expand Down Expand Up @@ -169,7 +182,7 @@ func (d *dashboards) AllowTigeraPolicy() *v3.NetworkPolicy {
Spec: v3.NetworkPolicySpec{
Order: &networkpolicy.HighPrecedenceOrder,
Tier: networkpolicy.TigeraComponentTierName,
Selector: fmt.Sprintf("job-name == '%s'", Name),
Selector: fmt.Sprintf("job-name contains '%s'", Name),
Types: []v3.PolicyType{v3.PolicyTypeEgress},
Egress: egressRules,
},
Expand Down Expand Up @@ -261,7 +274,7 @@ func (d *dashboards) Job() *batchv1.Job {

podTemplate := relasticsearch.DecorateAnnotations(&corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{"job-name": Name, "k8s-app": Name},
Labels: map[string]string{"job-name": GetJobName(), "k8s-app": GetJobName()},
Annotations: annotations,
},
Spec: corev1.PodSpec{
Expand All @@ -288,13 +301,13 @@ func (d *dashboards) Job() *batchv1.Job {
job := &batchv1.Job{
TypeMeta: metav1.TypeMeta{Kind: "Job", APIVersion: "batch/v1"},
ObjectMeta: metav1.ObjectMeta{
Name: Name,
Name: GetJobName(),
Namespace: d.cfg.Namespace,
},
Spec: batchv1.JobSpec{
Selector: &metav1.LabelSelector{
MatchLabels: map[string]string{
"job-name": Name,
"job-name": GetJobName(),
},
},
Template: *podTemplate,
Expand Down
40 changes: 20 additions & 20 deletions pkg/render/logstorage/dashboards/dashboards_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2024 Tigera, Inc. All rights reserved.
// Copyright (c) 2024-2025 Tigera, Inc. All rights reserved.

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -69,7 +69,7 @@ var _ = Describe("Dashboards rendering tests", func() {
expectedResources := []resourceTestObj{
{PolicyName, render.ElasticsearchNamespace, &v3.NetworkPolicy{}, nil},
{Name, render.ElasticsearchNamespace, &corev1.ServiceAccount{}, nil},
{Name, render.ElasticsearchNamespace, &batchv1.Job{}, nil},
{GetJobName(), render.ElasticsearchNamespace, &batchv1.Job{}, nil},
{Name, "", &rbacv1.ClusterRole{}, nil},
{Name, "", &rbacv1.ClusterRoleBinding{}, nil},
}
Expand Down Expand Up @@ -109,7 +109,7 @@ var _ = Describe("Dashboards rendering tests", func() {
Expect(component.ResolveImages(nil)).To(BeNil())
resources, _ := component.Objects()

role := rtest.GetResource(resources, "dashboards-installer", "", "rbac.authorization.k8s.io", "v1", "ClusterRole").(*rbacv1.ClusterRole)
role := rtest.GetResource(resources, Name, "", "rbac.authorization.k8s.io", "v1", "ClusterRole").(*rbacv1.ClusterRole)
Expect(role.Rules).To(ContainElement(rbacv1.PolicyRule{
APIGroups: []string{"security.openshift.io"},
Resources: []string{"securitycontextconstraints"},
Expand All @@ -124,7 +124,7 @@ var _ = Describe("Dashboards rendering tests", func() {
component := Dashboards(cfg)

resources, _ := component.Objects()
job, ok := rtest.GetResource(resources, Name, render.ElasticsearchNamespace, "batch", "v1", "Job").(*batchv1.Job)
job, ok := rtest.GetResource(resources, GetJobName(), render.ElasticsearchNamespace, "batch", "v1", "Job").(*batchv1.Job)
Expect(ok).To(BeTrue(), "Job not found")
Expect(job.Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
})
Expand All @@ -135,7 +135,7 @@ var _ = Describe("Dashboards rendering tests", func() {
component := Dashboards(cfg)

resources, _ := component.Objects()
job, ok := rtest.GetResource(resources, Name, render.ElasticsearchNamespace, "batch", "v1", "Job").(*batchv1.Job)
job, ok := rtest.GetResource(resources, GetJobName(), render.ElasticsearchNamespace, "batch", "v1", "Job").(*batchv1.Job)
Expect(ok).To(BeTrue(), "Job not found")
Expect(job.Spec.Template.Spec.Tolerations).To(ContainElement(corev1.Toleration{
Key: "kubernetes.io/arch",
Expand All @@ -156,7 +156,7 @@ var _ = Describe("Dashboards rendering tests", func() {
component := Dashboards(cfg)

resources, _ := component.Objects()
job, ok := rtest.GetResource(resources, Name, render.ElasticsearchNamespace, "batch", "v1", "Job").(*batchv1.Job)
job, ok := rtest.GetResource(resources, GetJobName(), render.ElasticsearchNamespace, "batch", "v1", "Job").(*batchv1.Job)
Expect(ok).To(BeTrue(), "Job not found")
Expect(job.Spec.Template.Spec.Tolerations).To(ConsistOf(t))
})
Expand Down Expand Up @@ -252,7 +252,7 @@ var _ = Describe("Dashboards rendering tests", func() {
}
component := Dashboards(cfg)
createResources, _ := component.Objects()
d, ok := rtest.GetResource(createResources, Name, cfg.Namespace, "batch", "v1", "Job").(*batchv1.Job)
d, ok := rtest.GetResource(createResources, GetJobName(), cfg.Namespace, "batch", "v1", "Job").(*batchv1.Job)
Expect(ok).To(BeTrue(), "Job not found")

// The deployment should have the hash annotation set, as well as a volume and volume mount for the client secret.
Expand Down Expand Up @@ -287,19 +287,19 @@ var _ = Describe("Dashboards rendering tests", func() {
component := Dashboards(cfg)
Expect(component).NotTo(BeNil())
resources, _ := component.Objects()
job := rtest.GetResource(resources, Name, cfg.Namespace, batchv1.GroupName, "v1", "Job").(*batchv1.Job)
job := rtest.GetResource(resources, GetJobName(), cfg.Namespace, batchv1.GroupName, "v1", "Job").(*batchv1.Job)
Expect(job).NotTo(BeNil())
sa := rtest.GetResource(resources, Name, cfg.Namespace, corev1.GroupName, "v1", "ServiceAccount").(*corev1.ServiceAccount)
Expect(sa).NotTo(BeNil())
netPol := rtest.GetResource(resources, fmt.Sprintf("allow-tigera.%s", Name), cfg.Namespace, "projectcalico.org", "v3", "NetworkPolicy").(*v3.NetworkPolicy)
netPol := rtest.GetResource(resources, PolicyName, cfg.Namespace, "projectcalico.org", "v3", "NetworkPolicy").(*v3.NetworkPolicy)
Expect(netPol).NotTo(BeNil())
})

It("should render multi-tenant environment variables", func() {
component := Dashboards(cfg)
Expect(component).NotTo(BeNil())
resources, _ := component.Objects()
job := rtest.GetResource(resources, Name, cfg.Namespace, batchv1.GroupName, "v1", "Job").(*batchv1.Job)
job := rtest.GetResource(resources, GetJobName(), cfg.Namespace, batchv1.GroupName, "v1", "Job").(*batchv1.Job)
envs := job.Spec.Template.Spec.Containers[0].Env
Expect(envs).To(ContainElement(corev1.EnvVar{Name: "KIBANA_SPACE_ID", Value: cfg.Tenant.Spec.ID}))
Expect(envs).To(ContainElement(corev1.EnvVar{Name: "KIBANA_SCHEME", Value: "https"}))
Expand Down Expand Up @@ -336,7 +336,7 @@ var _ = Describe("Dashboards rendering tests", func() {
component := Dashboards(cfg)

resources, _ := component.Objects()
job := rtest.GetResource(resources, Name, cfg.Namespace, batchv1.GroupName, "v1", "Job").(*batchv1.Job)
job := rtest.GetResource(resources, GetJobName(), cfg.Namespace, batchv1.GroupName, "v1", "Job").(*batchv1.Job)
Expect(job.Spec.Template.Spec.Containers).To(HaveLen(1))
Expect(job.Spec.Template.Spec.Containers[0].Name).To(Equal(Name))
Expect(job.Spec.Template.Spec.Containers[0].Resources).To(Equal(dashboardsJobResources))
Expand Down Expand Up @@ -394,7 +394,7 @@ var _ = Describe("Dashboards rendering tests", func() {
}
component := Dashboards(cfg)
createResources, _ := component.Objects()
d, ok := rtest.GetResource(createResources, Name, render.ElasticsearchNamespace, "batch", "v1", "Job").(*batchv1.Job)
d, ok := rtest.GetResource(createResources, GetJobName(), render.ElasticsearchNamespace, "batch", "v1", "Job").(*batchv1.Job)
Expect(ok).To(BeTrue(), "Job not found")

// The deployment should have the hash annotation set, as well as a volume and volume mount for the client secret.
Expand Down Expand Up @@ -423,7 +423,7 @@ var _ = Describe("Dashboards rendering tests", func() {
Expect(d.Spec.Template.Spec.Containers[0].Env).To(ContainElement(corev1.EnvVar{
Name: "KIBANA_MTLS_ENABLED", Value: "true",
}))
netPol := rtest.GetResource(createResources, fmt.Sprintf("allow-tigera.%s", Name), render.ElasticsearchNamespace, "projectcalico.org", "v3", "NetworkPolicy").(*v3.NetworkPolicy)
netPol := rtest.GetResource(createResources, PolicyName, render.ElasticsearchNamespace, "projectcalico.org", "v3", "NetworkPolicy").(*v3.NetworkPolicy)
Expect(netPol).NotTo(BeNil())
Expect(netPol.Spec.Egress).To(HaveLen(2))
Expect(netPol.Spec.Egress[1].Destination).To(Equal(v3.EntityRule{
Expand All @@ -436,11 +436,11 @@ var _ = Describe("Dashboards rendering tests", func() {
component := Dashboards(cfg)
Expect(component).NotTo(BeNil())
resources, _ := component.Objects()
job := rtest.GetResource(resources, Name, render.ElasticsearchNamespace, batchv1.GroupName, "v1", "Job").(*batchv1.Job)
job := rtest.GetResource(resources, GetJobName(), render.ElasticsearchNamespace, batchv1.GroupName, "v1", "Job").(*batchv1.Job)
Expect(job).NotTo(BeNil())
sa := rtest.GetResource(resources, Name, render.ElasticsearchNamespace, corev1.GroupName, "v1", "ServiceAccount").(*corev1.ServiceAccount)
Expect(sa).NotTo(BeNil())
netPol := rtest.GetResource(resources, fmt.Sprintf("allow-tigera.%s", Name), render.ElasticsearchNamespace, "projectcalico.org", "v3", "NetworkPolicy").(*v3.NetworkPolicy)
netPol := rtest.GetResource(resources, PolicyName, render.ElasticsearchNamespace, "projectcalico.org", "v3", "NetworkPolicy").(*v3.NetworkPolicy)
Expect(netPol).NotTo(BeNil())
Expect(netPol.Spec.Egress).To(HaveLen(2))
Expect(netPol.Spec.Egress[1].Destination).To(Equal(kibana.EntityRule))
Expand All @@ -450,7 +450,7 @@ var _ = Describe("Dashboards rendering tests", func() {
component := Dashboards(cfg)
Expect(component).NotTo(BeNil())
resources, _ := component.Objects()
d := rtest.GetResource(resources, Name, cfg.Namespace, batchv1.GroupName, "v1", "Job").(*batchv1.Job)
d := rtest.GetResource(resources, GetJobName(), cfg.Namespace, batchv1.GroupName, "v1", "Job").(*batchv1.Job)
envs := d.Spec.Template.Spec.Containers[0].Env
Expect(envs).To(ContainElement(corev1.EnvVar{Name: "KIBANA_SPACE_ID", Value: cfg.Tenant.Spec.ID}))
Expect(envs).To(ContainElement(corev1.EnvVar{Name: "KIBANA_SCHEME", Value: "https"}))
Expand Down Expand Up @@ -499,19 +499,19 @@ var _ = Describe("Dashboards rendering tests", func() {
component := Dashboards(cfg)
Expect(component).NotTo(BeNil())
resources, _ := component.Objects()
job := rtest.GetResource(resources, Name, render.ElasticsearchNamespace, batchv1.GroupName, "v1", "Job").(*batchv1.Job)
job := rtest.GetResource(resources, GetJobName(), render.ElasticsearchNamespace, batchv1.GroupName, "v1", "Job").(*batchv1.Job)
Expect(job).NotTo(BeNil())
sa := rtest.GetResource(resources, Name, render.ElasticsearchNamespace, corev1.GroupName, "v1", "ServiceAccount").(*corev1.ServiceAccount)
Expect(sa).NotTo(BeNil())
netPol := rtest.GetResource(resources, fmt.Sprintf("allow-tigera.%s", Name), render.ElasticsearchNamespace, "projectcalico.org", "v3", "NetworkPolicy").(*v3.NetworkPolicy)
netPol := rtest.GetResource(resources, PolicyName, render.ElasticsearchNamespace, "projectcalico.org", "v3", "NetworkPolicy").(*v3.NetworkPolicy)
Expect(netPol).NotTo(BeNil())
})

It("should render single-tenant environment variables", func() {
component := Dashboards(cfg)
Expect(component).NotTo(BeNil())
resources, _ := component.Objects()
d := rtest.GetResource(resources, Name, cfg.Namespace, batchv1.GroupName, "v1", "Job").(*batchv1.Job)
d := rtest.GetResource(resources, GetJobName(), cfg.Namespace, batchv1.GroupName, "v1", "Job").(*batchv1.Job)
envs := d.Spec.Template.Spec.Containers[0].Env
Expect(envs).To(ContainElement(corev1.EnvVar{Name: "KIBANA_SPACE_ID", Value: cfg.Tenant.Spec.ID}))
Expect(envs).To(ContainElement(corev1.EnvVar{Name: "KIBANA_SCHEME", Value: "https"}))
Expand Down Expand Up @@ -551,7 +551,7 @@ func compareResources(resources []client.Object, expectedResources []resourceTes
}

// Check job
job := rtest.GetResource(resources, Name, render.ElasticsearchNamespace, "batch", "v1", "Job").(*batchv1.Job)
job := rtest.GetResource(resources, GetJobName(), render.ElasticsearchNamespace, "batch", "v1", "Job").(*batchv1.Job)
ExpectWithOffset(1, job).NotTo(BeNil())

// Check containers
Expand Down
2 changes: 1 addition & 1 deletion pkg/render/testutils/expected_policies/dashboards.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"spec": {
"order": 1,
"tier": "allow-tigera",
"selector": "job-name == 'dashboards-installer'",
"selector": "job-name contains 'dashboards-installer'",
Copy link
Member

@caseydavenport caseydavenport Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should be using a different selector rather than the "contains" operator here. There's no reason we need to use the job-name label AFAIK - we could have role: dashboard-installer applied regardless of the actual name, for example.

My concern is just that contains is a pretty unusual operation, and given there's no concrete need for it here it makes this policy less intuitive.

"types": [
"Egress"
],
Expand Down
2 changes: 1 addition & 1 deletion pkg/render/testutils/expected_policies/dashboards_ocp.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"spec": {
"order": 1,
"tier": "allow-tigera",
"selector": "job-name == 'dashboards-installer'",
"selector": "job-name contains 'dashboards-installer'",
"types": [
"Egress"
],
Expand Down
Loading