Skip to content

Fix panic exit when containers ff could not be fetched (AST-86767) #1069

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

Merged
merged 13 commits into from
Mar 23, 2025
Merged

Fix panic exit when containers ff could not be fetched (AST-86767) #1069

Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b4c4363
Fix panic exit when containers ff could not be fetched (AST-86767)
greensd4 Mar 19, 2025
e44ddf5
Fix code
greensd4 Mar 19, 2025
f456710
Merge branch 'main' into bug/fix-ff-panic
elchananarb Mar 20, 2025
6f4ab3d
refactor new method
greensd4 Mar 20, 2025
c0e882a
Merge remote-tracking branch 'origin/bug/fix-ff-panic' into bug/fix-f…
greensd4 Mar 20, 2025
d7e442a
Merge branch 'main' into bug/fix-ff-panic
elchananarb Mar 20, 2025
8e04288
Add tests
greensd4 Mar 20, 2025
6bbf975
Merge remote-tracking branch 'origin/bug/fix-ff-panic' into bug/fix-f…
greensd4 Mar 20, 2025
e597658
Merge branch 'main' into bug/fix-ff-panic
greensd4 Mar 23, 2025
8825f28
Merge branch 'main' into bug/fix-ff-panic
greensd4 Mar 23, 2025
c86df91
Fix tests
greensd4 Mar 23, 2025
a17a6c6
Merge remote-tracking branch 'origin/bug/fix-ff-panic' into bug/fix-f…
greensd4 Mar 23, 2025
8535a17
Fix tests
greensd4 Mar 23, 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
19 changes: 16 additions & 3 deletions internal/commands/scan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1123,7 +1123,7 @@ func validateScanTypes(cmd *cobra.Command, jwtWrapper wrappers.JWTWrapper, featu
var scanTypes []string
var SCSScanTypes []string

containerEngineCLIEnabled, _ := featureFlagsWrapper.GetSpecificFlag(wrappers.ContainerEngineCLIEnabled)
runContainerEngineCLI := isContainersEngineEnabled(featureFlagsWrapper)
allowedEngines, err := jwtWrapper.GetAllowedEngines(featureFlagsWrapper)
if err != nil {
err = errors.Errorf("Error validating scan types: %v", err)
Expand All @@ -1140,7 +1140,7 @@ func validateScanTypes(cmd *cobra.Command, jwtWrapper wrappers.JWTWrapper, featu

scanTypes = strings.Split(userScanTypes, ",")
for _, scanType := range scanTypes {
if !allowedEngines[scanType] || (scanType == commonParams.ContainersType && !(containerEngineCLIEnabled.Status)) {
if !allowedEngines[scanType] || (scanType == commonParams.ContainersType && !(runContainerEngineCLI)) {
keys := reflect.ValueOf(allowedEngines).MapKeys()
err = errors.Errorf(engineNotAllowed, scanType, scanType, keys)
return err
Expand All @@ -1156,7 +1156,7 @@ func validateScanTypes(cmd *cobra.Command, jwtWrapper wrappers.JWTWrapper, featu

} else {
for k := range allowedEngines {
if k == commonParams.ContainersType && !(containerEngineCLIEnabled.Status) {
if k == commonParams.ContainersType && !(runContainerEngineCLI) {
continue
}
scanTypes = append(scanTypes, k)
Expand All @@ -1169,6 +1169,19 @@ func validateScanTypes(cmd *cobra.Command, jwtWrapper wrappers.JWTWrapper, featu
return nil
}

func isContainersEngineEnabled(featureFlagsWrapper wrappers.FeatureFlagsWrapper) bool {
runContainerEngineCLI := true
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this value necessary if it will be overwritten later anyway, whether there's an error or not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i need to define the var outside the if/else scope. but you are right we can initialize it as nil.


containerEngineCLIEnabled, err := featureFlagsWrapper.GetSpecificFlag(wrappers.ContainerEngineCLIEnabled)
if err != nil {
logger.PrintfIfVerbose("could not get CONTAINER_ENGINE_CLI_ENABLED FF. defaulting to `false` error: %s", err)
runContainerEngineCLI = false
Copy link
Contributor

Choose a reason for hiding this comment

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

If retrieving the Feature Flag fails (e.g., due to a network issue), the function defaults to false, which could mistakenly disable container scanning.

Should the default be true or false? Defining it clearly would avoid confusion.

or maybe, return an error and let the user decide...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if we can not determine a FF i would like it to default to false. we also add a log here so the user can understand what happen. do u think it should be defaulted to true?

Copy link
Contributor

Choose a reason for hiding this comment

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

Defaulting to false with clear logging makes the most sense. When we can't get the feature flag, we should turn scanning off and tell the user what happened. This way they know scanning is off and why.

We should check if false is really the safer default,
but good logs to user is good idea

} else {
runContainerEngineCLI = containerEngineCLIEnabled.Status
}
return runContainerEngineCLI
}

func scanTypeEnabled(scanType string) bool {
scanTypes := strings.Split(actualScanTypes, ",")
for _, a := range scanTypes {
Expand Down
Loading