fix: ensure input CAbundle always end with a newline #1339

zdtsw · 2024-11-04T16:57:20Z

missing newline will cause problem when inject data into configmap
this happens when user use kustomize with their own CA as overlayer for DSCI, it set to use |- not |
related to Remove Default Addition of Block Chomp (|-) on ConfigMaps that don't end with New Line kubernetes-sigs/kustomize#4602

i am not fond of the solution provided in https://issues.redhat.com/browse/RHOAIENG-11895 on a component base.
this could protentially cause other components to fail.

Description

How Has This Been Tested?

quay.io/wenzhou/opendatahub-operator:2.20.114

input

  trustedCABundle:
    customCABundle: -----BEGIN CERTIFICATE-----
      MIIFVjCCAz6gAwIBAgIUQ+NxE9izWRRdt86M/TX9b7wFjUUwDQYJKoZIhvcNAQEL
     ...
      IrrVQJLuM7IjWcmOvFjai57QGfIvWcaMY1q6n6MLsLOaXLoRuBLpDLvPbmyAhykU
      ------END ------

output in CM:

odh-ca-bundle.crt: |
    -----BEGIN CERTIFICATE----- MIIFVjCCAz6gAwIBAgIUQ+NxE9izWRRdt86M/TX9b7wFjUUwDQYJKoZIhvcNAQEL ...IrrVQJLuM7IjWcmOvFjai57QGfIvWcaMY1q6n6MLsLOaXLoRuBLpDLvPbmyAhykU 
   ------END ------

input

 trustedCABundle:
   customCABundle: |
     -----BEGIN CERTIFICATE----- 
     MIIFVjCCAz6gAwIBAgIUQ+NxE9izWRRdt86M/TX9b7wFjUUwDQYJKoZIhvcNAQEL 
     ...
     IrrVQJLuM7IjWcmOvFjai57QGfIvWcaMY1q6n6MLsLOaXLoRuBLpDLvPbmyAhykU 
     ------END ------
   managementState: Managed

output in CM:

  odh-ca-bundle.crt: |
    -----BEGIN CERTIFICATE----- 
    MIIFVjCCAz6gAwIBAgIUQ+NxE9izWRRdt86M/TX9b7wFjUUwDQYJKoZIhvcNAQEL 
    ...
    IrrVQJLuM7IjWcmOvFjai57QGfIvWcaMY1q6n6MLsLOaXLoRuBLpDLvPbmyAhykU 
    ------END ------

input

trustedCABundle:
   customCABundle: |-
     -----BEGIN CERTIFICATE-----
     MIIFVjCCAz6gAwIBAgIUQ+NxE9izWRRdt86M/TX9b7wFjUUwDQYJKoZIhvcNAQEL
     ...
     IrrVQJLuM7IjWcmOvFjai57QGfIvWcaMY1q6n6MLsLOaXLoRuBLpDLvPbmyAhykU
     ------END ------

output in CM:

  odh-ca-bundle.crt: |
    -----BEGIN CERTIFICATE-----
    MIIFVjCCAz6gAwIBAgIUQ+NxE9izWRRdt86M/TX9b7wFjUUwDQYJKoZIhvcNAQEL
    ...
    IrrVQJLuM7IjWcmOvFjai57QGfIvWcaMY1q6n6MLsLOaXLoRuBLpDLvPbmyAhykU
    ------END ------

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

codecov · 2024-11-04T17:19:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (incubation@1f5af72). Learn more about missing BASE report.

Additional details and impacted files

@@              Coverage Diff              @@
##             incubation    #1339   +/-   ##
=============================================
  Coverage              ?   19.08%           
=============================================
  Files                 ?       30           
  Lines                 ?     3369           
  Branches              ?        0           
=============================================
  Hits                  ?      643           
  Misses                ?     2657           
  Partials              ?       69

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

- missing newline will cause problem when inject data into configmap - this happens when user use kustomize with their own CA for DSCI, it set to use |- not | - fix lint Signed-off-by: Wen Zhou <[email protected]>

lburgazzoli · 2024-11-06T08:12:39Z

pkg/trustedcabundle/trustedcabundle.go

+	// Adding newline breaker if user input does not have it
+	if !strings.HasSuffix(customCAData, "\n") {
+		customCAData += "\n"
+	}


nit: maybe something we can do unconditionally, like

customCAData = strings.TrimSpace(customCAData) + "\n"

thats nicer

lburgazzoli · 2024-11-06T08:13:12Z

It would be nice to have the tests done manually also as part of the test suite

zdtsw · 2024-11-06T08:38:32Z

It would be nice to have the tests done manually also as part of the test suite

haha, we have an ongoing https://github.com/opendatahub-io/opendatahub-operator/pull/1346/files for this topic

Signed-off-by: Wen Zhou <[email protected]>

lburgazzoli · 2024-11-06T09:49:39Z

It would be nice to have the tests done manually also as part of the test suite

haha, we have an ongoing https://github.com/opendatahub-io/opendatahub-operator/pull/1346/files for this topic

Wonder if that PR should be merged after this and include also the tests you included in the PR description.

zdtsw · 2024-11-06T10:59:04Z

It would be nice to have the tests done manually also as part of the test suite

haha, we have an ongoing https://github.com/opendatahub-io/opendatahub-operator/pull/1346/files for this topic

Wonder if that PR should be merged after this and include also the tests you included in the PR description.

yep, i think so.

lburgazzoli · 2024-11-06T12:48:53Z

lgtm, I'd love to see this merged along with the related tests so we are sure it won't break anything.

I let @VaishnaviHire doing a final review as I probably don't know all the implication

VaishnaviHire · 2024-11-07T00:17:37Z

