Proxy: Socket file 'other' permission changes to support ACRN Hypervisor by vijaydhanraj · Pull Request #193 · kata-containers/proxy

vijaydhanraj · 2019-06-07T14:19:39Z

When binding the socket with the file path, the socket file permission
is changed to 755. The reason being the following,

Bind system call sets the socket file permission(mode) based on,
socket FD permission and umask bits set. Socket FD by default is
set to 777 and umask by default is set to 022. This results in 755.
ACRN doesn't plan to have an API to update the umask so added
the workaround in kata proxy, where if the ‘other’ permission
is not disabled, then kata proxy can override and disable it.

Fixes: #191

Signed-off-by: Vijay Dhanraj vijay.dhanraj@intel.com

codecov · 2019-06-07T14:43:26Z

Codecov Report

Merging #193 into master will decrease coverage by 0.93%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #193      +/-   ##
==========================================
- Coverage   36.56%   35.63%   -0.94%     
==========================================
  Files           2        2              
  Lines         268      275       +7     
==========================================
  Hits           98       98              
- Misses        159      166       +7     
  Partials       11       11

vijaydhanraj · 2019-06-07T14:50:41Z

Related to: kata-containers/runtime#1779

amshinde

lgtm

devimc · 2019-06-10T13:33:47Z

proxy.go

