diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 2725aa99..f4952b96 100644 --- a/.github/workflows/tests.yaml +++ b/.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') diff --git a/deploy/charts/discovery-agent/README.md b/deploy/charts/discovery-agent/README.md index 0a8ab342..f182efdf 100644 --- a/deploy/charts/discovery-agent/README.md +++ b/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` diff --git a/deploy/charts/discovery-agent/templates/deployment.yaml b/deploy/charts/discovery-agent/templates/deployment.yaml index dccb2390..fdde00f4 100644 --- a/deploy/charts/discovery-agent/templates/deployment.yaml +++ b/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 diff --git a/deploy/charts/discovery-agent/tests/deployment_test.yaml b/deploy/charts/discovery-agent/tests/deployment_test.yaml index d506f9fa..35f47ec5 100644 --- a/deploy/charts/discovery-agent/tests/deployment_test.yaml +++ b/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: diff --git a/deploy/charts/discovery-agent/values.schema.json b/deploy/charts/discovery-agent/values.schema.json index 6affa690..f40d6e24 100644 --- a/deploy/charts/discovery-agent/values.schema.json +++ b/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": [], diff --git a/deploy/charts/discovery-agent/values.yaml b/deploy/charts/discovery-agent/values.yaml index 885d3db1..b013c959 100644 --- a/deploy/charts/discovery-agent/values.yaml +++ b/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: "" diff --git a/hack/ngts/test-e2e.sh b/hack/ngts/test-e2e.sh index 3bfae452..06aa339a 100755 --- a/hack/ngts/test-e2e.sh +++ b/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}" diff --git a/pkg/agent/config.go b/pkg/agent/config.go index 487f18ad..45dae0b0 100644 --- a/pkg/agent/config.go +++ b/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")) diff --git a/pkg/agent/config_test.go b/pkg/agent/config_test.go index c7137ca6..364661c6 100644 --- a/pkg/agent/config_test.go +++ b/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,14 +1124,15 @@ 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(), @@ -1139,13 +1140,12 @@ func Test_ValidateAndCombineConfig_NGTS(t *testing.T) { 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) { diff --git a/pkg/client/client_ngts.go b/pkg/client/client_ngts.go index d4e27785..69cca123 100644 --- a/pkg/client/client_ngts.go +++ b/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, diff --git a/pkg/client/client_ngts_test.go b/pkg/client/client_ngts_test.go index b1718426..5f869106 100644 --- a/pkg/client/client_ngts_test.go +++ b/pkg/client/client_ngts_test.go @@ -51,46 +51,57 @@ func TestNewNGTSClient(t *testing.T) { errContains string }{ { - name: "valid credentials and tsg id", + name: "valid credentials with baseURL only", credentials: &NGTSServiceAccountCredentials{ ClientID: "test-client-id", PrivateKeyFile: keyFile, }, baseURL: "https://test.ngts.example.com", - tsgID: "test-tsg-id", + tsgID: "", wantErr: false, }, { - name: "missing tsg id", + name: "valid credentials with tsgID only", credentials: &NGTSServiceAccountCredentials{ ClientID: "test-client-id", PrivateKeyFile: keyFile, }, - baseURL: "https://test.ngts.example.com", - tsgID: "", - wantErr: true, - errContains: "tsgID cannot be empty", + baseURL: "", + tsgID: "test-tsg-id", + wantErr: false, }, { - name: "missing clientID without file", + name: "tsgID and baseURL are mutually exclusive", credentials: &NGTSServiceAccountCredentials{ - ClientID: "", + ClientID: "test-client-id", PrivateKeyFile: keyFile, }, baseURL: "https://test.ngts.example.com", tsgID: "test-tsg-id", wantErr: true, - errContains: "client_id cannot be empty", + errContains: "tsgID and baseURL are mutually exclusive", }, { - name: "default URL when empty", + name: "missing both tsgID and baseURL", credentials: &NGTSServiceAccountCredentials{ ClientID: "test-client-id", PrivateKeyFile: keyFile, }, - baseURL: "", - tsgID: "test-tsg-id", - wantErr: false, + baseURL: "", + tsgID: "", + wantErr: true, + errContains: "either tsgID or baseURL must be provided", + }, + { + name: "missing clientID without file", + credentials: &NGTSServiceAccountCredentials{ + ClientID: "", + PrivateKeyFile: keyFile, + }, + baseURL: "https://test.ngts.example.com", + tsgID: "", + wantErr: true, + errContains: "client_id cannot be empty", }, } @@ -114,7 +125,6 @@ func TestNewNGTSClient(t *testing.T) { require.NoError(t, err) assert.NotNil(t, client) - assert.Equal(t, tt.tsgID, client.tsgID) if tt.baseURL != "" { assert.Equal(t, tt.baseURL, client.baseURL.String()) return @@ -172,7 +182,7 @@ func TestNGTSClient_LoadClientIDFromFile(t *testing.T) { ClusterID: "test-cluster", } - client, err := NewNGTSClient(metadata, tt.credentials, "https://test.example.com", "test-tsg", nil) + client, err := NewNGTSClient(metadata, tt.credentials, "https://test.example.com", "", nil) if tt.wantErr { require.Error(t, err) @@ -255,7 +265,7 @@ func TestNGTSClient_LoadClientIDFromFileAlternativeNames(t *testing.T) { ClusterID: "test-cluster", } - client, err := NewNGTSClient(metadata, credentials, "https://test.example.com", "test-tsg", nil) + client, err := NewNGTSClient(metadata, credentials, "https://test.example.com", "", nil) if tt.wantErr { require.Error(t, err) @@ -312,8 +322,7 @@ func TestNGTSClient_PostDataReadingsWithOptions(t *testing.T) { ClusterID: "test-cluster", } - tsgID := "test-tsg-123" - client, err := NewNGTSClient(metadata, credentials, server.URL, tsgID, nil) + client, err := NewNGTSClient(metadata, credentials, server.URL, "", nil) require.NoError(t, err) // Test data upload @@ -388,7 +397,7 @@ func TestNGTSClient_AuthenticationFlow(t *testing.T) { ClusterID: "test-cluster", } - client, err := NewNGTSClient(metadata, credentials, server.URL, "test-tsg", nil) + client, err := NewNGTSClient(metadata, credentials, server.URL, "", nil) require.NoError(t, err) // Make multiple requests - should only authenticate once @@ -454,7 +463,7 @@ func TestNGTSClient_ErrorHandling(t *testing.T) { } metadata := &api.AgentMetadata{Version: "test", ClusterID: "test"} - client, err := NewNGTSClient(metadata, credentials, server.URL, "test-tsg", nil) + client, err := NewNGTSClient(metadata, credentials, server.URL, "", nil) require.NoError(t, err) readings := []*api.DataReading{{DataGatherer: "test", Data: &api.DynamicData{}}}