Feat integrate spiffe #1663

TessaIO · 2024-04-14T18:43:12Z

Resolves #1186
TODO:

Add docs
Add e2e

k8s-ci-robot · 2024-04-14T18:43:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: TessaIO
Once this PR has been reviewed and has the lgtm label, please assign marquiz for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

netlify · 2024-04-14T18:43:30Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`520dd72`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/6641180c9e9c520008910841
😎 Deploy Preview	https://deploy-preview-1663--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

TessaIO · 2024-04-15T06:59:12Z

/cc marquiz

ArangoGutierrez · 2024-04-15T13:38:31Z

Let's close #1434 ;)

ArangoGutierrez

Some first comments

pkg/utils/spiffe/spiffe.go

pkg/utils/spiffe/spiffe_test.go

pkg/utils/spiffe/spiffe.go

ArangoGutierrez · 2024-04-23T12:14:25Z

cmd/nfd-master/main.go

@@ -91,6 +91,8 @@ func main() {
 			klog.InfoS("-port is deprecated, will be removed in a future release along with the deprecated gRPC API")
 		case "verify-node-name":
 			klog.InfoS("-verify-node-name is deprecated, will be removed in a future release along with the deprecated gRPC API")
+		case "enable-spiffe":


@marquiz should we define this as a feature-gate???

Probably not. I think this will never be enabled by default so feature gate feels superfluous (and unintuitive). It would serve as a unnecessary second-level gate (we'd want the enable/disable setting anyway), i.e. you'd need to specify -feature-gates spiffe=true -enable-spiffe

pkg/utils/spiffe/spiffe.go

pkg/utils/spiffe/spiffe_test.go

ArangoGutierrez · 2024-04-23T12:15:30Z

pkg/utils/spiffe/spiffe_test.go

+	"github.com/stretchr/testify/assert"
+	"sigs.k8s.io/node-feature-discovery/api/nfd/v1alpha1"


Is this file go imports checked?

Does it contain any problem? My IDE does not complain about it.

marquiz · 2024-05-02T14:02:38Z

cmd/nfd-master/main.go

@@ -91,6 +91,8 @@ func main() {
 			klog.InfoS("-port is deprecated, will be removed in a future release along with the deprecated gRPC API")
 		case "verify-node-name":
 			klog.InfoS("-verify-node-name is deprecated, will be removed in a future release along with the deprecated gRPC API")
+		case "enable-spiffe":


Probably not. I think this will never be enabled by default so feature gate feels superfluous (and unintuitive). It would serve as a unnecessary second-level gate (we'd want the enable/disable setting anyway), i.e. you'd need to specify -feature-gates spiffe=true -enable-spiffe

marquiz · 2024-05-02T14:26:49Z

deployment/helm/node-feature-discovery/templates/spire-agent-daemonset.yaml

Wouldn't we want to depend on/refer to some official Spire Helm chart for deploying spire instead of maintaining our own?

yes for Helm charts that would be better. But I'm wondering how we're going to do that for Kustomize.

marquiz · 2024-05-02T14:30:37Z

pkg/nfd-master/nfd-master.go

@@ -85,6 +90,7 @@ type NFDConfig struct {
 	LeaderElection    LeaderElectionConfig
 	NfdApiParallelism int
 	Klog              klogutils.KlogConfigOpts
+	EnableSpiffe      bool


Hmm, looking at this maybe it would be safest to only have this as a command line argument (i.e. NOT at as a dynamically configurable config file setting). Just to make it very clear if/when the setting is changed. WDYT?

sounds good for me!

Signed-off-by: TessaIO <ahmedgrati1999@gmail.com>

k8s-ci-robot · 2024-05-12T21:29:30Z

@TessaIO: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-node-feature-discovery-build-image-cross-generic	`520dd72`	link	true	`/test pull-node-feature-discovery-build-image-cross-generic`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

codecov · 2024-05-15T00:36:26Z

Codecov Report

Attention: Patch coverage is 22.78481% with 122 lines in your changes are missing coverage. Please review.

Project coverage is 39.49%. Comparing base (eef9912) to head (520dd72).
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1663      +/-   ##
==========================================
- Coverage   39.85%   39.49%   -0.37%     
==========================================
  Files          80       81       +1     
  Lines        6839     6996     +157     
==========================================
+ Hits         2726     2763      +37     
- Misses       3859     3968     +109     
- Partials      254      265      +11

Files	Coverage Δ
cmd/nfd-worker/main.go	`53.91% <50.00%> (-0.15%)`	⬇️
cmd/nfd-master/main.go	`0.00% <0.00%> (ø)`
pkg/nfd-master/nfd-master.go	`39.78% <12.19%> (-1.24%)`	⬇️
pkg/nfd-worker/nfd-worker.go	`49.64% <7.31%> (-2.91%)`	⬇️
pkg/utils/spiffe/spiffe.go	`38.23% <38.23%> (ø)`

k8s-ci-robot · 2024-05-15T00:36:32Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

marquiz · 2024-05-24T08:22:09Z

/milestone v0.17

k8s-ci-robot requested review from adrianchiris and ArangoGutierrez April 14, 2024 18:43

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 14, 2024

TessaIO force-pushed the feat-integrate-spiffe branch 4 times, most recently from b60483c to 2917fe1 Compare April 14, 2024 20:45

k8s-ci-robot requested a review from marquiz April 15, 2024 06:59

ArangoGutierrez requested changes Apr 17, 2024

View reviewed changes

pkg/utils/spiffe/spiffe.go Outdated Show resolved Hide resolved

pkg/utils/spiffe/spiffe_test.go Outdated Show resolved Hide resolved

pkg/utils/spiffe/spiffe.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2024

TessaIO force-pushed the feat-integrate-spiffe branch 2 times, most recently from c433995 to 5e5183d Compare April 22, 2024 16:11

TessaIO requested a review from ArangoGutierrez April 22, 2024 16:12

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2024

ArangoGutierrez requested changes Apr 23, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2024

TessaIO force-pushed the feat-integrate-spiffe branch from 5e5183d to 4e5af99 Compare April 28, 2024 09:48

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2024

TessaIO force-pushed the feat-integrate-spiffe branch 2 times, most recently from 65e5b03 to aee2686 Compare April 28, 2024 09:52

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2024

marquiz reviewed May 2, 2024

View reviewed changes

TessaIO added 2 commits May 12, 2024 21:25

feat: add spiffe integration in master and worker

5090c6b

Signed-off-by: TessaIO <ahmedgrati1999@gmail.com>

deploy: add spire manifests in helm and kustomize

520dd72

Signed-off-by: TessaIO <ahmedgrati1999@gmail.com>

TessaIO force-pushed the feat-integrate-spiffe branch from aee2686 to 520dd72 Compare May 12, 2024 19:27

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2024

k8s-ci-robot added this to the v0.17 milestone May 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat integrate spiffe #1663

Feat integrate spiffe #1663

TessaIO commented Apr 14, 2024

k8s-ci-robot commented Apr 14, 2024

netlify bot commented Apr 14, 2024 •

edited

TessaIO commented Apr 15, 2024

ArangoGutierrez commented Apr 15, 2024

ArangoGutierrez left a comment

ArangoGutierrez Apr 23, 2024

marquiz May 2, 2024

ArangoGutierrez Apr 23, 2024

TessaIO Apr 28, 2024

marquiz May 2, 2024

marquiz May 2, 2024

TessaIO May 2, 2024

marquiz May 2, 2024

TessaIO May 2, 2024

k8s-ci-robot commented May 12, 2024

codecov bot commented May 15, 2024

k8s-ci-robot commented May 15, 2024

marquiz commented May 24, 2024

		"github.com/stretchr/testify/assert"
		"sigs.k8s.io/node-feature-discovery/api/nfd/v1alpha1"

Feat integrate spiffe #1663

Are you sure you want to change the base?

Feat integrate spiffe #1663

Conversation

TessaIO commented Apr 14, 2024

k8s-ci-robot commented Apr 14, 2024

netlify bot commented Apr 14, 2024 • edited

✅ Deploy Preview for kubernetes-sigs-nfd ready!

TessaIO commented Apr 15, 2024

ArangoGutierrez commented Apr 15, 2024

ArangoGutierrez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 12, 2024

codecov bot commented May 15, 2024

Codecov Report

k8s-ci-robot commented May 15, 2024

marquiz commented May 24, 2024

netlify bot commented Apr 14, 2024 •

edited