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

MEN-4325 File Transfer Limits #21

Conversation

merlin-northern
Copy link
Contributor

ChangeLog:none
Signed-off-by: Peter Grzybowski peter@northern.tech

@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from e4ffc62 to 44eaaad Compare March 14, 2021 20:45
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from 44eaaad to ec4969f Compare March 14, 2021 21:29
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from ec4969f to f0b6310 Compare March 14, 2021 21:35
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from f0b6310 to 0fc5ada Compare March 14, 2021 21:40
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from 0fc5ada to 58fe03a Compare March 14, 2021 22:02
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from 58fe03a to db2fa9f Compare March 14, 2021 22:30
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from db2fa9f to a39ef22 Compare March 15, 2021 08:44
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from a39ef22 to 2569263 Compare March 15, 2021 09:01
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from 2569263 to b84f5a0 Compare March 15, 2021 09:27
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from b84f5a0 to d9b6307 Compare March 15, 2021 09:39
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern
Copy link
Contributor Author

@mender-test-bot start pipeline please :)

@mender-test-bot
Copy link

Hello 😸 I created a pipeline for you here: Pipeline-270502493

Build Configuration Matrix

Key Value
AUDITLOGS_REV origin/master
BUILD_BEAGLEBONEBLACK true
BUILD_CLIENT true
BUILD_QEMUX86_64_BIOS_GRUB true
BUILD_QEMUX86_64_BIOS_GRUB_GPT true
BUILD_QEMUX86_64_UEFI_GRUB true
BUILD_VEXPRESS_QEMU true
BUILD_VEXPRESS_QEMU_FLASH true
BUILD_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true
CREATE_ARTIFACT_WORKER_REV origin/master
DEPLOYMENTS_ENTERPRISE_REV origin/master
DEPLOYMENTS_REV origin/master
DEVICEAUTH_REV origin/master
DEVICECONFIG_REV origin/master
DEVICECONNECT_REV origin/master
GUI_REV origin/master
INTEGRATION_REV master
INVENTORY_ENTERPRISE_REV origin/master
INVENTORY_REV origin/master
MENDER_ARTIFACT_REV origin/master
MENDER_CLI_REV origin/master
MENDER_CONNECT_REV pull/21/head
MENDER_REV origin/master
MTLS_AMBASSADOR_REV origin/master
RUN_INTEGRATION_TESTS true
TENANTADM_REV origin/master
TEST_QEMUX86_64_BIOS_GRUB true
TEST_QEMUX86_64_BIOS_GRUB_GPT true
TEST_QEMUX86_64_UEFI_GRUB true
TEST_VEXPRESS_QEMU true
TEST_VEXPRESS_QEMU_FLASH true
TEST_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true
USERADM_ENTERPRISE_REV origin/master
USERADM_REV origin/master
WORKFLOWS_ENTERPRISE_REV origin/master
WORKFLOWS_REV origin/master

Copy link
Contributor

@alfrunes alfrunes left a comment

Choose a reason for hiding this comment

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

Mostly suggestions, but the additions to the sessions.UploadHandler must come sooner or the file will be overwritten before reaching those checks.

config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated
@@ -255,6 +307,9 @@ func (c *MenderShellConfig) Validate() (err error) {
}

c.HTTPSClient.Validate()

//enforce the limits by default, if set to false it means permit everything
c.Limits.Enabled = true
Copy link
Contributor

Choose a reason for hiding this comment

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

... but won't the default (empty) limitation disable everything?

Copy link
Contributor Author

@merlin-northern merlin-northern Mar 17, 2021

Choose a reason for hiding this comment

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

not really -- thats why it is done like that; e.g.: FollowSymLinks: can be true or false, but the check will run anyways if limits are enabled. I want a central place to switch the limits on and off (similar to selinux enforce/permissive). so, unless you are adamant on this one :)

