fix: report 404 secrets correctly in Gitlab provider (#5104)

* fixed tests (resigning commit)

Signed-off-by: Alexander Chernov <alexander@chernov.it>

* fix: improve error handling for GitLab variable retrieval

Signed-off-by: Alexander Chernov <alexander@chernov.it>

---------

Signed-off-by: Alexander Chernov <alexander@chernov.it>
Co-authored-by: Gergely Brautigam <skarlso777@gmail.com>

docs/snippets/gitlab-secret-store.yaml

@@ -13,6 +13,6 @@ spec:
             name: gitlab-secret
             key: token
       projectID: "**project ID goes here**"
-      groupIDs: "**groupID(s) go here**"
+      groupIDs: ["**groupID(s) go here**"]
       inheritFromGroups: "**automatically looks for variables in parent groups**"
       environment: "**environment scope goes here**"

providers/v1/gitlab/fake/fake.go

@@ -17,6 +17,7 @@ limitations under the License.
 package fake
 
 import (
+	"errors"
 	"net/http"
 
 	gitlab "gitlab.com/gitlab-org/api/client-go"
@@ -88,7 +89,7 @@ func mockGetVariable[V GitVariable](keyExtractor extractKey[V], responses []APIR
 	return func(pid any, key string, options ...gitlab.RequestOptionFunc) (*V, *gitlab.Response, error) {
 		getCount++
 		if getCount > len(responses)-1 {
-			return nil, make404APIResponse(), nil
+			return nil, make404APIResponse(), errors.New("404 Not Found")
 		}
 		var match *V
 		for _, v := range responses[getCount].Output {
@@ -96,9 +97,6 @@ func mockGetVariable[V GitVariable](keyExtractor extractKey[V], responses []APIR
 				match = v
 			}
 		}
-		if match == nil {
-			return nil, make404APIResponse(), nil
-		}
 		return match, responses[getCount].Response, responses[getCount].Error
 	}
 }

providers/v1/gitlab/gitlab.go

@@ -312,57 +312,55 @@ func (g *gitlabBase) GetSecret(_ context.Context, ref esv1.ExternalSecretDataRem
 		vopts = &gitlab.GetProjectVariableOptions{Filter: &gitlab.VariableFilter{EnvironmentScope: g.store.Environment}}
 	}
 
-	// _Note_: getVariables potentially alters vopts environment variable.
-	data, resp, err := g.getVariables(ref, vopts)
-	if err != nil && (resp == nil || resp.StatusCode != http.StatusNotFound) {
-		return nil, err
+	data, err := g.getVariables(ref, vopts)
+	if err == nil {
+		return extractVariable(ref, data.Value)
 	}
 
-	err = g.ResolveGroupIDs()
-	if err != nil {
-		return nil, err
+	// If project variable not found, try group variables
+	if errors.Is(err, gitlab.ErrNotFound) {
+		return g.tryGroupVariables(ref, gopts, err)
 	}
 
-	var result []byte
-	if resp.StatusCode < 300 {
-		result, err = extractVariable(ref, data.Value)
+	return nil, err
+}
+
+// tryGroupVariables attempts to retrieve the secret from group variables when project lookup fails.
+func (g *gitlabBase) tryGroupVariables(ref esv1.ExternalSecretDataRemoteRef, gopts *gitlab.GetGroupVariableOptions, originalErr error) ([]byte, error) {
+	// Load groupIds from the `InheritFromGroups` property
+	if err := g.ResolveGroupIDs(); err != nil {
+		return nil, err
 	}
 
 	for i := len(g.store.GroupIDs) - 1; i >= 0; i-- {
 		groupID := g.store.GroupIDs[i]
-		if result != nil {
-			return result, nil
+		groupVar, _, err := g.getGroupVariables(groupID, ref, gopts)
+		if err == nil {
+			return extractVariable(ref, groupVar.Value)
 		}
 
-		groupVar, resp, err := g.getGroupVariables(groupID, ref, gopts)
-		if err != nil {
-			return nil, err
-		}
-		if resp != nil && resp.StatusCode < 300 {
-			result, _ = extractVariable(ref, groupVar.Value)
+		// If a 404 error, continue to the next stage, otherwise exit early with error
+		if errors.Is(err, gitlab.ErrNotFound) {
+			continue
 		}
+		return nil, err
 	}
 
-	if result != nil {
-		return result, nil
-	}
-	return nil, err
+	// No group variables found, return the original project error
+	return nil, originalErr
 }
 
 func extractVariable(ref esv1.ExternalSecretDataRemoteRef, value string) ([]byte, error) {
+	// If no property specified, return the raw value
 	if ref.Property == "" {
-		if value != "" {
-			return []byte(value), nil
+		if value == "" {
+			return nil, fmt.Errorf("invalid secret received. no secret string for key: %s", ref.Key)
 		}
-		return nil, fmt.Errorf("invalid secret received. no secret string for key: %s", ref.Key)
-	}
-
-	var payload string
-	if value != "" {
-		payload = value
+		return []byte(value), nil
 	}
 
-	val := gjson.Get(payload, ref.Property)
+	// Extract property from JSON value
+	val := gjson.Get(value, ref.Property)
 	if !val.Exists() {
 		return nil, fmt.Errorf("key %s does not exist in secret %s", ref.Property, ref.Key)
 	}

providers/v1/gitlab/gitlab_test.go

@@ -85,12 +85,12 @@ type secretManagerTestCase struct {
 
 func makeValidSecretManagerTestCase() *secretManagerTestCase {
 	smtc := secretManagerTestCase{
-		mockProjectsClient:       &fakegitlab.GitlabMockProjectsClient{},
-		mockProjectVarClient:     &fakegitlab.GitlabMockProjectVariablesClient{},
-		mockGroupVarClient:       &fakegitlab.GitlabMockGroupVariablesClient{},
-		apiInputProjectID:        makeValidAPIInputProjectID(),
-		apiInputKey:              makeValidAPIInputKey(),
-		apiInputEnv:              makeValidEnvironment(),
+		mockProjectsClient:   &fakegitlab.GitlabMockProjectsClient{},
+		mockProjectVarClient: &fakegitlab.GitlabMockProjectVariablesClient{},
+		mockGroupVarClient:   &fakegitlab.GitlabMockGroupVariablesClient{},
+		apiInputProjectID:    makeValidAPIInputProjectID(),
+		apiInputKey:          makeValidAPIInputKey(),
+		//apiInputEnv:              makeValidEnvironment(),
 		ref:                      makeValidRef(),
 		refFind:                  makeValidFindRef(),
 		projectID:                makeValidProjectID(),
@@ -145,10 +145,6 @@ func makeValidAPIInputKey() string {
 	return testKey
 }
 
-func makeValidEnvironment() string {
-	return environment
-}
-
 func makeValidProjectAPIResponse() *gitlab.Response {
 	return &gitlab.Response{
 		Response: &http.Response{
@@ -158,6 +154,15 @@ func makeValidProjectAPIResponse() *gitlab.Response {
 		TotalPages:  1,
 	}
 }
+func make404GitlabAPIResponse() *gitlab.Response {
+	return &gitlab.Response{
+		Response: &http.Response{
+			StatusCode: http.StatusNotFound,
+		},
+		CurrentPage: 1,
+		TotalPages:  1,
+	}
+}
 
 func makeValidProjectGroupsAPIResponse() *gitlab.Response {
 	return &gitlab.Response{
@@ -203,6 +208,21 @@ func makeValidProjectGroupsAPIOutput() []*gitlab.ProjectGroup {
 	}}
 }
 
+func make404ProjectAPIOutput(numResponses ...int) []*fakegitlab.APIResponse[[]*gitlab.ProjectVariable] {
+	count := 1
+	if len(numResponses) > 0 && numResponses[0] > 0 {
+		count = numResponses[0]
+	}
+	responses := make([]*fakegitlab.APIResponse[[]*gitlab.ProjectVariable], count)
+	for i := 0; i < count; i++ {
+		responses[i] = &fakegitlab.APIResponse[[]*gitlab.ProjectVariable]{
+			Response: make404GitlabAPIResponse(),
+			Error:    gitlab.ErrNotFound,
+		}
+	}
+	return responses
+}
+
 func makeValidGroupAPIOutput() *gitlab.GroupVariable {
 	return &gitlab.GroupVariable{
 		Key:              "groupKey",
@@ -407,22 +427,22 @@ func TestGetSecret(t *testing.T) {
 		smtc.groupAPIOutput = nil
 		smtc.expectedSecret = smtc.projectAPIOutput.Value
 	}
-	onlyWildcardSecret := func(smtc *secretManagerTestCase) {
-		smtc.projectAPIOutput.Value = ""
-		smtc.projectAPIResponse.Response.StatusCode = 404
-		smtc.groupAPIResponse = nil
-		smtc.groupAPIOutput = nil
-		smtc.expectedSecret = smtc.projectAPIOutput.Value
-	}
+
 	groupSecretProjectOverride := func(smtc *secretManagerTestCase) {
+		// given a secre t in the project and a group with the same key
+		// the project secret should override the group secret
 		smtc.projectAPIOutput.Value = projectvalue
 		smtc.groupAPIOutput.Key = testKey
 		smtc.groupAPIOutput.Value = groupvalue
 		smtc.expectedSecret = smtc.projectAPIOutput.Value
 	}
+
 	groupWithoutProjectOverride := func(smtc *secretManagerTestCase) {
+		// given a group secret without a project secret
+		// the group secret should be returned
 		smtc.groupIDs = []string{groupid}
-		smtc.projectAPIResponse.Response.StatusCode = 404
+		smtc.projectAPIOutput = nil
+		smtc.projectAPIOutputs = make404ProjectAPIOutput()
 		smtc.groupAPIOutput.Key = testKey
 		smtc.groupAPIOutput.Value = groupvalue
 		smtc.expectedSecret = smtc.groupAPIOutput.Value
@@ -430,7 +450,6 @@ func TestGetSecret(t *testing.T) {
 
 	successCases := []*secretManagerTestCase{
 		makeValidSecretManagerTestCaseCustom(onlyProjectSecret),
-		makeValidSecretManagerTestCaseCustom(onlyWildcardSecret),
 		makeValidSecretManagerTestCaseCustom(groupSecretProjectOverride),
 		makeValidSecretManagerTestCaseCustom(groupWithoutProjectOverride),
 		makeValidSecretManagerTestCaseCustom(setGroupWildcardVariable),
@@ -512,6 +531,7 @@ func TestGetAllSecrets(t *testing.T) {
 		smtc.refFind.Name = makeFindName("foo.*")
 	}
 	setUnmatchedEnvironmentFindString := func(smtc *secretManagerTestCase) {
+		smtc.apiInputEnv = environment
 		smtc.projectAPIOutput = &gitlab.ProjectVariable{
 			Key:              testKey,
 			Value:            projectvalue,
@@ -531,6 +551,7 @@ func TestGetAllSecrets(t *testing.T) {
 		smtc.refFind.Tags = map[string]string{"environment_scope": environment}
 	}
 	setEnvironmentConstrainedByStore := func(smtc *secretManagerTestCase) {
+		smtc.apiInputEnv = environment
 		smtc.expectedSecret = projectvalue
 		smtc.expectError = "'find.tags' is constrained by 'environment_scope' of the store"
 		smtc.refFind.Tags = map[string]string{"environment_scope": environment}
@@ -688,6 +709,8 @@ func TestGetAllSecrets(t *testing.T) {
 	}
 
 	cases := []*secretManagerTestCase{
+		makeValidSecretManagerGetAllTestCaseCustom(setUnmatchedEnvironmentFindString),
+		///
 		makeValidSecretManagerGetAllTestCaseCustom(setMissingFindRegex),
 		makeValidSecretManagerGetAllTestCaseCustom(setUnsupportedFindPath),
 		makeValidSecretManagerGetAllTestCaseCustom(setUnsupportedFindTag),
@@ -959,8 +982,12 @@ func ErrorContains(out error, want string) bool {
 type storeModifier func(*esv1.SecretStore) *esv1.SecretStore
 
 func setGroupWildcardVariable(smtc *secretManagerTestCase) {
+	// Given a group variable with a wildcard environment scope
+	// and a provider with no environment,
+	// the group variable should be returned
 	smtc.groupIDs = []string{groupid}
-	smtc.projectAPIResponse.Response.StatusCode = 404
+	smtc.projectAPIOutput = nil
+	smtc.projectAPIOutputs = make404ProjectAPIOutput(2)
 	smtc.groupAPIOutput.Key = testKey
 	smtc.groupAPIOutput.Value = groupvalue
 	smtc.groupAPIOutput.EnvironmentScope = "*"
@@ -968,8 +995,13 @@ func setGroupWildcardVariable(smtc *secretManagerTestCase) {
 }
 
 func setGroupWildcardVariableWithEnvironment(smtc *secretManagerTestCase) {
+	// Given a group variable with a wildcard environment scope
+	// and a provider with a specific environment,
+	// the group variable should be returned
+	// todo: verify that the flow for wildcard is observed. Have some doubts
 	smtc.groupIDs = []string{groupid}
-	smtc.projectAPIResponse.Response.StatusCode = 404
+	smtc.projectAPIOutput = nil
+	smtc.projectAPIOutputs = make404ProjectAPIOutput(2)
 	smtc.groupAPIOutput.Key = testKey
 	smtc.groupAPIOutput.Value = groupvalue
 	smtc.groupAPIOutput.EnvironmentScope = "*"
@@ -978,19 +1010,23 @@ func setGroupWildcardVariableWithEnvironment(smtc *secretManagerTestCase) {
 }
 
 func setGroupWildcardVariableNotFoundThenFound(smtc *secretManagerTestCase) {
+	// Given a group variable with a wildcard environment scope
+	// and a provider with a specific environment,
+	// the group variable should be returned
 	smtc.groupIDs = []string{groupid}
-	smtc.projectAPIResponse.Response.StatusCode = 404
 
-	// For this test, we'll use a simpler approach - just return the wildcard variable
-	smtc.groupAPIOutput = &gitlab.GroupVariable{
-		Key:              testKey,
-		Value:            groupvalue,
-		EnvironmentScope: "*",
-	}
-	smtc.groupAPIResponse = &gitlab.Response{
-		Response: &http.Response{
-			StatusCode: http.StatusOK,
-		},
+	smtc.projectAPIOutput = nil
+	smtc.projectAPIOutputs = make404ProjectAPIOutput(2)
+	smtc.groupAPIOutput = nil
+	smtc.groupAPIOutputs = []*fakegitlab.APIResponse[[]*gitlab.GroupVariable]{
+		{Response: make404GitlabAPIResponse(), Error: gitlab.ErrNotFound},
+		{Output: []*gitlab.GroupVariable{
+			{
+				Key:              testKey,
+				Value:            groupvalue,
+				EnvironmentScope: "*",
+			},
+		}, Response: makeValidGroupAPIResponse(), Error: nil},
 	}
 
 	smtc.apiInputEnv = environment

providers/v1/gitlab/provider.go

@@ -133,23 +133,31 @@ func (g *gitlabBase) getClient(ctx context.Context, provider *esv1.GitlabProvide
 	return client, nil
 }
 
-func (g *gitlabBase) getVariables(ref esv1.ExternalSecretDataRemoteRef, vopts *gitlab.GetProjectVariableOptions) (*gitlab.ProjectVariable, *gitlab.Response, error) {
-	data, resp, err := g.projectVariablesClient.GetVariable(g.store.ProjectID, ref.Key, vopts)
+func (g *gitlabBase) getVariables(ref esv1.ExternalSecretDataRemoteRef, vopts *gitlab.GetProjectVariableOptions) (*gitlab.ProjectVariable, error) {
+	// First attempt to get the variable
+	data, _, err := g.projectVariablesClient.GetVariable(g.store.ProjectID, ref.Key, vopts)
 	metrics.ObserveAPICall(constants.ProviderGitLab, constants.CallGitLabProjectVariableGet, err)
+
+	// If successful, return immediately
+	if err == nil {
+		return data, nil
+	}
+
+	// If not a "not found" error or environment is already wildcard, return the error
+	if !errors.Is(err, gitlab.ErrNotFound) || isEmptyOrWildcard(g.store.Environment) {
+		return nil, err
+	}
+
+	// Retry with wildcard environment scope
+	opts := &gitlab.GetProjectVariableOptions{Filter: &gitlab.VariableFilter{EnvironmentScope: "*"}}
+	data, _, err = g.projectVariablesClient.GetVariable(g.store.ProjectID, ref.Key, opts)
+	metrics.ObserveAPICall(constants.ProviderGitLab, constants.CallGitLabProjectVariableGet, err)
+
 	if err != nil {
-		if resp != nil && resp.StatusCode == http.StatusNotFound && !isEmptyOrWildcard(g.store.Environment) {
-			vopts.Filter.EnvironmentScope = "*"
-			data, resp, err = g.projectVariablesClient.GetVariable(g.store.ProjectID, ref.Key, vopts)
-			metrics.ObserveAPICall(constants.ProviderGitLab, constants.CallGitLabProjectVariableGet, err)
-			if err != nil || resp == nil {
-				return nil, resp, fmt.Errorf("error getting variable %s from GitLab: %w", ref.Key, err)
-			}
-		} else {
-			return nil, resp, err
-		}
+		return nil, fmt.Errorf("error getting variable %s from GitLab (including wildcard retry): %w", ref.Key, err)
 	}
 
-	return data, resp, nil
+	return data, nil
 }
 
 // ValidateStore validates the GitLab store configuration.

providers/v1/gitlab/provider_test.go

@@ -101,7 +101,7 @@ func TestGetClientWithCABundle(t *testing.T) {
 
 	// Call getVariables to trigger a network request to the mock server.
 	// The request will only succeed if the custom CA is correctly configured.
-	_, _, err = gl.getVariables(esv1.ExternalSecretDataRemoteRef{Key: "test-secret"}, nil)
+	_, err = gl.getVariables(esv1.ExternalSecretDataRemoteRef{Key: "test-secret"}, nil)
 	assert.NoError(t, err, "getVariables should succeed with the correct CA")
 }
 
@@ -197,7 +197,7 @@ func TestGetClientWithCAProviderSecret(t *testing.T) {
 	gl.groupVariablesClient = client.GroupVariables
 
 	// Call getVariables to trigger a network request to the mock server.
-	_, _, err = gl.getVariables(esv1.ExternalSecretDataRemoteRef{Key: "test-secret"}, nil)
+	_, err = gl.getVariables(esv1.ExternalSecretDataRemoteRef{Key: "test-secret"}, nil)
 	assert.NoError(t, err, "getVariables should succeed with the correct CA from Secret")
 }
 
@@ -259,7 +259,7 @@ func TestGetClientWithCAProviderConfigMap(t *testing.T) {
 	gl.groupVariablesClient = client.GroupVariables
 
 	// Call getVariables to trigger a network request to the mock server.
-	_, _, err = gl.getVariables(esv1.ExternalSecretDataRemoteRef{Key: "test-secret"}, nil)
+	_, err = gl.getVariables(esv1.ExternalSecretDataRemoteRef{Key: "test-secret"}, nil)
 	assert.NoError(t, err, "getVariables should succeed with the correct CA from ConfigMap")
 }