Skip to content

refactor: Optimize FleetMode checks and improve code clarity #239

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

Open
TheUndeadKing wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactor: Optimize FleetMode checks and improve code clarity #239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion build/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ COPY . .
RUN make go-build

####
FROM registry.access.redhat.com/ubi9/ubi-minimal:9.7-1775623882
FROM registry.access.redhat.com/ubi9/ubi-minimal:9.7-1776104705

ENV USER_UID=1001 \
USER_NAME=ocm-agent-operator
Expand Down
2 changes: 1 addition & 1 deletion build/Dockerfile.olm-registry
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ COPY ${SAAS_OPERATOR_DIR} manifests
RUN initializer --permissive

# ubi-micro does not work for clusters with fips enabled unless we make OpenSSL available
FROM registry.access.redhat.com/ubi9/ubi-minimal:9.7-1775623882
FROM registry.access.redhat.com/ubi9/ubi-minimal:9.7-1776104705

COPY --from=builder /bin/registry-server /bin/registry-server
COPY --from=builder /bin/grpc_health_probe /bin/grpc_health_probe
Expand Down
17 changes: 8 additions & 9 deletions pkg/ocmagenthandler/ocmagenthandler_configmap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,22 +81,21 @@ func buildCAMOConfigMap(ocmAgent ocmagentv1alpha1.OcmAgent) (*corev1.ConfigMap,
func (o *ocmAgentHandler) ensureAllConfigMaps(ocmAgent ocmagentv1alpha1.OcmAgent) error {

// Ensure the OCM Agent ConfigMap
// Determine the cluster ID, used as a configmap value
cv, err := o.fetchClusterVersion()
if err != nil {
o.Log.Error(err, "unable to fetch cluster ID for creating configmap")
return err
}
clusterID := string(cv.Spec.ClusterID)

var oaCM *corev1.ConfigMap
// Only fetch cluster version for non-FleetMode to avoid unnecessary API calls
if ocmAgent.Spec.FleetMode {
oaCM = buildOCMAgentConfigMap(ocmAgent, "")
} else {
cv, err := o.fetchClusterVersion()
if err != nil {
o.Log.Error(err, "unable to fetch cluster ID for creating configmap")
return err
}
clusterID := string(cv.Spec.ClusterID)
oaCM = buildOCMAgentConfigMap(ocmAgent, clusterID)
}

err = o.ensureConfigMap(ocmAgent, oaCM, true)
err := o.ensureConfigMap(ocmAgent, oaCM, true)
if err != nil {
return err
}
Expand Down
12 changes: 4 additions & 8 deletions pkg/ocmagenthandler/ocmagenthandler_configmap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,20 +274,16 @@ var _ = Describe("OCM Agent ConfigMap Handler", func() {
})

It("ensureAllConfigMaps handles fleet mode and errors", func() {
testClusterVersion := &configv1.ClusterVersion{
ObjectMeta: metav1.ObjectMeta{Name: "version"},
Spec: configv1.ClusterVersionSpec{ClusterID: "test-cluster-id"},
}

// Test fleet mode (no CAMO, no cluster ID)
// Test fleet mode (no cluster version fetch, no CAMO, no cluster ID)
testOcmAgent.Spec.FleetMode = true
mockClient.EXPECT().Get(gomock.Any(), types.NamespacedName{Name: "version"}, gomock.Any()).SetArg(2, *testClusterVersion)
// FleetMode creates: OCM Agent ConfigMap + Trusted CA ConfigMap (not CAMO)
mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(k8serrs.NewNotFound(schema.GroupResource{}, "")).Times(2)
mockClient.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).Times(2)
err := testOcmAgentHandler.ensureAllConfigMaps(testOcmAgent)
Expect(err).ToNot(HaveOccurred())
Comment on lines +279 to 283
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 15, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fleet-mode test is under-asserted for created resources.

Line 280-Line 281 only check call counts with gomock.Any(). This can miss regressions where the wrong ConfigMaps are created (or expected data is wrong) while counts still equal 2.

Proposed assertion-tightening diff 
 		It("ensureAllConfigMaps handles fleet mode and errors", func() {
 			// Test fleet mode (no cluster version fetch, no CAMO, no cluster ID)
 			testOcmAgent.Spec.FleetMode = true
 			// FleetMode creates: OCM Agent ConfigMap + Trusted CA ConfigMap (not CAMO)
-			mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(k8serrs.NewNotFound(schema.GroupResource{}, "")).Times(2)
-			mockClient.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).Times(2)
+			notFound := k8serrs.NewNotFound(schema.GroupResource{}, "")
+			created := map[string]*corev1.ConfigMap{}
+			mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(notFound).Times(2)
+			mockClient.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn(
+				func(_ context.Context, d *corev1.ConfigMap, _ ...client.CreateOptions) error {
+					created[d.Name] = d
+					return nil
+				}).Times(2)
 			err := testOcmAgentHandler.ensureAllConfigMaps(testOcmAgent)
 			Expect(err).ToNot(HaveOccurred())
+			Expect(created).To(HaveKey(testOcmAgent.Name + testconst.TestConfigMapSuffix))
+			Expect(created).To(HaveKey("trusted-ca-bundle"))
+			Expect(created).ToNot(HaveKey(oahconst.CAMOConfigMapNamespacedName.Name))
+			Expect(created[testOcmAgent.Name+testconst.TestConfigMapSuffix].Data).ToNot(HaveKey(oahconst.OCMAgentConfigClusterID))
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/ocmagenthandler/ocmagenthandler_configmap_test.go` around lines 279 -
283, The test uses mockClient.EXPECT().Get(...).Return(...).Times(2) and
mockClient.EXPECT().Create(...).Return(nil).Times(2) with gomock.Any(), which
only verifies call counts and can miss wrong resources; update the expectations
to assert the actual ConfigMap objects created by ensureAllConfigMaps: replace
gomock.Any() for the object arg with a matcher like
gomock.AssignableToTypeOf(&corev1.ConfigMap{}) and/or capture the created
argument using gomock.Do or a custom gomock.Matcher, then assert the ConfigMap
metadata (Name, Namespace) and Data keys/values for the OCM Agent ConfigMap and
the Trusted CA ConfigMap; keep ctx param as gomock.Any() but verify the second
argument (the object) against expected names and contents when setting up
mockClient.EXPECT().Create and similarly tighten Get expectations to match the
correct names.


// Test cluster version fetch error
// Test non-fleet mode cluster version fetch error
testOcmAgent.Spec.FleetMode = false
fetchError := errors.New("fetch failed")
mockClient.EXPECT().Get(gomock.Any(), types.NamespacedName{Name: "version"}, gomock.Any()).Return(fetchError)
err = testOcmAgentHandler.ensureAllConfigMaps(testOcmAgent)
Expand Down
3 changes: 1 addition & 2 deletions pkg/ocmagenthandler/ocmagenthandler_deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,7 @@ func buildOCMAgentArgs(ocmAgent ocmagentv1alpha1.OcmAgent) []string {
}
if !ocmAgent.Spec.FleetMode {
command = append(command, fmt.Sprintf("--cluster-id=@%s", clusterIDPath), fmt.Sprintf("--access-token=@%s", accessTokenPath))
}
if ocmAgent.Spec.FleetMode {
} else {
command = append(command, "--fleet-mode")
}

Expand Down
14 changes: 7 additions & 7 deletions pkg/ocmagenthandler/ocmagenthandler_networkpolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,18 @@ func buildNetworkPolicy(ocmAgent ocmagentv1alpha1.OcmAgent, namespace string) ne
return np
}

func (o *ocmAgentHandler) ensureAllNetworkPolicies(ocmAgent ocmagentv1alpha1.OcmAgent) error {
func getNetworkPolicyNamespaces(ocmAgent ocmagentv1alpha1.OcmAgent) []string {
var namespaces []string
if ocmAgent.Spec.FleetMode {
namespaces = append(namespaces, oah.NamespaceMonitorng, oah.NamespaceRHOBS, oah.NamespaceOBO)
} else {
namespaces = append(namespaces, oah.NamespaceMonitorng, oah.NamespaceMUO)
}
return namespaces
}

func (o *ocmAgentHandler) ensureAllNetworkPolicies(ocmAgent ocmagentv1alpha1.OcmAgent) error {
namespaces := getNetworkPolicyNamespaces(ocmAgent)
for _, ns := range namespaces {
err := o.ensureNetworkPolicy(ocmAgent, ns)
if err != nil {
Expand Down Expand Up @@ -134,12 +139,7 @@ func (o *ocmAgentHandler) ensureNetworkPolicy(ocmAgent ocmagentv1alpha1.OcmAgent
}

func (o *ocmAgentHandler) ensureAllNetworkPoliciesDeleted(ocmAgent ocmagentv1alpha1.OcmAgent) error {
var namespaces []string
if ocmAgent.Spec.FleetMode {
namespaces = append(namespaces, oah.NamespaceMonitorng, oah.NamespaceRHOBS, oah.NamespaceOBO)
} else {
namespaces = append(namespaces, oah.NamespaceMonitorng, oah.NamespaceMUO)
}
namespaces := getNetworkPolicyNamespaces(ocmAgent)
for _, ns := range namespaces {
err := o.ensureNetworkPolicyDeleted(ocmAgent, ns)
if err != nil {
Expand Down