Skip to content
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.

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

keep the drive letter prefix in the path for windows #7094

Conversation

alexrohozneanu
Copy link
Contributor

@alexrohozneanu alexrohozneanu commented Oct 3, 2024

Why the changes in this PR are needed?

Because i fixes issue #6910 and #4521. The windows like paths, prefixed with the drive letter are being stripped.

What are the changes in this PR?

Notes to assist PR review:

Further comments:

@alexrohozneanu alexrohozneanu force-pushed the keep_drive_letter_prefix_windows branch 3 times, most recently from 815a372 to 5dbefd7 Compare October 3, 2024 10:51
Copy link
Contributor

@johanfylling johanfylling left a comment

Choose a reason for hiding this comment

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

I'm afraid this PR constitutes a breaking change that might adversely affect users.

The way OPA separates prefix and file path with a : has an unfortunate syntactic overlap with how drive letters are specified in Windows paths.

On Windows, to specify an absolute path that includes a drive letter, the file:///c:/a/b/c format should be used.

loader/loader_test.go Outdated Show resolved Hide resolved
@alexrohozneanu alexrohozneanu force-pushed the keep_drive_letter_prefix_windows branch from 5dbefd7 to 03d4654 Compare October 8, 2024 08:54
Copy link

netlify bot commented Oct 8, 2024

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 545d9f1
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/6707b7ec125b1600088275b3
😎 Deploy Preview https://deploy-preview-7094--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@alexrohozneanu alexrohozneanu force-pushed the keep_drive_letter_prefix_windows branch from 03d4654 to 1c148b5 Compare October 8, 2024 08:59
Signed-off-by: Alex Rohozneanu <[email protected]>
johanfylling
johanfylling previously approved these changes Oct 9, 2024
Copy link
Contributor

@johanfylling johanfylling left a comment

Choose a reason for hiding this comment

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

Nice and clean solution! 😃

cmd/run.go Outdated Show resolved Hide resolved
Signed-off-by: Alex Rohozneanu <[email protected]>
@johanfylling
Copy link
Contributor

I noticed an issue with these changes that we need to fix before this can be merged into main. Once loaded, the TLS cert, private key, and CA pool files are continually checked for changes on disk. Your changes unfortunately only accounts for the first load, and not subsequent reloads. You can test this by either changing one of the specified cert/key material files after starting the OPA server; or by additionally adding the --tls-cert-refresh-period flag, that'll force a periodic reload of the files (e.g. --tls-cert-refresh-period 5s for a 5 second reload period).

I think what needs to change to account for this is to also set the params.rt.CertificateFile, params.rt.CertificateKeyFile, and params.rt.CertPoolFile properties to the cleaned-up file paths here. It looks like these fields will then propagate to the reloading mechanism. Easiest is probably to move the fileurl.Clean() calls to initRuntime(), store the result in local variables that we then assign to the params.rt fields and pass to loadCertificate() and loadCertPool().

Copy link
Contributor

@johanfylling johanfylling left a comment

Choose a reason for hiding this comment

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

Thank you, great work!

@johanfylling johanfylling merged commit 20bb002 into open-policy-agent:main Oct 10, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants