tests: e2e test for trusted-ca-bundle #1346

Sara4994 · 2024-11-05T19:32:04Z

Description

This PR is intended to include e2e tests for the trusted CA bundle feature.

JIRA issue: https://issues.redhat.com/browse/RHOAIENG-3924

How Has This Been Tested?

Screenshot or short clip

Merge criteria

You have read the contributors guide.
Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

openshift-ci · 2024-11-05T19:32:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from sara4994. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sara4994 · 2024-11-05T19:33:52Z

/cc @VaishnaviHire

VaishnaviHire · 2024-11-05T19:52:16Z

tests/e2e/helper_test.go

@@ -129,45 +129,45 @@ func setupDSCInstance(name string) *dscv1.DataScienceCluster {
 				},
 				ModelMeshServing: modelmeshserving.ModelMeshServing{
 					Component: components.Component{
-						ManagementState: operatorv1.Managed,
+						ManagementState: operatorv1.Removed,


The ManagementState changes need to be reverted

oops.. yea my bad. just reverted them

codecov · 2024-11-05T21:19:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (incubation@8fb9bd7). Learn more about missing BASE report.

Additional details and impacted files

@@              Coverage Diff              @@
##             incubation    #1346   +/-   ##
=============================================
  Coverage              ?   18.91%           
=============================================
  Files                 ?       30           
  Lines                 ?     3399           
  Branches              ?        0           
=============================================
  Hits                  ?      643           
  Misses                ?     2687           
  Partials              ?       69

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

VaishnaviHire · 2024-11-07T00:20:32Z

@Sara4994 As part of #1339 @zdtsw introduced changes, that can be validated as part of this PR. The test cases are included in PR description.

zdtsw · 2024-11-07T09:15:20Z

@Sara4994 As part of #1339 @zdtsw introduced changes, that can be validated as part of this PR. The test cases are included in PR description.

i dont think it need to be too complicated in test case to cover all 3 senaiors from that "fix" PR.
the real one we need is to check if the foundConfigMap.Data[CADataFieldName] has an ending newline, then it is fine.

zdtsw · 2024-11-07T09:28:19Z

tests/e2e/creation_test.go

@@ -444,6 +451,48 @@ func (tc *testContext) testDefaultCertsAvailable() error {
 	return nil
 }

+func (tc *testContext) testTrustedCABundle() error {


not sure i understand the logic here:
if we are making e2e test (not unit-test) , what we should test is:

set managementStateChangeTrustedCA := true

check if Configmap is created in tc.testDSCI.Spec.ApplicationsNamespace

check content of CADataFieldName == tc.testDSCI.Spec.TrustedCABundle.CustomCABundle

check content of CADataFieldName has newline as ending (this is new from current code base)

set managementStateChangeTrustedCA := false

check no Configmap in tc.testDSCI.Spec.ApplicationsNamespace

ok, seems confusion for the above comments:
set managementStateChangeTrustedCA := true
is not meant to be set in the testcase, this is only to illustrate the case when set to Managed VS Removed
what need to be done is tc.testDSCI.Spec.TrustedCABundle.ManagementState: Managed

Sara4994 · 2024-11-15T07:38:32Z

/test opendatahub-operator-e2e

grdryn · 2024-11-27T13:15:01Z

@Sara4994 what's the status of this PR? It looks like it hasn't seen any update in a couple of weeks, and the last time the e2e test job ran it failed on the newly added test.

VaishnaviHire · 2024-12-03T17:49:32Z

tests/e2e/creation_test.go

+			return fmt.Errorf("odh-trusted-ca-bundle in config map does not match with DSCI's TrustedCABundle.CustomCABundle, needs update: %w", err)
+		}
+
+		err = trustedcabundle.AddCABundleCMInAllNamespaces(tc.ctx, tc.customClient, logr.Logger{}, tc.testDSCI)


Why are we explicitly adding and removing the configmap from all namespaces? Shouldn't this be created by the controller?

VaishnaviHire · 2024-12-05T15:30:28Z

tests/e2e/creation_test.go

+
+		if k8serr.IsNotFound(err) {
+			fmt.Printf("Config map not found in the namespace")
+		}


We should return other errors here

@VaishnaviHire this is done

VaishnaviHire · 2024-12-05T15:31:53Z

tests/e2e/helper_test.go

+      MIIFVjCCAz6gAwIBAgIUQ+NxE9izWRRdt86M/TX9b7wFjUUwDQYJKoZIhvcNAQEL
+     ...
+      IrrVQJLuM7IjWcmOvFjai57QGfIvWcaMY1q6n6MLsLOaXLoRuBLpDLvPbmyAhykU
+      ------END ------` + "\n",


I am not sure we need to add \n here, since that was the change - to introduce newline if its not in input
cc: @zdtsw correct me if I missed something here

@VaishnaviHire I thought the same but if i dont add newline it there, the check here

opendatahub-operator/tests/e2e/creation_test.go

Line 474 in f72a8a2

if foundConfigMap.Data[CADataFieldName] != tc.testDSCI.Spec.TrustedCABundle.CustomCABundle {

fails, I mean it says string in foundConfigMap.Data[CADataFieldName] and tc.testDSCI.Spec.TrustedCABundle.CustomCABundle are different

I am not sure we need to add \n here, since that was the change - to introduce newline if its not in input

correct, user input data in DSCI should not need to explicility add newline ending

Sara4994 · 2024-12-06T06:23:10Z

/test opendatahub-operator-e2e

zdtsw · 2024-12-06T11:27:32Z

LGTM, @grdryn you wanna give another review?

tests/e2e/creation_test.go

grdryn · 2024-12-06T12:25:01Z

tests/e2e/creation_test.go

+		if checkNewline == false {
+			fmt.Print("Newline not found at the end of configmap")
+		}


Are both cases acceptable here (newline or not), or should this assert one of the cases?

Also, the check looks like it will pass if a newline character is found anywhere in the string, not just at the end.

@grdryn yea true, i have modified the check here

zdtsw · 2024-12-09T07:26:46Z

target branch should be "main"

Sara4994 · 2024-12-09T08:39:23Z

@zdtsw @grdryn while switching the target branch, there were so many conflicts from lot many commits. So I have created a new PR to avoid mess. I will close this one if new PR looks fine

grdryn · 2024-12-09T14:12:26Z

@Sara4994 sounds good. I think it's fine to close this one already now. Feel free to re-open if you disagree.

…flux/component-updates/odh-model-controller-v2-17 chore(deps): update odh-model-controller-v2-17 to 3d61985

openshift-ci bot requested review from etirelli and MarianMacik November 5, 2024 19:32

openshift-ci bot requested a review from VaishnaviHire November 5, 2024 19:33

Sara4994 force-pushed the trustedcabundle-e2e-tests branch from 52a655a to 7897693 Compare November 5, 2024 19:37

openshift-ci bot removed request for etirelli and MarianMacik November 5, 2024 19:48

VaishnaviHire reviewed Nov 5, 2024

View reviewed changes

Sara4994 force-pushed the trustedcabundle-e2e-tests branch 2 times, most recently from a439e36 to 37d0b92 Compare November 5, 2024 20:02

zdtsw added the odh-2.21 label Nov 6, 2024

zdtsw reviewed Nov 7, 2024

View reviewed changes

Sara4994 force-pushed the trustedcabundle-e2e-tests branch from 37d0b92 to 96ec2c2 Compare November 13, 2024 19:35

zdtsw removed the odh-2.21 label Nov 14, 2024

tests: e2e test for trusted-ca-bundle

a0ac2d2

Sara4994 force-pushed the trustedcabundle-e2e-tests branch 3 times, most recently from cf7fea7 to 5449a4c Compare December 1, 2024 17:18

VaishnaviHire reviewed Dec 3, 2024

View reviewed changes

Sara4994 force-pushed the trustedcabundle-e2e-tests branch 5 times, most recently from 6d5d524 to 0516e50 Compare December 5, 2024 10:34

Sara4994 force-pushed the trustedcabundle-e2e-tests branch 3 times, most recently from b639713 to f72a8a2 Compare December 5, 2024 13:36

VaishnaviHire reviewed Dec 5, 2024

View reviewed changes

Sara4994 force-pushed the trustedcabundle-e2e-tests branch 3 times, most recently from 2ddc6ad to bede6ac Compare December 5, 2024 17:38

zdtsw requested a review from grdryn December 6, 2024 08:59

grdryn reviewed Dec 6, 2024

View reviewed changes

test trustedCABundle ManagementState conditions

fcc2720

Sara4994 force-pushed the trustedcabundle-e2e-tests branch from bede6ac to fcc2720 Compare December 6, 2024 15:28

Sara4994 changed the base branch from incubation to main December 9, 2024 07:48

Sara4994 changed the base branch from main to incubation December 9, 2024 07:49

Sara4994 mentioned this pull request Dec 9, 2024

tests: e2e test for trusted-ca-bundle #1434

Merged

5 tasks

grdryn closed this Dec 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: e2e test for trusted-ca-bundle #1346

tests: e2e test for trusted-ca-bundle #1346

Sara4994 commented Nov 5, 2024 •

edited

Loading

openshift-ci bot commented Nov 5, 2024

Sara4994 commented Nov 5, 2024

VaishnaviHire Nov 5, 2024

Sara4994 Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading

VaishnaviHire commented Nov 7, 2024

zdtsw commented Nov 7, 2024

zdtsw Nov 7, 2024

zdtsw Nov 13, 2024

Sara4994 commented Nov 15, 2024

grdryn commented Nov 27, 2024

VaishnaviHire Dec 3, 2024

VaishnaviHire Dec 5, 2024

Sara4994 Dec 5, 2024

VaishnaviHire Dec 5, 2024

Sara4994 Dec 5, 2024 •

edited

Loading

zdtsw Dec 5, 2024

Sara4994 commented Dec 6, 2024

zdtsw commented Dec 6, 2024

grdryn Dec 6, 2024 •

edited

Loading

Sara4994 Dec 9, 2024

zdtsw commented Dec 9, 2024

Sara4994 commented Dec 9, 2024 •

edited

Loading

grdryn commented Dec 9, 2024

tests: e2e test for trusted-ca-bundle #1346

tests: e2e test for trusted-ca-bundle #1346

Conversation

Sara4994 commented Nov 5, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

openshift-ci bot commented Nov 5, 2024

Sara4994 commented Nov 5, 2024

Choose a reason for hiding this comment

Sara4994 Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 5, 2024 • edited Loading

Codecov Report

VaishnaviHire commented Nov 7, 2024

zdtsw commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sara4994 commented Nov 15, 2024

grdryn commented Nov 27, 2024

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

Sara4994 Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sara4994 commented Dec 6, 2024

zdtsw commented Dec 6, 2024

grdryn Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdtsw commented Dec 9, 2024

Sara4994 commented Dec 9, 2024 • edited Loading

grdryn commented Dec 9, 2024

Sara4994 commented Nov 5, 2024 •

edited

Loading

Sara4994 Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading

Sara4994 Dec 5, 2024 •

edited

Loading

grdryn Dec 6, 2024 •

edited

Loading

Sara4994 commented Dec 9, 2024 •

edited

Loading