Merge pull request #812 from jetstack/no_change

Make --tsg-id and --ngts-server-url mutually exclusive

Author: inteon

.github/workflows/tests.yaml

@@ -146,9 +146,9 @@ jobs:
       - run: make -j ngts-test-e2e
         env:
           OCI_BASE: ${{ secrets.NGTS_OCI_BASE }}
-          NGTS_CLIENT_ID: ${{ secrets.NGTS_CLIENT_ID }}
+          NGTS_CLIENT_ID: e3c8bde7-5f13-11f1-99f4-5e067e231041
           NGTS_PRIVATE_KEY: ${{ secrets.NGTS_PRIVATE_KEY }}
-          NGTS_TSG_ID: ${{ secrets.NGTS_TSG_ID }}
+          NGTS_TSG_URL: https://1806660206.ngts.qa.venafi.io
 
   test-e2e:
     if: contains(github.event.pull_request.labels.*.name, 'test-e2e')

deploy/charts/discovery-agent/README.md

@@ -12,7 +12,7 @@ The Discovery Agent connects your Kubernetes or OpenShift cluster to Palo Alto N
 > ""
 > ```
 
-Required: The TSG (Tenant Service Group) ID to use when connecting to SCM.
+The TSG (Tenant Service Group) ID to use when connecting to SCM. The production SCM server URL is derived from this value. Required unless config.serverURL is set. Mutually exclusive with config.serverURL.
 
 
 #### **config.clusterName** ~ `string`

deploy/charts/discovery-agent/templates/deployment.yaml

@@ -72,11 +72,14 @@ spec:
             - "-c"
             - "/etc/discovery-agent/config.yaml"
             - --ngts
-            - --tsg-id
-            - {{ required "config.tsgID is required" .Values.config.tsgID | include "discovery-agent.stringOrNumber" | quote }}
-            {{- with .Values.config.serverURL }}
+            {{- if and .Values.config.tsgID .Values.config.serverURL }}
+              {{- fail "config.tsgID and config.serverURL are mutually exclusive; set exactly one" }}
+            {{- else if .Values.config.serverURL }}
             - --ngts-server-url
-            - {{ . | quote }}
+            - {{ .Values.config.serverURL | quote }}
+            {{- else }}
+            - --tsg-id
+            - {{ required "config.tsgID is required when config.serverURL is not set" .Values.config.tsgID | include "discovery-agent.stringOrNumber" | quote }}
             {{- end }}
             {{- if or .Values.config.clientID .Values.config.clientId }}
             - --client-id

deploy/charts/discovery-agent/tests/deployment_test.yaml

@@ -165,10 +165,9 @@ tests:
           path: spec.template.spec.containers[0].args
           content: --enable-pprof
 
-  - it: should include custom server URL when set
+  - it: should include custom server URL when set, omitting --tsg-id
     set:
       config.clusterName: test-cluster
-      config.tsgID: "123456"
       config.serverURL: "https://custom.example.com"
     asserts:
       - contains:
@@ -177,6 +176,25 @@ tests:
       - contains:
           path: spec.template.spec.containers[0].args
           content: "https://custom.example.com"
+      - notContains:
+          path: spec.template.spec.containers[0].args
+          content: --tsg-id
+
+  - it: should fail when both tsgID and serverURL are set
+    set:
+      config.clusterName: test-cluster
+      config.tsgID: "123456"
+      config.serverURL: "https://custom.example.com"
+    asserts:
+      - failedTemplate:
+          errorMessage: "config.tsgID and config.serverURL are mutually exclusive; set exactly one"
+
+  - it: should fail when neither tsgID nor serverURL is set
+    set:
+      config.clusterName: test-cluster
+    asserts:
+      - failedTemplate:
+          errorMessage: "config.tsgID is required when config.serverURL is not set"
 
   - it: should include client ID when set
     set:

deploy/charts/discovery-agent/values.schema.json

@@ -178,12 +178,12 @@
     },
     "helm-values.config.serverURL": {
       "default": "",
-      "description": "Explicit SCM server URL (optional).\nIf not set, a production SCM server URL will be created based on the TSG ID. This value is intended for development purposes only and should not be set in production.",
+      "description": "Explicit SCM server URL (optional).\nIf not set, the production SCM server URL is derived from config.tsgID. This value is intended for development purposes only and should not be set in production.\nMutually exclusive with config.tsgID.",
       "type": "string"
     },
     "helm-values.config.tsgID": {
       "default": "",
-      "description": "Required: The TSG (Tenant Service Group) ID to use when connecting to SCM."
+      "description": "The TSG (Tenant Service Group) ID to use when connecting to SCM. The production SCM server URL is derived from this value. Required unless config.serverURL is set. Mutually exclusive with config.serverURL."
     },
     "helm-values.extraArgs": {
       "default": [],

deploy/charts/discovery-agent/values.yaml

@@ -1,6 +1,8 @@
 # Configuration for the Discovery Agent
 config:
-  # Required: The TSG (Tenant Service Group) ID to use when connecting to SCM.
+  # The TSG (Tenant Service Group) ID to use when connecting to SCM.
+  # The production SCM server URL is derived from this value.
+  # Required unless config.serverURL is set. Mutually exclusive with config.serverURL.
   # +docs:property
   # +docs:type=number,string
   tsgID: ""
@@ -61,8 +63,9 @@ config:
   secretName: discovery-agent-credentials
 
   # Explicit SCM server URL (optional).
-  # If not set, a production SCM server URL will be created based on the TSG ID.
+  # If not set, the production SCM server URL is derived from config.tsgID.
   # This value is intended for development purposes only and should not be set in production.
+  # Mutually exclusive with config.tsgID.
   # +docs:hidden
   serverURL: ""
 

hack/ngts/test-e2e.sh

@@ -20,7 +20,7 @@ set -o pipefail
 # NGTS API configuration
 : ${NGTS_CLIENT_ID?}
 : ${NGTS_PRIVATE_KEY?}
-: ${NGTS_TSG_ID?}
+: ${NGTS_TSG_URL?}
 
 # The base URL of the OCI registry used for Docker images and Helm charts
 # E.g. ttl.sh/7e6ca67c-96dc-4dea-9437-80b0f3a69fb1
@@ -77,18 +77,17 @@ pprof:
 
 fullnameOverride: discovery-agent
 
-imageRegistry: ${OCI_BASE}
+imageRegistry: "${OCI_BASE}"
 imageNamespace: ""
 
 image:
-  digest: ${NGTS_IMAGE_DIGEST}
+  digest: "${NGTS_IMAGE_DIGEST}"
 
 config:
   clusterName: "e2e-test-cluster-ngts"
   clusterDescription: "A temporary cluster for E2E testing NGTS"
   period: 10s
-  tsgID: "${NGTS_TSG_ID}"
-  serverURL: "https://${NGTS_TSG_ID}.ngts.dev.venafi.io"
+  serverURL: "${NGTS_TSG_URL}"
 
 podLabels:
   "discovery-agent.ngts/test-id": "${RANDOM}"

pkg/agent/config.go

@@ -188,10 +188,13 @@ type AgentCmdFlags struct {
 	NGTSMode bool
 
 	// TSGID (--tsg-id) is the TSG (Tenant Service Group) ID for NGTS mode.
+	// The production NGTS server URL is derived from this value. Mutually
+	// exclusive with --ngts-server-url.
 	TSGID string
 
 	// NGTSServerURL (--ngts-server-url) is a hidden flag for developers to
-	// override the NGTS server URL for testing purposes.
+	// point the agent at a custom NGTS server URL for testing purposes.
+	// Mutually exclusive with --tsg-id.
 	NGTSServerURL string
 }
 
@@ -350,13 +353,15 @@ func InitAgentCmdFlags(c *cobra.Command, cfg *AgentCmdFlags) {
 		"ngts",
 		false,
 		"Enables NGTS mode. The agent will authenticate using key pair authentication and send data to NGTS endpoints. "+
-			"Must be used in conjunction with --tsg-id and --private-key-path. --client-id is optional if provided in the credentials secret.",
+			"Must be used with --private-key-path and exactly one of --tsg-id or --ngts-server-url. "+
+			"--client-id is optional if provided in the credentials secret.",
 	)
 	c.PersistentFlags().StringVar(
 		&cfg.TSGID,
 		"tsg-id",
 		"",
-		"The TSG (Tenant Service Group) ID for NGTS mode. Required when using --ngts.",
+		"The TSG (Tenant Service Group) ID for NGTS mode. The production NGTS server URL is derived from this value. "+
+			"Mutually exclusive with --ngts-server-url; exactly one must be provided when using --ngts.",
 	)
 
 	ngtsServerURLFlag := "ngts-server-url"
@@ -365,7 +370,8 @@ func InitAgentCmdFlags(c *cobra.Command, cfg *AgentCmdFlags) {
 		&cfg.NGTSServerURL,
 		ngtsServerURLFlag,
 		"",
-		"Override the NGTS server URL for testing purposes. This flag is intended for agent development and should not need to be set.",
+		"Override the NGTS server URL for testing purposes. This flag is intended for agent development and should not need to be set. "+
+			"Mutually exclusive with --tsg-id.",
 	)
 
 	// ngts-server-url is intended only for developers, so hide it from help
@@ -505,7 +511,7 @@ func ValidateAndCombineConfig(log logr.Logger, cfg Config, flags AgentCmdFlags)
 		default:
 			return CombinedConfig{}, nil, fmt.Errorf("no output mode specified. " +
 				"To enable one of the output modes, you can:\n" +
-				" - Use --ngts with --tsg-id and --private-key-path to use the " + string(NGTS) + " mode (--client-id is optional if provided in the credentials secret).\n" +
+				" - Use --ngts with --private-key-path and exactly one of --tsg-id or --ngts-server-url to use the " + string(NGTS) + " mode (--client-id is optional if provided in the credentials secret).\n" +
 				" - Use (--venafi-cloud with --credentials-file) or (--client-id with --private-key-path) to use the " + string(VenafiCloudKeypair) + " mode.\n" +
 				" - Use --venafi-connection for the " + string(VenafiCloudVenafiConnection) + " mode.\n" +
 				" - Use --credentials-file alone if you want to use the " + string(JetstackSecureOAuth) + " mode.\n" +
@@ -523,8 +529,11 @@ func ValidateAndCombineConfig(log logr.Logger, cfg Config, flags AgentCmdFlags)
 
 	// Validation of NGTS mode requirements.
 	if res.OutputMode == NGTS {
-		if flags.TSGID == "" {
-			errs = multierror.Append(errs, fmt.Errorf("--tsg-id is required when using --ngts"))
+		switch {
+		case flags.TSGID != "" && flags.NGTSServerURL != "":
+			errs = multierror.Append(errs, fmt.Errorf("--tsg-id and --ngts-server-url are mutually exclusive; exactly one must be provided when using --ngts"))
+		case flags.TSGID == "" && flags.NGTSServerURL == "":
+			errs = multierror.Append(errs, fmt.Errorf("either --tsg-id or --ngts-server-url is required when using --ngts"))
 		}
 		if flags.PrivateKeyPath == "" {
 			errs = multierror.Append(errs, fmt.Errorf("--private-key-path is required when using --ngts"))

pkg/agent/config_test.go

@@ -195,7 +195,7 @@ func Test_ValidateAndCombineConfig(t *testing.T) {
 		)
 		assert.EqualError(t, err, testutil.Undent(`
 			no output mode specified. To enable one of the output modes, you can:
