Skip to content
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.

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

Sync UP4 plugin with the latest pipeline #426

Merged
merged 1 commit into from Jan 26, 2022
Merged

Conversation

osinstom
Copy link
Contributor

@osinstom osinstom commented Jan 26, 2022

This PR syncs UP4 plugin with the latest pipeline, but does not handle application filtering rules. UP4 termination tables are configured with "don't care" app_id.

@osinstom osinstom self-assigned this Jan 26, 2022
@osinstom osinstom marked this pull request as ready for review January 26, 2022 12:45
@osinstom
Copy link
Contributor Author

CC: @daniele-moro

@@ -507,19 +510,28 @@ func (t *P4rtTranslator) buildUplinkTerminationsEntry(pdr pdr, shouldDrop bool,
return nil, err
}

// FIXME: replace app_id with a meaningful value once we implement the full support for app filtering
if err := t.withExactMatchField(entry, FieldApplicationID, uint8(0)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

uint8(0) define constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make sense? It's a temporary change and I will replace that soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that constant is used in #410 as the default behavior (here: https://github.com/omec-project/upf-epc/pull/410/files#diff-b2e5401707df74f20e19da386241442387f6ef81557463d84d849f7f9ebe6c45R692). Defining a constant helps as intrinsic documentation as well. Maybe just keep this comment valid for #410 and leave this as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's keep this comment valid for #410. This line will be changed in that PR.

pfcpiface/p4rt_translator.go Show resolved Hide resolved
@osinstom osinstom merged commit cc9cae5 into master Jan 26, 2022
@osinstom osinstom deleted the sync-with-up4-pipeline branch January 26, 2022 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants