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

feat: generate tls certs for ui on helm install #16601

Merged
merged 1 commit into from Jun 29, 2021
Merged

feat: generate tls certs for ui on helm install #16601

merged 1 commit into from Jun 29, 2021

Conversation

yandzee
Copy link
Contributor

@yandzee yandzee commented Jun 21, 2021 •
edited

No description provided.

@yandzee yandzee requested a review from gandro June 21, 2021 11:43
@maintainer-s-little-helper
Copy link

Commits 8c72ca3f82b69149d3d2805ed40b81d2a34b8041, 6b3878c3d9c43572744adc970a8daa95bd92adc4, d8c5b97cc56720128c293c4d7465e4080160a078 do 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 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 Jun 21, 2021
@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 Jun 21, 2021
@yandzee yandzee marked this pull request as ready for review June 21, 2021 11:46
@yandzee yandzee requested a review from a team as a code owner June 21, 2021 11:46
@yandzee yandzee requested review from a team, kaworu and nathanjsweet June 21, 2021 11:46
@yandzee yandzee added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/hubble Impacts hubble server or relay labels Jun 21, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 21, 2021
@yandzee yandzee force-pushed the pr/renat/ui-tls-certs branch 2 times, most recently from 921115b to 9b4f451 Compare June 21, 2021 11:49
@gandro
Copy link
Member

gandro commented Jun 21, 2021 •
edited

For context:

kaworu
kaworu reviewed Jun 21, 2021
View reviewed changes
Copy link
Member

@kaworu kaworu left a comment •
edited

Choose a reason for hiding this comment

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

Patch LGTM overall. As UI is also a server, I have some suggestions to make it clearer which end of the connection is protected over mTLS (i.e. between UI and Relay).

Note that some of the suggested changes (to env variables) need to be reflected at cilium/hubble-ui#151 as well.

install/kubernetes/cilium/templates/hubble-ui-deployment.yaml Outdated Show resolved Hide resolved
install/kubernetes/cilium/templates/hubble-ui-deployment.yaml Outdated Show resolved Hide resolved
install/kubernetes/cilium/templates/hubble-ui-deployment.yaml Outdated Show resolved Hide resolved
install/kubernetes/cilium/templates/hubble-ui-deployment.yaml Outdated Show resolved Hide resolved
install/kubernetes/cilium/values.yaml Outdated Show resolved Hide resolved
@gandro
Copy link
Member

gandro commented Jun 21, 2021

Considering this needs cilium/hubble-ui#151 to be merged and finalized first (as there seem to be a few concerns regarding the naming), I'll add the blocked label to this PR for now. We want cilium/hubble-ui#151 to get merged first.

@gandro gandro added the dont-merge/blocked Another PR must be merged before this one. label Jun 21, 2021
@gandro gandro added dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. and removed dont-merge/blocked Another PR must be merged before this one. labels Jun 21, 2021
@gandro gandro removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 21, 2021
gandro
gandro approved these changes Jun 21, 2021
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

LGTM. One minor nit, besides what Alex already pointed out.

install/kubernetes/cilium/templates/hubble-ui-deployment.yaml Outdated Show resolved Hide resolved
@yandzee yandzee force-pushed the pr/renat/ui-tls-certs branch 2 times, most recently from 0d454a0 to 058d5e9 Compare June 22, 2021 09:31
@gandro gandro requested a review from kaworu June 22, 2021 09:32
@yandzee yandzee mentioned this pull request Jun 22, 2021
Closed
nathanjsweet
nathanjsweet approved these changes Jun 25, 2021
View reviewed changes
Copy link
Member

@nathanjsweet nathanjsweet left a comment

Choose a reason for hiding this comment

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

Wow, nice!

@gandro gandro added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 28, 2021
@yandzee
feat: generate tls certs for ui on helm install
9e7b55a 
Signed-off-by: Renat Tuktarov <yandzeek@gmail.com>
@yandzee yandzee removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 29, 2021
@gandro
Copy link
Member

gandro commented Jun 29, 2021 •
edited

Given that we do not have CI coverage for Hubble UI in this repository, but have manually tested this, I don't think it makes sense to run the full CI suite here. Marking ready to merge.

@gandro gandro added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 29, 2021
@aanm aanm merged commit 9324ba0 into master Jun 29, 2021
@aanm aanm deleted the pr/renat/ui-tls-certs branch June 29, 2021 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@kaworu kaworu kaworu approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/hubble Impacts hubble server or relay
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yandzee @gandro @kaworu @nathanjsweet @aanm