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

Jump to bottom

helm: Generate experimental-install.yaml #11907

Merged
merged 2 commits into from
Jun 9, 2020
Merged

helm: Generate experimental-install.yaml #11907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d044c7c
helm: Fix the name/namespace order
michi-covalent Jun 5, 2020
bbdd87a
helm: Generate experimental-install.yaml
michi-covalent Jun 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/helm-check.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Check quick-install.yaml
- name: Check quick-install.yaml and experimental-install.yaml
run: |
cd install/kubernetes
make all
Expand Down
14 changes: 12 additions & 2 deletions install/kubernetes/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ include ../../Makefile.defs
MANAGED_ETCD_VERSION := "v2.0.7"

QUICK_INSTALL := "$(ROOT_DIR)/$(RELATIVE_DIR)/quick-install.yaml"
EXPERIMENTAL_INSTALL := "$(ROOT_DIR)/$(RELATIVE_DIR)/experimental-install.yaml"
MANAGED_ETCD_PATH := "$(ROOT_DIR)/$(RELATIVE_DIR)/cilium/charts/managed-etcd/values.yaml"
CILIUM_CHARTS := "$(ROOT_DIR)/$(RELATIVE_DIR)/cilium/"
CILIUM_VALUES := "$(CILIUM_CHARTS)/values.yaml"
Expand All @@ -16,12 +17,21 @@ DEV_VERSION_REGEX := '[0-9]\+\.[0-9]\+\.[0-9]\+-dev'
CILIUM_CHART_REGEX := '\([vV]ersion:\) '$(VERSION_REGEX)
CILIUM_TAG_REGEX := '\(tag:\) \(v'$(VERSION_REGEX)'\|latest\)'
CILIUM_PULLPOLICY_REGEX := '\(pullPolicy:\) .*'
EXPERIMENTAL_OPTIONS := \
--set global.hubble.enabled=true \
--set global.hubble.listenAddress=":4244" \
Copy link
Member

Choose a reason for hiding this comment

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

Should we enable this by default without any warning to the user? (I know there's a warning in the logs but that would be it)

Copy link
Member

Choose a reason for hiding this comment

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

@aanm Are you saying that we should enable these options by default for all cilium installs or asking whether we should warn users when these settings are enabled?

Copy link
Member

Choose a reason for hiding this comment

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

@aanm Are you saying that we should enable these options by default for all cilium installs or asking whether we should warn users when these settings are enabled?

warn users when these settings are enabled since this might be unnoticed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aanm @rolinh i was thinking of simply putting a warning in the cilium doc whenever we mention kubectl apply -f experimental-install.yaml. i'd be happy to add warnings in other place if we already have some sort of convention to warn users about experimental features with security implications.

Copy link
Member

Choose a reason for hiding this comment

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

@aanm How would you suggest the warning should be delivered to users? In cilium status? IMO if it is not possible for the user to use the experimental options without explicitly invoking experimental-install.yaml on the command line warnings in the docs that document such use should be enough.

Copy link
Member

Choose a reason for hiding this comment

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

Note that we can generate custom text in the Helm NOTES output depending on specific helm options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is how the warning is worded right now: https://github.com/cilium/cilium/pull/11882/files#diff-0806f700bf4c3d27724e99235d5f13a8R47

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joestringer yeah i think it makes sense to add a warning in NOTES.txt if global.hubble.listenAddress is set. i will open a separate PR for that since it's not directly related to experimental-install.yaml.

--set global.hubble.metrics.enabled="{dns,drop,tcp,flow,port-distribution,icmp,http}" \
--set global.hubble.relay.enabled=true \
--set global.hubble.ui.enabled=true

all: update-versions $(QUICK_INSTALL)
all: update-versions $(QUICK_INSTALL) $(EXPERIMENTAL_INSTALL)

$(QUICK_INSTALL): $(shell find cilium/ -type f)
$(QUIET)helm template cilium --namespace=kube-system $(OPTS) > $(QUICK_INSTALL)

$(EXPERIMENTAL_INSTALL): $(shell find cilium/ -type f)
$(QUIET)helm template cilium --namespace=kube-system $(EXPERIMENTAL_OPTIONS) > $(EXPERIMENTAL_INSTALL)

update-versions:
$(ECHO_GEN) " -> Updating version to $(VERSION)"
@# Update chart versions to point to the current version.
Expand All @@ -43,6 +53,6 @@ update-versions:
$(QUIET)sed -i 's/'$(VERSION)'/'$(MANAGED_ETCD_VERSION)'/' $(MANAGED_ETCD_PATH)

clean:
$(RM) $(QUICK_INSTALL)
$(RM) $(QUICK_INSTALL) $(EXPERIMENTAL_INSTALL)

.phony: all clean update-versions
2 changes: 1 addition & 1 deletion install/kubernetes/cilium/charts/hubble-ui/templates/serviceaccount.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
apiVersion: v1
kind: ServiceAccount
metadata:
namespace: {{ .Release.Namespace }}
name: hubble-ui
namespace: {{ .Release.Namespace }}
{{- if .Values.serviceAccount.annotations }}
annotations:
{{ toYaml .Values.serviceAccount.annotations | indent 4 }}
Expand Down