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

ui: updated helm charts for hubble-ui 0.7 #13170

Merged
merged 1 commit into from Sep 17, 2020

Conversation

geakstr
Copy link
Contributor

@geakstr geakstr commented Sep 14, 2020

Signed-off-by: Dmitry Kharitonov dmitry@isovalent.com

@geakstr geakstr requested review from a team as code owners September 14, 2020 18:30
@geakstr geakstr requested review from a team September 14, 2020 18:30
@geakstr geakstr self-assigned this Sep 14, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 14, 2020
@joestringer
Copy link
Member

I think you need to run make -C install/kubernetes to generate the quick install / experimental install YAMLs based on the changes.

@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Sep 14, 2020
@maintainer-s-little-helper 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 14, 2020
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM, please squash the two commit together, and it should be good to go.

Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Few minor points

install/kubernetes/cilium/charts/hubble-ui/values.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,66 @@
---
apiVersion: v1
Copy link
Member

Choose a reason for hiding this comment

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

dumb question, if this configmap changes, will envoy pick it up (e.g. similar to coredns) ?

ports:
- name: http
port: 80
targetPort: 12000
targetPort: 8081
Copy link
Member

Choose a reason for hiding this comment

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

we need to update few docs for this port changes as well ? For example, https://github.com/cilium/cilium/blob/master/Documentation/gettingstarted/hubble-enable.rst

Copy link
Contributor

Choose a reason for hiding this comment

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

no, port-forward command will continue to work

@maintainer-s-little-helper
Copy link

Commit 8dbebd8cdb25097be1af8e75fca23921b35d8b72 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 maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 15, 2020
@geakstr geakstr force-pushed the pr/dima/helm-charts-for-hubble-ui-0.7 branch from eba5742 to 7def310 Compare September 15, 2020 12:58
@maintainer-s-little-helper
Copy link

Commit 8dbebd8cdb25097be1af8e75fca23921b35d8b72 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

@geakstr geakstr force-pushed the pr/dima/helm-charts-for-hubble-ui-0.7 branch from 7def310 to 5e2b658 Compare September 15, 2020 12:59
@maintainer-s-little-helper
Copy link

Commit 8dbebd8cdb25097be1af8e75fca23921b35d8b72 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

@geakstr geakstr force-pushed the pr/dima/helm-charts-for-hubble-ui-0.7 branch from 5e2b658 to e1e8adb Compare September 15, 2020 15:08
@maintainer-s-little-helper 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 15, 2020
Copy link
Member

@fristonio fristonio left a comment

Choose a reason for hiding this comment

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

Mostly looks good 🚀 Just some minor nits around formatting.

@geakstr geakstr force-pushed the pr/dima/helm-charts-for-hubble-ui-0.7 branch 2 times, most recently from d9bc0ec to 68b52fc Compare September 16, 2020 06:33
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

We can also have a dedicated file for the envoy configuration and import it in the configmap.

diff --git a/install/kubernetes/cilium/charts/hubble-ui/files/envoy/envoy.yaml b/install/kubernetes/cilium/charts/hubble-ui/files/envoy/envoy.yaml
new file mode 100644
index 000000000..31c778b61
--- /dev/null
+++ b/install/kubernetes/cilium/charts/hubble-ui/files/envoy/envoy.yaml
@@ -0,0 +1,58 @@
+static_resources:
+  listeners:
+    - name: listener_hubble_ui
+      address:
+        socket_address:
+          address: 0.0.0.0
+          port_value: 8081
+      filter_chains:
+        - filters:
+            - name: envoy.filters.network.http_connection_manager
+              config:
+                codec_type: auto
+                stat_prefix: ingress_http
+                route_config:
+                  name: local_route
+                  virtual_hosts:
+                    - name: local_service
+                      domains: ['*']
+                      routes:
+                        - match:
+                            prefix: '/api/'
+                          route:
+                            cluster: backend
+                            max_grpc_timeout: 0s
+                            prefix_rewrite: '/'
+                        - match:
+                            prefix: '/'
+                          route:
+                            cluster: frontend
+                      cors:
+                        allow_origin_string_match:
+                          - prefix: '*'
+                        allow_methods: GET, PUT, DELETE, POST, OPTIONS
+                        allow_headers: keep-alive,user-agent,cache-control,content-type,content-transfer-encoding,x-accept-content-transfer-encoding,x-accept-response-streaming,x-user-agent,x-grpc-web,grpc-timeout
+                        max_age: '1728000'
+                        expose_headers: grpc-status,grpc-message
+                http_filters:
+                  - name: envoy.filters.http.grpc_web
+                  - name: envoy.filters.http.cors
+                  - name: envoy.filters.http.router
+  clusters:
+    - name: frontend
+      connect_timeout: 0.25s
+      type: strict_dns
+      lb_policy: round_robin
+      hosts:
+        - socket_address:
+            address: 127.0.0.1
+            port_value: 8080
+    - name: backend
+      connect_timeout: 0.25s
+      type: logical_dns
+      lb_policy: round_robin
+      http2_protocol_options: {}
+      hosts:
+        - socket_address:
+            address: 127.0.0.1
+            port_value: 8090
diff --git a/install/kubernetes/cilium/charts/hubble-ui/templates/configmap.yaml b/install/kubernetes/cilium/charts/hubble-ui/templates/configmap.yaml
index bc43ee70c..e69517e6b 100644
--- a/install/kubernetes/cilium/charts/hubble-ui/templates/configmap.yaml
+++ b/install/kubernetes/cilium/charts/hubble-ui/templates/configmap.yaml
@@ -5,62 +5,4 @@ metadata:
   name: hubble-ui-envoy
   namespace: {{ .Release.Namespace }}
 data:
