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

helm: Generate experimental-install.yaml #11907

Merged
merged 2 commits into from Jun 9, 2020
Merged

helm: Generate experimental-install.yaml #11907

merged 2 commits into from Jun 9, 2020

Conversation

michi-covalent
Copy link
Contributor

@michi-covalent michi-covalent commented Jun 5, 2020

Generate experimental-install.yaml in which we can enable experimental/beta
features without changing default values in the Helm chart. This makes
it easier for users to play around with bleeding edge features with a
simple kubectl apply.

Ref #11902

Signed-off-by: Michi Mutsuzaki michi@isovalent.com

@michi-covalent michi-covalent added release-note/misc This PR makes changes that have no direct user impact. sig/hubble Impacts hubble server or relay area/helm Impacts helm charts and user deployment experience needs-backport/1.8 labels Jun 5, 2020
@michi-covalent michi-covalent requested review from a team as code owners June 5, 2020 00:17
@michi-covalent michi-covalent requested a review from a team June 5, 2020 00:17
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Jun 5, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.0 Jun 5, 2020
@coveralls
Copy link

coveralls commented Jun 5, 2020

Coverage Status

Coverage increased (+0.05%) to 36.993% when pulling bbdd87a on pr/michi/ultimate into 02b43fc on master.

aanm
aanm previously requested changes Jun 5, 2020
@@ -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:\) .*'
ULTIMATE_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.

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.

@@ -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:\) .*'
ULTIMATE_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.

@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?

install/kubernetes/ultimate-install.yaml Outdated Show resolved Hide resolved
install/kubernetes/ultimate-install.yaml Outdated Show resolved Hide resolved
@errordeveloper
Copy link
Contributor

errordeveloper commented Jun 5, 2020

@michi-covalent I am not very clear on why it's 'ultimate', from the description it sounds like 'experimental' would be more appropriate... Could you explain the intention please?

@michi-covalent
Copy link
Contributor Author

@michi-covalent I am not very clear on why it's 'ultimate', from the description it sounds like 'experimental' would be more appropriate... Could you explain the intention please?

i like experimental-install.yaml better. will rename

Ref: #11907 (comment)

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
Generate experimental-install.yaml in which we can enable experimental
features without changing default values in the Helm chart. This makes
it easier for users to play around with bleeding edge features with a
simple `kubectl apply`.

Ref #11902

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@michi-covalent michi-covalent changed the title helm: Generate ultimate-install.yaml helm: Generate experimental-install.yaml Jun 6, 2020
@michi-covalent
Copy link
Contributor Author

test-me-please

@maintainer-s-little-helper 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 Jun 9, 2020
@aanm aanm removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 9, 2020
@maintainer-s-little-helper 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 Jun 9, 2020
@aanm aanm removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 9, 2020
@maintainer-s-little-helper 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 Jun 9, 2020
@maintainer-s-little-helper 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 Jun 9, 2020
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.

No test

@aanm aanm added the area/helm Impacts helm charts and user deployment experience label Jun 9, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 9, 2020
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.

test

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.

t

@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jun 9, 2020
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.

t

@maintainer-s-little-helper 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 Jun 9, 2020
@aanm aanm merged commit ed65830 into master Jun 9, 2020
1.8.0 automation moved this from In progress to Merged Jun 9, 2020
@aanm aanm deleted the pr/michi/ultimate branch June 9, 2020 12:33
aanm pushed a commit that referenced this pull request Jun 9, 2020
Ref: #11907 (comment)

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 9, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 9, 2020
aanm pushed a commit that referenced this pull request Jun 9, 2020
[ upstream commit a67554d ]

Ref: #11907 (comment)

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm mentioned this pull request Jun 9, 2020
aanm pushed a commit that referenced this pull request Jun 10, 2020
[ upstream commit a67554d ]

Ref: #11907 (comment)

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
Signed-off-by: André Martins <andre@cilium.io>
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 12, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Impacts helm charts and user deployment experience ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/hubble Impacts hubble server or relay
Projects
No open projects
1.8.0
  
Merged
1.8.0
Backport done to v1.8
Development

Successfully merging this pull request may close these issues.

None yet

8 participants