feat: allow installing hubble ui as standalone #17473

eddycharly · 2021-09-26T20:31:30Z

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Thanks for contributing!

This PR allows installing hubble ui as standalone.

Sometimes, a cluster comes with culium and hubble relay already provisioned (kOps for example manages cilium and hubble relay). In this context, installing Hubble ui on top of the already installed components using the cilium helm chart would be very handy.

maintainer-s-little-helper · 2021-09-26T20:31:31Z

Commit efaab453d6183f532a650be6ac5c3fb0c3cbd502 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2021-09-27T12:01:14Z

Commit b31591c1edb5041af88478d477bc7c9a64e3eab1 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

eddycharly · 2021-10-01T08:59:45Z

Hello, is there anything I can do to help moving forward ?

kaworu

Hi @eddycharly and thank you for the PR.

Overall LGTM, but we need to figure out whether we need to generate the certificates for UI or not when .Values.hubble.ui.allowStandalone is set.

Also there is this PR which aim to accomplish about the same thing (although in a different way). Personally, I like the explicit .Values.hubble.ui.allowStandalone of this PR but let's see what the team think about it.

/cc @cilium/helm

install/kubernetes/cilium/values.yaml

eddycharly · 2021-10-04T14:31:16Z

@kaworu i pushed a version addressing the certs related comment.

If hubble relay comes pre installed and tls is enabled on the server side, it should be the responsibility of the end user to provide the client certificates to hubble ui when installing it.

For this, i added a certsVolume stanza in the values file.

An end user can install hubble ui with the following values file for example:

agent: false

operator:
  enabled: false

cni:
  install: false

hubble:
  enabled: false

  relay:
    enabled: false
    tls:
      server:
        enabled: true

  ui:
    enabled: true
    standalone:
      enabled: true
      certsVolume:
        projected:
          defaultMode: 420
          sources:
          - secret:
              name: my-hubble-ui-client-certs
              items:
              - key: tls.crt
                path: client.crt
              - key: tls.key
                path: client.key
              - key: ca.crt
                path: hubble-relay-ca.crt

Of course this has to match the Hubble Relay config and is not trivial but i guess no magic can be done here, except allowing the end user to configure it and hope he's got the config right.

At least i'm checking that a certs volume is provided when in standalone and relay tls is enabled.

WDYT ?

eddycharly · 2021-10-06T11:56:32Z

@kaworu do you have comments on the new changes related to certificates ?

eddycharly · 2021-10-11T07:12:10Z

Hello, any chance to get more review on this PR ?
Let me know if I can do anything to help moving forward 🙏

kaworu

Hi @eddycharly,

The patch overall LGTM. I think we need to add documentation for this as this is a non-trivial use-case of install Hubble UI. I don't know where is the best place in the documentation for such a custom installation so I'm deferring to @cilium/docs-structure here.

install/kubernetes/cilium/values.yaml

eddycharly · 2021-10-11T19:04:44Z

Thanks @kaworu i'll try to find my way through the documentation process.

eddycharly · 2021-10-12T11:56:46Z

@kaworu I added the section below in the README of the chart. WDYT ?

Installing Hubble UI in standalone

Clusters sometimes come with Cilium, Hubble and Hubble relay already installed.
When this is the case you can still use this helm chart to install only Hubble UI on top of the pre installed components.

You will need to set hubble.ui.standalone.enabled to true and optionally provide a volume to mount Hubble relay client certificates
if tls is enabled on Hubble relay server side.

Below is an example deploying Hubble UI as standalone, with client certificates mounted from a my-hubble-ui-client-certs secret.

agent: false

operator:
  enabled: false

cni:
  install: false

