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

docs(config): add descriptions to all configuration variables and tests #682

Closed
Show file tree
Hide file tree
Changes from 2 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
73 changes: 57 additions & 16 deletions packages/opal-client/opal_client/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,22 @@ class EngineLogFormat(str, Enum):
class OpalClientConfig(Confi):
# opa client (policy store) configuration
POLICY_STORE_TYPE = confi.enum(
"POLICY_STORE_TYPE", PolicyStoreTypes, PolicyStoreTypes.OPA
"POLICY_STORE_TYPE",
PolicyStoreTypes,
PolicyStoreTypes.OPA,
description="The type of policy store to use (e.g., OPA, Cedar)"
)
POLICY_STORE_URL = confi.str(
"POLICY_STORE_URL",
"http://localhost:8181",
description="URL of the policy store"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is exactly what written on the key, please explain more...

)
POLICY_STORE_URL = confi.str("POLICY_STORE_URL", "http://localhost:8181")

POLICY_STORE_AUTH_TYPE = confi.enum(
"POLICY_STORE_AUTH_TYPE", PolicyStoreAuth, PolicyStoreAuth.NONE
"POLICY_STORE_AUTH_TYPE",
PolicyStoreAuth,
PolicyStoreAuth.NONE,
description="Authentication type for the policy store"
)
POLICY_STORE_AUTH_TOKEN = confi.str(
"POLICY_STORE_AUTH_TOKEN",
Expand Down Expand Up @@ -132,7 +142,11 @@ def load_policy_store():
# opa runner configuration (OPA can optionally be run by OPAL) ----------------

# whether or not OPAL should run OPA by itself in the same container
INLINE_OPA_ENABLED = confi.bool("INLINE_OPA_ENABLED", True)
INLINE_OPA_ENABLED = confi.bool(
"INLINE_OPA_ENABLED",
True,
description="Whether to run OPA inline within OPAL"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even the code comment has more description of this description. The task is to write descriptions of the variables, not just mark them as solved. Please do some understanding work around OPAL, and explain the configuration variables in a concise but descriptive way.

)

# if inline OPA is indeed enabled, user can pass cli options
# (configuration) that affects how OPA will run
Expand All @@ -144,13 +158,20 @@ def load_policy_store():
)

INLINE_OPA_LOG_FORMAT: EngineLogFormat = confi.enum(
"INLINE_OPA_LOG_FORMAT", EngineLogFormat, EngineLogFormat.NONE
"INLINE_OPA_LOG_FORMAT",
EngineLogFormat,
EngineLogFormat.NONE,
description="Specifies the log format for inline OPA. Options are: none, minimal, http, or full."
)

# Cedar runner configuration (Cedar-engine can optionally be run by OPAL) ----------------

# whether or not OPAL should run the Cedar agent by itself in the same container
INLINE_CEDAR_ENABLED = confi.bool("INLINE_CEDAR_ENABLED", True)
INLINE_CEDAR_ENABLED = confi.bool(
"INLINE_CEDAR_ENABLED",
True,
description="Whether to run Cedar inline within OPAL"
)

# if inline Cedar is indeed enabled, user can pass cli options
# (configuration) that affects how the agent will run
Expand All @@ -162,19 +183,31 @@ def load_policy_store():
)

INLINE_CEDAR_LOG_FORMAT: EngineLogFormat = confi.enum(
"INLINE_CEDAR_LOG_FORMAT", EngineLogFormat, EngineLogFormat.NONE
"INLINE_CEDAR_LOG_FORMAT",
EngineLogFormat,
EngineLogFormat.NONE,
description="Specifies the log format for inline Cedar. Options are: none, minimal, http, or full."
)

# configuration for fastapi routes
ALLOWED_ORIGINS = ["*"]

# general configuration for pub/sub clients
KEEP_ALIVE_INTERVAL = confi.int("KEEP_ALIVE_INTERVAL", 0)
KEEP_ALIVE_INTERVAL = confi.int(
"KEEP_ALIVE_INTERVAL",
0,
description="Interval for keep-alive messages in seconds (0 to disable)"
)

# Opal Server general configuration -------------------------------------------

# opal server url
SERVER_URL = confi.str("SERVER_URL", "http://localhost:7002", flags=["-s"])
SERVER_URL = confi.str(
"SERVER_URL",
"http://localhost:7002",
description="URL of the OPAL server",
flags=["-s"]
)
# opal server pubsub url
OPAL_WS_ROUTE = "/ws"
SERVER_WS_URL = confi.str(
Expand All @@ -184,17 +217,20 @@ def load_policy_store():
"http", "ws"
)
),
description="WebSocket URL of the OPAL server"
)
SERVER_PUBSUB_URL = confi.str(
"SERVER_PUBSUB_URL", confi.delay("{SERVER_WS_URL}" + f"{OPAL_WS_ROUTE}")
"SERVER_PUBSUB_URL",
confi.delay("{SERVER_WS_URL}" + f"{OPAL_WS_ROUTE}"),
description="PubSub URL of the OPAL server"
)

# opal server auth token
CLIENT_TOKEN = confi.str(
"CLIENT_TOKEN",
"THIS_IS_A_DEV_SECRET",
description="opal server auth token",
flags=["-t"],
description="Authentication token for the OPAL server",
flags=["-t"]
)

# client-api server
Expand Down Expand Up @@ -229,7 +265,7 @@ def load_policy_store():
"POLICY_SUBSCRIPTION_DIRS",
["."],
delimiter=":",
description="directories in policy repo we should subscribe to",
description="Directories in the policy repo to subscribe to"
)

# Data updater configuration --------------------------------------------------
Expand All @@ -243,7 +279,7 @@ def load_policy_store():
DATA_TOPICS = confi.list(
"DATA_TOPICS",
[DEFAULT_DATA_TOPIC],
description="Data topics to subscribe to",
description="Data topics to subscribe to"
)

DEFAULT_DATA_SOURCES_CONFIG_URL = confi.str(
Expand Down Expand Up @@ -272,6 +308,7 @@ def load_policy_store():
"headers": {"content-type": "application/json"},
"process_data": False,
},
description="Default configuration for update callbacks, including HTTP method, headers, and data processing options."
)

DEFAULT_UPDATE_CALLBACKS = confi.model(
Expand Down Expand Up @@ -305,7 +342,11 @@ def load_policy_store():

OPA_HEALTH_CHECK_POLICY_PATH = "engine/healthcheck/opal.rego"

Choose a reason for hiding this comment

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

@onyedikachi-david missing here


SCOPE_ID = confi.str("SCOPE_ID", "default", description="OPAL Scope ID")
SCOPE_ID = confi.str(
"SCOPE_ID",
"default",
description="OPAL Scope ID"
)

STORE_BACKUP_PATH = confi.str(
"STORE_BACKUP_PATH",
Expand Down Expand Up @@ -340,4 +381,4 @@ def on_load(self):
self.DATA_UPDATER_CONN_RETRY = self.DATA_STORE_CONN_RETRY


opal_client_config = OpalClientConfig(prefix="OPAL_")
opal_client_config = OpalClientConfig(prefix="OPAL_")
13 changes: 13 additions & 0 deletions packages/opal-client/opal_client/tests/test_config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@

import pytest
from opal_client.config import opal_client_config
from opal_common.confi.types import ConfiEntry

def test_client_config_descriptions():
missing_descriptions = []

for key, entry in opal_client_config.entries.items():
if isinstance(entry, ConfiEntry) and not entry.description:
missing_descriptions.append(key)

assert not missing_descriptions, f"The following config variables are missing descriptions: {', '.join(missing_descriptions)}"
27 changes: 14 additions & 13 deletions packages/opal-common/opal_common/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from opal_common.confi import Confi, confi

_LOG_FORMAT_WITHOUT_PID = "<green>{time}</green> | <blue>{name: <40}</blue>|<level>{level:^6} | {message}</level>\n{exception}"
_LOG_FORMAT_WITH_PID = "<green>{time}</green> | {process} | <blue>{name: <40}</blue>|<level>{level:^6} | {message}</level>\n{exception}"
_LOG_FORMAT_WITH_PID = "<green>{time}</green> | {process} | <blue>{name: <40></blue>|<level>{level:^6} | {message}</level>\n{exception}"

Choose a reason for hiding this comment

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

@onyedikachi-david why > and not }?



class OpalCommonConfig(Confi):
Expand All @@ -16,7 +16,7 @@ class OpalCommonConfig(Confi):
PROCESS_NAME = ""
# Logging
# - Log formatting
LOG_FORMAT_INCLUDE_PID = confi.bool("LOG_FORMAT_INCLUDE_PID", False)
LOG_FORMAT_INCLUDE_PID = confi.bool("LOG_FORMAT_INCLUDE_PID", False, description="Include process ID in log format")
LOG_FORMAT = confi.str(
"LOG_FORMAT",
confi.delay(
Expand Down Expand Up @@ -106,19 +106,19 @@ class OpalCommonConfig(Confi):
# Fetching Providers
# - where to load providers from
FETCH_PROVIDER_MODULES = confi.list(
"FETCH_PROVIDER_MODULES", ["opal_common.fetcher.providers"]
"FETCH_PROVIDER_MODULES", ["opal_common.fetcher.providers"], description="Modules to load fetch providers from"
)

# Fetching engine
# Max number of worker tasks handling fetch events concurrently
FETCHING_WORKER_COUNT = confi.int("FETCHING_WORKER_COUNT", 6)
FETCHING_WORKER_COUNT = confi.int("FETCHING_WORKER_COUNT", 6, description="Number of worker tasks for handling fetch events concurrently")
# Time in seconds to wait on the queued fetch task.
FETCHING_CALLBACK_TIMEOUT = confi.int("FETCHING_CALLBACK_TIMEOUT", 10)
FETCHING_CALLBACK_TIMEOUT = confi.int("FETCHING_CALLBACK_TIMEOUT", 10, description="Timeout in seconds for fetch task callbacks")
# Time in seconds to wait for queuing a new task (if the queue is full)
FETCHING_ENQUEUE_TIMEOUT = confi.int("FETCHING_ENQUEUE_TIMEOUT", 10)
FETCHING_ENQUEUE_TIMEOUT = confi.int("FETCHING_ENQUEUE_TIMEOUT", 10, description="Timeout in seconds for enqueueing new fetch tasks")

GIT_SSH_KEY_FILE = confi.str(
"GIT_SSH_KEY_FILE", str(Path.home() / ".ssh/opal_repo_ssh_key")
"GIT_SSH_KEY_FILE", str(Path.home() / ".ssh/opal_repo_ssh_key"), description="Path to SSH key file for Git operations"
)

# Trust self signed certificates (Advanced Usage - only affects OPAL client) -----------------------------
Expand All @@ -139,11 +139,12 @@ class OpalCommonConfig(Confi):

# security
AUTH_PUBLIC_KEY_FORMAT = confi.enum(
"AUTH_PUBLIC_KEY_FORMAT", EncryptionKeyFormat, EncryptionKeyFormat.ssh
"AUTH_PUBLIC_KEY_FORMAT", EncryptionKeyFormat, EncryptionKeyFormat.ssh, description="Format of the public key used for authentication"
)
AUTH_PUBLIC_KEY = confi.delay(
lambda AUTH_PUBLIC_KEY_FORMAT=None: confi.public_key(
"AUTH_PUBLIC_KEY", default=None, key_format=AUTH_PUBLIC_KEY_FORMAT
"AUTH_PUBLIC_KEY", default=None, key_format=AUTH_PUBLIC_KEY_FORMAT,
description="The public key used for authentication and JWT token verification"
)
)
AUTH_JWT_ALGORITHM = confi.enum(
Expand All @@ -152,15 +153,15 @@ class OpalCommonConfig(Confi):
getattr(JWTAlgorithm, "RS256"),
description="jwt algorithm, possible values: see: https://pyjwt.readthedocs.io/en/stable/algorithms.html",
)
AUTH_JWT_AUDIENCE = confi.str("AUTH_JWT_AUDIENCE", "https://api.opal.ac/v1/")
AUTH_JWT_ISSUER = confi.str("AUTH_JWT_ISSUER", f"https://opal.ac/")
AUTH_JWT_AUDIENCE = confi.str("AUTH_JWT_AUDIENCE", "https://api.opal.ac/v1/", description="Audience claim for JWT tokens")
AUTH_JWT_ISSUER = confi.str("AUTH_JWT_ISSUER", f"https://opal.ac/", description="Issuer claim for JWT tokens")
POLICY_REPO_POLICY_EXTENSIONS = confi.list(
"POLICY_REPO_POLICY_EXTENSIONS",
[".rego"],
description="List of extensions to serve as policy modules",
)

ENABLE_METRICS = confi.bool("ENABLE_METRICS", False)
ENABLE_METRICS = confi.bool("ENABLE_METRICS", False, description="Enable metrics collection")

# optional APM tracing with datadog
ENABLE_DATADOG_APM = confi.bool(
Expand All @@ -176,4 +177,4 @@ class OpalCommonConfig(Confi):
)


opal_common_config = OpalCommonConfig(prefix="OPAL_")
opal_common_config = OpalCommonConfig(prefix="OPAL_")
13 changes: 13 additions & 0 deletions packages/opal-common/opal_common/tests/test_config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import pytest
from opal_common.config import opal_common_config
from opal_common.confi.types import ConfiEntry


def test_common_config_descriptions():

Choose a reason for hiding this comment

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

@onyedikachi-david why duplicate the test code and not use a common shared config_test.py class?

missing_descriptions = []

for key, entry in opal_common_config.entries.items():
if isinstance(entry, ConfiEntry) and not entry.description:
missing_descriptions.append(key)

assert not missing_descriptions, f"The following config variables are missing descriptions: {', '.join(missing_descriptions)}"
Loading
Loading