Wonder if that PR should be merged after this and include also the tests you included in the PR description.

Agree, we can add all the test cases above.

Thanks for the changes @zdtsw

/lgtm

openshift-ci · 2024-11-07T00:17:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VaishnaviHire

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [VaishnaviHire]

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

…1339) * fix: ensure input CAbundle always end with a newline - missing newline will cause problem when inject data into configmap - this happens when user use kustomize with their own CA for DSCI, it set to use |- not | - fix lint Signed-off-by: Wen Zhou <[email protected]> * update: better way to add newline Signed-off-by: Wen Zhou <[email protected]> --------- Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit 656d53e)

* fix: ensure input CAbundle always end with a newline (#1339) * fix: ensure input CAbundle always end with a newline - missing newline will cause problem when inject data into configmap - this happens when user use kustomize with their own CA for DSCI, it set to use |- not | - fix lint Signed-off-by: Wen Zhou <[email protected]> * update: better way to add newline Signed-off-by: Wen Zhou <[email protected]> --------- Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit 656d53e) * fix: add check to remove old kube-rbac-proxy container in modelregistry operator, fixes RHOAIENG-15328 (#1341) * fix: add check to remove old kube-rbac-proxy container in modelregistry operator, fixes RHOAIENG-15328 Signed-off-by: Dhiraj Bokde <[email protected]> * update: remove check in ModelReg code, add it to upgrade logic Signed-off-by: Wen Zhou <[email protected]> --------- Signed-off-by: Dhiraj Bokde <[email protected]> Signed-off-by: Wen Zhou <[email protected]> Co-authored-by: Wen Zhou <[email protected]> (cherry picked from commit 92251e4) * fix: cherry-pick error - remove new logger - dummy: request from devops to verify auto-sync Signed-off-by: Wen Zhou <[email protected]> --------- Signed-off-by: Wen Zhou <[email protected]> Co-authored-by: Dhiraj Bokde <[email protected]>

openshift-ci bot requested review from grdryn and StevenTobin November 4, 2024 16:57

zdtsw removed request for grdryn and StevenTobin November 4, 2024 16:57

zdtsw marked this pull request as draft November 4, 2024 16:57

openshift-ci bot added the do-not-merge/work-in-progress label Nov 4, 2024

zdtsw force-pushed the chore_137 branch from 952776f to 1cb9c64 Compare November 4, 2024 17:13

zdtsw force-pushed the chore_137 branch from 1cb9c64 to 4444682 Compare November 5, 2024 06:57

zdtsw marked this pull request as ready for review November 5, 2024 06:57

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 5, 2024

openshift-ci bot requested review from asanzgom and grdryn November 5, 2024 06:57

zdtsw requested review from VaishnaviHire and removed request for asanzgom and grdryn November 5, 2024 06:57

zdtsw force-pushed the chore_137 branch 2 times, most recently from 726a4bf to 65977fb Compare November 5, 2024 10:48

fix: ensure input CAbundle always end with a newline

78797ec

- missing newline will cause problem when inject data into configmap - this happens when user use kustomize with their own CA for DSCI, it set to use |- not | - fix lint Signed-off-by: Wen Zhou <[email protected]>

zdtsw force-pushed the chore_137 branch from 65977fb to 78797ec Compare November 5, 2024 11:05

lburgazzoli reviewed Nov 6, 2024

View reviewed changes

update: better way to add newline

dd0393f

Signed-off-by: Wen Zhou <[email protected]>

zdtsw added the odh-2.21 label Nov 6, 2024

openshift-ci bot assigned VaishnaviHire Nov 7, 2024

openshift-ci bot added the lgtm label Nov 7, 2024

VaishnaviHire approved these changes Nov 7, 2024

View reviewed changes

openshift-ci bot added the approved label Nov 7, 2024

VaishnaviHire mentioned this pull request Nov 7, 2024

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

Open

5 tasks

openshift-merge-bot bot merged commit 656d53e into opendatahub-io:incubation Nov 7, 2024
10 checks passed

zdtsw mentioned this pull request Nov 8, 2024

[sync]: change from incubation to rhoai + dummy change #1357

Merged

5 tasks

jiridanek mentioned this pull request Nov 14, 2024

RHOAIENG-15743: fix(odh-nbc): trim ca-bundle certData to avoid logging scary messages opendatahub-io/kubeflow#455

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ensure input CAbundle always end with a newline #1339

fix: ensure input CAbundle always end with a newline #1339

zdtsw commented Nov 4, 2024 •

edited

Loading

codecov bot commented Nov 4, 2024

lburgazzoli Nov 6, 2024

zdtsw Nov 6, 2024

lburgazzoli commented Nov 6, 2024

zdtsw commented Nov 6, 2024

lburgazzoli commented Nov 6, 2024

zdtsw commented Nov 6, 2024

lburgazzoli commented Nov 6, 2024

VaishnaviHire commented Nov 7, 2024

openshift-ci bot commented Nov 7, 2024

fix: ensure input CAbundle always end with a newline #1339

fix: ensure input CAbundle always end with a newline #1339

Conversation

zdtsw commented Nov 4, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

codecov bot commented Nov 4, 2024

Codecov Report

lburgazzoli Nov 6, 2024

Choose a reason for hiding this comment

zdtsw Nov 6, 2024

Choose a reason for hiding this comment

lburgazzoli commented Nov 6, 2024

zdtsw commented Nov 6, 2024

lburgazzoli commented Nov 6, 2024

zdtsw commented Nov 6, 2024

lburgazzoli commented Nov 6, 2024

VaishnaviHire commented Nov 7, 2024

openshift-ci bot commented Nov 7, 2024

zdtsw commented Nov 4, 2024 •

edited

Loading