Skip to content

Add support for SSL with custom certificates for Azure #2620

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

Jump to bottom
Merged
merged 11 commits into from
Oct 6, 2025
Merged

Add support for SSL with custom certificates for Azure #2620

merged 11 commits into from
Oct 6, 2025
+33 −6

Conversation

blublinsky
Copy link
Contributor

Description

Add support for SSL with custom certificates for Azure

Type of change

  • Refactor
  • New feature
  • [ x] Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change

Related Tickets & Documents

  • Related Issue #
    OpenShift Lightspeed ServiceOLS-2112
  • Closes #
    OpenShift Lightspeed ServiceOLS-2112

Checklist before requesting a review

  • [ x] I have performed a self-review of my code.
  • [ x] PR was tested by the customer
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

@openshift-ci openshift-ci bot requested review from tisnik and xrajesh September 26, 2025 10:59
@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 26, 2025
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Sep 26, 2025

Hi @blublinsky. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@onmete
Copy link
Contributor

onmete commented Oct 1, 2025

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 1, 2025
@onmete
Copy link
Contributor

onmete commented Oct 1, 2025

/ok-to-test

@blublinsky
Copy link
Contributor Author

/retest

xrajesh
xrajesh reviewed Oct 1, 2025
View reviewed changes
pdm.lock Outdated
strategy = ["inherit_metadata"]
lock_version = "4.5.0"
content_hash = "sha256:ce8969ef24388dae7b8205727984c23b4e3036d514b43c259a0202e20667b436"
content_hash = "sha256:f52973e650c7b62620102963425c2e51eca278cbb08ac7b396d1ad534db99e3e"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this pdm.lock update part of this PR ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove it tomorrow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could not remove it completely, sorry

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not?

xrajesh
xrajesh reviewed Oct 1, 2025
View reviewed changes
tests/unit/llms/providers/test_azure_openai.py
with open(cert_in_certificates_store_path, "rb") as cert_file:
cert_store.write(cert_file.read())
return cert_store_path

Copy link
Contributor

Choose a reason for hiding this comment

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

@blublinsky What is the use of this fixture , the tests dont use them - Am I missing anything here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, two methods are using it as a parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the reason why unit tests were failing

Copy link
Contributor

Choose a reason for hiding this comment

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

The same fixture is defined in other providers that support custom certs.

This fixture stores the cert permanently into the location. Which is wrong - unit test shouldn't leave artifacts.
But it is out of the scope of this PR to fix it (probably not worth it, given the lcore migration anyway).

@onmete
Copy link
Contributor

onmete commented Oct 2, 2025

/test e2e-ols-cluster

@blublinsky
Copy link
Contributor Author

/retest

@onmete
Copy link
Contributor

onmete commented Oct 2, 2025

/test e2e-ols-cluster

@blublinsky
Copy link
Contributor Author

/retest

1 similar comment
@blublinsky
Copy link
Contributor Author

/retest

@raptorsun
Copy link
Contributor

/lgtm
/approve

e2e test failuires are about data collector, not related to this change. but let's give it another try

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 3, 2025
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Oct 3, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: raptorsun

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2025
@raptorsun
Copy link
Contributor

/retest
for build-source-image task in Red Hat Konflux / lightspeed-service-on-pull-request

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 363a3da and 2 for PR HEAD 376cade in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD acbf2ad and 1 for PR HEAD 376cade in total

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 3, 2025
@blublinsky
Copy link
Contributor Author

/retest

@xrajesh
Copy link
Contributor

xrajesh commented Oct 6, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2025
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 66ee713 and 2 for PR HEAD 2537ac4 in total

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Oct 6, 2025

@blublinsky: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit 73682fc into openshift:main Oct 6, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xrajesh xrajesh xrajesh left review comments

@tisnik tisnik Awaiting requested review from tisnik

+1 more reviewer

@onmete onmete onmete left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@raptorsun raptorsun

@xrajesh xrajesh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@blublinsky @onmete @raptorsun @openshift-ci-robot @xrajesh