limits/filetransfer/limits.go Outdated Show resolved Hide resolved
limits/filetransfer/limits.go Outdated Show resolved Hide resolved
Comment on lines 362 to 369
sinceLastUpdateS := time.Now().Unix() - deviceCountersLastH.bytesTransferredLastUpdate.Unix()
if deviceCountersLastH.bytesTransferred != 0 {
deviceCountersLastH.currentTxRate = float64(deviceCountersLastH.bytesTransferred*1.0) / float64(sinceLastUpdateS)
}
sinceLastUpdateS = time.Now().Unix() - deviceCountersLastH.bytesReceivedLastUpdate.Unix()
if deviceCountersLastH.bytesReceived != 0 {
deviceCountersLastH.currentRxRate = float64(deviceCountersLastH.bytesReceived*1.0) / float64(sinceLastUpdateS)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these values used for anything except for logging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

those are for "future use" just in case we need to reach for the previous hour. I can remove them, they are leftovers as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed to hold just an average of last minute, for extra info, not used in the limits atm.

Comment on lines +99 to +105
// By default we only allow to send/put regular files
RegularFilesOnly bool
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this only affects stat requests? Uploads and downloads are already restricted to regular files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

really? I tried with socket and fifo, did not dare to try char/block /dev'ices but I think this limit makes sense to have explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wrong, it does affect downloads. Uploads are however restricted to regular files because of this line:

err = fd.Chmod(os.FileMode(*params.Mode) & os.ModePerm)

And the way the file is created:
fd, err = os.OpenFile(filename, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0200)

config/config.go Outdated Show resolved Hide resolved
return nil
}

if (mode & S_ISUID) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will never be the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you send mode=4755 it will

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you have the "PreserveModes" config set, only the permission modes counts:

err = fd.Chmod(os.FileMode(*params.Mode) & os.ModePerm)

limits/filetransfer/limits.go Show resolved Hide resolved
session/model/filetransfer.go Show resolved Hide resolved
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

4 similar comments
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from 6ab9208 to 642aea8 Compare March 18, 2021 22:32
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

1 similar comment
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from cf60df5 to 80a0ba7 Compare March 25, 2021 20:47
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

Copy link
Contributor

@alfrunes alfrunes left a comment

Choose a reason for hiding this comment

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

I'm pretty sure the transfer-rate computation is incorrect, and I also thinks that section deserves some cleanup. Also, the unit tests job is failing.

time.Sleep(time.Duration(countersUpdateSleepTimeS) * time.Second)
bytesRXLastMinute += deviceCounters.bytesReceived - bytesRXLastMinute0
bytesTXLastMinute += deviceCounters.bytesTransferred - bytesTXLastMinute0
deviceCounters.bytesReceivedLast1W = expWeight1m*deviceCounters.bytesReceivedLast1W +
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what the intention of this counter is "Last 1 Week?".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

last 1 minute weighted, sorry. I will change that. I think I changed in one place, this one was left.

Comment on lines 373 to 374
bytesRXLastMinute += deviceCounters.bytesReceived - bytesRXLastMinute0
bytesTXLastMinute += deviceCounters.bytesTransferred - bytesTXLastMinute0
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is correct.
Every transfer time interval is counted multiple times except the very last.

Comment on lines 349 to 382
func updatePerHourCounters() {
const one60th = 0.016666666666666666
if counterUpdateRunning {
counterUpdateStarted <- false
return
}

counterUpdateRunning = true
counterUpdateStarted <- true
//exponential decrease for "avg" over 1 minute: \exp(-dt[s]/(1*60[s]))
expWeight1m := math.Exp(-float64(countersUpdateSleepTimeS) * one60th) // / 60.0)
//analogy to the uptime weighted avg
//expWeight5m := math.Exp(-float64(countersUpdateSleepTimeS) * 0.003333333333333333) // / (5*60.0))
//expWeight15m := math.Exp(-float64(countersUpdateSleepTimeS) * 0.001111111111111111) // / (15*60.0))
deviceCounters.bytesReceivedLast1W = 0.0
deviceCounters.bytesTransferredLast1W = 0.0

for counterUpdateRunning {
bytesRXLastMinute0 := deviceCounters.bytesReceived
bytesTXLastMinute0 := deviceCounters.bytesTransferred
bytesRXLastMinute := uint64(0)
bytesTXLastMinute := uint64(0)
for i := 0; i < int(int64(time.Minute)/int64(countersUpdateSleepTimeS)/int64(time.Second)); i++ {
time.Sleep(time.Duration(countersUpdateSleepTimeS) * time.Second)
bytesRXLastMinute += deviceCounters.bytesReceived - bytesRXLastMinute0
bytesTXLastMinute += deviceCounters.bytesTransferred - bytesTXLastMinute0
deviceCounters.bytesReceivedLast1W = expWeight1m*deviceCounters.bytesReceivedLast1W +
float64(bytesRXLastMinute) - expWeight1m*float64(bytesRXLastMinute)
deviceCounters.bytesTransferredLast1W = expWeight1m*deviceCounters.bytesTransferredLast1W +
float64(bytesTXLastMinute) - expWeight1m*float64(bytesTXLastMinute)
}
deviceCounters.rateTransferredLastMinute = float64(bytesTXLastMinute) * one60th
deviceCounters.rateReceivedLastMinute = float64(bytesRXLastMinute) * one60th
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please simplify this a little? I found it very difficult to read.

Suggested change
func updatePerHourCounters() {
const one60th = 0.016666666666666666
if counterUpdateRunning {
counterUpdateStarted <- false
return
}
counterUpdateRunning = true
counterUpdateStarted <- true
//exponential decrease for "avg" over 1 minute: \exp(-dt[s]/(1*60[s]))
expWeight1m := math.Exp(-float64(countersUpdateSleepTimeS) * one60th) // / 60.0)
//analogy to the uptime weighted avg
//expWeight5m := math.Exp(-float64(countersUpdateSleepTimeS) * 0.003333333333333333) // / (5*60.0))
//expWeight15m := math.Exp(-float64(countersUpdateSleepTimeS) * 0.001111111111111111) // / (15*60.0))
deviceCounters.bytesReceivedLast1W = 0.0
deviceCounters.bytesTransferredLast1W = 0.0
for counterUpdateRunning {
bytesRXLastMinute0 := deviceCounters.bytesReceived
bytesTXLastMinute0 := deviceCounters.bytesTransferred
bytesRXLastMinute := uint64(0)
bytesTXLastMinute := uint64(0)
for i := 0; i < int(int64(time.Minute)/int64(countersUpdateSleepTimeS)/int64(time.Second)); i++ {
time.Sleep(time.Duration(countersUpdateSleepTimeS) * time.Second)
bytesRXLastMinute += deviceCounters.bytesReceived - bytesRXLastMinute0
bytesTXLastMinute += deviceCounters.bytesTransferred - bytesTXLastMinute0
deviceCounters.bytesReceivedLast1W = expWeight1m*deviceCounters.bytesReceivedLast1W +
float64(bytesRXLastMinute) - expWeight1m*float64(bytesRXLastMinute)
deviceCounters.bytesTransferredLast1W = expWeight1m*deviceCounters.bytesTransferredLast1W +
float64(bytesTXLastMinute) - expWeight1m*float64(bytesTXLastMinute)
}
deviceCounters.rateTransferredLastMinute = float64(bytesTXLastMinute) * one60th
deviceCounters.rateReceivedLastMinute = float64(bytesRXLastMinute) * one60th
}
}
func updateTransferRates() {
if counterUpdateRunning {
counterUpdateStarted <- false
return
}
counterUpdateRunning = true
counterUpdateStarted <- true
var (
lastRX uint64
lastTX uint64
avgRX float64
avgTX float64
// exp factor: 1 - exp(-dt[s]/(1*60[s]))
_w1 = math.Exp(-(float64(countersUpdateSleepTimeS) * (1.0 / 60.0)))
w1 = 1 - _w1
)
tick := time.NewTicker(time.Duration(countersUpdateSleepTimeS) * time.Second)
defer tick.Stop()
for counterUpdateRunning {
select {
case <-tick.C:
rxTot := deviceCounters.bytesTransferred
txTot := deviceCounters.bytesReceived
rx := rxTot - lastRX
tx := txTot - lastTX
lastRX = rxTot
lastTX = rxTot
// avg[n+1] = w * Y[n] + (1 - w) avg[n]
avgRX = w1*float64(rx) + _w1*avgRX
avgTX = w1*float64(tx) + _w1*avgTX
deviceCounters.rateTransferredLastMinute = avgTX
deviceCounters.rateReceivedLastMinute = avgRX
// case <-ctx.Done(): ?
}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing.

Comment on lines +99 to +105
// By default we only allow to send/put regular files
RegularFilesOnly bool
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wrong, it does affect downloads. Uploads are however restricted to regular files because of this line:

err = fd.Chmod(os.FileMode(*params.Mode) & os.ModePerm)

And the way the file is created:
fd, err = os.OpenFile(filename, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0200)

@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from 80a0ba7 to 7a28c6f Compare March 29, 2021 15:35
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern
Copy link
Contributor Author

@mender-test-bot start pipeline and I beg you let it pass. sugar pretty please with cherry on top.

@mender-test-bot
Copy link

Hello 😸 I created a pipeline for you here: Pipeline-278324343

Build Configuration Matrix

Key Value
AUDITLOGS_REV origin/master
BUILD_BEAGLEBONEBLACK true
BUILD_CLIENT true
BUILD_QEMUX86_64_BIOS_GRUB true
BUILD_QEMUX86_64_BIOS_GRUB_GPT true
BUILD_QEMUX86_64_UEFI_GRUB true
BUILD_VEXPRESS_QEMU true
BUILD_VEXPRESS_QEMU_FLASH true
BUILD_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true
CREATE_ARTIFACT_WORKER_REV origin/master
DEPLOYMENTS_ENTERPRISE_REV origin/master
DEPLOYMENTS_REV origin/master
DEVICEAUTH_REV origin/master
DEVICECONFIG_REV origin/master
DEVICECONNECT_REV origin/master
GUI_REV origin/master
INTEGRATION_REV master
INVENTORY_ENTERPRISE_REV origin/master
INVENTORY_REV origin/master
MENDER_ARTIFACT_REV origin/master
MENDER_CLI_REV origin/master
MENDER_CONNECT_REV pull/21/head
MENDER_REV origin/master
MTLS_AMBASSADOR_REV origin/master
RUN_INTEGRATION_TESTS true
TENANTADM_REV origin/master
TEST_QEMUX86_64_BIOS_GRUB true
TEST_QEMUX86_64_BIOS_GRUB_GPT true
TEST_QEMUX86_64_UEFI_GRUB true
TEST_VEXPRESS_QEMU true
TEST_VEXPRESS_QEMU_FLASH true
TEST_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true
USERADM_ENTERPRISE_REV origin/master
USERADM_REV origin/master
WORKFLOWS_ENTERPRISE_REV origin/master
WORKFLOWS_REV origin/master

@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from 7a28c6f to f0ef192 Compare March 29, 2021 21:15
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern
Copy link
Contributor Author

@mender-test-bot start pipeline pretty please

@mender-test-bot
Copy link

Hello 😸 I created a pipeline for you here: Pipeline-278468367

Build Configuration Matrix

Key Value
AUDITLOGS_REV origin/master
BUILD_BEAGLEBONEBLACK true
BUILD_CLIENT true
BUILD_QEMUX86_64_BIOS_GRUB true
BUILD_QEMUX86_64_BIOS_GRUB_GPT true
BUILD_QEMUX86_64_UEFI_GRUB true
BUILD_VEXPRESS_QEMU true
BUILD_VEXPRESS_QEMU_FLASH true
BUILD_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true
CREATE_ARTIFACT_WORKER_REV origin/master
DEPLOYMENTS_ENTERPRISE_REV origin/master
DEPLOYMENTS_REV origin/master
DEVICEAUTH_REV origin/master
DEVICECONFIG_REV origin/master
DEVICECONNECT_REV origin/master
GUI_REV origin/master
INTEGRATION_REV master
INVENTORY_ENTERPRISE_REV origin/master
INVENTORY_REV origin/master
MENDER_ARTIFACT_REV origin/master
MENDER_CLI_REV origin/master
MENDER_CONNECT_REV pull/21/head
MENDER_REV origin/master
MTLS_AMBASSADOR_REV origin/master
RUN_INTEGRATION_TESTS true
TENANTADM_REV origin/master
TEST_QEMUX86_64_BIOS_GRUB true
TEST_QEMUX86_64_BIOS_GRUB_GPT true
TEST_QEMUX86_64_UEFI_GRUB true
TEST_VEXPRESS_QEMU true
TEST_VEXPRESS_QEMU_FLASH true
TEST_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true
USERADM_ENTERPRISE_REV origin/master
USERADM_REV origin/master
WORKFLOWS_ENTERPRISE_REV origin/master
WORKFLOWS_REV origin/master

}
}

func updatePerHourCounters() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The function name is a bit imprecise.

Copy link
Contributor Author

@merlin-northern merlin-northern Mar 30, 2021

Choose a reason for hiding this comment

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

crap! and I so renamed all the structs. fixing. but look, I renamed the test func :)

ChangeLog:none
Signed-off-by: Peter Grzybowski <peter@northern.tech>
@merlin-northern merlin-northern force-pushed the men_4325_file_transfer_restrictions_review branch from f0ef192 to 1b72a8a Compare March 30, 2021 07:04
@mender-test-bot
Copy link

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

@merlin-northern merlin-northern merged commit 7f567d2 into mendersoftware:master Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants