Skip to content

feat(session_proxy): add a flag to disable Service Requested Unit AVP#8697

Merged
uri200 merged 1 commit intomagma:masterfrom
uri200:session_proxy_disable_granted_units_AVP
Aug 20, 2021
Merged

feat(session_proxy): add a flag to disable Service Requested Unit AVP#8697
uri200 merged 1 commit intomagma:masterfrom
uri200:session_proxy_disable_granted_units_AVP

Conversation

@uri200
Copy link
Copy Markdown
Contributor

@uri200 uri200 commented Aug 19, 2021

Signed-off-by: Oriol Batalla [email protected]

Summary

To support some specific vendors we need to set Service Requested Unit AVP as empty. This PR adds a flag to session_proxy service to disable that AVP.

To disable that, go to .env file and crate

DISABLE_REQUESTED_SERVICE_UNIT_AVP=1

Test Plan

Additional Information

  • This change is backwards-breaking

@uri200 uri200 requested review from a team and themarwhal August 19, 2021 16:41
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines. label Aug 19, 2021
@uri200 uri200 requested a review from emakeev August 19, 2021 16:41
@github-actions
Copy link
Copy Markdown
Contributor

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@uri200 uri200 removed the request for review from themarwhal August 19, 2021 16:41
@uri200 uri200 added apply-v1.6 apply-v1.7 apply-v1.5 Apply this commit to the v1.5 release branch as well. labels Aug 19, 2021
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 19, 2021

feg-workflow

    2 files  145 suites   30s ⏱️
338 tests 338 ✔️ 0 💤 0 ❌
352 runs  352 ✔️ 0 💤 0 ❌

Results for commit 18fe0a6.

♻️ This comment has been updated with latest results.

configsPtr := &mconfig.SessionProxyConfig{}
err := managed_configs.GetServiceConfigs(credit_control.SessionProxyServiceName, configsPtr)
siStr := diameter.GetValueOrEnv(OCSServiceIdentifierFlag, OCSServiceIdentifierEnv, "")
avp437 := diameter.GetValueOrEnv(OCSServiceIdentifierFlag, DisableRequestedGrantedUnitsAVPEnv, "")
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.

copy/paste, should be:
avp437 := diameter.GetValueOrEnv(DisableRequestedGrantedUnitsAVPFlag, DisableRequestedGrantedUnitsAVPEnv, "")

Copy link
Copy Markdown
Contributor

@emakeev emakeev left a comment

Choose a reason for hiding this comment

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

please see comment inline.

Copy link
Copy Markdown
Contributor

@emakeev emakeev left a comment

Choose a reason for hiding this comment

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

pls, see comments inline

OCSServiceIdentifier string
DisableGy bool
VirtualApnRules []*credit_control.VirtualApnRule
DisableServiceGrantedUnitsAVP string
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.

Wouldn't it be better to make it bool? Like DisableGy above

_ = flag.String(GyInitMethodFlag, "", "Gy init method (per_key|per_session)")
_ = flag.String(OCSApnOverwriteFlag, "", "OCS APN to use instead of request's APN")
_ = flag.String(OCSServiceIdentifierFlag, "", "OCS ServiceIdentifier to use in Gy requests")
_ = flag.String(DisableRequestedGrantedUnitsAVPFlag, "", "Disable Requested-Service-Unit AVP (437)")
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.

I'd make it Bool here.

avp437Flag = flag.Bool(DisableRequestedGrantedUnitsAVPFlag, false, "Disable Requested-Service-Unit AVP (437)")

configsPtr := &mconfig.SessionProxyConfig{}
err := managed_configs.GetServiceConfigs(credit_control.SessionProxyServiceName, configsPtr)
siStr := diameter.GetValueOrEnv(OCSServiceIdentifierFlag, OCSServiceIdentifierEnv, "")
avp437 := diameter.GetValueOrEnv(OCSServiceIdentifierFlag, DisableRequestedGrantedUnitsAVPEnv, "")
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.

I think, it may be cleaner to make it bool, you can do something like this:

avp437 := *avp437Flag || util.IsTruthyEnv(DisableRequestedGrantedUnitsAVPEnv)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nice

@uri200 uri200 force-pushed the session_proxy_disable_granted_units_AVP branch from 56113b2 to ce45c4a Compare August 19, 2021 19:24
@uri200 uri200 force-pushed the session_proxy_disable_granted_units_AVP branch from ce45c4a to 18fe0a6 Compare August 19, 2021 20:12
@uri200 uri200 linked an issue Aug 19, 2021 that may be closed by this pull request
Copy link
Copy Markdown
Contributor

@emakeev emakeev left a comment

Choose a reason for hiding this comment

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

LGTM

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.

[FeG] AVP 437 Requested-Service-Unit inside MSCC must be empty

3 participants