+		other := int(fileInfo.Mode().Perm()) & otherMask
+		err := os.Chmod(agentLogsAddr, os.FileMode(other))
+		if err != nil {
+			logger().Infof("Cannot change socket permissions for 'other' %d", err)


use WithField and Error

logger().WithField("error", err).Error("Cannot change socket permissions for 'other'")

done. used WithError instead of WithField as discussed.

devimc · 2019-06-10T13:35:59Z

proxy.go

+		if err != nil {
+			logger().Infof("Cannot change socket permissions for 'other' %d", err)
+		} else {
+			logger().Infof("Chaning socket permissions to %4.4o", other)


please move this log before os.Chmod and use WithField instead of %4.4o

devimc · 2019-06-10T13:36:29Z

proxy.go

 	other := int(fileInfo.Mode().Perm()) & otherMask
 	if other != 0 {
-		return fmt.Errorf("All socket permissions for 'other' should be disabled, got %3.3o", other)
+		logger().Infof("All socket permissions for 'other' should be disabled, got %4.4o", int(fileInfo.Mode().Perm()))


use WithField instead of %4.4o

devimc · 2019-06-10T18:34:38Z

/test

Version bump no changes Signed-off-by: katacontainersbot <katacontainersbot@gmail.com>

jodh-intel · 2019-06-11T08:04:07Z

proxy.go

 	if other != 0 {
-		return fmt.Errorf("All socket permissions for 'other' should be disabled, got %3.3o", other)
+		logger().WithField("socket permissions", fmt.Sprintf("%4.4o", fileInfo.Mode().Perm())).
+			Infof("All socket permissions for 'other' should be disabled")


Nit: This can now be Info() as you don't have any formats to expand in the string.

jodh-intel · 2019-06-11T08:04:26Z

proxy.go

+			Infof("All socket permissions for 'other' should be disabled")
+
+		// Setting permissions socket for "other" to 0.
+		otherMask := 0770


You could make this const

This is actually not a declaration. I was trying to reassign a new value to the previously declared variable.
PS: I have now changed this from a declaration to an assignment.

Good point! 😄

jodh-intel · 2019-06-11T08:08:23Z

proxy.go

+		// Setting permissions socket for "other" to 0.
+		otherMask := 0770
+		other := int(fileInfo.Mode().Perm()) & otherMask
+		logger().WithField("other", fmt.Sprintf("%4.4o", other)).Infof("Chaning socket permissions of 'other'")


Nits:

typo: "Changing".

You can also use Info() here rather than Infof().

When binding the socket with the file path, the socket file permission is changed to 755. The reason being the following, Bind system call sets the socket file permission(mode) based on, socket FD permission and umask bits set. Socket FD by default is set to 777 and umask by default is set to 022. This results in 755. ACRN doesn't plan to have an API to update the umask so added the workaround in kata proxy, where if the ‘other’ permission is not disabled, then kata proxy can override and disable it. v1->v2: Changed formatting for logging proxy trace. Fixes: kata-containers#191 Signed-off-by: Vijay Dhanraj <vijay.dhanraj@intel.com>

vijaydhanraj · 2019-06-11T20:29:51Z

@GWhaley: Can you please help me reproduce the PR-check failure?

…branch-bump # Kata Containers 1.8.0-alpha1

grahamwhaley · 2019-06-12T08:30:21Z

Hi @vijaydhanraj Looks like we need to re-trigger the CIs as you updated the PR, so first we:
/test

and then, it looks like the PR-check CI had some sort of system failure, so let's re-trigger that as well:
/zuul-recheck

and then we see how that goes in a couple of hours...

amshinde · 2019-06-12T21:54:05Z

@grahamwhaley Looks like the zuul checks are still failing here, can you take a look? I dont see any logs on the build site.

grahamwhaley · 2019-06-13T08:36:55Z

/zuul-recheck
Oh Zuul zuul.... sigh, so, even though I'm sure we've had the PR-check working (otherwise we could not have merged anything), the logs shown on this link starting from 21st March all show one of NODE_FAILURE/FAILURE/RETRY_LIMIT ?
Maybe the logs only show jobs that fail?? I dunno
https://zuul.opendev.org/t/kata-containers/builds?pipeline=PR-check

Anyway, it looks like this PR (#193 - that is the clue to finding the relevant items for this PR in the log/job list on the above link) shows we hit another NODE_FAILURE. One more time, with gusto then....
/zuul-recheck
(and slapping one a the front of the comment, just in case as well)

vijaydhanraj · 2019-06-13T17:07:59Z

/zuul-recheck

grahamwhaley · 2019-06-13T17:19:20Z

Doh, @vijaydhanraj @amshinde - heh, that is the PR-check we are looking at - that is the WIP Zuul CI check we have that does not currently pass ( see #154 ) - so, don't expect that to pass. The other two Zuul CIs have passed (WIP and SOB) - so, we are good to push the merge maybe...

Version bump no changes Signed-off-by: Peng Tao <bergwolf@hyper.sh>

…nch-bump # Kata Containers 1.8.0-alpha2

vijaydhanraj · 2019-06-17T22:44:32Z

Any other comments on this patch? If not, can this be merged?

jodh-intel · 2019-06-20T08:48:13Z

ok - ARM CI failure is known, so let's just get this merged...

Proxy: Socket file 'other' permission changes to support ACRN Hypervisor

jodh-intel · 2019-06-20T08:49:50Z

... which we cannot do as the branch has conflicts. Could you update please @vijaydhanraj ?

vijaydhanraj · 2019-06-20T17:53:08Z

@jodh-intel I didn't close the issue, just pushed my changes after rebase. Please let me know anything else is needed.

amshinde · 2019-06-20T19:16:34Z

@jodh-intel @vijaydhanraj Looks like this got merged manually vs the github UI.
Weird as I saw this PR open today morning.
@jodh-intel Any idea about this?

jodh-intel · 2019-06-21T07:43:47Z

@amshinde - I tried to merge from the UI, got the green "confirm merge" button and clicked it, but then got an error saying the branch was conflicted. However, I checked by grabbing a copy of the PR locally and it merged correctly so this may have been a "github gremlin" 👽 I left the PR in an open state so not sure what happened subsequently.

vijaydhanraj closed this Jun 7, 2019

vijaydhanraj reopened this Jun 7, 2019

amshinde reviewed Jun 7, 2019

View reviewed changes

devimc reviewed Jun 10, 2019

View reviewed changes

vijaydhanraj force-pushed the acrn-proxy branch from fee49f4 to 339152c Compare June 10, 2019 17:53

devimc approved these changes Jun 10, 2019

View reviewed changes

release: Kata Containers 1.8.0-alpha1

9e2a698

Version bump no changes Signed-off-by: katacontainersbot <katacontainersbot@gmail.com>

jodh-intel approved these changes Jun 11, 2019

View reviewed changes

vijaydhanraj force-pushed the acrn-proxy branch from 339152c to 006462d Compare June 11, 2019 19:16

Merge pull request kata-containers#194 from katabuilder/1.8.0-alpha1-…

d1dc863

…branch-bump # Kata Containers 1.8.0-alpha1

bergwolf and others added 2 commits June 17, 2019 16:31

release: Kata Containers 1.8.0-alpha2

9074db6

Version bump no changes Signed-off-by: Peng Tao <bergwolf@hyper.sh>

Merge pull request kata-containers#198 from bergwolf/1.8.0-alpha2-bra…

8d16905

…nch-bump # Kata Containers 1.8.0-alpha2

Merge pull request kata-containers#193 from vijaydhanraj/acrn-proxy

4c6ae8a

Proxy: Socket file 'other' permission changes to support ACRN Hypervisor

vijaydhanraj closed this Jun 20, 2019

Conversation

vijaydhanraj commented Jun 7, 2019

Uh oh!

codecov bot commented Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vijaydhanraj commented Jun 7, 2019

Uh oh!

amshinde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devimc commented Jun 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vijaydhanraj commented Jun 11, 2019

Uh oh!

grahamwhaley commented Jun 12, 2019

Uh oh!

amshinde commented Jun 12, 2019

Uh oh!

grahamwhaley commented Jun 13, 2019

Uh oh!

vijaydhanraj commented Jun 13, 2019

Uh oh!

grahamwhaley commented Jun 13, 2019

Uh oh!

vijaydhanraj commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jodh-intel commented Jun 20, 2019

Uh oh!

jodh-intel commented Jun 20, 2019

Uh oh!

vijaydhanraj commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amshinde commented Jun 20, 2019

Uh oh!

jodh-intel commented Jun 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov bot commented Jun 7, 2019 •

edited

Loading

vijaydhanraj commented Jun 17, 2019 •

edited

Loading

vijaydhanraj commented Jun 20, 2019 •

edited

Loading