Feat integrate spiffe #1663

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

Resolves #1186

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

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`a73437c`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/67960ad8344cb00008d64918
😎 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!

We don't have dynamic configuration anymore so we can have both, I think.

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

/milestone v0.17

TessaIO · 2024-12-24T10:27:43Z

ping @marquiz @ArangoGutierrez Can you please take a look?

TessaIO · 2024-12-30T11:27:46Z

/retest

k8s-ci-robot · 2025-01-09T14:41:21Z

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.

Signed-off-by: TessaIO <[email protected]>

k8s-ci-robot · 2025-01-26T10:13:49Z

[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 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

k8s-ci-robot · 2025-01-26T10:14:23Z

@TessaIO: The following tests 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-gh-pages-generic	`a73437c`	link	true	`/test pull-node-feature-discovery-build-gh-pages-generic`
pull-node-feature-discovery-e2e-test-master	`a73437c`	link	true	`/test pull-node-feature-discovery-e2e-test-master`
pull-node-feature-discovery-verify-docs-master	`a73437c`	link	true	`/test pull-node-feature-discovery-verify-docs-master`
pull-node-feature-discovery-build-image-cross-generic	`a73437c`	link	true	`/test pull-node-feature-discovery-build-image-cross-generic`
pull-node-feature-discovery-build-master	`a73437c`	link	true	`/test pull-node-feature-discovery-build-master`
pull-node-feature-discovery-verify-master	`a73437c`	link	true	`/test pull-node-feature-discovery-verify-master`

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.

marquiz · 2025-01-30T12:09:34Z

Thanks for the update @TessaIO. We need to forward this, not let it linger it in a limbo state forever. I'd want to get this in as an experimental feature soon.

/assign @ArangoGutierrez

marquiz · 2025-01-30T12:18:09Z

deployment/helm/node-feature-discovery/Chart.lock

+dependencies:
+- name: spire
+  repository: https://spiffe.github.io/helm-charts-hardened/
+  version: 0.24.1


Update this. Spire v1 has been released(?)

marquiz · 2025-01-30T12:19:13Z

deployment/helm/node-feature-discovery/Chart.yaml

+dependencies:
+  - name: spire
+    version: 0.24.1
+    repository: https://spiffe.github.io/helm-charts-hardened/


We should have a conditional for this. Only require if spiffe is enabled. Right(?)

marquiz · 2025-01-30T12:29:31Z

pkg/nfd-master/nfd-master.go

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


We don't have dynamic configuration anymore so we can have both, I think.

marquiz · 2025-01-30T12:30:04Z

deployment/helm/node-feature-discovery/values.yaml

@@ -597,3 +597,47 @@ prometheus:
  enable: false
  scrapeInterval: 10s
  labels: {}
+
+spire:
+  enabled: true


Default should be false(?)

marquiz · 2025-02-10T12:12:02Z

deployment/helm/node-feature-discovery/templates/master.yaml

+            path: /run/spire/agent-sockets
+            type: Directory


nit: would narrowing down work

Suggested change

path: /run/spire/agent-sockets

type: Directory

path: /run/spire/agent-sockets/api.sock

type: Socket

marquiz · 2025-02-10T12:21:08Z

pkg/nfd-master/nfd-master.go

+		}
+
+		if isSignatureVerified {
+			klog.InfoS("NodeFeature verified", "NodeFeature name", obj.Name)


more comprehensive reference:

Suggested change

klog.InfoS("NodeFeature verified", "NodeFeature name", obj.Name)

klog.InfoS("NodeFeature verified", "nodefeature", klog.KObj(obj))

marquiz · 2025-02-10T12:21:35Z

pkg/nfd-master/nfd-master.go

+			klog.InfoS("NodeFeature verified", "NodeFeature name", obj.Name)
+			verifiedObjects = append(verifiedObjects, obj)
+		} else {
+			klog.InfoS("NodeFeature not verified, skipping...", "NodeFeature name", obj.Name)


Suggested change

klog.InfoS("NodeFeature not verified, skipping...", "NodeFeature name", obj.Name)

klog.InfoS("NodeFeature not verified, skipping...", "nodefeature", klog.KObj(obj))

marquiz · 2025-02-10T15:08:23Z

pkg/nfd-worker/nfd-worker.go

@@ -667,6 +682,14 @@ func (m *nfdWorker) updateNodeFeatureObject(labels Labels) error {
 		}
 		klog.InfoS("creating NodeFeature object", "nodefeature", klog.KObj(nfr))

+		// If Spiffe is enabled, we add the signature to the annotations section


Small note: I think we should add Namespace: namespace to the nfr created above so that the "SpiffeData" (and thus the checksum) will be identical in the next discovery cycle when we get the object from the api server (i.e. !errors.IsNotFound(err))

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 force-pushed the feat-integrate-spiffe branch from aee2686 to 520dd72 Compare May 12, 2024 19:27

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

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 20, 2024

TessaIO force-pushed the feat-integrate-spiffe branch from da1af98 to 18e6360 Compare December 21, 2024 09:58

TessaIO changed the title ~~Feat integrate spiffe~~ [WIP] Feat integrate spiffe Dec 21, 2024

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 21, 2024

TessaIO force-pushed the feat-integrate-spiffe branch 3 times, most recently from 903a7cf to c6c6b85 Compare December 23, 2024 23:04

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 23, 2024

TessaIO force-pushed the feat-integrate-spiffe branch from c6c6b85 to 9b0d16a Compare December 23, 2024 23:06

TessaIO changed the title ~~[WIP] Feat integrate spiffe~~ Feat integrate spiffe Dec 24, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 24, 2024

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

deploy: add spire manifests in helm and kustomize

a73437c

Signed-off-by: TessaIO <[email protected]>

TessaIO force-pushed the feat-integrate-spiffe branch from 9b0d16a to a73437c Compare January 26, 2025 10:13

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 26, 2025

k8s-ci-robot assigned ArangoGutierrez Jan 30, 2025

marquiz reviewed Jan 30, 2025

View reviewed changes

marquiz reviewed Feb 10, 2025

View reviewed changes

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 •

edited

Loading

netlify bot commented Apr 14, 2024 •

edited

Loading

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

marquiz Jan 30, 2025

marquiz commented May 24, 2024

TessaIO commented Dec 24, 2024

TessaIO commented Dec 30, 2024

k8s-ci-robot commented Jan 9, 2025

k8s-ci-robot commented Jan 26, 2025

k8s-ci-robot commented Jan 26, 2025

marquiz commented Jan 30, 2025

marquiz Jan 30, 2025

marquiz Jan 30, 2025

marquiz Jan 30, 2025

marquiz Jan 30, 2025

marquiz Feb 10, 2025

marquiz Feb 10, 2025

marquiz Feb 10, 2025

marquiz Feb 10, 2025

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

	klog.InfoS("NodeFeature verified", "NodeFeature name", obj.Name)
	klog.InfoS("NodeFeature verified", "nodefeature", klog.KObj(obj))

	klog.InfoS("NodeFeature not verified, skipping...", "NodeFeature name", obj.Name)
	klog.InfoS("NodeFeature not verified, skipping...", "nodefeature", klog.KObj(obj))

Feat integrate spiffe #1663

Are you sure you want to change the base?

Feat integrate spiffe #1663

Conversation

TessaIO commented Apr 14, 2024 • edited Loading

netlify bot commented Apr 14, 2024 • edited Loading

✅ 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

Choose a reason for hiding this comment

marquiz commented May 24, 2024

TessaIO commented Dec 24, 2024

TessaIO commented Dec 30, 2024

k8s-ci-robot commented Jan 9, 2025

k8s-ci-robot commented Jan 26, 2025

k8s-ci-robot commented Jan 26, 2025

marquiz commented Jan 30, 2025

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

TessaIO commented Apr 14, 2024 •

edited

Loading

netlify bot commented Apr 14, 2024 •

edited

Loading