Static Manifest Support by kfox1111 · Pull Request #411 · spiffe/spire-controller-manager

kfox1111 · 2024-09-15T00:48:57Z

Enables watching a directory for CRs to configure SPIRE with rather then get them from Kubernetes. This allows for simple to configure stand alone SPIRE servers, easy integration with traditional configuration management systems, and/or easy syncing of config from git.

It supports:

ClusterStaticEntries
ClusterFederatedTrustDomains

Enables watching a directory for CRs to configure SPIRE with rather then get them from Kubernetes. This allows for simple to configure stand alone SPIRE servers, easy integration with traditional configuration management systems, and/or easy syncing of config from git. It supports: * ClusterStaticEntries * ClusterFederatedTrustDomains Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

MarcosDY · 2025-01-28T13:29:45Z

+		return nil, err
+	}
+	for _, file := range files {
+		var entry = ClusterFederatedTrustDomain{}


Suggested change

var entry = ClusterFederatedTrustDomain{}

var entry ClusterFederatedTrustDomain

but instead of creating variable here, what do you think about LoadClusterFederatedTrustDomainFromFile returning the Entry object instead?

is LoadClusterFederatedTrustDomainFromFile intended to be used out of this package?
if not can you remove and use loadClusterFederatedTrustDomainFile instead?

MarcosDY · 2025-03-13T13:41:30Z

+		if entry.APIVersion != "spire.spiffe.io/v1alpha1" || entry.Kind != "ClusterStaticEntry" {
+			continue
+		}


if parse fails entry is going to be empty, and always continue here, so error is dismissed, can you check error first?
or are you trying to always process all files? if that is the came, maybe we can add logs?

~~Yeah. Was thinking dropping in one bad file shouldn't break syncing the good ones. +1 for logging.~~

Actually, re reviewing the code, it does throw an error in the next conditional. The idea being, if you typo an entry you previously had, it doesn't retract the entry during sync, breaking something working. Better to break syncing, which you can catch, then retract something.

The APIVersion/Kind thing though happens before error checking, as it could have tried to parse other types in the same dir, and the check ignores failures in files its not intending to parse.

Added some comments to help try and make things more clear.

MarcosDY · 2025-03-13T13:50:01Z

-		retval.reconcile.ClusterSPIFFEIDs = true
 		retval.reconcile.ClusterFederatedTrustDomains = true
 		retval.reconcile.ClusterStaticEntries = true
+		if retval.ctrlConfig.StaticManifestPath == nil {


can you add a quick comment about only one is supported?

That only one static manifest path can be watched?

Oh... This is setting the defaults.

If the user didn't set which things are reconciled explicitly, this sets the defaults depending on if in static mode.

if static, default reconcile option is:

clusterSPIFFEIDs: false clusterFederatedTrustDomains: true clusterStaticEntries: true

if k8s mode, default reconcile option is:

clusterSPIFFEIDs: true clusterFederatedTrustDomains: true clusterStaticEntries: true

I guess the conditional could be removed and just set:

retval.reconcile.ClusterSPIFFEIDs = retval.ctrlConfig.StaticManifestPath == nil

Not sure that is more clear or not though...What do you think?

This comment refers to the old implementation and is no longer relevant after the recent refactor. However, adding a brief explanation for why ClusterSPIFFEIDs is set to true when clusterStaticEntry is set would still be valuable for future readers.

Regarding the if vs ==, using if feels clear to me since the intent is easy to understand at a glance.

MarcosDY · 2025-03-13T13:52:40Z

+	if mainConfig.ctrlConfig.StaticManifestPath != nil {
+		mgr, err = ctrl.NewManager(&rest.Config{}, mainConfig.options)
+	} else {
+		mgr, err = ctrl.NewManager(ctrl.GetConfigOrDie(), mainConfig.options)
+	}


how about:

restConfig := &rest.Config{} if mainConfig.ctrlConfig.StaticManifestPath == nil { restConfig = ctrl.GetConfigOrDie() } mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), mainConfig.options)

Looks good. except splitting the two run functions made this no longer needed.

MarcosDY · 2025-03-13T13:53:47Z


 func run(mainConfig Config) (err error) {
 	webhookEnabled := os.Getenv("ENABLE_WEBHOOKS") != "false"
+	if mainConfig.ctrlConfig.StaticManifestPath != nil {


may we add documentation or a way so users can know than webhook is not going to work if staticmanifestpath is set?

MarcosDY · 2025-03-13T14:00:59Z

+	if mainConfig.ctrlConfig.StaticManifestPath == nil {
+		k8sClient = mgr.GetClient()
+	}


client is already set in 352, is this new get required? (same comment to all lines like this)

Not sure. was just reproducing the existing behavior before the change. But the duplicated run now undoes this change.

MarcosDY · 2025-04-14T11:14:39Z

+	if mainConfig.ctrlConfig.StaticManifestPath != nil {
+		mgr, err = ctrl.NewManager(&rest.Config{}, mainConfig.options)
+	} else {
+		mgr, err = ctrl.NewManager(ctrl.GetConfigOrDie(), mainConfig.options)


this is never going to be true right?
we are only going to call static when StaticManifestPath != nil,
if that is the case may we fail here?

oh. left over from before. will fix.

MarcosDY · 2025-04-14T11:18:56Z

+		}
+	}
+	go func() {
+		err = entryReconciler.Run(context.TODO())


should we run this only when entryReconciler is initialized? maybe using a pointer here?
Same for federation

can you use the same context for everything? the one from ctx := ctrl.SetupSignalHandler() so if that on is canceled all finish as expected

should we run this only when entryReconciler is initialized? maybe using a pointer here?
Same for federation

Hmm.... without the interleaved k8s code in the staticRun function, I think we can just move it up into the conditionals above to guard that. Done

MarcosDY · 2025-04-14T11:28:52Z

+	}
+
+	setupLog.Info("starting manager")
+	if err := mgr.Start(ctx); err != nil {


is it possible to get into an edge case caused by go routines didnt start at before this one?
if that is the case may we use groups?

Yeah. Thats a good thought. Will add

MarcosDY · 2025-04-14T11:49:54Z

+		clusterStaticEntries, err = k8sapi.ListClusterStaticEntries(ctx, r.config.K8sClient)
+	} else {
+		// FIXME prebuild / pass scheme?
+		scheme := runtime.NewScheme()


are you planing to improve this schema? if it is not used until the very end may we create it there?

Not intending on it. Moved.

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

MarcosDY · 2025-04-16T13:52:23Z

-		err = LoadClusterFederatedTrustDomainFromFile(fullfile, scheme, &entry, expandEnv)
+		entry, err := loadClusterFederatedTrustDomainFile(fullfile, scheme, expandEnv)
+		// Ignore files of the wrong type in manifestPath
 		if entry.APIVersion != "spire.spiffe.io/v1alpha1" || entry.Kind != "ClusterFederatedTrustDomain" {


is not this going to cause than all errors are avoid? because we are going to return an empty entry and continue is going to always call,
may we log errors so users can understand why their files are not loaded? (same comment to statis entry)

The same directory contains multiple types of files, parsed by different loops. so a file ignored here are picked up in say, api/v1alpha1/clusterstaticentry_loader.go. The only way to log the ignored ones would be to keep track of the files processed by each of the two reconciliation loops, synchronizing the passes, and then log the difference. Very tricky. :(

The juice is probably not worth the squeeze. Maybe we don't and if users request it, we reevaluate?

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Co-authored-by: Marcos Yacob <marcosyacob@gmail.com> Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>

kfox1111 requested review from MarcosDY and azdagron as code owners September 15, 2024 00:48

kfox1111 marked this pull request as draft September 15, 2024 00:49

kfox1111 commented Sep 15, 2024

View reviewed changes

Comment thread cmd/main.go Outdated

kfox1111 added 5 commits December 14, 2024 13:04

Merge branch 'main' into static

738e9ee

Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>

Make things configurable

aafa013

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Lint fixes

3eb973f

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Fix more lint

07baade

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Fix more lint

d1084dc

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

kfox1111 marked this pull request as ready for review December 15, 2024 03:33

kfox1111 mentioned this pull request Dec 15, 2024

The Bottom Turtle Reference Architecture(s) spiffe/spire#5206

Open

6 tasks

kfox1111 added 15 commits December 21, 2024 14:44

Log filename on bad parse

c71c812

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Merge branch 'main' into static

c3c00c3

Merge branch 'main' into static

03988d3

Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>

Merge branch 'main' into static

1994809

Merge branch 'main' into static

038a853

Merge branch 'main' into static

9e188b6

Merge branch 'main' into static

5d4d020

Merge branch 'main' into static

bece8a4

Merge branch 'main' into static

217e17c

Merge branch 'main' into static

6e61355

Merge branch 'main' into static

cfca1bb

Merge branch 'main' into static

10acc00

Merge branch 'main' into static

045fced

Merge branch 'main' into static

fba1954

Explore forking the run function instead

fed0759

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

MarcosDY reviewed Apr 14, 2025

View reviewed changes

kfox1111 added 2 commits April 14, 2025 07:17

Incorperate feedback

6322652

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Merge branch 'main' into static

5da8b2a

Merge branch 'main' into static

a5cabbf

MarcosDY reviewed Apr 16, 2025

View reviewed changes

kfox1111 and others added 2 commits April 16, 2025 07:17

Incorperate feedback

fe509b6

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Apply suggestions from code review

5c0d622

Co-authored-by: Marcos Yacob <marcosyacob@gmail.com> Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>

MarcosDY approved these changes Apr 16, 2025

View reviewed changes

MarcosDY mentioned this pull request Apr 16, 2025

Add Integration Test and Demo for Static Manifest Feature #524

Open

kfox1111 merged commit e994f56 into spiffe:main Apr 16, 2025
6 checks passed

MarcosDY added this to the 0.6.2 milestone Apr 16, 2025

	var entry = ClusterFederatedTrustDomain{}
	var entry ClusterFederatedTrustDomain

Conversation

kfox1111 commented Sep 15, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfox1111 Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kfox1111 Apr 14, 2025 •

edited

Loading