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

SPIRE install: Set CA name + disable kubelet validation by default #25160

Merged
merged 1 commit into from Apr 28, 2023
Merged

SPIRE install: Set CA name + disable kubelet validation by default #25160

merged 1 commit into from Apr 28, 2023

Conversation

meyskens
Copy link
Member

This PR fixes two things to make the SPIRE installation that comes with Cilium friction less:

  • Set the CN of the CA to make the CA cert valid
  • Disable kubelet validation by default, this results in issues on many k8s distros even kind
Fix issues that caused SPIRE not to install properly

@meyskens meyskens requested review from a team as code owners April 27, 2023 11:17
@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 Apr 27, 2023
@meyskens meyskens added release-note/bug This PR fixes an issue in a previous release of Cilium. area/servicemesh GH issues or PRs regarding servicemesh labels Apr 27, 2023
@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 Apr 27, 2023
@meyskens
Set CA name + disable kubelet validation in SPIRE
dbafe52 
This sets a CN on the CA to be valid. It also disables the kubelet
validation by default as this breaks many installations such as on
kind.

Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>
@meyskens
Copy link
Member Author

/test

youngnick
youngnick reviewed Apr 27, 2023
View reviewed changes
install/kubernetes/cilium/values.yaml.tmpl
@@ -2669,7 +2669,7 @@ auth:
# -- SPIRE agent labels
labels: { }
# -- SPIRE Workload Attestor kubelet verification.
skipKubeletVerification: false
skipKubeletVerification: true
Copy link
Contributor

Choose a reason for hiding this comment

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

This does worry me a little bit, in that we're already doing some funky stuff with attestation in having the Cilium agent have heaps of the process delegated to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That said, now I wrote that, maybe it is less of a problem because the kubelet is not actually involved in any attestation aside from the Cilium Agent and Operator getting their identities.

Copy link
Member

Choose a reason for hiding this comment

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

My original approach is to go with secured sensible default values, and user can still change it via helm values based on k8s distro they are using (e.g. kind, minikube).

joestringer
joestringer approved these changes Apr 27, 2023
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Helm changes are trivial.

sayboras
sayboras approved these changes Apr 28, 2023
View reviewed changes
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.

One comment for changing default values of skipKubeletVerification, if this flag is not required, we can still ship it.

install/kubernetes/cilium/values.yaml.tmpl
@@ -2703,7 +2703,7 @@ auth:
subject:
country: "US"
organization: "SPIRE"
commonName: ""
commonName: "Cilium SPIRE CA"
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I had this in the local temp value, but didn't pass the default value here.

install/kubernetes/cilium/values.yaml.tmpl
@@ -2669,7 +2669,7 @@ auth:
# -- SPIRE agent labels
labels: { }
# -- SPIRE Workload Attestor kubelet verification.
skipKubeletVerification: false
skipKubeletVerification: true
Copy link
Member

Choose a reason for hiding this comment

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

My original approach is to go with secured sensible default values, and user can still change it via helm values based on k8s distro they are using (e.g. kind, minikube).

kaworu
kaworu approved these changes Apr 28, 2023
View reviewed changes
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Helm changes LGTM besides the ongoing discussion about skipKubeletVerification.

@meyskens meyskens mentioned this pull request Apr 28, 2023
Closed
@meyskens
Copy link
Member Author

Made an issue to track this in: #25193

@joestringer
Copy link
Member

k8s-1.26-kernel-net-next hit #15455
runtime hit #25178

@joestringer joestringer merged commit 75345d0 into cilium:main Apr 28, 2023
54 of 56 checks passed
@meyskens meyskens deleted the meyskens/spire-install-fix branch May 2, 2023 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaworu kaworu kaworu approved these changes

@joestringer joestringer joestringer approved these changes

@sayboras sayboras sayboras approved these changes

@youngnick youngnick youngnick approved these changes

Assignees
No one assigned
Labels
area/servicemesh GH issues or PRs regarding servicemesh release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@meyskens @joestringer @kaworu @sayboras @youngnick