Skip to content

feat(core): support AICORE_SERVICE_KEY env var for credentials #58

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.

Sign up for GitHub

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

Open
yamaceay wants to merge 3 commits into
base: main
Choose a base branch
from
+129 −2
Open

feat(core): support AICORE_SERVICE_KEY env var for credentials #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cbc2752
feat(core): support AICORE_SERVICE_KEY env var for credentials
yamaceay Aug 12, 2026
0271d5b
fix: uv lock updated
yamaceay Aug 12, 2026
0de2494
docs: update pydoc3 documentation [skip ci]
sap-ai-sdk-bot[bot] Aug 12, 2026
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
39 changes: 38 additions & 1 deletion packages/core/ai_core_sdk/credentials.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

from ai_core_sdk.helpers import get_home
from ai_core_sdk.helpers.constants import (AI_CORE_PREFIX, AUTH_ENDPOINT_SUFFIX, CONFIG_FILE_ENV_VAR, PROFILE_ENV_VAR,
VCAP_AICORE_SERVICE_NAME, VCAP_SERVICES_ENV_VAR)
SERVICE_KEY_ENV_VAR, VCAP_AICORE_SERVICE_NAME, VCAP_SERVICES_ENV_VAR)
from ai_core_sdk.helpers.logging import get_logger

logger = get_logger()
Expand Down Expand Up @@ -241,6 +241,37 @@ def _str_or_none(value) -> Optional[str]:
return str(value) if value else None


def _parse_service_key(credential_values: List[CredentialsValue]) -> Optional[Callable[[CredentialsValue], Optional[str]]]:

@ZhongpinWang ZhongpinWang Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[req] The function is called _parse_service_key. Based on its name, nobody would expect it to return a callable. Function name should really be speaking. The actual json parsing part is like a precondition to the function.

Also credential_values is never used. This function is reading SERVICE_KEY_ENV_VAR from env.

This function is more like create_service_key_getter or something.

@ZhongpinWang ZhongpinWang Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[req] Consider separating the JSON parsing / validation part out into a different function.

"""Return a source getter for AICORE_SERVICE_KEY if the env var is set and valid JSON, else None.

AICORE_SERVICE_KEY is expected to be the raw JSON string of a BTP service key, i.e. the

@ZhongpinWang ZhongpinWang Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pp] Not entirely. Service key and the credentials object from the VCAP variable are still... let's say not entirely the same. It is just people tend to provide the similar naming. But different services have different VCAP naming convention. The key might not even be called as credentials. Better use the word transform or map.

``credentials`` object from a VCAP_SERVICES binding without the outer envelope. Credential
fields are extracted using the ``vcap_key`` paths already defined on each ``CredentialsValue``,
but with the leading ``'credentials'`` segment stripped (same as the CLI's load_service_key).
"""
raw = os.environ.get(SERVICE_KEY_ENV_VAR)

@ZhongpinWang ZhongpinWang Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[req] raw is a bad name and not speaking. Consider calling it service_key_json_string.

if not raw:
return None

@mwien mwien Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It could be an option to not return here but write an empty dict into service_key and then return a Callable instead of Optional[Callable]. Could make the code below more consistent.

try:
service_key = json.loads(raw)
except json.JSONDecodeError as exc:
raise ValueError(
f"{SERVICE_KEY_ENV_VAR} is set but contains invalid JSON: {exc}"
) from exc

def _get(cv: CredentialsValue) -> Optional[str]:
if not cv.vcap_key:

@ZhongpinWang ZhongpinWang Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[req] (maybe for the future) After struggling and looking into the codebase, I finally understand what is a vcap_key........ It is a JSON object access path starting from credentials stored in the form of a tuple...

I know it is not super relevant to this PR. But refactoring needed... And allow me to just put my thoughts here as a reference. I added this comment to the refactoring BLI.

I think we should really consider renaming few stuff.

  • "Key" means a single property name
  • "Access Path" means obj.credentials.clientid
  • "Value" means the content of that key value pair, what we get when calling get(key)

Also it is pretty hard to understand what CredentialsValue actually means. We call the whole object inside VCAP_SERVICES['aicore'][0] a service binding.

And why do we define object schema in such a complicated way. Can we not just define the data class of the credentials object such as AiCoreCredentials? Then transform_fn takes an AI CORE service binding object and map it to AiCoreDestination? (I used the term Destination as it is pretty much the transformed object)

@mwien to give you an overview of how VCAP_SERVICES could actually look like:

{
	"VCAP_SERVICES": {
		"aicore": [
			{
				"label": "aicore",
				"provider": null,
				"plan": "<plan>",
				"name": "default_aicore",
				"tags": [],
				"instance_guid": "<some uuid>",
				"instance_name": "default_aicore",
				"binding_guid": "<some_other_uuid>",
				"binding_name": null,
				"credentials": {
					"serviceurls": {
						"AI_API_URL": "https://api.ai.......ml.hana.ondemand.com"
					},
					"appname": "<appname>",
					"clientid": "<clientid>",
					"clientsecret": "<clientsecret>",
					"identityzone": "<subdomain of the subaccount>",
					"identityzoneid": "<tenant id / subaccount id>",
					"url": "<auth_url>"
				},
				"syslog_drain_url": null,
				"volume_mounts": []
			}
		],
        // ...
    }
}

return None
# vcap_key is e.g. ('credentials', 'clientid') — drop the 'credentials' prefix

@marikaner marikaner Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[pp] I had a hard time understanding what this comment was trying to tell me. Information that helped me understand:

  1. vcap_key is a tuple (due to the comment I somehow thought it could be "credentials" or "clientid" or so)
  2. the tuple always starts with an element "credentials"
  3. we're dropping the first element, not a "prefix"

This might be partially me as a python noob, but consider sharpening the wording a bit (at least something like that: "prefix" => leading "credentials" element).

@ZhongpinWang ZhongpinWang Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# vcap_key is e.g. ('credentials', 'clientid') — drop the 'credentials' prefix
# `vcap_key` is a tuple representing the access path to properties such as
# `clientid` and `clientsecret` in the `aicore` service binding, e.g.
# `('credentials', 'clientid')`. Skip the leading path element `credentials` to
# access the nested value in the JSON object.

What about this.

key_path = cv.vcap_key[1:]
try:
return _str_or_none(get_nested_value(service_key, key_path))
except KeyError:
return None

return _get


def fetch_credentials(profile: str = None, credential_values: List[CredentialsValue] = CORE_CREDENTIAL_VALUES,
validate: bool = True, **kwargs) -> Dict[str, str]:
"""
Expand All @@ -261,11 +292,17 @@ def fetch_credentials(profile: str = None, credential_values: List[CredentialsVa
except KeyError:
vcap_service = None

service_key_getter = _parse_service_key(credential_values)

@mwien mwien Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If the change suggested in line 304 is done, this could be moved to an inline call to _parse_service_key there


sources = [
Source("kwargs",
lambda cv: _str_or_none(kwargs.get(cv.name))),
Source("environment variables",
lambda cv: _str_or_none(os.environ.get(f'{AI_CORE_PREFIX}_{cv.name.upper()}'))),
*(
[Source(SERVICE_KEY_ENV_VAR, service_key_getter)]
if service_key_getter is not None else []

@mwien mwien Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe an option to keep this more consistent would be to have service_key_getter be a Callable instead of Optional[Callable] and have it always return None in case the env var doesn't exist. See comment above.

),
Source("config file",
lambda cv: _str_or_none(config.get(f'{AI_CORE_PREFIX}_{cv.name.upper()}'))),
Source("VCAP service",
Expand Down
1 change: 1 addition & 0 deletions packages/core/ai_core_sdk/helpers/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
DEFAULT_HOME_PATH = os.path.join(os.path.expanduser('~'), '.aicore')
HOME_PATH_ENV_VAR = f'{AI_CORE_PREFIX}_HOME'
PROFILE_ENV_VAR = f'{AI_CORE_PREFIX}_PROFILE'
SERVICE_KEY_ENV_VAR = f'{AI_CORE_PREFIX}_SERVICE_KEY'

@marikaner marikaner Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[pp] I think AICORE_SERVICE_KEY would be a simpler name and easier to find throughout the code. That said, the other variables follow a different naming pattern and changing this would introduce an inconsistency. Merely personal perference though.

@ZhongpinWang ZhongpinWang Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[q/pp] I see that AI_CORE_PREFIX is a constant equal to string AICORE.

Maybe a follow-up: Overall I find the current constant list a bit too complex.

I understand the idea behind "having constant strings defined as variable to avoid typo" methodology here. Perhaps follow what React does: Similar to using REACT_APP_ as prefix for all env var, we can rename all these env vars into

  • ENV_VAR_AICORE_HOME
  • ENV_VAR_AICORE_PROFILE
  • ENV_VAR_AICORE_SERVICE_KEY
  • ...

It improves the auto completion experience.

Also I would prefer inline AI_CORE_PREFIX with AICORE. It is just six characters, and whenever it is used, it is combined with other stuff like {AI_CORE_PREFIX}_CERT_FILE_PATH which I think typing the rest part is more error-prone :)

It doesn't have to be part of this PR though. But since we might have a lot of things needing refactoring, I will create a refactoring BLI and collect some ideas incl. this one.

@ZhongpinWang ZhongpinWang Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Btw, some old tradition for PR review:

q: Question (Something I am not sure about)
pp: Personal preference (Not a must but nice to have)
req: Request changes (This should be changed)

Some of us this kind of form to indicate the type of review comments for better communication.

VCAP_AICORE_SERVICE_NAME = 'aicore'
VCAP_SERVICES_ENV_VAR = 'VCAP_SERVICES'

Expand Down
1 change: 1 addition & 0 deletions packages/core/docs/ai_core_sdk.credentials.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@
<strong>NoDefault</strong> = NoDefault<br>
<strong>Optional</strong> = typing.Optional<br>
<strong>PROFILE_ENV_VAR</strong> = 'AICORE_PROFILE'<br>
<strong>SERVICE_KEY_ENV_VAR</strong> = 'AICORE_SERVICE_KEY'<br>
<strong>Tuple</strong> = typing.Tuple<br>
<strong>VCAP_AICORE_SERVICE_NAME</strong> = 'aicore'<br>
<strong>VCAP_SERVICES_ENV_VAR</strong> = 'VCAP_SERVICES'<br>
Expand Down
1 change: 1 addition & 0 deletions packages/core/docs/ai_core_sdk.helpers.constants.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@
<strong>DEFAULT_HOME_PATH</strong> = '/home/runner/.aicore'<br>
<strong>HOME_PATH_ENV_VAR</strong> = 'AICORE_HOME'<br>
<strong>PROFILE_ENV_VAR</strong> = 'AICORE_PROFILE'<br>
<strong>SERVICE_KEY_ENV_VAR</strong> = 'AICORE_SERVICE_KEY'<br>
<strong>VCAP_AICORE_SERVICE_NAME</strong> = 'aicore'<br>
<strong>VCAP_SERVICES_ENV_VAR</strong> = 'VCAP_SERVICES'</td></tr></table>
</body></html>

@ZhongpinWang ZhongpinWang Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pp] Consider not making unnecessary changes.

Also it is a good practice to always leave an empty line at the end of a file (coming from a unix world).

7 changes: 7 additions & 0 deletions packages/core/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,17 @@ dev = [
"pyhamcrest==2.1.0",
"pytest-dotenv>=0.5.2",
]
docs = [
"sphinx<9.0.0",
"sphinxawesome-theme",
]
Comment on lines +40 to +43

@marikaner marikaner Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[req] this is unrelated to this change, right?


[tool.pytest.ini_options]
testpaths = ["tests"]
norecursedirs = ["integration_tests"]
# Prevent pytest-dotenv from loading the repo-root .env (which contains real credentials)

@ZhongpinWang ZhongpinWang Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[q/req] Is there anywhere documented that we should store .env in the root (for development I guess)? Or is it just your local setup, and you stored .env at the root?

Later we will have a sample server folder / package, which will be used for trying out purposes and we can put .env in that folder? cc @mwien

# into unit tests. Integration tests load it explicitly via conftest.
env_files = []

[project.scripts]
aicore = "ai_core_sdk.cli:cli"
Expand Down
73 changes: 72 additions & 1 deletion packages/core/tests/ai_core_client/test_credentials.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
init_conf, CORE_CREDENTIAL_VALUES,
)
from ai_core_sdk.helpers.constants import (AI_CORE_PREFIX, HOME_PATH_ENV_VAR, PROFILE_ENV_VAR, VCAP_SERVICES_ENV_VAR,
VCAP_AICORE_SERVICE_NAME, CONFIG_FILE_ENV_VAR)
VCAP_AICORE_SERVICE_NAME, CONFIG_FILE_ENV_VAR, SERVICE_KEY_ENV_VAR)

@marikaner marikaner Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[pp] Not related to this PR, but possibly a nice new backlog item: I noticed that the variables are imported in a different order here. So this does not seem to be covered by linting. Possibly worth improving, if you think so too.


VCAP_SERVICE_DICT = {
VCAP_AICORE_SERVICE_NAME: [{
Expand Down Expand Up @@ -290,6 +290,77 @@ def test_init_conf_permission_error(self, mock_logger):
# Restore permissions for cleanup in teardown
config_file.chmod(0o644)

@patch('ai_core_sdk.credentials.logger')
def test_fetch_credentials_from_service_key(self, mock_logger):
mock_logger.debug = MagicMock()

service_key = {
'clientid': 'sk-client-id',
'clientsecret': 'sk-client-secret',
'url': 'https://sk-auth-url',
'serviceurls': {'AI_API_URL': 'https://sk-api-url'},
}
with patch.dict(os.environ, {SERVICE_KEY_ENV_VAR: json.dumps(service_key)}):

@marikaner marikaner Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[req] I believe this should be AICORE_SERVICE_KEY?

Suggested change
with patch.dict(os.environ, {SERVICE_KEY_ENV_VAR: json.dumps(service_key)}):
with patch.dict(os.environ, {AICORE_SERVICE_KEY: json.dumps(service_key)}):

Also, if I am not mistaken, why do the tests pass? 🧐

@ZhongpinWang ZhongpinWang Aug 13, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@marikaner It was also quite confusing to me.. If I understand it right, SERVICE_KEY_ENV_VAR looks like the constant that stores AICORE_SERVICE_KEY as the value.

I suggested above to change those constant names in the future. At least to something like ENV_VAR_AICORE_SERVICE_KEY (if we want to avoid typing strings everywhere)

credentials = fetch_credentials()

self.assertEqual(credentials['client_id'], 'sk-client-id')
self.assertEqual(credentials['client_secret'], 'sk-client-secret')
self.assertEqual(credentials['auth_url'], 'https://sk-auth-url/oauth/token')
self.assertEqual(credentials['base_url'], 'https://sk-api-url/v2')
mock_logger.debug.assert_any_call(f"Using credentials from: {SERVICE_KEY_ENV_VAR}")

@patch('ai_core_sdk.credentials.logger')
def test_fetch_credentials_from_service_key_x509(self, mock_logger):
mock_logger.debug = MagicMock()

service_key = {
'clientid': 'sk-client-id',
'certurl': 'https://sk-cert-url',
'certificate': 'sk-cert-content',
'key': 'sk-key-content',
'serviceurls': {'AI_API_URL': 'https://sk-api-url'},
}
with patch.dict(os.environ, {SERVICE_KEY_ENV_VAR: json.dumps(service_key)}):
credentials = fetch_credentials()

self.assertEqual(credentials['client_id'], 'sk-client-id')
self.assertEqual(credentials['cert_str'], 'sk-cert-content')
self.assertEqual(credentials['key_str'], 'sk-key-content')
self.assertEqual(credentials['auth_url'], 'https://sk-cert-url/oauth/token')
self.assertEqual(credentials['base_url'], 'https://sk-api-url/v2')
mock_logger.debug.assert_any_call(f"Using credentials from: {SERVICE_KEY_ENV_VAR}")

@patch('ai_core_sdk.credentials.logger')
def test_service_key_lower_precedence_than_env_vars(self, mock_logger):
mock_logger.debug = MagicMock()

service_key = {
'clientid': 'sk-client-id',
'clientsecret': 'sk-client-secret',
'url': 'https://sk-auth-url',
'serviceurls': {'AI_API_URL': 'https://sk-api-url'},
}
with patch.dict(os.environ, {
SERVICE_KEY_ENV_VAR: json.dumps(service_key),
f'{AI_CORE_PREFIX}_CLIENT_ID': 'env-client-id',
f'{AI_CORE_PREFIX}_CLIENT_SECRET': 'env-client-secret',
f'{AI_CORE_PREFIX}_AUTH_URL': 'https://env-auth-url',
f'{AI_CORE_PREFIX}_BASE_URL': 'https://env-base-url',
Comment on lines +347 to +348

@marikaner marikaner Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[pp] If you're not checking those, maybe it is not needed to add them here?

}):
credentials = fetch_credentials()

# env vars win
self.assertEqual(credentials['client_id'], 'env-client-id')
self.assertEqual(credentials['client_secret'], 'env-client-secret')
mock_logger.debug.assert_any_call("Using credentials from: environment variables")

def test_service_key_invalid_json_raises(self):
with patch.dict(os.environ, {SERVICE_KEY_ENV_VAR: 'not-valid-json'}):
with self.assertRaises(ValueError) as ctx:
fetch_credentials()
self.assertIn(SERVICE_KEY_ENV_VAR, str(ctx.exception))
self.assertIn('invalid JSON', str(ctx.exception))

@patch('ai_core_sdk.credentials.logger')
def test_injecting_credential_values(self, mock_logger):
mock_logger.debug = MagicMock()
Expand Down
9 changes: 9 additions & 0 deletions uv.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.