Feature/multi pull secrets #83

steelion · 2022-05-06T21:03:16Z

adds the feature to use multiple pull-secrets from the k8s pods
additionally there is a possibility to specify a particular pull secret that is used in case all pull secrets from the pods are failing (the global custom pull secret must be in the same namespace as the sbom-operator itself)

… by one when necessary

steelion · 2022-05-06T21:10:11Z

@derkoe can you also please take a look at my implementation
it at least works in our OCP Clusters

ckotzbauer

Thanks for your PR @steelion, much appreciated.
This fits the needs for my proposed solution of #76 (comment).

The main topics about my comments:

The name of the parameter
The structure of the ContainerImage struct.
The iteration through the secrets: The struct always has to be "valid", so no nil and empty states should be there before the iteration through the secrets has started.
The reason for this: The "Job-Image"-Feature from the 0.10.0 release must not break. (see the job package and the job-images folder for details) There are two possibilities:
- The serialized job-config still only use the first secret
- The imageConfig struct has to be changed to hold multiple secrets, which also needs changes to the vcn and cas job-image entrypoint.sh files.

For now: We should be fine with the first option for the job-feature.

If you need help, please contact me, I can support you by integrating this PR 😉

README.md

internal/kubernetes/kubernetes.go

ckotzbauer · 2022-05-07T11:52:00Z

internal/kubernetes/kubernetes.go

+	sbomOperatorNamespace, _, err = kubeConfig.Namespace()
+	if err != nil {
+		logrus.WithError(err).Fatal("namespace could not be read!")
+	}


I'm not sure if the namespace of the in-cluster kubeconfig is always correct here.
Please use the POD_NAMESPACE environment variable here (as already used in the job-package).
With this the job package can be refactored a bit, as the namespace is than already known from the kubernetes package and the parameter from CreateJob and CreateJobSecret could be removed.

The POD_NAMESPACE env-variable is than mandatory for the manifests and th helm-chart.

sbom-operator is now using only the POD_NAMESPACE env variable for fetching the fallback secret
but i tried it to be no breaking change. in case POD_NAMESPACES is not set, the fallbacksecret will not be read, and a debug message tells the user every time that this env-var should be set too.
unfortunately i had no time yet to check where to get the own namespace reliably.

The log-message is good, but please use Debug instead of Info.

Just saw the Debug message below in the logic, please remove one of them and use Debug level for the other.

kept the debug message in LoadImageInfos function, because only at this point the variable is really needed

internal/kubernetes/kubernetes.go

internal/syft/syft.go

- refactor the ContainerImage struct. - refactor secrets iteration

steelion · 2022-05-09T13:13:18Z

I refactored the code a bit to not break the job-images and to make sure to meet your criteria (at least i hope so) 😉
learned a lot about go in the last few days 😆

…space_name env-variable

ckotzbauer

Hi @steelion,
sorry for the delay. I have another bunch of comments for you. The logic in general looks is good, but there are some circumstances which need further improvements.

README.md

ckotzbauer · 2022-05-11T15:57:30Z

internal/config.go