hubble:
  enabled: false

  relay:
    # set this to false as Hubble relay is already installed
    enabled: false

    tls:
      server:
        # set this to true if tls is enabled on Hubble relay server side
        enabled: true

  ui:
    # enable Hubble UI
    enabled: true

    standalone:
      # enable Hubble UI standalone deployment
      enabled: true

      # provide a volume containing Hubble relay client certificates to mount in Hubble UI pod
      certsVolume:
        projected:
          defaultMode: 420
          sources:
          - secret:
              name: my-hubble-ui-client-certs
              items:
              - key: tls.crt
                path: client.crt
              - key: tls.key
                path: client.key
              - key: ca.crt
                path: hubble-relay-ca.crt

Please note that Hubble UI expects the certificate files to be available under the following paths:

        - name: TLS_RELAY_CA_CERT_FILES
          value: /var/lib/hubble-ui/certs/hubble-relay-ca.crt
        - name: TLS_RELAY_CLIENT_CERT_FILE
          value: /var/lib/hubble-ui/certs/client.crt
        - name: TLS_RELAY_CLIENT_KEY_FILE
          value: /var/lib/hubble-ui/certs/client.key

Keep this in mind when providing the volume containing the certificate.

aanm · 2021-10-21T00:30:13Z

/test

eddycharly · 2021-10-21T07:50:00Z

ConformanceEKS test failed but i don't think it's related to this PR 🤔

kaworu · 2021-10-21T08:15:05Z

ConformanceEKS test failed but i don't think it's related to this PR

@eddycharly Could you rebase on top of master now that #17645 has been merged please?

eddycharly · 2021-10-21T08:49:23Z

@kaworu done 🤞

eddycharly · 2021-10-21T09:10:59Z

/test

eddycharly · 2021-10-21T10:00:41Z

ConformanceEKS test failed again 😢

eddycharly · 2021-10-22T07:24:16Z

@kaworu sorry for pinging you again 🙈

Do you have an idea how to make the failing test green ?

kaworu · 2021-10-22T07:35:36Z

/ci-eks

eddycharly · 2021-10-22T09:35:43Z

Finally, it's green 🎉

kaworu · 2021-10-22T11:35:38Z

/test

eddycharly · 2021-10-22T12:42:05Z

😢

rolinh · 2021-10-22T12:45:53Z

test-runtime

EDIT: vm provisioning failure

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig Encapsulation Check iptables masquerading with random-fully

Failure Output

FAIL: Connectivity test between nodes failed

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

eddycharly · 2021-10-22T14:00:24Z

/mlh new-flake Cilium-PR-K8s-GKE

👍 created #17672

eddycharly · 2021-10-22T14:01:36Z

Thanks @rolinh !
Can i rerun other tests ?

rolinh · 2021-10-22T14:10:49Z

Thanks @rolinh ! Can i rerun other tests ?

It looks like the VM couldn't be provisioned again for the runtime test. I'll check with the CI team if there are infra issues or other known issues.

eddycharly · 2021-10-22T14:40:41Z

/test-gke

rolinh · 2021-10-22T14:49:10Z

@eddycharly The runtime test failure is actually legit. Could you please run make -C Documentation update-helm-values and amend your commit with the updated file?

Relevant stack trace (click to expand)

