Skip to content

Commit 96b4329

Browse files
committed
ESO-323: incorporates coderabbit suggestions
Signed-off-by: Bharath B <bhb@redhat.com>
1 parent 7eda154 commit 96b4329

9 files changed

Lines changed: 118 additions & 88 deletions

File tree

Makefile

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,7 @@ test-e2e: ## Run e2e tests against a cluster.
212212
-count 1 -v -p 1 \
213213
-tags e2e ./e2e \
214214
-ginkgo.v \
215+
-ginkgo.trace \
215216
-ginkgo.show-node-events \
216217
-ginkgo.label-filter=$(E2E_GINKGO_LABEL_FILTER)
217218

hack/govulncheck.sh

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,11 @@ set -o errexit
2020
## Below vulnerabilities are in the kubernetes package, which impacts the server and not the operator, which is the client.
2121
# - https://pkg.go.dev/vuln/GO-2025-3521 - Kubernetes GitRepo Volume Inadvertent Local Repository Access in k8s.io/kubernetes
2222
# - https://pkg.go.dev/vuln/GO-2025-3547 - Kubernetes kube-apiserver Vulnerable to Race Condition in k8s.io/kubernetes
23-
KNOWN_VULNS_PATTERN="GO-2025-3521|GO-2025-3547"
23+
#
24+
## Below vulnerabilities are in the go packages, which impacts the operator code and requires the fix to be available downstream.
25+
# - https://pkg.go.dev/vuln/GO-2026-4601 - Incorrect parsing of IPv6 host literals in net/url
26+
# - https://pkg.go.dev/vuln/GO-2026-4602 - FileInfo can escape from a Root in os
27+
KNOWN_VULNS_PATTERN="GO-2025-3521|GO-2025-3547|GO-2026-4601|GO-2026-4602"
2428

2529
GOVULNCHECK_BIN="${1:-}"
2630
OUTPUT_DIR="${2:-}"

