Auto populate DNS names #122

faisal-memon · 2023-03-15T06:58:51Z

Auto populate DNS names from services attached to pods. Similar to the way its done in the reconcile mode k8s-workload-registrar.

Creates a field indexer for endpoints, index by pod uid
When pod comes up we lookup all endpoints with the correct uid
Adds the following DNS names: name, name.namespace, name.namespace.svc, name.namespace.svc.clusterdomain. Not a fan of the bare name but added for completeness sake.
Enabled with AutoPopulateDNSNames configurable globally and then individually on each cluster spifffe id.

fixes #48

azdagron · 2023-06-20T14:54:22Z

Sorry it has taken so long to get around to this @faisal-memon. @MarcosDY and I will try and get it reviewed sometime this week.

MarcosDY

Thank Faisal!!!
can you update crd on demo too?

controllers/endpoints_controller.go

controllers/pod_controller.go

main.go

migration/README.md

MarcosDY · 2023-06-21T15:06:48Z

pkg/spireentry/entries.go

 	}
+	sort.Strings(dnsNames) // Sort the list to provide consistent results


this sorting stuff, can be problematic in some ways, for example, if you want to set a DNS as CN the CN will be the first element on this list, so how can a user say that they want to have an specific value un CN?

in case order is not important, we can simplify add functions to work only with map (since you are verifying using that map that DNS does not exist) and then parse map to []string,
but I still think we may respect order and more in case of template ones that must go at the start of the list

Thanks @MarcosDY for catching this. Its updated to only sort the auto added dns names. The templated ones will be first and in the order specified in the crd.

MarcosDY · 2023-06-21T16:07:38Z

pkg/spireentry/entries_test.go

