Merge pull request #454 from wallrj/432-fix-istiod-revision-certifica…

…te-dns-names

Use `istiod-` as the prefix for the DNS names for Istio revisions

deploy/charts/istio-csr/templates/certificate.yaml

@@ -1,3 +1,10 @@
+{{- /*
+
+NOTE: If you change this Certificate remember to also update `pkg/istiodcert`.
+   This template is designed to statically create the same Certificate resource
+   that may also be dynamically generated by that Go package.
+
+*/ -}}
 {{- if eq (toString .Values.app.tls.istiodCertificateEnable) "true" }}
 apiVersion: cert-manager.io/v1
 kind: Certificate

pkg/istiodcert/worker.go

@@ -14,6 +14,12 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+/*
+NOTE: If you change this package, remember to also update `deploy/charts/istio-csr/templates/certificate.yaml`.
+   This package is designed to dynamically create the same Certificate resource
+   that may also be generated by Helm.
+*/
+
 package istiodcert
 
 import (
@@ -239,10 +245,15 @@ func (dicp *DynamicIstiodCertProvisioner) Reconcile(ctx context.Context, req ctr
 }
 
 // makeDNSNamesFromRevisions takes a list of istio revisions and produces a list of
-// corresponding DNS names for the istiod cert, as well as returning the value for the common name
+// corresponding DNS names for the istiod cert, as well as returning the value for the common name.
+// The host label of the non-default DNS names must have the prefix "istiod-",
+// because that is the hostname pattern used by clients that connect to the
+// Istio Citadel API hosted by Istio CSR.
+// This function must produce commonName and dnsNames matching those of the Helm generated certificate, here:
+// * https://github.com/cert-manager/istio-csr/blob/main/deploy/charts/istio-csr/templates/certificate.yaml
 func makeDNSNamesFromRevisions(namespace string, istioRevisions []string) (string, []string) {
 	if len(istioRevisions) == 0 {
-		// It's unlikely to have no revisisions since the default is to have a
+		// It's unlikely to have no revisions since the default is to have a
 		// list of revisions containing just "default".
 		// In any case, treat an empty list of revisions as being the same as just
 		// the default revision.
@@ -261,7 +272,7 @@ func makeDNSNamesFromRevisions(namespace string, istioRevisions []string) (strin
 			continue
 		}
 
-		dnsNames = append(dnsNames, fmt.Sprintf("istiod%s.%s.svc", revision, namespace))
+		dnsNames = append(dnsNames, fmt.Sprintf("istiod-%s.%s.svc", revision, namespace))
 	}
 
 	// Always return the default SAN as the commonName to match the behaviour of the static istiod cert

pkg/istiodcert/worker_test.go

@@ -0,0 +1,95 @@
+/*
+Copyright 2024 The cert-manager Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package istiodcert
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestMakeDNSNamesFromRevisions(t *testing.T) {
+	type testCase struct {
+		name      string
+		namespace string
+		revisions []string
+	}
+
+	tests := []testCase{
+		{
+			name:      "empty-revisions",
+			namespace: "test1",
+		},
+		{
+			name:      "default-only-revision",
+			namespace: "test2",
+			revisions: []string{"default"},
+		},
+		{
+			name:      "default-first-among-revisions",
+			namespace: "test3",
+			revisions: []string{"default", "1-21-0", "1-22-1"},
+		},
+		{
+			name:      "default-among-revisions",
+			namespace: "test3",
+			revisions: []string{"1-21-0", "default", "1-22-1"},
+		},
+		{
+			name:      "non-default-revisions",
+			namespace: "test4",
+			revisions: []string{"1-21-0", "1-22-1"},
+		},
+		{
+			name:      "duplicate-revisions",
+			namespace: "test5",
+			revisions: []string{"default", "1-21-0", "default", "1-22-1", "1-21-0"},
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			expectedRevisionPrefix := "istiod-"
+			expectedSuffix := fmt.Sprintf(".%s.svc", tc.namespace)
+			expectedCommonName := "istiod" + expectedSuffix
+
+			commonName, dnsNames := makeDNSNamesFromRevisions(tc.namespace, tc.revisions)
+
+			assert.Equal(t, expectedCommonName, commonName,
+				"The commonName should always be istiod.<namespace>.svc")
+
+			foundRevisions := make([]string, len(dnsNames))
+			for i, dnsName := range dnsNames {
+				var revision string
+				if dnsName == commonName {
+					revision = "default"
+				} else {
+					revision = dnsName[len(expectedRevisionPrefix) : len(dnsName)-len(expectedSuffix)]
+				}
+				foundRevisions[i] = revision
+			}
+
+			if len(tc.revisions) == 0 {
+				assert.Equal(t, []string{commonName}, dnsNames,
+					"The dnsNames should contain only the commonName if the supplied revisions list is empty")
+			} else {
+				assert.Equal(t, tc.revisions, foundRevisions,
+					"The original ordered revisions should be recoverable from the DNS names")
+			}
+		})
+	}
+}