test/e2e/bitwarden_es_test.go

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -149,18 +149,20 @@ func ensureBitwardenOperandReady(ctx context.Context) error {
149149
var _ = Describe("Bitwarden Provider", Ordered, Label("Provider:Bitwarden", "Suite:Bitwarden"), func() {
150150
ctx := context.Background()
151151
var (
152-
clientset *kubernetes.Clientset
153-
dynamicClient *dynamic.DynamicClient
154-
runtimeClient client.Client
155-
loader utils.DynamicResourceLoader
156-
testNamespace string
157-
clusterStoreName string
158-
tlsMaterials *utils.BitwardenTLSMaterials
159-
originalESC *operatorv1alpha1.ExternalSecretsConfig
160-
cssObj *unstructured.Unstructured
161-
bitwardenOrgID string
162-
bitwardenProjectID string
163-
pushedRemoteKeyForUUIDTest string // set by first It, used by second It to look up secret UUID
152+
clientset *kubernetes.Clientset
153+
dynamicClient *dynamic.DynamicClient
154+
runtimeClient client.Client
155+
loader utils.DynamicResourceLoader
156+
testNamespace string
157+
clusterStoreName string
158+
tlsMaterials *utils.BitwardenTLSMaterials
159+
originalESC *operatorv1alpha1.ExternalSecretsConfig
160+
cssObj *unstructured.Unstructured
161+
bitwardenOrgID string
162+
bitwardenProjectID string
163+
// pushedRemoteKeyForUUIDTest is set by the first It block and consumed by the second.
164+
// These tests must run in order (Ordered container) due to this shared state dependency.
165+
pushedRemoteKeyForUUIDTest string
164166
)
165167

166168
BeforeAll(func() {

test/e2e/e2e_suite_test.go

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,18 +51,13 @@ var (
5151
)
5252

5353
func getTestDir() string {
54-
if os.Getenv("OPENSHIFT_CI") == "true" {
55-
if d := os.Getenv("ARTIFACT_DIR"); d != "" {
56-
return d
57-
}
58-
}
5954
if d := os.Getenv("ARTIFACT_DIR"); d != "" {
6055
return d
6156
}
6257
// Local run: use repo _output.
6358
cwd, err := os.Getwd()
6459
if err == nil {
65-
return filepath.Clean(filepath.Join(cwd, "..", "_output"))
60+
return filepath.Clean(filepath.Join(cwd, "..", "..", "_output"))
6661
}
6762
return "/tmp"
6863
}

test/utils/bitwarden.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,9 @@ func FetchBitwardenCredsFromSecret(ctx context.Context, client kubernetes.Interf
188188
return "", "", "", fmt.Errorf("get Bitwarden creds secret %s/%s: %w", secretNamespace, secretName, err)
189189
}
190190
token = string(secret.Data[TokenSecretKey])
191+
if token == "" {
192+
return "", "", "", fmt.Errorf("bitwarden creds secret %s/%s missing required key %q", secretNamespace, secretName, TokenSecretKey)
193+
}
191194
if v, ok := secret.Data[BitwardenCredKeyOrgID]; ok {
192195
orgID = string(v)
193196
}
@@ -291,7 +294,7 @@ func WaitForBitwardenSDKServerReachableFromCluster(ctx context.Context, client k
291294
},
292295
Containers: []corev1.Container{{
293296
Name: "curl",
294-
Image: "docker.io/curlimages/curl:latest",
297+
Image: bitwardenAPITestRunnerImage,
295298
Command: cmd,
296299
SecurityContext: &corev1.SecurityContext{
297300
AllowPrivilegeEscalation: &allowPrivilegeEscalation,

test/utils/bitwarden_api_runner.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,9 @@ func RunBitwardenAPIJob(ctx context.Context, client kubernetes.Interface, namesp
9090
return -1, "", fmt.Errorf("create job %s: %w", jobName, err)
9191
}
9292
propagationBackground := metav1.DeletePropagationBackground
93-
defer func() { _ = client.BatchV1().Jobs(namespace).Delete(ctx, jobName, metav1.DeleteOptions{PropagationPolicy: &propagationBackground}) }()
93+
defer func() {
94+
_ = client.BatchV1().Jobs(namespace).Delete(ctx, jobName, metav1.DeleteOptions{PropagationPolicy: &propagationBackground})
95+
}()
9496

9597
err = wait.PollUntilContextTimeout(ctx, 2*time.Second, timeout, true, func(ctx context.Context) (bool, error) {
9698
j, getErr := client.BatchV1().Jobs(namespace).Get(ctx, jobName, metav1.GetOptions{})
@@ -111,9 +113,12 @@ func RunBitwardenAPIJob(ctx context.Context, client kubernetes.Interface, namesp
111113

112114
// Find the pod and get exit code and logs.
113115
pods, err := client.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{LabelSelector: "job-name=" + jobName})
114-
if err != nil || len(pods.Items) == 0 {
116+
if err != nil {
115117
return -1, "", fmt.Errorf("list pods for job %s: %w", jobName, err)
116118
}
119+
if len(pods.Items) == 0 {
120+
return -1, "", fmt.Errorf("no pods found for job %s", jobName)
121+
}
117122
pod := pods.Items[0]
118123
for _, cs := range pod.Status.ContainerStatuses {
119124
if cs.State.Terminated != nil {

test/utils/cleanup.go

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ package utils
2222
import (
2323
"context"
2424

25+
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
26+
rbacv1 "k8s.io/api/rbac/v1"
2527
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
2628
apiextensionsclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
2729
k8serrors "k8s.io/apimachinery/pkg/api/errors"
@@ -60,7 +62,10 @@ func CleanupESOOperandAndRelated(ctx context.Context, cfg *rest.Config) {
6062
}
6163

6264
// 1. Delete all instances of operand CRDs (label app=external-secrets).
63-
crdList, _ := extClient.ApiextensionsV1().CustomResourceDefinitions().List(ctx, metav1.ListOptions{LabelSelector: operandLabelSelector})
65+
crdList, err := extClient.ApiextensionsV1().CustomResourceDefinitions().List(ctx, metav1.ListOptions{LabelSelector: operandLabelSelector})
66+
if err != nil || crdList == nil {
67+
crdList = &apiextensionsv1.CustomResourceDefinitionList{}
68+
}
6469
for i := range crdList.Items {
6570
crd := &crdList.Items[i]
6671
version := preferredVersion(crd)
@@ -99,17 +104,26 @@ func CleanupESOOperandAndRelated(ctx context.Context, cfg *rest.Config) {
99104
_ = dynamicClient.Resource(escGVR).Delete(ctx, externalSecretsConfigName, metav1.DeleteOptions{})
100105

101106
// 3. Remove webhooks first so namespace deletion can proceed.
102-
webhooks, _ := clientset.AdmissionregistrationV1().ValidatingWebhookConfigurations().List(ctx, metav1.ListOptions{LabelSelector: operandLabelSelector})
107+
webhooks, err := clientset.AdmissionregistrationV1().ValidatingWebhookConfigurations().List(ctx, metav1.ListOptions{LabelSelector: operandLabelSelector})
108+
if err != nil || webhooks == nil {
109+
webhooks = &admissionregistrationv1.ValidatingWebhookConfigurationList{}
110+
}
103111
for i := range webhooks.Items {
104112
_ = clientset.AdmissionregistrationV1().ValidatingWebhookConfigurations().Delete(ctx, webhooks.Items[i].Name, metav1.DeleteOptions{})
105113
}
106114

107115
// 4. ClusterRoleBindings before ClusterRoles.
108-
crbs, _ := clientset.RbacV1().ClusterRoleBindings().List(ctx, metav1.ListOptions{LabelSelector: operandLabelSelector})
116+
crbs, err := clientset.RbacV1().ClusterRoleBindings().List(ctx, metav1.ListOptions{LabelSelector: operandLabelSelector})
117+
if err != nil || crbs == nil {
118+
crbs = &rbacv1.ClusterRoleBindingList{}
119+
}
109120
for i := range crbs.Items {
110121
_ = clientset.RbacV1().ClusterRoleBindings().Delete(ctx, crbs.Items[i].Name, metav1.DeleteOptions{})
111122
}
112-
crList, _ := clientset.RbacV1().ClusterRoles().List(ctx, metav1.ListOptions{LabelSelector: operandLabelSelector})
123+
crList, err := clientset.RbacV1().ClusterRoles().List(ctx, metav1.ListOptions{LabelSelector: operandLabelSelector})
124+
if err != nil || crList == nil {
125+
crList = &rbacv1.ClusterRoleList{}
126+
}
113127
for i := range crList.Items {
114128
_ = clientset.RbacV1().ClusterRoles().Delete(ctx, crList.Items[i].Name, metav1.DeleteOptions{})
115129
}

test/utils/conditions.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,7 @@ func DeleteAWSSecretFromCredsSecret(ctx context.Context, k8sClient *kubernetes.C
265265
svc := secretsmanager.New(sess)
266266
_, err = svc.DeleteSecret(&secretsmanager.DeleteSecretInput{
267267
SecretId: aws.String(awsSecretName),
268-
ForceDeleteWithoutRecovery: aws.Bool(true),
268+
ForceDeleteWithoutRecovery: aws.Bool(true), // permanently delete without 7-day wait
269269
})
270270
if err != nil {
271271
return fmt.Errorf("failed to delete AWS secret: %w", err)

test/utils/dynamic_resources.go

Lines changed: 67 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import (
2323
"context"
2424
"testing"
2525

26-
"github.com/stretchr/testify/require"
2726
k8serrors "k8s.io/apimachinery/pkg/api/errors"
2827
"k8s.io/apimachinery/pkg/api/meta"
2928
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -34,6 +33,8 @@ import (
3433
"k8s.io/client-go/dynamic"
3534
"k8s.io/client-go/kubernetes"
3635
"k8s.io/client-go/restmapper"
36+
37+
"github.com/stretchr/testify/require"
3738
)
3839

3940
type DynamicResourceLoader struct {
@@ -56,60 +57,10 @@ func NewDynamicResourceLoader(context context.Context, t *testing.T) DynamicReso
5657
}
5758
}
5859

59-
func (d DynamicResourceLoader) noErrorSkipExists(err error) {
60-
if !k8serrors.IsAlreadyExists(err) {
61-
require.NoError(d.t, err)
62-
}
63-
}
64-
65-
func (d DynamicResourceLoader) noErrorSkipNotExisting(err error) {
66-
if !k8serrors.IsNotFound(err) {
67-
require.NoError(d.t, err)
68-
}
69-
}
70-
71-
func (d DynamicResourceLoader) do(do doFunc, assetFunc func(name string) ([]byte, error), filename string, overrideNamespace string) {
72-
b, err := assetFunc(filename)
73-
require.NoError(d.t, err)
74-
75-
decoder := yamlutil.NewYAMLOrJSONDecoder(bytes.NewReader(b), 1024)
76-
var rawObj runtime.RawExtension
77-
err = decoder.Decode(&rawObj)
78-
require.NoError(d.t, err)
79-
80-
obj, gvk, err := yaml.NewDecodingSerializer(unstructured.UnstructuredJSONScheme).Decode(rawObj.Raw, nil, nil)
81-
require.NoError(d.t, err)
82-
83-
unstructuredMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
84-
require.NoError(d.t, err)
85-
86-
unstructuredObj := &unstructured.Unstructured{Object: unstructuredMap}
87-
88-
gr, err := restmapper.GetAPIGroupResources(d.KubeClient.Discovery())
89-
require.NoError(d.t, err)
90-
91-
mapper := restmapper.NewDiscoveryRESTMapper(gr)
92-
mapping, err := mapper.RESTMapping(gvk.GroupKind(), gvk.Version)
93-
require.NoError(d.t, err)
94-
95-
var dri dynamic.ResourceInterface
96-
if mapping.Scope.Name() == meta.RESTScopeNameNamespace {
97-
if overrideNamespace != "" {
98-
unstructuredObj.SetNamespace(overrideNamespace)
99-
}
100-
require.NotEmpty(d.t, unstructuredObj.GetNamespace(), "Namespace can not be empty!")
101-
dri = d.DynamicClient.Resource(mapping.Resource).Namespace(unstructuredObj.GetNamespace())
102-
} else {
103-
dri = d.DynamicClient.Resource(mapping.Resource)
104-
}
105-
106-
do(d.t, unstructuredObj, dri)
107-
}
108-
10960
func (d DynamicResourceLoader) DeleteFromFile(assetFunc func(name string) ([]byte, error), filename string, overrideNamespace string) {
11061
d.t.Logf("Deleting resource %v\n", filename)
11162
deleteFunc := func(t *testing.T, unstructured *unstructured.Unstructured, dynamicResourceInterface dynamic.ResourceInterface) {
112-
err := dynamicResourceInterface.Delete(context.Background(), unstructured.GetName(), metav1.DeleteOptions{})
63+
err := dynamicResourceInterface.Delete(d.context, unstructured.GetName(), metav1.DeleteOptions{})
11364
d.noErrorSkipNotExisting(err)
11465
}
11566

@@ -120,7 +71,7 @@ func (d DynamicResourceLoader) DeleteFromFile(assetFunc func(name string) ([]byt
12071
func (d DynamicResourceLoader) CreateFromFile(assetFunc func(name string) ([]byte, error), filename string, overrideNamespace string) {
12172
d.t.Logf("Creating resource %v\n", filename)
12273
createFunc := func(t *testing.T, unstructured *unstructured.Unstructured, dynamicResourceInterface dynamic.ResourceInterface) {
123-
_, err := dynamicResourceInterface.Create(context.Background(), unstructured, metav1.CreateOptions{})
74+
_, err := dynamicResourceInterface.Create(d.context, unstructured, metav1.CreateOptions{})
12475
d.noErrorSkipExists(err)
12576
}
12677

@@ -142,31 +93,86 @@ func (d DynamicResourceLoader) CreateFromUnstructured(unstructuredObj *unstructu
14293
// Use from Ginkgo tests with Expect(err).NotTo(HaveOccurred()) to see the actual API error on failure.
14394
func (d DynamicResourceLoader) CreateFromUnstructuredReturnErr(unstructuredObj *unstructured.Unstructured, overrideNamespace string) error {
14495
dri := d.getResourceInterface(unstructuredObj, overrideNamespace)
145-
_, err := dri.Create(context.Background(), unstructuredObj, metav1.CreateOptions{})
96+
_, err := dri.Create(d.context, unstructuredObj, metav1.CreateOptions{})
14697
return err
14798
}
14899

149100
// DeleteFromUnstructured deletes a resource by name. For cluster-scoped resources, namespace is ignored.
150101
func (d DynamicResourceLoader) DeleteFromUnstructured(unstructuredObj *unstructured.Unstructured, overrideNamespace string) {
151102
dri := d.getResourceInterface(unstructuredObj, overrideNamespace)
152-
err := dri.Delete(context.Background(), unstructuredObj.GetName(), metav1.DeleteOptions{})
153-
d.noErrorSkipNotExisting(err)
154-
d.t.Logf("Resource %s deleted\n", unstructuredObj.GetName())
103+
err := dri.Delete(d.context, unstructuredObj.GetName(), metav1.DeleteOptions{})
104+
if err != nil && !k8serrors.IsNotFound(err) {
105+
require.NoError(d.t, err)
106+
}
107+
if err == nil || k8serrors.IsNotFound(err) {
108+
d.t.Logf("Resource %s deleted\n", unstructuredObj.GetName())
109+
}
155110
}
156111

157112
func (d DynamicResourceLoader) getResourceInterface(unstructuredObj *unstructured.Unstructured, overrideNamespace string) dynamic.ResourceInterface {
158-
gvk := unstructuredObj.GroupVersionKind()
113+
objCopy := unstructuredObj.DeepCopy()
114+
gvk := objCopy.GroupVersionKind()
159115
gr, err := restmapper.GetAPIGroupResources(d.KubeClient.Discovery())
160116
require.NoError(d.t, err)
161117
mapper := restmapper.NewDiscoveryRESTMapper(gr)
162118
mapping, err := mapper.RESTMapping(gvk.GroupKind(), gvk.Version)
163119
require.NoError(d.t, err)
164120
if mapping.Scope.Name() == meta.RESTScopeNameNamespace {
165121
if overrideNamespace != "" {
166-
unstructuredObj.SetNamespace(overrideNamespace)
122+
objCopy.SetNamespace(overrideNamespace)
167123
}
168-
require.NotEmpty(d.t, unstructuredObj.GetNamespace(), "Namespace can not be empty for namespaced resource")
169-
return d.DynamicClient.Resource(mapping.Resource).Namespace(unstructuredObj.GetNamespace())
124+
require.NotEmpty(d.t, objCopy.GetNamespace(), "Namespace can not be empty for namespaced resource")
125+
return d.DynamicClient.Resource(mapping.Resource).Namespace(objCopy.GetNamespace())
170126
}
171127
return d.DynamicClient.Resource(mapping.Resource)
172128
}
129+
130+
func (d DynamicResourceLoader) noErrorSkipExists(err error) {
131+
if !k8serrors.IsAlreadyExists(err) {
132+
require.NoError(d.t, err)
133+
}
134+
}
135+
136+
func (d DynamicResourceLoader) noErrorSkipNotExisting(err error) {
137+
if !k8serrors.IsNotFound(err) {
138+
require.NoError(d.t, err)
139+
}
140+
}
141+
142+
func (d DynamicResourceLoader) do(do doFunc, assetFunc func(name string) ([]byte, error), filename string, overrideNamespace string) {
143+
b, err := assetFunc(filename)
144+
require.NoError(d.t, err)
145+
146+
decoder := yamlutil.NewYAMLOrJSONDecoder(bytes.NewReader(b), 1024)
147+
var rawObj runtime.RawExtension
148+
err = decoder.Decode(&rawObj)
149+
require.NoError(d.t, err)
150+
151+
obj, gvk, err := yaml.NewDecodingSerializer(unstructured.UnstructuredJSONScheme).Decode(rawObj.Raw, nil, nil)
152+
require.NoError(d.t, err)
153+
154+
unstructuredMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
155+
require.NoError(d.t, err)
156+
157+
unstructuredObj := &unstructured.Unstructured{Object: unstructuredMap}
158+
159+
gr, err := restmapper.GetAPIGroupResources(d.KubeClient.Discovery())
160+
require.NoError(d.t, err)
161+
162+
mapper := restmapper.NewDiscoveryRESTMapper(gr)
163+
mapping, err := mapper.RESTMapping(gvk.GroupKind(), gvk.Version)
164+
require.NoError(d.t, err)
165+
166+
var dri dynamic.ResourceInterface
167+
if mapping.Scope.Name() == meta.RESTScopeNameNamespace {
168+
if overrideNamespace != "" {
169+
unstructuredObj.SetNamespace(overrideNamespace)
170+
}
171+
require.NotEmpty(d.t, unstructuredObj.GetNamespace(), "Namespace can not be empty!")
172+
dri = d.DynamicClient.Resource(mapping.Resource).Namespace(unstructuredObj.GetNamespace())
173+
} else {
174+
dri = d.DynamicClient.Resource(mapping.Resource)
175+
}
176+
177+
do(d.t, unstructuredObj, dri)
178+
}

0 commit comments

Comments
 (0)