@@ -21,4 +21,5 @@ var (
 	ConfigKeyJobImage               = "job-image"
 	ConfigKeyJobImagePullSecret     = "job-image-pull-secret"
 	ConfigKeyJobTimeout             = "job-timeout"
+	ConfigKeyCustomGlobalPullSecret = "custom-global-pull-secret"


This has to be renamed too.

sure - missed that with the previous commit :(

ckotzbauer · 2022-05-11T15:58:40Z

internal/kubernetes/kubernetes.go

+	sbomOperatorNamespace, _, err = kubeConfig.Namespace()
+	if err != nil {
+		logrus.WithError(err).Fatal("namespace could not be read!")
+	}


The log-message is good, but please use Debug instead of Info.

ckotzbauer · 2022-05-11T16:00:24Z

internal/kubernetes/kubernetes.go

@@ -88,6 +100,7 @@ func (client *KubeClient) listPods(namespace, labelSelector string) ([]corev1.Po
 func (client *KubeClient) LoadImageInfos(namespaces []corev1.Namespace, podLabelSelector string) (map[string]ContainerImage, []ContainerImage) {
 	images := map[string]ContainerImage{}
 	allImages := []ContainerImage{}
+	allImageCreds := []KubeCreds{}


Why this is declared outside the loop?

moved inside the loop

ckotzbauer · 2022-05-11T16:01:14Z

internal/kubernetes/kubernetes.go

+	sbomOperatorNamespace, _, err = kubeConfig.Namespace()
+	if err != nil {
+		logrus.WithError(err).Fatal("namespace could not be read!")
+	}


Just saw the Debug message below in the logic, please remove one of them and use Debug level for the other.

ckotzbauer · 2022-05-11T16:06:45Z

main.go

@@ -62,6 +62,7 @@ func init() {
 	rootCmd.PersistentFlags().String(internal.ConfigKeyKubernetesClusterId, "default", "Kubernetes Cluster ID")
 	rootCmd.PersistentFlags().String(internal.ConfigKeyJobImage, "", "Custom Job-Image")
 	rootCmd.PersistentFlags().String(internal.ConfigKeyJobImagePullSecret, "", "Custom Job-Image-Pull-Secret")
+	rootCmd.PersistentFlags().String(internal.ConfigKeyCustomGlobalPullSecret, "", "Custom Global-Pull-Secret")


Please rename the display-name.

sure - missed that with the previous commit :(

ckotzbauer · 2022-05-11T16:12:11Z

internal/registry/registry.go

-	if len(image.Auth) > 0 {
-		cfg, err := ResolveAuthConfig(image)
-		empty := types.AuthConfig{}
+	for pullSecretIndex, pullSecret := range image.PullSecrets {


This logic is good in general, except the fact that it assumes, that there is always at least one pull-secret. That's wrong. The pull-secrets are always optional.

yeah - you are right
made some changes to the pullSecret iteration handling

one commit is still missing (returning error instead of nil when necessary)

done
the pullsecret handling should work now and i added some tests

ckotzbauer · 2022-05-11T16:13:34Z

internal/registry/registry.go

-			return err
-		}
+		if len(pullSecret.SecretCredsData) > 0 {
+			cfg, err := ResolveAuthConfigWithPullSecretIndex(image, pullSecretIndex)


Please directly use the pullSecret here instead of accessing it with the index.

made some changes to the pullSecret iteration handling

ckotzbauer · 2022-05-11T16:15:20Z

internal/registry/registry.go

+
+func ResolveAuthConfig(image kubernetes.ContainerImage) (types.AuthConfig, error) {
+	// to not break JobImages this function needs to redirect to the actual resolve-function
+	return ResolveAuthConfigWithPullSecretIndex(image, 0)


You have to return an empty types.AuthConfig, if there are no pull-secrets the ResolveAuthConfigWithPullSecretIndex will panic.

ckotzbauer · 2022-05-11T16:15:59Z

internal/syft/syft_test.go

@@ -55,7 +55,8 @@ func marshalCyclonedx(t *testing.T, x interface{}) string {
 func testJsonSbom(t *testing.T, name, imageID string) {
 	format := "json"
 	s := syft.New(format).WithVersion("v9.9.9")
-	sbom, err := s.ExecuteSyft(kubernetes.ContainerImage{ImageID: imageID, Auth: []byte{}})
+	sbom, err := s.ExecuteSyft(kubernetes.ContainerImage{ImageID: imageID, PullSecrets: []kubernetes.KubeCreds{{SecretName: "syft-test-creds", SecretCredsData: []byte{}}}})
+


Please use an empty PullSecrets slice for at least one test-case.

always return allImageCreds removed unnecessary error message from log-message removed unnecessary info message

…t is present

update testcases for registry and syft-analysis

steelion added 12 commits May 6, 2022 22:50

adding all pull secrets to the ContainerImage objects and loading one…

91edcb2

… by one when necessary

try to debug what exactly is used for authentication

1de418f

more debugging

22924ed

add the secret name to the structure for better understanding

f45a589

remove debugging messages

c620063

add global credentials from sbom-operator namespace

73dad19

fix secret localObjectName

c50f56e

get ownNamespace

beef6c2

fix add missing variables to error message

3f92692

try reading ownNamespace

02a931d

add custom global pull-secret

ad636b6

add new parameter to documentation

0d03429

github-actions bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 6, 2022

ckotzbauer requested changes May 7, 2022

View reviewed changes

- The name of the startup-parameter

7230879

- refactor the ContainerImage struct. - refactor secrets iteration

steelion added 2 commits May 9, 2022 15:28

prevent the upgrade from having a breaking change because of the name…

eaaf498

…space_name env-variable

add the POD_NAMESPACE env variable to the documentation

f5daa90

ckotzbauer requested changes May 11, 2022

View reviewed changes

finally renamed the configParameter

5b60bcb

always return allImageCreds removed unnecessary error message from log-message removed unnecessary info message

ckotzbauer added the kind/feature Categorizes issue or PR as related to a new feature. label May 13, 2022

steelion and others added 6 commits May 13, 2022 09:32

fix condition for reading the fallbackPullSecret

9edfc90

fix environment variable value for POD_NAMESPACE

f1dd78b

use pullSecret without index and fix the situation where no pullsecre…

43ffdb2

…t is present

fix pullSecret iteration handling

6ebdfdd

Merge branch 'ckotzbauer:main' into feature/multi-pull-secrets

f6429f1

return proper error when necessary

22a53eb

update testcases for registry and syft-analysis

ckotzbauer mentioned this pull request May 14, 2022

Multiple pull-secrets and optional fallback-secret #98

Merged

steelion closed this May 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/multi pull secrets #83

Feature/multi pull secrets #83

steelion commented May 6, 2022

steelion commented May 6, 2022

ckotzbauer left a comment

ckotzbauer May 7, 2022

steelion May 9, 2022

ckotzbauer May 11, 2022

ckotzbauer May 11, 2022

steelion May 13, 2022

steelion commented May 9, 2022

ckotzbauer left a comment

ckotzbauer May 11, 2022

steelion May 13, 2022

ckotzbauer May 11, 2022

ckotzbauer May 11, 2022

steelion May 13, 2022

ckotzbauer May 11, 2022

ckotzbauer May 11, 2022

steelion May 13, 2022

ckotzbauer May 11, 2022

steelion May 13, 2022

steelion May 13, 2022

steelion May 13, 2022

ckotzbauer May 11, 2022

steelion May 13, 2022

ckotzbauer May 11, 2022

steelion May 13, 2022

ckotzbauer May 11, 2022

steelion May 13, 2022

Feature/multi pull secrets #83

Feature/multi pull secrets #83

Conversation

steelion commented May 6, 2022

steelion commented May 6, 2022

ckotzbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steelion commented May 9, 2022

ckotzbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment