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.

Sign up for GitHub

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

Jump to bottom

UE ip pool allocation by UPF #203

Merged
merged 1 commit into from Feb 5, 2021
Merged

UE ip pool allocation by UPF #203

merged 1 commit into from Feb 5, 2021

Conversation

badhrinathpa
Copy link
Contributor

Changes include :

  1. config for ip pool and ip pool feature flag
  2. send UPF features flag in Association response with UEIP flag.
  3. alloc UE ip in case when upf supports this feature and smf has sent IP with only CHV4 flag.
  4. send PDR with UE info in establishment response

thakurajayL
thakurajayL reviewed Feb 2, 2021
View reviewed changes
pfcpiface/ip_pool.go Outdated
@@ -0,0 +1,55 @@
// SPDX-License-Identifier: Apache-2.0
// Copyright(c) 2020 Intel Corporation
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add ONF copyright header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

thakurajayL
thakurajayL reviewed Feb 2, 2021
View reviewed changes
pfcpiface/session-pdr.go Outdated
if (pdr.allocIpFlag) && (pdr.srcIface == core) {
log.Println("pdrID : ", pdr.pdrID)
var ueIP net.IP = int2ip(pdr.dstIP)
log.Println("ueIP : ", ueIP.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

Clubbing 2 logs would help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

thakurajayL
thakurajayL reviewed Feb 2, 2021
View reviewed changes
pfcpiface/utils.go
}

func has2ndBit(f uint8) bool {
return (f&0x02)>>1 == 1
Copy link
Contributor

Choose a reason for hiding this comment

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

if bool return is not required then we can avoid >> 1 & == 1 check.
return (f & 0x02)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we would have to add check for ==1 wherever we call has2ndBit. So will let it be as it is for now.

@thakurajayL
Copy link
Contributor

In general changes look good.

@krsna1729
Copy link
Member

@badhrinathpa @thakurajayL what do you think of using something like this for IPAM?
https://github.com/metal-stack/go-ipam

krsna1729
krsna1729 reviewed Feb 2, 2021
View reviewed changes
pfcpiface/bess.go Outdated
@@ -278,6 +278,12 @@ func (b *bess) setUpfInfo(u *upf, conf *Conf) {
u.simInfo = simInfo
}

u.ippool_cidr = conf.CPIface.UeIPPool
Copy link
Member

Choose a reason for hiding this comment

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

change all naming to not have underscore. I thought one of the linting tool checks and throws warning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

krsna1729
krsna1729 reviewed Feb 2, 2021
View reviewed changes
pfcpiface/main.go Outdated
DestIP string `json:"nb_dst_ip"`
SrcIP string `json:"nb_src_ip"`
FQDNHost string `json:"hostname"`
EnableUeIPAlloc bool `json:"enable_ue_ip_alloc"`
Copy link
Member

Choose a reason for hiding this comment

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

keep this before UE IP pool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

krsna1729
krsna1729 reviewed Feb 2, 2021
View reviewed changes
pfcpiface/ip_pool.go Outdated
Comment on lines 18 to 14
func needAllocIp(ueIPaddr *ie.UEIPAddressFields) bool {
if has2ndBit(ueIPaddr.Flags) && !has5thBit(ueIPaddr.Flags) {
return false
}
return true
}
Copy link
Member

Choose a reason for hiding this comment

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

move this to parse-pdr.go itself? that way this file will purely be IP allocation related.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

krsna1729
krsna1729 reviewed Feb 2, 2021
View reviewed changes
pfcpiface/parse-pdr.go
@@ -212,7 +227,7 @@ func (p *pdr) parsePDI(pdiIEs []*ie.IE, appPFDs map[string]appPFD) error {
return nil
}

func (p *pdr) parsePDR(ie1 *ie.IE, seid uint64, appPFDs map[string]appPFD) error {
func (p *pdr) parsePDR(ie1 *ie.IE, seid uint64, appPFDs map[string]appPFD, upf *upf) error {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to pass entire UPF object or just ip_pool struct is enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its just pointer passing. might need other values later. So keeping as it is.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking in terms layer/scope instead pointer or not.

@thakurajayL thakurajayL merged commit a775a96 into omec-project:master Feb 5, 2021
@yoooou
Copy link

yoooou commented Feb 5, 2021

retest this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thakurajayL thakurajayL thakurajayL left review comments

@krsna1729 krsna1729 krsna1729 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@badhrinathpa @thakurajayL @krsna1729 @yoooou