15:32:14      runtime: ./check-helmvalues.sh
15:32:14      runtime: diff --git a/Documentation/helm-values.rst b/Documentation/helm-values.rst
15:32:14      runtime: index 9a1b1343f..79b3ee49b 100644
15:32:14      runtime: --- a/Documentation/helm-values.rst
15:32:14      runtime: +++ b/Documentation/helm-values.rst
15:32:14      runtime: @@ -793,6 +793,14 @@
15:32:14      runtime:       - Whether to set the security context on the Hubble UI pods.
15:32:14      runtime:       - bool
15:32:14      runtime:       - ``true``
15:32:14      runtime: +   * - hubble.ui.standalone.enabled
15:32:14      runtime: +     - When true, it will allow installing the Hubble UI only, without checking dependencies. It is useful if a cluster already has cilium and Hubble relay installed and you just want Hubble UI to be deployed. When installed via helm, installing UI should be done via ``helm upgrade`` and when installed via the cilium cli, then ``cilium hubble enable --ui``
15:32:14      runtime: +     - bool
15:32:14      runtime: +     - ``false``
15:32:14      runtime: +   * - hubble.ui.standalone.tls.certsVolume
15:32:14      runtime: +     - When deploying Hubble UI in standalone, with tls enabled for Hubble relay, it is required to provide a volume for mounting the client certificates.
15:32:14      runtime: +     - object
15:32:14      runtime: +     - ``{}``
15:32:14      runtime:     * - hubble.ui.tls.client
15:32:14      runtime:       - base64 encoded PEM values used to connect to hubble-relay This keypair is presented to Hubble Relay instances for mTLS authentication and is required when hubble.relay.tls.server.enabled is true. These values need to be set manually if hubble.tls.auto.enabled is false.
15:32:14      runtime:       - object
15:32:14      runtime: HINT: to fix this, run 'make -C Documentation update-helm-values'
15:32:14      runtime: Makefile:43: recipe for target 'check' failed
15:32:14      runtime: make[1]: *** [check] Error 1
15:32:14      runtime: make[1]: Leaving directory '/home/vagrant/go/src/github.com/cilium/cilium/Documentation'
15:32:14      runtime: Makefile:564: recipe for target 'postcheck' failed
15:32:14      runtime: make: *** [postcheck] Error 2

eddycharly · 2021-10-22T14:58:18Z

@rolinh It doesn't seem to work for me, am i missing something ? Am i supposed to run that at the root of the repo ?

