Skip to content

feat: add an option to restrict comm between units of the charm#109

Closed
adhityaravi wants to merge 4 commits intomainfrom
feat/SMS-490-intra-app-comm-pattern
Closed

feat: add an option to restrict comm between units of the charm#109
adhityaravi wants to merge 4 commits intomainfrom
feat/SMS-490-intra-app-comm-pattern

Conversation

@adhityaravi
Copy link
Member

Issue

fixes issue #106

Solution

  • The ServiceMeshConsumer now includes a bollean argument auto_allow_intra_app_access
  • When set to True, the build_mesh_policies method automatically appends an UnitPolicy with from the target apps service account selecting all the pods with the target app label

Testing Instructions

Can be tested using scaled Service Mesh Tester with more than 1 unit by setting the auto_allow_intra_app_access in the ServiceMeshConsumer to true and curling another units pod url from a unit.

@adhityaravi
Copy link
Member Author

A few questions -

  • The auto_allow_intra_app_access is not dynamic meaning for ex. it cannot be parametrized as a charm config to allow the user to enable ti and disable it neither does it observe any juju event. IMO it wasnt necessary.
  • The auto_allow_intra_app_access name is debatable - I dont like it the best but I couldnt think of a better alternative that wasnt too long.

Opinions on both?

@adhityaravi

This comment was marked as outdated.

@simskij
Copy link
Member

simskij commented Aug 27, 2025

  • The auto_allow_intra_app_access is not dynamic meaning for ex. it cannot be parametrized as a charm config to allow the user to enable ti and disable it neither does it observe any juju event. IMO it wasnt necessary.

That's fine. Should default to allowing to not break the current behavior. I'd invert it to signal that it's an opt-out.

  • The auto_allow_intra_app_access name is debatable - I dont like it the best but I couldnt think of a better alternative that wasnt too long.

restrict_crossunit_communication or similar seems like a reasonable name.

@adhityaravi adhityaravi force-pushed the feat/SMS-490-intra-app-comm-pattern branch from a065c6c to 62770a3 Compare August 27, 2025 21:01
@adhityaravi
Copy link
Member Author

Doc update here

@adhityaravi adhityaravi marked this pull request as ready for review August 28, 2025 12:49
@adhityaravi adhityaravi changed the title feat: add an option to auto allow comm between units of the charm feat: add an option to restrict comm between units of the charm Sep 3, 2025
cross_model_mesh_provides_name: str = "provide-cmr-mesh",
policies: Optional[List[Union[Policy, AppPolicy, UnitPolicy]]] = None,
auto_join: bool = True,
restrict_cross_unit_communication: bool = False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this name a bit shorter. peer_communication is my suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think it should be an allow option and default to false. In other words, apps should not be able to talk to themselves unless they set this to True.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like peer_communication the best of all the suggestion. I will go with that.

@dstathis
Copy link
Contributor

dstathis commented Sep 3, 2025

  • The auto_allow_intra_app_access is not dynamic meaning for ex. it cannot be parametrized as a charm config to allow the user to enable ti and disable it neither does it observe any juju event. IMO it wasnt necessary.

That's fine. Should default to allowing to not break the current behavior. I'd invert it to signal that it's an opt-out.

  • The auto_allow_intra_app_access name is debatable - I dont like it the best but I couldnt think of a better alternative that wasnt too long.

restrict_crossunit_communication or similar seems like a reasonable name.

The reason I think this should be opt in is that it forces the charm author to actually acknowledge the flag and think about it. I don't think we really care about the breaking change at this point since we are the only current consumers. I am not strong on this so happy to go with whatever @simskij says in response to this.

@adhityaravi
Copy link
Member Author

adhityaravi commented Sep 9, 2025

superseded by #112

@adhityaravi adhityaravi closed this Sep 9, 2025
@adhityaravi adhityaravi deleted the feat/SMS-490-intra-app-comm-pattern branch January 7, 2026 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants