Add variable `grafana_ingress_host` to expose Grafana #468

surajssd · 2020-05-21T11:20:10Z

Fixes: #456

invidian

I think commits can be squashed for it into one.

pkg/components/prometheus-operator/template.go

docs/configuration-reference/components/prometheus-operator.md

pkg/components/prometheus-operator/component.go

pkg/components/prometheus-operator/template.go

johananl · 2020-05-26T18:59:10Z

TestAWSIngress: aws_test.go:49: got an HTTP error: Get "https://httpbin.ci1590498787-sj.test.lokomotive-k8s.net/get": dial tcp 3.121.121.100:443: connect: connection refused
    TestAWSIngress: aws_test.go:67: could not get a successful HTTP response in time

invidian

LGTM, though again, I'd squash into one commit, as both commits are related to 1 specific feature. If we revert the feature, docs should be reverted as well. Same with bisecting, if we stop at one commit, the other should be in place as well.

pkg/components/prometheus-operator/template.go

surajssd · 2020-05-28T12:47:08Z

So it was decided in a meeting that we create block for grafana like this:

component "prometheus-operator" {
...
  grafana {
    admin_password = ""

    ingress {
      host = ""

      # In future
      ingress_class = ""
      certmanager_cluster_issuer = ""
    }
  }
}

invidian · 2020-05-29T10:25:37Z

Grafana admin password should not be a compulsory option. If not provided the chart automatically creates one for you, just like Rook Ceph.

Hm, okay. I think we should have a bit more documentation how to obtain this password then, so it is not so cryptic to the users.

surajssd · 2020-05-29T10:26:31Z

Hm, okay. I think we should have a bit more documentation how to obtain this password then, so it is not so cryptic to the users.

That I plan to handle as a part of #480

iaguis · 2020-05-29T10:29:27Z

I wonder if it makes sense to allow exposing grafana and allow having an insecure default password so easily. So if a user does

  grafana {
    ingress {
      host                       = "grafana.mydomain.net"
      class                      = "contour"
      certmanager_cluster_issuer = "letsencrypt-production"
    }
  }

and this exposes grafana with admin:admin password, this is surprising and insecure. Should we do something about it?

surajssd · 2020-05-29T10:32:48Z

I wonder if it makes sense to allow exposing grafana and allow having an insecure default password so easily.

@iaguis there is a default password but it is not admin. The password if not specified is generated by helm automatically and stored in a secret, as mentioned in the docs.

iaguis · 2020-05-29T10:57:03Z

@iaguis there is a default password but it is not admin. The password if not specified is generated by helm automatically and stored in a secret, as mentioned in the docs.

Ah, all good then 👍

pkg/components/util/types.go

invidian · 2020-05-29T10:56:01Z

pkg/components/prometheus-operator/component.go

@@ -115,16 +122,26 @@ func newComponent() *component {
 				"tier":    "control-plane",
 			},
 		},
+		Grafana: &Grafana{},


Do we need it here?

Yes, we do need it. Otherwise you will see some nil pointer exceptions.

I think those can be avoided by checking them then? It doesn't make much sense to me to put it here.

Tests are also passing if I remove it.

Ah, I'm able to trigger it when I modified the tests, hmm.

Maybe we can make Grafana field not a pointer?

What's wrong with it? What do you think would be potential implications if we don't make it a pointer?

I'm not sure about the possible implications, but if it's a pointer, it means we should handle the cases when it's nil. Also, if defined here, if provides no "default" values, as we usually use newComponent for, so I'd remove it.

Please have a look at the patch I posted. It adds the handling and test for it.

invidian · 2020-05-29T11:56:34Z

How about something like this?

diff --git a/pkg/components/prometheus-operator/component.go b/pkg/components/prometheus-operator/component.go
index b3791da3..137b997f 100644
--- a/pkg/components/prometheus-operator/component.go
+++ b/pkg/components/prometheus-operator/component.go
@@ -123,7 +123,6 @@ func newComponent() *component {
                                "tier":    "control-plane",
                        },
                },
-               Grafana: &Grafana{},
        }
 }