$ make -C Documentation update-helm-values
make: *** No rule to make target `update-helm-values'.  Stop.

eddycharly · 2021-10-22T14:59:50Z

make -C Documentation helm-values.rst maybe ?

eddycharly · 2021-10-22T16:51:57Z

/test

joestringer · 2021-10-22T17:28:26Z

/test

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig Encapsulation Check connectivity with VXLAN encapsulation

Failure Output

FAIL: Pods are not ready in time: timed out waiting for pods with filter  to be ready: 4m0s timeout expired

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.21-kernel-4.9' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sServicesTest Checks service across nodes with L7 policy Tests NodePort with L7 Policy

Failure Output

FAIL: Request from k8s1 to service http://[fd03::b89f]:10080 failed

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.21-kernel-4.9 so I can create a new GitHub issue to track it.

kkourt · 2021-10-25T06:18:35Z

@eddycharly Hi! Thanks you for your contribution!

There is one more minor thing to fix with the documentation, I'm afraid.

https://github.com/cilium/cilium/runs/3978620593?check_suite_focus=true, reports:

Please fix the following spelling mistakes:

Documentation/helm-reference.rst:800: (certsVolume)

Documentation/helm-values.rst:800: (certsVolume)

If the words are not misspelled, run:
Documentation/update-spelling_wordlist.sh certsVolume

You can reproduce this by running: make -C Documentation html at the top level cilium directory. Running:

Documentation/update-spelling_wordlist.sh certsVolume

As suggested by the check will result in the following diff:

diff --git a/Documentation/spelling_wordlist.txt b/Documentation/spelling_wordlist.txt
index aaabf601fd..06e96e4a87 100644
--- a/Documentation/spelling_wordlist.txt
+++ b/Documentation/spelling_wordlist.txt
@@ -222,6 +222,7 @@ certManagerIssuerRef
 certValidityDuration
 certgen
 certmanager
+certsVolume
 cgroup
 chainingMode
 changelog

Including this in your commits should fix the documentation failure.

Thanks!

Signed-off-by: Charles-Edouard Brétéché <charled.breteche@gmail.com>

eddycharly · 2021-10-25T08:19:45Z

@kkourt thanks !
I just ran the scripts and pushed again 🤞

kkourt · 2021-10-25T09:51:46Z

/test

eddycharly · 2021-10-25T10:43:31Z

/ci-gke

eddycharly · 2021-10-25T13:21:17Z

Wow, all tests green ! 🎉 🍾

eddycharly requested a review from a team as a code owner September 26, 2021 20:31

eddycharly requested review from a team September 26, 2021 20:31

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Sep 26, 2021

eddycharly requested review from kaworu and errordeveloper September 26, 2021 20:31

maintainer-s-little-helper bot assigned kaworu and errordeveloper Sep 26, 2021

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 26, 2021

kaworu requested a review from geakstr September 27, 2021 09:18

maintainer-s-little-helper bot assigned geakstr Sep 27, 2021

kaworu added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Sep 27, 2021

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 27, 2021

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. and removed dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. labels Sep 27, 2021

kaworu requested changes Oct 4, 2021

View reviewed changes

install/kubernetes/cilium/values.yaml Outdated Show resolved Hide resolved

eddycharly requested a review from kaworu October 4, 2021 17:52

errordeveloper approved these changes Oct 5, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned errordeveloper Oct 5, 2021

kaworu requested changes Oct 11, 2021

View reviewed changes

install/kubernetes/cilium/values.yaml Show resolved Hide resolved

kaworu mentioned this pull request Oct 11, 2021

helm: allow hubble ui to be installed without enabling hubble relay #17368

Closed

6 tasks

maintainer-s-little-helper bot unassigned geakstr Oct 20, 2021

feat: allow installing hubble ui as standalone

e2031cf

Signed-off-by: Charles-Edouard Brétéché <charled.breteche@gmail.com>

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 25, 2021

kkourt merged commit cc09bde into cilium:master Oct 25, 2021

feat: allow installing hubble ui as standalone #17473

feat: allow installing hubble ui as standalone #17473

Conversation

eddycharly commented Sep 26, 2021 • edited by joestringer

maintainer-s-little-helper bot commented Sep 26, 2021

maintainer-s-little-helper bot commented Sep 27, 2021

eddycharly commented Oct 1, 2021

kaworu left a comment

Choose a reason for hiding this comment

eddycharly commented Oct 4, 2021

eddycharly commented Oct 6, 2021 • edited

eddycharly commented Oct 11, 2021

kaworu left a comment

Choose a reason for hiding this comment

eddycharly commented Oct 11, 2021 • edited

eddycharly commented Oct 12, 2021

Installing Hubble UI in standalone

aanm commented Oct 21, 2021

eddycharly commented Oct 21, 2021

kaworu commented Oct 21, 2021

eddycharly commented Oct 21, 2021 • edited

eddycharly commented Oct 21, 2021

eddycharly commented Oct 21, 2021

eddycharly commented Oct 22, 2021

kaworu commented Oct 22, 2021

eddycharly commented Oct 22, 2021

kaworu commented Oct 22, 2021

eddycharly commented Oct 22, 2021

rolinh commented Oct 22, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

eddycharly commented Oct 22, 2021 • edited by maintainer-s-little-helper bot

eddycharly commented Oct 22, 2021

rolinh commented Oct 22, 2021

eddycharly commented Oct 22, 2021

rolinh commented Oct 22, 2021

eddycharly commented Oct 22, 2021

eddycharly commented Oct 22, 2021

eddycharly commented Oct 22, 2021

joestringer commented Oct 22, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

kkourt commented Oct 25, 2021 • edited

eddycharly commented Oct 25, 2021

kkourt commented Oct 25, 2021

eddycharly commented Oct 25, 2021

eddycharly commented Oct 25, 2021

eddycharly commented Sep 26, 2021 •

edited by joestringer

eddycharly commented Oct 6, 2021 •

edited

eddycharly commented Oct 11, 2021 •

edited

eddycharly commented Oct 21, 2021 •

edited

rolinh commented Oct 22, 2021 •

edited by maintainer-s-little-helper bot

eddycharly commented Oct 22, 2021 •

edited by maintainer-s-little-helper bot

joestringer commented Oct 22, 2021 •

edited by maintainer-s-little-helper bot

kkourt commented Oct 25, 2021 •

edited