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

Ignore setting OCSP env if already set #1199

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

Conversation

concaf
Copy link

@concaf concaf commented Aug 19, 2024

Description

Currently, if the user sets the OCSP cache server URL

SF_OCSP_RESPONSE_CACHE_SERVER_URL

it is never respected and always overwritten.

This commit only sets the OCSP env vars if they are not already set by the user.

Checklist

  • Created tests which fail without the change (if possible)
  • Extended the README / documentation, if necessary

Currently, if the user sets the OCSP cache server URL
```
SF_OCSP_RESPONSE_CACHE_SERVER_URL
```
it is never respected and always overwritten.

This commit only sets the OCSP env vars if they are not already set by the
user.
@concaf concaf requested a review from a team as a code owner August 19, 2024 09:11
Copy link

github-actions bot commented Aug 19, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@concaf
Copy link
Author

concaf commented Aug 19, 2024

I have read the CLA Document and I hereby sign the CLA

@sfc-gh-dheyman-1
Copy link
Collaborator

Hi @concaf. Thanks for your suggestion.

The thing is that apart from cache URL environment variable, there are also some others that have to be set apart from it (mainly Privatelink related). Also another issue with your solution is that it might introduce some unexpected behaviour when driver is as a multi-user instance.

Could you explain your use-case?

@concaf
Copy link
Author

concaf commented Oct 10, 2024

@sfc-gh-dheyman-1 my use case is that our codebase is deployed on an internal cluster that does not allow reaching out to port 80 (http://), so I need to override it to connect over HTTPS - but when I set SF_OCSP_RESPONSE_CACHE_SERVER_URL it's not respected and still uses the default cache server URL (https://ocsp.snowflakecomputing.com/ocsp_response_cache.json)

return nil, err

// only set OCSP envs if not already set
if _, set := os.LookupEnv(cacheServerURLEnv); !set {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you move this check to ocsp.go to setupOCSPEnvVars function?

@sfc-gh-dheyman-1
Copy link
Collaborator

Hi again @concaf. Sorry for the late response. I discussed your case with my colleagues and we reached agreement that your solution won't interfere with current driver behaviour and if this actually helps you, let's add it :)
I've just added one minor comment.

@sfc-gh-pfus
Copy link
Collaborator

Hi @concaf ! Are you planning to continue working on it?

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.

3 participants