-			 - Use --ngts with --tsg-id and --private-key-path to use the NGTS mode (--client-id is optional if provided in the credentials secret).
+			 - Use --ngts with --private-key-path and exactly one of --tsg-id or --ngts-server-url to use the NGTS mode (--client-id is optional if provided in the credentials secret).
 			 - Use (--venafi-cloud with --credentials-file) or (--client-id with --private-key-path) to use the Venafi Cloud Key Pair Service Account mode.
 			 - Use --venafi-connection for the Venafi Cloud VenafiConnection mode.
 			 - Use --credentials-file alone if you want to use the Jetstack Secure OAuth mode.
@@ -1124,28 +1124,28 @@ func Test_ValidateAndCombineConfig_NGTS(t *testing.T) {
 				period: 1h
 				cluster_name: test-cluster
 			`)),
-			withCmdLineFlags("--ngts", "--tsg-id", "test-tsg-123", "--client-id", "test-client-id", "--private-key-path", privKeyPath, "--ngts-server-url", "https://ngts.test.example.com"))
+			withCmdLineFlags("--ngts", "--client-id", "test-client-id", "--private-key-path", privKeyPath, "--ngts-server-url", "https://ngts.test.example.com"))
 		require.NoError(t, err)
 		assert.Equal(t, NGTS, got.OutputMode)
+		assert.Equal(t, "", got.TSGID)
 		assert.Equal(t, "https://ngts.test.example.com", got.NGTSServerURL)
 		assert.IsType(t, &client.NGTSClient{}, cl)
 	})
 
-	t.Run("ngts: missing --ngts flag should not trigger NGTS mode", func(t *testing.T) {
+	t.Run("ngts: --tsg-id and --ngts-server-url are mutually exclusive", func(t *testing.T) {
 		t.Setenv("POD_NAMESPACE", "venafi")
 		privKeyPath := withFile(t, fakePrivKeyPEM)
 		_, _, err := ValidateAndCombineConfig(discardLogs(),
 			withConfig(testutil.Undent(`
 				period: 1h
 				cluster_name: test-cluster
 			`)),
-			withCmdLineFlags("--tsg-id", "test-tsg-123", "--client-id", "test-client-id", "--private-key-path", privKeyPath))
-		// Should select VenafiCloudKeypair mode instead when --ngts is not specified
+			withCmdLineFlags("--ngts", "--tsg-id", "test-tsg-123", "--client-id", "test-client-id", "--private-key-path", privKeyPath, "--ngts-server-url", "https://ngts.test.example.com"))
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "venafi-cloud.upload_path")
+		assert.Contains(t, err.Error(), "--tsg-id and --ngts-server-url are mutually exclusive")
 	})
 
-	t.Run("ngts: missing --tsg-id should error", func(t *testing.T) {
+	t.Run("ngts: missing both --tsg-id and --ngts-server-url should error", func(t *testing.T) {
 		t.Setenv("POD_NAMESPACE", "venafi")
 		privKeyPath := withFile(t, fakePrivKeyPEM)
 		_, _, err := ValidateAndCombineConfig(discardLogs(),
@@ -1155,7 +1155,21 @@ func Test_ValidateAndCombineConfig_NGTS(t *testing.T) {
 			`)),
 			withCmdLineFlags("--ngts", "--client-id", "test-client-id", "--private-key-path", privKeyPath))
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "--tsg-id is required when using --ngts")
+		assert.Contains(t, err.Error(), "either --tsg-id or --ngts-server-url is required when using --ngts")
+	})
+
+	t.Run("ngts: missing --ngts flag should not trigger NGTS mode", func(t *testing.T) {
+		t.Setenv("POD_NAMESPACE", "venafi")
+		privKeyPath := withFile(t, fakePrivKeyPEM)
+		_, _, err := ValidateAndCombineConfig(discardLogs(),
+			withConfig(testutil.Undent(`
+				period: 1h
+				cluster_name: test-cluster
+			`)),
+			withCmdLineFlags("--tsg-id", "test-tsg-123", "--client-id", "test-client-id", "--private-key-path", privKeyPath))
+		// Should select VenafiCloudKeypair mode instead when --ngts is not specified
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "venafi-cloud.upload_path")
 	})
 
 	t.Run("ngts: missing --client-id should error", func(t *testing.T) {

pkg/client/client_ngts.go

@@ -38,7 +38,6 @@ type NGTSClient struct {
 	baseURL       *url.URL
 	agentMetadata *api.AgentMetadata
 
-	tsgID         string
 	privateKey    crypto.PrivateKey
 	jwtSigningAlg jwt.SigningMethod
 	lock          sync.RWMutex
@@ -87,8 +86,8 @@ const (
 )
 
 // NewNGTSClient creates a new NGTS client that authenticates using keypair authentication
-// and uploads data to NGTS endpoints. The baseURL parameter can override the default
-// NGTS server URL for testing purposes.
+// and uploads data to NGTS endpoints. Exactly one of tsgID or baseURL must be provided:
+// tsgID derives the production NGTS URL; baseURL sets a custom URL for testing.
 func NewNGTSClient(agentMetadata *api.AgentMetadata, credentials *NGTSServiceAccountCredentials, baseURL string, tsgID string, rootCAs *x509.CertPool) (*NGTSClient, error) {
 	// Load ClientID from file if not provided directly
 	if err := credentials.LoadClientIDIfNeeded(); err != nil {
@@ -103,8 +102,11 @@ func NewNGTSClient(agentMetadata *api.AgentMetadata, credentials *NGTSServiceAcc
 	// https://pan.dev/scm/api/tenancy/delete-tenancy-v-1-tenant-service-groups-tsg-id/
 	// > Possible values: >= 10 characters and <= 10 characters, Value must match regular expression ^1[0-9]+$
 	// For now, leaving this check simple
-	if tsgID == "" {
-		return nil, fmt.Errorf("cannot create NGTSClient: tsgID cannot be empty")
+	switch {
+	case tsgID != "" && baseURL != "":
+		return nil, fmt.Errorf("cannot create NGTSClient: tsgID and baseURL are mutually exclusive; exactly one must be provided")
+	case tsgID == "" && baseURL == "":
+		return nil, fmt.Errorf("cannot create NGTSClient: either tsgID or baseURL must be provided")
 	}
 
 	privateKey, jwtSigningAlg, err := parsePrivateKeyAndExtractSigningMethod(credentials.PrivateKeyFile)
@@ -113,8 +115,6 @@ func NewNGTSClient(agentMetadata *api.AgentMetadata, credentials *NGTSServiceAcc
 	}
 
 	actualBaseURL := baseURL
-
-	// Create prod NGTS URL if no explicit URL provided
 	if actualBaseURL == "" {
 		actualBaseURL = fmt.Sprintf(ngtsProdURLFormat, tsgID)
 	}
@@ -145,7 +145,6 @@ func NewNGTSClient(agentMetadata *api.AgentMetadata, credentials *NGTSServiceAcc
 		agentMetadata: agentMetadata,
 		credentials:   credentials,
 		baseURL:       parsedBaseURL,
-		tsgID:         tsgID,
 		accessToken:   &ngtsAccessToken{},
 		Client: &http.Client{
 			Timeout:   time.Minute,