[Feature] add support for context variables for better use of templating #11

djivey · 2025-01-13T18:05:00Z

What does this PR do?

This PR introduces several enhancements to the Kubernetes extension:

Template Context Support
- Adds context variable support for Kubernetes resource states
- Enables dynamic template configuration for deployments, pods, services, configmaps, and secrets
Secret Management Improvements
- Added base64 encoding handling
- Preserves secret types during updates
Service Specification Enhancement
- Enhanced service port specifications
Connection Handling
- Simplified kubeconfig management
- Removed individual override options
Updated Tests
- Added unit tests for use of context variables
- Added functional tests for the state and execution modules kubernetes and kubernetesmod
- Added integration tests for the state and execution modules kubernetes and kubernetesmod
Renamed the kubernetesmod.py state module to kubernetes.py to resolve conflicts with salt core kubernetes module

What issues does this PR fix or reference?

Fixes: #10 - Add support for context variables in Kubernetes templates.
Potentially fixes - saltstack/salt#59758

Previous Behavior

Templates lacked context support
Did not handle base64 encoded strings required for secrets
Limited service port configuration options

Merge requirements satisfied?

Docs - Updated all affected function docstrings
Changelog - Added entries in changelog/2.feature.md
Tests - Added for all new features

Commits signed with GPG?

Yes

…entSpec

…n favor of AppsV1Api for deployment operations

…w functionality

…mment

codecov · 2025-01-14T14:35:16Z

Codecov Report

Attention: Patch coverage is 89.79821% with 182 lines in your changes missing coverage. Please review.

Project coverage is 79.68%. Comparing base (9df015d) to head (1c09885).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/saltext/kubernetes/modules/kubernetesmod.py	68.65%	54 Missing and 30 partials ⚠️
tests/functional/modules/test_kubernetesmod.py	89.14%	30 Missing and 31 partials ⚠️
tests/conftest.py	79.36%	12 Missing and 1 partial ⚠️
tests/integration/states/test_kubernetes.py	95.65%	8 Missing and 4 partials ⚠️
tests/integration/modules/test_kubernetesmod.py	92.99%	11 Missing ⚠️
tests/functional/states/test_kubernetes.py	99.69%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #11       +/-   ##
===========================================
+ Coverage   55.63%   79.68%   +24.05%     
===========================================
  Files          13       17        +4     
  Lines        2439     4037     +1598     
  Branches      290      380       +90     
===========================================
+ Hits         1357     3217     +1860     
+ Misses       1066      709      -357     
- Partials       16      111       +95

Flag	Coverage Δ
Linux	`79.68% <89.79%> (+24.13%)`	⬆️
macOS	`42.46% <20.29%> (+7.67%)`	⬆️
project	`63.06% <70.52%> (+26.01%)`	⬆️
py310	`79.63% <89.63%> (+23.95%)`	⬆️
py39	`79.68% <89.79%> (+24.13%)`	⬆️
salt_3006_9	`79.68% <89.79%> (+24.05%)`	⬆️
salt_3007_1	`79.63% <89.63%> (+23.95%)`	⬆️
tests	`91.26% <93.46%> (+3.73%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

djivey · 2025-01-15T23:06:13Z

I have some more changes for this pr, but having issues with new unit tests.

…or context, update service_spec, and helper function for base64 checks

djivey · 2025-01-18T18:49:18Z

I marked this back to draft, I am going to try and write functional/integration tests for it.

…drop unused changes

…v fixture to tests/conftest.py and add a decorator to only run on linux

…efore running tests against it

…es deletions, relying on the module to raise exceptions for errors rather than checking specific response codes

…1.26.15

…t accross versions

djivey · 2025-01-27T23:07:45Z

@lkubb I think I am finished writing tests for this now. Although, I may need to reassess some of the asserts I used early on. I may have over done it with spec validation and looking back on it; I probably should have done some of the testing I did in functional tests in unit tests instead. Given how much error testing I did in the execution module functional tests -> I lightened up on the rest of the tests and focused on correct usage. Thank you for your help to get this done. The versions of kubernetes I chose was based on these release notes https://github.com/kubernetes-sigs/kind/releases/tag/v0.25.0.

@benfiedler If you have time, can you test this PR in your environment? You may need to clear cache and remove the old version of the state module file kubernetesmod.py as I renamed the module due to a conflict with the existing kubernetes.py in salt core code. It works in my environment.

Side Note: I do not like the way the state module is not actually idempotent. If it has an existing resource the module replaces it instead of saying it exists and exit. I think we should only replace if we pass in an arg/kwarg replace=true, but I think that is too much work for this PR and should be a separate issue.

…dle everything, also added metadata and type args needed

djivey · 2025-01-29T23:07:39Z

Thanks to some testing by @benfiedler I realized that I over complicated secrets management. Should be good now.

…neType error, and remove default to opaque type

djivey · 2025-01-30T22:05:02Z

This last change was due to a use case @benfiedler gave me where when creating a secret with data as an empty dictionary, kubernetes will generate the token automatically. So, I added a test for this use case, and updated secrets to handle the NoneType error and remove the default to type opaque.

benfiedler · 2025-01-31T20:12:18Z

Thanks, as @djivey mentioned my tests were successful after some minor tweaks.

There was a minor issue during testing with one of my source+context secrets that renders data that is already base64 encoded - it gets double base64 encoded, but I was able to workaround it by updating my source file to just instead have it in plain text.

I can investigate that one later when I have more time to see if I can find out how/why that's happening.

…4 values and update is_base64 to remove whitespace and use to confirm base64

djivey · 2025-02-03T21:16:06Z

Thanks, as @djivey mentioned my tests were successful after some minor tweaks.

There was a minor issue during testing with one of my source+context secrets that renders data that is already base64 encoded - it gets double base64 encoded, but I was able to workaround it by updating my source file to just instead have it in plain text.

I can investigate that one later when I have more time to see if I can find out how/why that's happening.

It is related to a use case where I was not handling newlines/whitespace in the __is_base64 function and returns false causing the double encoding.

Example data:

type: "kubernetes.io/tls"
data:
  tls.crt: |
      "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUM5akNDQWQ2Z0F3SUJBZ0lSQVA4"
      "Y3NuYmovVS9nWHJ4VDR5dXk5OUF3RFFZSktvWklodmNOQVFFTEJRQXcKRlRFVE1CRUdB"
      "MVVFQXhNS2EzVmlaWEp1WlhSbGN6QWVGdzB5TkRBeE1UY3hOekEwTWpkYUZ3MHpOREF4"
      "TVRReApOekEwTWpkYU1CVXhFekFSQmdOVkJBTVRDbXQxWW1WeWJtVjBaWE13Z2dFaU1B"
      "MEdDU3FHU0liM0RRRUJBUVVBQ"
  tls.key: |
      "LS0tLS1CRUdJTiBQUklWQVRFIEtFWS0tLS0tCk1JSUV2UUlCQURBTkJna3Foa2lHOXcw"
      "QkFRRUZBQVNDQktjd2dnU2pBZ0VBQW9JQkFRQzFyZkdjdGhFaXk3K0YKLzdSOEd6TmFh"
      "d29PdEVHVHZvWWFPMlF1b2JEcUd0NitTZFZ1Y2NTS2dDYWh3V09XN0dTTzhNRjJzaEtE"
      "WHlsegp1VzZySjN2WlJOaVgyMy9TV1J3d0xXYzBHZUNVT3VXQVlVR2N1THQ5OVplUzRQ"
      "eWQ5UmRnNTRZRlhMZ1FKV0"

I updated the function to support this use case and added a test case in the module functional tests to validate functionality.

lkubb

Thanks a lot (once again =D) for going beyond and implementing a proper functional/integration test suite + sorry for taking so long to review it. This PR gives the extension a solid foundation for future changes.

Edit: Not sure if you'll notice this edit, but I forgot mentioning: Something that would be nice is some functional tests for the state module that verify that test=True works as expected (correctly reports changes/no changes) and especially does not apply these changes.

lkubb · 2025-03-03T13:33:08Z

noxfile.py

@@ -290,7 +290,7 @@ def _lint(session, rcfile, flags, paths, tee_output=True):
            stdout.seek(0)
            contents = stdout.read()
            if contents:
-                contents = contents.decode("utf-8")


question: Is this change intentional?

I tried a lot of things through this process, I could have dropped that on accident, will test it and make sure.

lkubb · 2025-03-03T13:36:20Z

pyproject.toml

@@ -71,11 +73,16 @@ lint = [
 ]
 tests = [
    "pytest>=7.2.0",
-    "pytest-salt-factories>=1.0.0",
-    "pytest-custom-exit-code>=0.3",
+    "pytest-salt-factories>=1.0.0rc19",


issue: I don't think the version bound should be lowered.

pyproject.toml

lkubb · 2025-03-03T13:40:18Z

pyproject.toml

+    "PyYAML>=5.3.1",
+    "mock>=4.0.3",
+    "pytest-custom-exit-code>=0.3",


question: Why did you add these test dependencies?

Dropped these and reinitiated a venv to validate tests still worked.

lkubb · 2025-03-03T15:01:47Z

src/saltext/kubernetes/states/kubernetes.py

+    context
+        Variables to be passed into the template.


note: We need to ensure this change is documented prominently since before this patch, this parameter was used for something unrelated.

Where do I document this better? Module docstring?

This was mostly intended to be a reminder, it's probably fine if it's mentioned in the changelog like I proposed, especially since this module has likely been broken for quite some time.

lkubb · 2025-03-04T08:26:46Z

changelog/10.added.md

issue: This news fragment needs to render to a bullet point in the changelog list, which won't work because it contains newlines, headers and lists itself.

note: You don't need to add testing infrastructure changes here, it's just for user-facing changes.

In essence, please cut this down to a single line like: Added support for specifying template context variables (or something that you feel describes the initial motivation for this PR). Additionally, we need to document breaking changes, which should get their own news fragment. They don't need their own issue. Example:

echo 'Removed the ability to override minion configuration in function parameters (`context`, `kubeconfig`, `kubeconfig_data`)' > changelog/+config.removed.md echo 'Reused the removed `context` parameter for Jinja2 context variables! Take care when upgrading from earlier versions.' > changelog/+context.breaking.md

lkubb · 2025-03-04T08:40:58Z

src/saltext/kubernetes/states/kubernetes.py

+    source=None,
+    template=None,
+    context=None,
+    saltenv="base",


note: I think this is a breaking change as well, right? We're going from saltenv=__env__ to saltenv="base".

I was having a lot of trouble in the integration tests here, I will test this out again.

lkubb · 2025-03-04T08:43:35Z

src/saltext/kubernetes/states/kubernetes.py

+    try:
+        ret["changes"] = {
+            # Omit values from the return. They are unencrypted
+            # and can contain sensitive data.
+            "data": list(res["data"])
+        }
+    # TypeError: 'NoneType' object is not iterable
+    except TypeError:
+        ret["changes"] = {"data": []}


suggestion: If you only need to protect against None here, you can just do this instead:

Suggested change

try:

ret["changes"] = {

# Omit values from the return. They are unencrypted

# and can contain sensitive data.

"data": list(res["data"])

}

# TypeError: 'NoneType' object is not iterable

except TypeError:

ret["changes"] = {"data": []}

ret["changes"] = {

# Omit values from the return. They are unencrypted

# and can contain sensitive data.

"data": list(res["data"] or [])

}

lkubb · 2025-03-04T08:51:01Z

tests/conftest.py

+        cluster.create()
+
+        # Initial wait for cluster to start
+        time.sleep(10)


question: Is this idle time necessary? Can we just remove it and adjust the retries below?

Removed, the retries that exist are sufficient

lkubb · 2025-03-04T08:53:11Z

tests/conftest.py

+                    log.error("stdout: %s", exc.stdout)
+                    log.error("stderr: %s", exc.stderr)
+                    raise
+                time.sleep(10)


question: Don't the called commands above wait?

If not:

note: I'd much prefer this decreased to 1s and the retries adjusted accordingly because we don't want to waste time in CI.

djivey and others added 8 commits January 9, 2025 08:55

Fix undefined variable errors by importing V1Deployment and V1Deploym…

1e68c23

…entSpec

fix unit test api versions

7ea238d

Update the function to include a default value for the selector field

ea13fa5

found another issue that ExtensionsV1Api was deprecated and removed i…

2c5b1c3

…n favor of AppsV1Api for deployment operations

add support for using context variables and add unit tests for the ne…

f984245

…w functionality

add example usage of context to the module doc string and remove a co…

0fc04d6

…mment

fix lower bounds for kubernetes and update changelog

f092f88

Merge branch 'main' into feat_add_context_support

5c6cabe

djivey marked this pull request as ready for review January 14, 2025 14:39

djivey marked this pull request as draft January 15, 2025 14:10

djivey added 2 commits January 15, 2025 18:28

update secret functions to support more types, remove cli arguments f…

6b7850e

…or context, update service_spec, and helper function for base64 checks

remove the config override test and remove the rest of the kwargs from

67f03b0

djivey marked this pull request as ready for review January 16, 2025 15:06

djivey marked this pull request as draft January 18, 2025 18:48

djivey added 13 commits January 20, 2025 16:03

add minimal functional tests and environment

6955bb0

drop the noxfile changes

0f98615

test adding platform darwin to docker req, move to dependencies, and …

cf9f54f

…drop unused changes

explicit version testing rather than relying on defaults, move the en…

694e73d

…v fixture to tests/conftest.py and add a decorator to only run on linux

change to mark.skipif decorator and use kubectl to validate cluster b…

d092596

…efore running tests against it

add pytestmark to the functional tests to skip tests

9191fa0

remove the manual update in the kubernetes fixture

8eca687

add functional test for the kubernetesmod execution module

8658434

fix unit tests after the changes to accommodate functional tests

1d9e9f2

fix doc string in create_service

77b29ea

add functional test for the kubernetesmod state module

6be7eff

modify pod and secret absent funtions to work how secret_absent handl…

4920ac5

…es deletions, relying on the module to raise exceptions for errors rather than checking specific response codes

set log warning to info and rename the state module to kubernetes.py

c2bc17b

djivey added 7 commits January 24, 2025 09:13

remove old doc rst file, update changelog and remove testing on k8s v…

dda3e17

…1.26.15

test potential race condition scenarios for pods

089907d

add integration tests for the execution module kubernetesmod.py

1f3c417

modify config handling and drop extra debug stuff

ad73fdf

add cryptograpphy dependency for tests and increase pod delete timeout

e45e3ba

add state module integration tests and updated changelog

85dbb95

increase timeouts and retries for pod lificycle timing is inconsisten…

efe6847

…t accross versions

djivey marked this pull request as ready for review January 27, 2025 23:07

simplified secrets management and update __dict_to_object_meta to han…

3f867cb

…dle everything, also added metadata and type args needed

add functional test to test handling empty data dictionary, handle No…

3711a5b

…neType error, and remove default to opaque type

add a test case to the module functional tests using multi-line base6…

3fffa6c

…4 values and update is_base64 to remove whitespace and use to confirm base64

chore: Correct news fragment resolution type

1c09885

lkubb self-assigned this Feb 5, 2025

lkubb requested changes Mar 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] add support for context variables for better use of templating #11

[Feature] add support for context variables for better use of templating #11

djivey commented Jan 13, 2025 •

edited

Loading

codecov bot commented Jan 14, 2025 •

edited

Loading

djivey commented Jan 15, 2025

djivey commented Jan 18, 2025

djivey commented Jan 27, 2025 •

edited

Loading

djivey commented Jan 29, 2025

djivey commented Jan 30, 2025 •

edited

Loading

benfiedler commented Jan 31, 2025

djivey commented Feb 3, 2025

lkubb left a comment •

edited

Loading

lkubb Mar 3, 2025

djivey Mar 4, 2025

djivey Mar 5, 2025

lkubb Mar 3, 2025

djivey Mar 4, 2025

lkubb Mar 3, 2025

djivey Mar 4, 2025

lkubb Mar 3, 2025

djivey Mar 4, 2025

lkubb Mar 4, 2025 •

edited

Loading

lkubb Mar 4, 2025 •

edited

Loading

djivey Mar 4, 2025

lkubb Mar 4, 2025

djivey Mar 4, 2025

lkubb Mar 4, 2025

lkubb Mar 4, 2025

djivey Mar 5, 2025

lkubb Mar 4, 2025

djivey Mar 5, 2025

[Feature] add support for context variables for better use of templating #11

Are you sure you want to change the base?

[Feature] add support for context variables for better use of templating #11

Conversation

djivey commented Jan 13, 2025 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

Merge requirements satisfied?

Commits signed with GPG?

codecov bot commented Jan 14, 2025 • edited Loading

Codecov Report

djivey commented Jan 15, 2025

djivey commented Jan 18, 2025

djivey commented Jan 27, 2025 • edited Loading

djivey commented Jan 29, 2025

djivey commented Jan 30, 2025 • edited Loading

benfiedler commented Jan 31, 2025

djivey commented Feb 3, 2025

lkubb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkubb Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

lkubb Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djivey commented Jan 13, 2025 •

edited

Loading

codecov bot commented Jan 14, 2025 •

edited

Loading

djivey commented Jan 27, 2025 •

edited

Loading

djivey commented Jan 30, 2025 •

edited

Loading

lkubb left a comment •

edited

Loading

lkubb Mar 4, 2025 •

edited

Loading

lkubb Mar 4, 2025 •

edited

Loading