Refactor config parsing, refactor state.from_charm for complexity, ad…

…d unit tests
canonical · Feb 1, 2024 · d75af60 · d75af60
1 parent 2e9bf3a
commit d75af60
Show file tree

Hide file tree

Showing 6 changed files with 101 additions and 28 deletions.
diff --git a/config.yaml b/config.yaml
@@ -18,14 +18,15 @@ options:
       Plugins installed by the user and their dependencies will be removed automatically if not on
       the list. Included plugins are not automatically installed.
     default: "bazaar,blueocean,dependency-check-jenkins-plugin,docker-build-publish,git,kubernetes,ldap,matrix-combinations-parameter,oic-auth,openid,pipeline-groovy-lib,postbuildscript,rebuild,reverse-proxy-auth-plugin,ssh-agent,thinBackup"
-  external-url:
+  remoting-external-url:
     type: string
     description: >
       Configure the charm to use this URL when establishing relations with agent charms.
       This is useful when connecting agents from outside of the charm's Kubernetes cluster.
-      It is assumed that this url is reachable to the agents.
+      It is assumed that this url is reachable to the agents. A schema (http:// or https://)
+      is required
     default: ""
-  agent-enable-websocket:
+  remoting-enable-websocket:
     type: boolean
     description: >
       Configure inbound agents to use Websocket and skip TCP port 50000.

diff --git a/src/agent.py b/src/agent.py
@@ -80,7 +80,7 @@ def _on_deprecated_agent_relation_joined(self, event: ops.RelationJoinedEvent) -
             event.defer()
             return
 
-        enable_websocket = bool(self.state.agent_enable_websocket)
+        enable_websocket = bool(self.state.remoting_config.enable_websocket)
         self.charm.unit.status = ops.MaintenanceStatus("Adding agent node.")
         try:
             jenkins.add_agent_node(
@@ -95,10 +95,11 @@ def _on_deprecated_agent_relation_joined(self, event: ops.RelationJoinedEvent) -
             self.charm.unit.status = ops.BlockedStatus(f"Jenkins API exception. {exc=!r}")
             return
 
+        configured_remoting_external_url = self.state.remoting_config.external_url
         jenkins_url = (
             f"http://{host}:{jenkins.WEB_PORT}"
-            if self.state.external_url
-            else str(self.state.external_url)
+            if not configured_remoting_external_url
+            else str(configured_remoting_external_url)
         )
         event.relation.data[self.model.unit].update(
             AgentRelationData(url=jenkins_url, secret=secret)
@@ -131,7 +132,7 @@ def _on_agent_relation_joined(self, event: ops.RelationJoinedEvent) -> None:
             event.defer()
             return
 
-        enable_websocket = bool(self.state.agent_enable_websocket)
+        enable_websocket = bool(self.state.remoting_config.enable_websocket)
         self.charm.unit.status = ops.MaintenanceStatus("Adding agent node.")
         try:
             jenkins.add_agent_node(
@@ -145,10 +146,11 @@ def _on_agent_relation_joined(self, event: ops.RelationJoinedEvent) -> None:
             self.charm.unit.status = ops.BlockedStatus(f"Jenkins API exception. {exc=!r}")
             return
 
+        configured_remoting_external_url = self.state.remoting_config.external_url
         jenkins_url = (
             f"http://{host}:{jenkins.WEB_PORT}"
-            if self.state.external_url
-            else str(self.state.external_url)
+            if not configured_remoting_external_url
+            else str(configured_remoting_external_url)
         )
         event.relation.data[self.model.unit].update(
             {

diff --git a/src/jenkins.py b/src/jenkins.py
@@ -521,6 +521,7 @@ def _get_node_config(
         agent_meta: The Jenkins agent metadata to create the node from.
         container: The Jenkins workload container.
         host: The Jenkins server ip address for direct agent tunnel connection.
+        enable_websocket: Whether to use websocket for inbound agent connections.
 
     Returns:
         A dictionary mapping of agent configuration values.
@@ -542,7 +543,7 @@ def _get_node_config(
     meta = json.loads(attribs["json"])
     # Websocket is mutually exclusive with tunnel connect through
     if enable_websocket:
-        meta["enable-websocket"] = enable_websocket
+        meta["launcher"]["webSocket"] = enable_websocket
     else:
         # the field can either take "HOST:PORT", ":PORT", or "HOST:"
         meta["launcher"]["tunnel"] = f"{host}:"
@@ -562,6 +563,7 @@ def add_agent_node(
         agent_meta: The Jenkins agent metadata to create the node from.
         container: The Jenkins workload container.
         host: The Jenkins server ip address for direct agent tunnel connection.
+        enable_websocket: Whether to use websocket for inbound agent connections.
 
     Raises:
         JenkinsError: if an error occurred running groovy script creating the node.

diff --git a/src/state.py b/src/state.py
@@ -227,6 +227,40 @@ def from_env(cls) -> typing.Optional["ProxyConfig"]:
         )
 
 
+class RemotingConfig(BaseModel):
+    """Configuration for inbound agent connections.
+
+    Attributes:
+        external_url: External URL for inbound agent connections.
+        enable_websocket: Use websocket for inbound agent connections.
+    """
+
+    external_url: typing.Optional[AnyHttpUrl]
+    enable_websocket: StrictBool
+
+    @classmethod
+    def from_config(cls, config: ops.ConfigData) -> "RemotingConfig":
+        """Instantiate RemotingConfig from juju charm config data.
+
+        Args:
+            config: the charm's config data
+
+        Returns:
+            RemotingConfig with validated attributes.
+        """
+        config_remoting_external_url = config.get("remoting-external-url")
+        external_url = (
+            tools.parse_obj_as(AnyHttpUrl, config_remoting_external_url)
+            if config_remoting_external_url
+            else None
+        )
+        enable_websocket = bool(config.get("remoting-enable-websocket"))
+        return cls(
+            external_url=external_url,
+            enable_websocket=enable_websocket,
+        )
+
+
 @dataclasses.dataclass(frozen=True)
 class State:
     """The Jenkins k8s operator charm state.
@@ -238,8 +272,7 @@ class State:
             deprecated agent relation.
         proxy_config: Proxy configuration to access Jenkins upstream through.
         plugins: The list of allowed plugins to install.
-        external_url: Configured external url for inbound agents.
-        agent_enable_websocket: Use websocket for inbound agents.
+        remoting_config: Configuration for inbound agents.
 
     """
 
@@ -250,8 +283,7 @@ class State:
     ]
     proxy_config: typing.Optional[ProxyConfig]
     plugins: typing.Optional[typing.Iterable[str]]
-    external_url: typing.Optional[AnyHttpUrl]
-    agent_enable_websocket: StrictBool
+    remoting_config: RemotingConfig
 
     @classmethod
     def from_charm(cls, charm: ops.CharmBase) -> "State":
@@ -268,19 +300,19 @@ def from_charm(cls, charm: ops.CharmBase) -> "State":
             CharmRelationDataInvalidError: if invalid relation data was received.
             CharmIllegalNumUnitsError: if more than 1 unit of Jenkins charm is deployed.
         """
-        time_range_str = charm.config.get("restart-time-range")
-        external_url = charm.config.get("external-url")
-        agent_enable_websocket = charm.config.get("agent-enable-websocket")
-        if time_range_str:
-            try:
+        try:
+            remoting_config = RemotingConfig.from_config(config=charm.config)
+            time_range_str = charm.config.get("restart-time-range")
+            if time_range_str:
                 restart_time_range = Range.from_str(time_range_str)
-            except InvalidTimeRangeError as exc:
-                logger.error("Invalid config value for restart-time-range, %s", exc)
-                raise CharmConfigInvalidError(
-                    "Invalid config value for restart-time-range."
-                ) from exc
-        else:
-            restart_time_range = None
+            else:
+                restart_time_range = None
+        except InvalidTimeRangeError as exc:
+            logger.error("Invalid config value for restart-time-range, %s", exc)
+            raise CharmConfigInvalidError("Invalid config value for restart-time range.") from exc
+        except ValidationError as exc:
+            logger.error("Invalid charm configuration, %s", exc)
+            raise CharmConfigInvalidError("Invalid charm configuration.") from exc
 
         try:
             agent_relation_meta_map = _get_agent_meta_map_from_relation(
@@ -315,6 +347,5 @@ def from_charm(cls, charm: ops.CharmBase) -> "State":
             deprecated_agent_relation_meta=deprecated_agent_meta_map,
             plugins=plugins,
             proxy_config=proxy_config,
-            external_url=tools.parse_obj_as(AnyHttpUrl, external_url) or "",
-            agent_enable_websocket=agent_enable_websocket,
+            remoting_config=remoting_config,
         )
diff --git a/tests/unit/test_jenkins.py b/tests/unit/test_jenkins.py
@@ -602,6 +602,7 @@ def test_add_agent_node_fail(
             state.AgentMeta(executors="3", labels="x86_64", name="agent_node_0"),
             container,
             host=mock_ip_addr,
+            enable_websocket=False,
         )
 
 
@@ -620,6 +621,7 @@ def test_add_agent_node_already_exists(
         state.AgentMeta(executors="3", labels="x86_64", name="agent_node_0"),
         container,
         host=mock_ip_addr,
+        enable_websocket=False,
     )
 
 
@@ -638,6 +640,26 @@ def test_add_agent_node(
         state.AgentMeta(executors="3", labels="x86_64", name="agent_node_0"),
         container,
         host=mock_ip_addr,
+        enable_websocket=False,
+    )
+
+
+@pytest.mark.usefixtures("patch_jenkins_node")
+def test_add_agent_node_websocket(
+    container: ops.Container, mock_client: MagicMock, mock_ip_addr: IPv4Address
+):
+    """
+    arrange: given a mocked jenkins client.
+    act: when add_agent is called.
+    assert: no exception is raised.
+    """
+    mock_client.create_node_with_config.return_value = MagicMock(spec=jenkins.Node)
+
+    jenkins.add_agent_node(
+        state.AgentMeta(executors="3", labels="x86_64", name="agent_node_0"),
+        container,
+        host=mock_ip_addr,
+        enable_websocket=True,
     )
 
 

diff --git a/tests/unit/test_state.py b/tests/unit/test_state.py
@@ -169,3 +169,18 @@ def test_invalid_num_units(mock_charm: MagicMock):
 
     with pytest.raises(state.CharmIllegalNumUnitsError):
         state.State.from_charm(mock_charm)
+
+
+def test_remotingconfig_invalid(mock_charm: MagicMock):
+    """
+    arrange: given a mock charm with invalid remoting configuration.
+    act: when charm state is initialized.
+    assert: CharmConfigInvalidError is raised.
+    """
+    mock_charm.config = {
+        "remoting-external-url": "invalid",
+        "remoting-enable-websocket": "invalid",
+    }
+
+    with pytest.raises(state.CharmConfigInvalidError):
+        state.State.from_charm(mock_charm)