diff --git a/pkg/components/prometheus-operator/component_test.go b/pkg/components/prometheus-operator/component_test.go
index 8e2608fb..b0b6cd39 100644
--- a/pkg/components/prometheus-operator/component_test.go
+++ b/pkg/components/prometheus-operator/component_test.go
@@ -26,18 +26,25 @@ func TestEmptyConfig(t *testing.T) {
        c := newComponent()
        emptyConfig := hcl.EmptyBody()
        evalContext := hcl.EvalContext{}
-       diagnostics := c.LoadConfig(&emptyConfig, &evalContext)

-       if diagnostics.HasErrors() {
+       if d := c.LoadConfig(&emptyConfig, &evalContext); d.HasErrors() {
                t.Fatalf("Empty config should not return errors")
        }
+
+       m, err := c.RenderManifests()
+       if err != nil {
+               t.Fatalf("Rendering manifests with valid config should succeed, got: %s", err)
+       }
+       if len(m) <= 0 {
+               t.Fatalf("Rendered manifests shouldn't be empty")
+       }
 }

 func TestRenderManifest(t *testing.T) {
        configHCL := `
 component "prometheus-operator" {
   grafana {
-       admin_password = "foobar"
+    admin_password = "foobar"
   }
   namespace = "monitoring"
 }
diff --git a/pkg/components/prometheus-operator/template.go b/pkg/components/prometheus-operator/template.go
index 3a96fe0f..7089874c 100644
--- a/pkg/components/prometheus-operator/template.go
+++ b/pkg/components/prometheus-operator/template.go
@@ -48,7 +48,6 @@ alertmanager:
               storage: "{{.AlertManagerStorageSize}}"

 grafana:
-  adminPassword: {{.Grafana.AdminPassword}}
   plugins: "grafana-piechart-panel"
   testFramework:
     enabled: false
@@ -57,6 +56,8 @@ grafana:
       searchNamespace: ALL
   rbac:
     pspUseAppArmor: false
+  {{- if .Grafana}}
+  adminPassword: {{.Grafana.AdminPassword}}
   {{ if .Grafana.Ingress }}
   ingress:
     enabled: true
@@ -71,6 +72,7 @@ grafana:
       - {{ .Grafana.Ingress.Host }}
       secretName: {{ .Grafana.Ingress.Host }}-tls
   {{ end }}
+  {{- end}}

 kubeEtcd:
   enabled: {{.Monitor.Etcd}}

iaguis

Very small nit but LGTM

pkg/components/prometheus-operator/component_test.go

surajssd · 2020-05-29T12:33:23Z

@invidian PTAL, added table driven tests. Also taken care of the test that fails.

pkg/components/prometheus-operator/component_test.go

invidian · 2020-05-29T12:41:21Z

pkg/components/prometheus-operator/component_test.go

+			t.Errorf("%s - Wrong config should have returned an error", tc.desc)
+		}
+
+		m, err := c.RenderManifests()


If I want error, meaning loading the HCL failed, this should not be executed, or?

By want error it means that the test is expecting an error.

yes, sorry. I mean, if we fail to load the HCL, the component will be in the possibly undefined state, so we should not try to render it then.

Yes changed that now it is using t.Run so now I can return using t.Fatal.

invidian

Just one nit, but otherwise LGTM

pkg/components/prometheus-operator/component.go

* Changes the UX for grafana component. Now all the grafana related information goes into a block. * Adds a new variable `grafana.ingress.host` where user can provide the ingress Host URL to expose Grafana on. * Adds a new variable `grafana.ingress.class` so that user can provide the ingress class to be used for Grafana's ingress object. * Adds a new variable `grafana.ingress.certmanager_cluster_issuer` so that user can choose the cluster issuer for certmanager so that it can generate TLS cert for Grafana's Ingress. * Adds documentation of the above variables. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

- Modify the tests to run in a table. - Remove the test which checks for empty config. It is not a failure condition and is incorporated in above table. - Add test that verifies if the user provides ingress host or not. It is a compulsory field if not provided the test should fail. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

invidian

LGTM. One last thing which can be done independently of this patch, I guess we should test if exposing ingress works as expected via e2e test?

surajssd · 2020-05-29T14:06:08Z

LGTM. One last thing which can be done independently of this patch, I guess we should test if exposing ingress works as expected via e2e test?

Testing on packet can be tricky because:

Someone has to expose the EIP on Route53.
How do we use unique EIP per CI run? And make corresponding entries?

invidian · 2020-05-29T14:07:00Z

The testing can be done on any platform. But should be done at at least one.

surajssd requested review from invidian, ipochi and johananl as code owners May 21, 2020 11:20

invidian suggested changes May 21, 2020

View reviewed changes

pkg/components/prometheus-operator/template.go Outdated Show resolved Hide resolved

docs/configuration-reference/components/prometheus-operator.md Outdated Show resolved Hide resolved

surajssd force-pushed the surajssd/add-support-for-grafana-ingress branch from 1db5fd8 to 0af84cd Compare May 21, 2020 16:26

surajssd requested a review from invidian May 21, 2020 16:26

invidian previously approved these changes May 21, 2020

View reviewed changes

johananl reviewed May 22, 2020

View reviewed changes

surajssd dismissed invidian’s stale review via 08362fe May 22, 2020 01:20

surajssd force-pushed the surajssd/add-support-for-grafana-ingress branch 2 times, most recently from 08362fe to 69b5df9 Compare May 22, 2020 01:52

surajssd requested a review from johananl May 26, 2020 05:12

surajssd mentioned this pull request May 26, 2020

docs: How to setup monitoring stack? #480

Merged

johananl reviewed May 26, 2020

View reviewed changes

docs/configuration-reference/components/prometheus-operator.md Outdated Show resolved Hide resolved

johananl reviewed May 26, 2020

View reviewed changes

pkg/components/prometheus-operator/component.go Outdated Show resolved Hide resolved

johananl reviewed May 26, 2020

View reviewed changes

pkg/components/prometheus-operator/template.go Outdated Show resolved Hide resolved

surajssd force-pushed the surajssd/add-support-for-grafana-ingress branch from 69b5df9 to c5dc6e6 Compare May 26, 2020 13:10

surajssd requested review from johananl and invidian May 26, 2020 13:10

surajssd force-pushed the surajssd/add-support-for-grafana-ingress branch 3 times, most recently from 4acff23 to 89d1350 Compare May 27, 2020 00:06

invidian suggested changes May 27, 2020

View reviewed changes

surajssd force-pushed the surajssd/add-support-for-grafana-ingress branch from 89d1350 to 6d0a61e Compare May 27, 2020 11:25

surajssd requested a review from invidian May 27, 2020 12:26

invidian previously approved these changes May 27, 2020

View reviewed changes

iaguis reviewed May 27, 2020

View reviewed changes

pkg/components/prometheus-operator/template.go Outdated Show resolved Hide resolved

surajssd dismissed invidian’s stale review via 7ab76bd May 29, 2020 05:11

surajssd force-pushed the surajssd/add-support-for-grafana-ingress branch from 35acb77 to 45a5ec3 Compare May 29, 2020 10:22

surajssd requested a review from invidian May 29, 2020 10:25

invidian suggested changes May 29, 2020

View reviewed changes

surajssd force-pushed the surajssd/add-support-for-grafana-ingress branch from 45a5ec3 to 9d7efe6 Compare May 29, 2020 11:32

iaguis previously approved these changes May 29, 2020

View reviewed changes

pkg/components/prometheus-operator/component_test.go Outdated Show resolved Hide resolved

surajssd dismissed iaguis’s stale review via 503beb8 May 29, 2020 12:27

surajssd force-pushed the surajssd/add-support-for-grafana-ingress branch from 9d7efe6 to 503beb8 Compare May 29, 2020 12:27

surajssd requested a review from invidian May 29, 2020 12:29

invidian suggested changes May 29, 2020

View reviewed changes

surajssd force-pushed the surajssd/add-support-for-grafana-ingress branch from 503beb8 to 152bc61 Compare May 29, 2020 13:35

invidian previously approved these changes May 29, 2020

View reviewed changes

pkg/components/prometheus-operator/component.go Outdated Show resolved Hide resolved

surajssd added 4 commits May 29, 2020 19:25

ci: Remove optional field Grafana password

22fd404

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

examples: Remove optional field grafana admin pass

dc247e8

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

surajssd dismissed invidian’s stale review via dc247e8 May 29, 2020 13:56

surajssd force-pushed the surajssd/add-support-for-grafana-ingress branch from 152bc61 to dc247e8 Compare May 29, 2020 13:56

invidian approved these changes May 29, 2020

View reviewed changes

iaguis approved these changes May 29, 2020

View reviewed changes

surajssd merged commit 5640eed into master May 29, 2020

surajssd deleted the surajssd/add-support-for-grafana-ingress branch May 29, 2020 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add variable `grafana_ingress_host` to expose Grafana #468

Add variable `grafana_ingress_host` to expose Grafana #468

surajssd commented May 21, 2020

invidian left a comment

johananl commented May 26, 2020

invidian left a comment

surajssd commented May 28, 2020

invidian commented May 29, 2020

surajssd commented May 29, 2020

iaguis commented May 29, 2020

surajssd commented May 29, 2020

iaguis commented May 29, 2020

invidian May 29, 2020

surajssd May 29, 2020

invidian May 29, 2020

invidian May 29, 2020

invidian May 29, 2020

invidian May 29, 2020 •

edited

Loading

surajssd May 29, 2020

invidian May 29, 2020

invidian commented May 29, 2020

iaguis left a comment

surajssd commented May 29, 2020

invidian May 29, 2020

surajssd May 29, 2020

invidian May 29, 2020

surajssd May 29, 2020

invidian left a comment

invidian left a comment

surajssd commented May 29, 2020

invidian commented May 29, 2020

Add variable grafana_ingress_host to expose Grafana #468

Add variable grafana_ingress_host to expose Grafana #468

Conversation

surajssd commented May 21, 2020

invidian left a comment

Choose a reason for hiding this comment

johananl commented May 26, 2020

invidian left a comment

Choose a reason for hiding this comment

surajssd commented May 28, 2020

invidian commented May 29, 2020

surajssd commented May 29, 2020

iaguis commented May 29, 2020

surajssd commented May 29, 2020

iaguis commented May 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invidian May 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invidian commented May 29, 2020

iaguis left a comment

Choose a reason for hiding this comment

surajssd commented May 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

surajssd commented May 29, 2020

invidian commented May 29, 2020

Add variable `grafana_ingress_host` to expose Grafana #468

Add variable `grafana_ingress_host` to expose Grafana #468

invidian May 29, 2020 •

edited

Loading