Skip to content

feat: use gaf to get sast settings #319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

feat: use gaf to get sast settings #319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f9dea3d
feat: use gaf to get sast settings
acke Mar 25, 2025
6b8a56a
fix: cyclic dependency on SastResponse
acke Mar 26, 2025
bfc24c0
fix: reorder imports
acke Mar 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions internal/api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package api
import (
"encoding/json"
"fmt"
"github.com/snyk/go-application-framework/pkg/common"
"io"
"net/http"
"net/url"
Expand All @@ -21,7 +22,7 @@ type ApiClient interface {
GetFeatureFlag(flagname string, origId string) (bool, error)
GetUserMe() (string, error)
GetSelf() (contract.SelfResponse, error)
GetSastSettings(orgId string) (contract.SastResponse, error)
GetSastSettings(orgId string) (common.SastResponse, error)
}

var _ ApiClient = (*snykApiClient)(nil)
Expand Down Expand Up @@ -196,9 +197,9 @@ func (a *snykApiClient) GetSelf() (contract.SelfResponse, error) {
return selfData, nil
}

func (a *snykApiClient) GetSastSettings(orgId string) (contract.SastResponse, error) {
var response contract.SastResponse
var defaultResult contract.SastResponse
func (a *snykApiClient) GetSastSettings(orgId string) (common.SastResponse, error) {
var response common.SastResponse
var defaultResult common.SastResponse
Comment on lines +201 to +202
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PeterSchafer fyi we need SastResponse struct to be outside internal pkg to use it in ls.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understand! Can we find a more descriptive name than common?


endpoint := a.url + "/v1/cli-config/settings/sast?org=" + url.QueryEscape(orgId)
res, err := a.client.Get(endpoint)
Expand Down
6 changes: 4 additions & 2 deletions internal/mocks/api.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 23 additions & 0 deletions pkg/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,28 @@ func defaultFuncOrganizationSlug(engine workflow.Engine, config configuration.Co
return callback
}

func defaultFuncGetSastSettings(engine workflow.Engine, config configuration.Configuration, logger *zerolog.Logger, apiClientFactory func(url string, client *http.Client) api.ApiClient) configuration.DefaultValueFunction {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: Please don't add domain logic into app, which is more or less domain independent. There is already a function registered in the code_workflow, please let's use this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we want:

  1. Rewrite the engine.GetConfiguration() to have the sast settings instead of just a ConfigurationSastEnabled configuration.
  2. Setup the callback in code_workflow that will query the API for sast settings whenever asked for.
  3. NOT setup a callback for sast settings in app.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, what I wanted to say is, to keep product and domain specific logic close to each other. This means in this case that the default function for SAST specific configuration values should be registered here and implemented somewhere there as well.

One benefit is, that these values only get available if an application registers the code workflow and are not wide spread through the code base.

callback := func(existingValue interface{}) (interface{}, error) {
if existingValue != nil {
return existingValue, nil
}
client := engine.GetNetworkAccess().GetHttpClient()
url := config.GetString(configuration.API_URL)
apiClient := apiClientFactory(url, client)
orgId := config.GetString(configuration.ORGANIZATION)
if len(orgId) == 0 {
return existingValue, nil
}
response, err := apiClient.GetSastSettings(orgId)
Comment on lines +52 to +59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we have caching here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this and it might not make sense to have caching for this in GAF and leave it up to the consumer to cache if necessary.

if err != nil {
logger.Err(err).Msg("Failed to access settings.")
return false, err
}
return response, nil
}
return callback
}

func defaultFuncOrganization(engine workflow.Engine, config configuration.Configuration, logger *zerolog.Logger, apiClientFactory func(url string, client *http.Client) api.ApiClient) configuration.DefaultValueFunction {
callback := func(existingValue interface{}) (interface{}, error) {
client := engine.GetNetworkAccess().GetHttpClient()
Expand Down Expand Up @@ -213,6 +235,7 @@ func initConfiguration(engine workflow.Engine, config configuration.Configuratio

config.AddDefaultValue(configuration.ORGANIZATION, defaultFuncOrganization(engine, config, logger, apiClientFactory))
config.AddDefaultValue(configuration.ORGANIZATION_SLUG, defaultFuncOrganizationSlug(engine, config, logger, apiClientFactory))
config.AddDefaultValue(configuration.SAST_SETTINGS, defaultFuncGetSastSettings(engine, config, logger, apiClientFactory))

config.AddDefaultValue(configuration.FF_OAUTH_AUTH_FLOW_ENABLED, func(existingValue any) (any, error) {
if existingValue == nil {
Expand Down
2 changes: 1 addition & 1 deletion internal/api/contract/SastSettings.go → pkg/common/sast_settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package contract
package common

type LocalCodeEngine struct {
AllowCloudUpload bool `json:"allowCloudUpload"`
Expand Down
1 change: 1 addition & 0 deletions pkg/configuration/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const (
WORKING_DIRECTORY string = "internal_working_dir"
IS_FEDRAMP string = "internal_is_fedramp"
ORGANIZATION_SLUG string = "internal_org_slug"
SAST_SETTINGS string = "internal_sast_settings"
AUTHENTICATION_SUBDOMAINS string = "internal_auth_subdomain" // array of additional subdomains to add authentication for
AUTHENTICATION_ADDITIONAL_URLS string = "internal_additional_auth_urls" // array of additional urls to add authentication for
ADD_TRUSTED_CA_FILE string = "internal_additional_trusted_ca_file" // pem file location containing additional CAs to trust
Expand Down
1 change: 1 addition & 0 deletions pkg/configuration/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package configuration
import (
"context"
"encoding/json"

"os"
"path/filepath"
"sync"
Expand Down
16 changes: 4 additions & 12 deletions pkg/local_workflows/code_workflow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import (
"github.com/spf13/pflag"

"github.com/snyk/go-application-framework/internal/api"
"github.com/snyk/go-application-framework/internal/api/contract"
"github.com/snyk/go-application-framework/internal/utils"
"github.com/snyk/go-application-framework/pkg/common"
"github.com/snyk/go-application-framework/pkg/configuration"
"github.com/snyk/go-application-framework/pkg/local_workflows/code_workflow"
"github.com/snyk/go-application-framework/pkg/local_workflows/config_utils"
Expand Down Expand Up @@ -45,18 +45,9 @@ func GetCodeFlagSet() *pflag.FlagSet {
// WORKFLOWID_CODE defines a new workflow identifier
var WORKFLOWID_CODE workflow.Identifier = workflow.NewWorkflowIdentifier(codeWorkflowName)

func getSastSettings(engine workflow.Engine) (*contract.SastResponse, error) {
func getSastSettings(engine workflow.Engine) (*common.SastResponse, error) {
config := engine.GetConfiguration()
Comment on lines +48 to 49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this func is still needed.
Shouldn't the code_workflow get the sastSettings from the config?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://github.com/snyk/go-application-framework/pull/319/files#r2013897675

org := config.GetString(configuration.ORGANIZATION)
key := fmt.Sprintf("CACHE_SAST_RESPONSE_%s", org)

cachedContent := config.Get(key)
if cachedContent != nil {
cachedResponse, ok := cachedContent.(*contract.SastResponse)
if ok {
return cachedResponse, nil
}
}

client := engine.GetNetworkAccess().GetHttpClient()
url := config.GetString(configuration.API_URL)
Expand All @@ -67,7 +58,7 @@ func getSastSettings(engine workflow.Engine) (*contract.SastResponse, error) {
return &tmp, err
}

engine.GetConfiguration().Set(key, &tmp)
engine.GetConfiguration().Set(org, &tmp)
return &tmp, nil
}

Expand Down Expand Up @@ -151,6 +142,7 @@ func codeWorkflowEntryPoint(invocationCtx workflow.InvocationContext, _ []workfl
logger := invocationCtx.GetEnhancedLogger()

sastEnabledI, err := config.GetWithError(code_workflow.ConfigurationSastEnabled)

if err != nil {
return result, err
}
Expand Down
7 changes: 4 additions & 3 deletions pkg/local_workflows/code_workflow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package localworkflows
import (
"encoding/json"
"fmt"
"github.com/snyk/go-application-framework/pkg/common"
"math/rand"
"net/http"
"net/http/httptest"
Expand Down Expand Up @@ -37,9 +38,9 @@ func Test_Code_entrypoint(t *testing.T) {
fmt.Println(r.URL)
if strings.HasSuffix(r.URL.String(), "/v1/cli-config/settings/sast?org="+org) {
sastSettingsCalled++
sastSettings := &contract.SastResponse{
sastSettings := &common.SastResponse{
SastEnabled: true,
LocalCodeEngine: contract.LocalCodeEngine{
LocalCodeEngine: common.LocalCodeEngine{
Enabled: true, /* ensures that legacycli will be called */
},
}
Expand Down Expand Up @@ -98,7 +99,7 @@ func Test_Code_entrypoint(t *testing.T) {
assert.NoError(t, err)
assert.NotNil(t, rs)
assert.Equal(t, expectedData, rs[0].GetPayload().(string))
assert.Equal(t, 1, sastSettingsCalled)
assert.Equal(t, 2, sastSettingsCalled)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor Author

@acke acke Mar 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because now its also called when then default func is setup here: initConfiguration to initiate configuration.SAST_SETTINGS

config.AddDefaultValue(configuration.SAST_SETTINGS, defaultFuncGetSastSettings(engine, config, logger, apiClientFactory))

And it is also called in InitCodeWorkflow to set config for code_workflow.ConfigurationSastEnabled.

engine.GetConfiguration().AddDefaultValue(code_workflow.ConfigurationSastEnabled, getSastEnabled(engine))

Before adding the default func, we only did the second config call in the test case.

}

func Test_Code_legacyImplementation_happyPath(t *testing.T) {
Expand Down