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

feat: add support for http_proxy #175

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions charmcraft.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -89,3 +89,15 @@ config:
httpreq_username:
type: string
description: Basic authentication username
http_proxy:
description: URL of the HTTP proxy eg http://proxy.internal:6666, it will set the HTTP_PROXY var in the workload environment
type: string
default: ''
https_proxy:
description: URL of the HTTPS proxy eg http://proxy.internal:6666, it will set the HTTPS_PROXY var in the workload environment
type: string
default: ''
no_proxy:
description: Domains that need to be excluded from proxying no_proxy="test.com,test.co.uk", it is a comma separate list
type: string
default: ''
Comment on lines +92 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

Just like it is done in GitHub Runners charm and other IS charms, let's use the Juju env variables for this.

Reference

Copy link
Author

Choose a reason for hiding this comment

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

I'm happy to change it

although using those env variables might be counterproductive

ideally you have 2 layers, one where u use those by default and another where u can override that behaviour

Copy link
Contributor

Choose a reason for hiding this comment

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

(I placed the same comment on the lego k8s lib, let's discuss it over there)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion, but I see there was a discussion here. And the point was made to keep consistency across charms.
In addition using the juju model env vars would allow us to handle this in the lib only without the need to change any of the Lego charms.

44 changes: 43 additions & 1 deletion lib/charms/lego_base_k8s/v0/lego_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ def _push_csr_to_workload(self, csr: str) -> None:
def _execute_lego_cmd(self) -> bool:
"""Execute lego command in workload container."""
process = self._container.exec(
self._cmd, timeout=300, working_dir="/tmp", environment=self._plugin_config
self._cmd, timeout=300, working_dir="/tmp", environment=self._common_config | self._plugin_config
Copy link
Contributor

Choose a reason for hiding this comment

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

This lib is owned by the lego k8s project. Let's make the change over there, bump the lib version, then pull it here.

Copy link
Author

Choose a reason for hiding this comment

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

)
try:
stdout, error = process.wait_output()
Expand Down Expand Up @@ -316,6 +316,27 @@ def _cmd(self) -> List[str]:
"run",
]

@property
def _common_config(self) -> Dict[str, str]:
"""Common configuration for the command

Used to set the environment context

Returns:
dict[str, str]: common configuration.
"""
common_config = {
"HTTPREQ_ENDPOINT": self._httpreq_endpoint,
}
if self._http_proxy:
common_config["HTTP_PROXY"] = self._http_proxy
if self._https_proxy:
common_config["HTTPS_PROXY"] = self._https_proxy
if self._no_proxy:
common_config["NO_PROXY"] = self._no_proxy

return common_config

@property
@abstractmethod
def _plugin_config(self) -> Dict[str, str]:
Expand Down Expand Up @@ -358,3 +379,24 @@ def _server(self) -> Optional[str]:
if not isinstance(server, str):
return None
return server

@property
def _http_proxy(self) -> Optional[str]:
http_proxy = self.model.config.get("http_proxy", None)
if not isinstance(http_proxy, str):
return None
return http_proxy

@property
def _https_proxy(self) -> Optional[str]:
https_proxy = self.model.config.get("https_proxy", None)
if not isinstance(https_proxy, str):
return None
return https_proxy

@property
def _no_proxy(self) -> Optional[str]:
no_proxy = self.model.config.get("no_proxy", None)
if not isinstance(no_proxy, str):
return None
return no_proxy