-	require.Len(t, entry.DNSNames, len(spec.DNSNameTemplates)-1)
+	// DNS names are unique
+	dnsNamesSet := make(map[string]struct{})
+	for _, dnsName := range entry.DNSNames {


with this approach we can get more items than expected, why not to compare 2 lists? and see that we get the exact list we expect?

Added a length check to the test.

faisal-memon · 2023-08-06T06:53:36Z

All comments have been addressed. The demo crds have been updated.

api/v1alpha1/controllermanagerconfig_types.go

faisal-memon · 2023-08-24T05:58:53Z

controllers/pod_controller.go

+	ctx := context.Background()
+	err := mgr.GetFieldIndexer().IndexField(ctx, &corev1.Endpoints{}, reconciler.EndpointUID, func(rawObj client.Object) []string {


Not sure if context.Background() is correct here or if we should use something else.

main that is calling this one has a ctx, maybe you may use that one?

Thanks, I missed that one. Change is in.

MarcosDY · 2023-09-04T14:18:57Z

controllers/pod_controller.go

+	ctx := context.Background()
+	err := mgr.GetFieldIndexer().IndexField(ctx, &corev1.Endpoints{}, reconciler.EndpointUID, func(rawObj client.Object) []string {


main that is calling this one has a ctx, maybe you may use that one?

MarcosDY · 2023-09-04T14:30:24Z

controllers/pod_controller.go

+		log := log.FromContext(ctx)
+		endpoints, ok := rawObj.(*corev1.Endpoints)
+		if !ok {
+			log.Error(nil, "unexpected type indexing fields", "type", fmt.Sprintf("%T", rawObj), "expecteed", "*corev1.Endpoints")


is this expected to happen? I'm worried we'll start to spam logs

should not happen ever

MarcosDY · 2023-09-04T15:23:05Z

migration/README.md

-    matchLabels:
-      spiffe.io/spiffe-id: "true"
-  dnsNameTemplates: ["{{ index .PodMeta.Labels \"app\" }}.{{ .PodMeta.Namespace }}.svc.cluster.local"]
+1. Enable `autoPopulateDNSNames` globally in the [Controller Manager Config](https://github.com/spiffe/spire-controller-manager/blob/main/docs/clusterspiffeid-crd.md). See [example](config/spire-controller-manager-config.yaml).


may we use the "local" path for Controller Manager Config like used in example (same comment for all paths in this file)

MarcosDY · 2023-09-04T15:24:22Z

migration/README.md

-      spiffe.io/spiffe-id: "true"
-  dnsNameTemplates: ["{{ index .PodMeta.Labels \"app\" }}.{{ .PodMeta.Namespace }}.svc.cluster.local"]
+1. Enable `autoPopulateDNSNames` globally in the [Controller Manager Config](https://github.com/spiffe/spire-controller-manager/blob/main/docs/clusterspiffeid-crd.md). See [example](config/spire-controller-manager-config.yaml).
+1. For each  [ClusterSPIFFEID](https://github.com/spiffe/spire-controller-manager/blob/main/docs/clusterspiffeid-crd.md) you to auto populate DNS names for, set the `autoPopulateDNSNames` field there. See [example](config/clusterspiffeid.yaml).


Have each listed as 1. i think is better. Then you don't have to update every line if you add an item in the middle.

MarcosDY · 2023-09-04T15:27:09Z

migration/README.md

-      spiffe.io/spiffe-id: "true"
-  dnsNameTemplates: ["{{ index .PodMeta.Labels \"app\" }}.{{ .PodMeta.Namespace }}.svc.cluster.local"]
+1. Enable `autoPopulateDNSNames` globally in the [Controller Manager Config](https://github.com/spiffe/spire-controller-manager/blob/main/docs/clusterspiffeid-crd.md). See [example](config/spire-controller-manager-config.yaml).
+1. For each  [ClusterSPIFFEID](https://github.com/spiffe/spire-controller-manager/blob/main/docs/clusterspiffeid-crd.md) you to auto populate DNS names for, set the `autoPopulateDNSNames` field there. See [example](config/clusterspiffeid.yaml).


~~you~~ to auto populate DNS names for

MarcosDY · 2023-09-04T16:15:42Z

pkg/spireentry/entries.go

@@ -160,6 +166,23 @@ func renderDNSName(tmpl *template.Template, data *templateData) (string, error)
 	return rendered, nil
 }

+func addServiceDNSNames(dnsNames []string, dnsNamesSet map[string]struct{}, endpointsList *corev1.EndpointsList, clusterDomain string) []string {


what do you think about this function returning the list of dns from endpoints (only) and sorted.
and in the caller add the new dns to the list?

dsnNames = append(dnsNames, dnsNamesFromEndpoints( dnsNamesSet, endpointsList, clusterDomain))

that wil simplify this code

Change made.

faisal-memon · 2023-09-20T04:42:06Z

Docs updated. Trying to rebase to fix the dco causes a lot merge conflicts. Whenever this is approved Ill squash and then rebase to fix the dco.

MarcosDY

looks good, just a simple nit, and can you solve DCO?

MarcosDY · 2023-10-04T15:20:52Z

controllers/pod_controller.go

+	// Index endpoints by UID. Later when we reconcile the Pod this will make it easy to find the associated endpoints
+	// and auto populate DNS names.
+	err := mgr.GetFieldIndexer().IndexField(ctx, &corev1.Endpoints{}, reconciler.EndpointUID, func(rawObj client.Object) []string {
+		log := log.FromContext(ctx)


log is only used whn error, can you move into if?

Signed-off-by: Faisal Memon <[email protected]>

faisal-memon · 2023-10-04T21:08:00Z

@MarcosDY all comments addressed, dco fixed.

faisal-memon requested review from azdagron and MarcosDY as code owners March 15, 2023 06:58

azdagron mentioned this pull request Jun 13, 2023

Production Implementation #100

Closed

MarcosDY reviewed Jun 21, 2023

View reviewed changes

faisal-memon commented Aug 6, 2023

View reviewed changes

api/v1alpha1/controllermanagerconfig_types.go Outdated Show resolved Hide resolved

faisal-memon commented Aug 24, 2023

View reviewed changes

MarcosDY reviewed Sep 4, 2023

View reviewed changes

MarcosDY reviewed Oct 4, 2023

View reviewed changes

faisal-memon force-pushed the 48 branch from 9ac025a to 72591cb Compare October 4, 2023 20:52

Auto populate DNS names

7569c86

Signed-off-by: Faisal Memon <[email protected]>

faisal-memon force-pushed the 48 branch from 72591cb to 7569c86 Compare October 4, 2023 20:54

faisal-memon added 2 commits October 4, 2023 14:02

Fix rebase errors

5941adf

Signed-off-by: Faisal Memon <[email protected]>

Move log to if

1390180

Signed-off-by: Faisal Memon <[email protected]>

MarcosDY approved these changes Oct 5, 2023

View reviewed changes

MarcosDY merged commit 091d297 into spiffe:main Oct 5, 2023
6 checks passed

MarcosDY added this to the 0.4.0 milestone Nov 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto populate DNS names #122

Auto populate DNS names #122

faisal-memon commented Mar 15, 2023 •

edited

Loading

azdagron commented Jun 20, 2023

MarcosDY left a comment

MarcosDY Jun 21, 2023

MarcosDY Jun 21, 2023

faisal-memon Aug 6, 2023

MarcosDY Jun 21, 2023

faisal-memon Aug 6, 2023

faisal-memon commented Aug 6, 2023

faisal-memon Aug 24, 2023

MarcosDY Sep 4, 2023

faisal-memon Sep 11, 2023

MarcosDY Sep 4, 2023

MarcosDY Sep 4, 2023

MarcosDY Sep 4, 2023

faisal-memon Sep 11, 2023

MarcosDY Sep 4, 2023

faisal-memon Sep 11, 2023

MarcosDY Sep 4, 2023

faisal-memon Sep 11, 2023

MarcosDY Sep 4, 2023

faisal-memon Sep 11, 2023

MarcosDY Sep 4, 2023

faisal-memon Sep 11, 2023

faisal-memon commented Sep 20, 2023 •

edited

Loading

MarcosDY left a comment

MarcosDY Oct 4, 2023

faisal-memon Oct 4, 2023

faisal-memon commented Oct 4, 2023

		}
		sort.Strings(dnsNames) // Sort the list to provide consistent results

		ctx := context.Background()
		err := mgr.GetFieldIndexer().IndexField(ctx, &corev1.Endpoints{}, reconciler.EndpointUID, func(rawObj client.Object) []string {

Auto populate DNS names #122

Auto populate DNS names #122

Conversation

faisal-memon commented Mar 15, 2023 • edited Loading

azdagron commented Jun 20, 2023

MarcosDY 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

faisal-memon commented Aug 6, 2023

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

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

faisal-memon commented Sep 20, 2023 • edited Loading

MarcosDY left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faisal-memon commented Oct 4, 2023

faisal-memon commented Mar 15, 2023 •

edited

Loading

faisal-memon commented Sep 20, 2023 •

edited

Loading