-  envoy.yaml: >
-    static_resources:
-      listeners:
-        - name: listener_hubble_ui
-          address:
-            socket_address:
-              address: 0.0.0.0
-              port_value: 8081
-          filter_chains:
-            - filters:
-                - name: envoy.filters.network.http_connection_manager
-                  config:
-                    codec_type: auto
-                    stat_prefix: ingress_http
-                    route_config:
-                      name: local_route
-                      virtual_hosts:
-                        - name: local_service
-                          domains: ['*']
-                          routes:
-                            - match:
-                                prefix: '/api/'
-                              route:
-                                cluster: backend
-                                max_grpc_timeout: 0s
-                                prefix_rewrite: '/'
-                            - match:
-                                prefix: '/'
-                              route:
-                                cluster: frontend
-                          cors:
-                            allow_origin_string_match:
-                              - prefix: '*'
-                            allow_methods: GET, PUT, DELETE, POST, OPTIONS
-                            allow_headers: keep-alive,user-agent,cache-control,content-type,content-transfer-encoding,x-accept-content-transfer-encoding,x-accept-response-streaming,x-user-agent,x-grpc-web,grpc-timeout
-                            max_age: '1728000'
-                            expose_headers: grpc-status,grpc-message
-                    http_filters:
-                      - name: envoy.filters.http.grpc_web
-                      - name: envoy.filters.http.cors
-                      - name: envoy.filters.http.router
-      clusters:
-        - name: frontend
-          connect_timeout: 0.25s
-          type: strict_dns
-          lb_policy: round_robin
-          hosts:
-            - socket_address:
-                address: 127.0.0.1
-                port_value: 8080
-        - name: backend
-          connect_timeout: 0.25s
-          type: logical_dns
-          lb_policy: round_robin
-          http2_protocol_options: {}
-          hosts:
-            - socket_address:
-                address: 127.0.0.1
-                port_value: 8090
+{{ (.Files.Glob "files/envoy/*").AsConfig | indent 2 }}
\ No newline at end of file
diff --git a/install/kubernetes/experimental-install.yaml b/install/kubernetes/experimental-install.yaml
index ce90557df..af1701c93 100644
--- a/install/kubernetes/experimental-install.yaml
+++ b/install/kubernetes/experimental-install.yaml
@@ -184,7 +184,7 @@ metadata:
   name: hubble-ui-envoy
   namespace: kube-system
 data:
-  envoy.yaml: >
+  envoy.yaml: |
     static_resources:
       listeners:
         - name: listener_hubble_ui

install/kubernetes/cilium/charts/hubble-ui/values.yaml Outdated Show resolved Hide resolved
Signed-off-by: Dmitry Kharitonov <dmitry@isovalent.com>
@geakstr geakstr force-pushed the pr/dima/helm-charts-for-hubble-ui-0.7 branch from 68b52fc to 75db096 Compare September 16, 2020 09:47
@geakstr
Copy link
Contributor Author

geakstr commented Sep 16, 2020

Cool, thanks

@aanm
Copy link
Member

aanm commented Sep 16, 2020

test-me-please

Copy link
Member

@fristonio fristonio left a comment

Choose a reason for hiding this comment

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

LGTM

@aanm
Copy link
Member

aanm commented Sep 17, 2020

hit #13198

@aanm aanm merged commit e518c51 into master Sep 17, 2020
@aanm aanm deleted the pr/dima/helm-charts-for-hubble-ui-0.7 branch September 17, 2020 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants