Skip to content

Conversation

@ksayid
Copy link
Collaborator

@ksayid ksayid commented Sep 20, 2024

Previously, broken base64 policies in the ARM template or YAML caused the tool to exit unconditionally, even when generating a new policy without diff mode.

Now:

  • In non-diff mode, the process continues and no longer exits when encountering invalid base64 data. The error is logged as a warning, and policy generation proceeds.
  • In diff mode, broken base64 policies raise an error, and the tool exits with a clear message, as expected.

Explored a few approaches, but after talking with Seth, landed on this one:

  • Chosen Approach: raising exceptions and handling at a higher level
    • raise exceptions for base64 errors in low-level utility functions (base64_to_str) and handle them at a higher level (e.g., load_policy_from_arm_template_str and load_policy_from_virtual_node_yaml_str)
    • keeps utility functions focused on their tasks (decoding base64, decomposing policy) without needing to know about diff_mode. high-level functions decide how to handle the error (e.g., ignore in non-diff mode, exit in diff mode).

Other explored ideas:

  • Old: Bubbling diff_mode down to utility functions
    • Involves passing the diff_mode flag down to functions like decompose_confidential_properties and base64_to_str to handle errors differently based on the mode.
    • easy fix, but utility functions shouldn’t depend on high-level logic like mode.
  • Old: Modifying decompose_confidential_properties and extract_confidential_properties
    • handle base64 errors directly inside these functions and adjust behavior based on the mode.
    • works but more complex. embeds diff mode logic into functions that should only focus on data extraction/decomposition

Tested manually and with azdev test confcom and azdev style confcom
image
image

@ksayid ksayid marked this pull request as draft September 27, 2024 17:53
@ksayid ksayid marked this pull request as ready for review September 27, 2024 19:46
@ksayid ksayid requested a review from hgarvison September 27, 2024 19:46
Copy link
Owner

@SethHollandsworth SethHollandsworth left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@ksayid ksayid merged commit dbe1440 into main Sep 27, 2024
@ksayid ksayid deleted the broken-policy-fix branch September 27, 2024 20:02
SethHollandsworth pushed a commit that referenced this pull request Nov 5, 2024
* handle broken base64
* address style fixes
SethHollandsworth added a commit that referenced this pull request Nov 11, 2024
* read only logic for some mount types (#62)

* offloading error checking and updating tests

* Gracefully handle broken base64 policies in non-diff mode (#64)

* handle broken base64
* address style fixes

* adding flag to omit ID from policy

* adding ability to not use sidecars via ARM tag

* adding workload identity support for vn2

* Add user prompt to confirm policy overwrite for VN2 YAMLs

* support for image attached fragments

* updating locations where executables are found

* updating version to 1.1.0

* updating test value

* taking out unused dependency

* fixing errors in docs and types

* getting rid of whitespace

* updating kata tests for linux

* updating kata tests for windows

* can't have binary files

---------

Co-authored-by: Khalil Sayid <[email protected]>
Co-authored-by: Khalil Sayid <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants