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

Use correct calico-node UID when running in non-privileged mode. #2645

Merged
merged 5 commits into from
May 18, 2023

Conversation

sridhartigera
Copy link
Contributor

@sridhartigera sridhartigera commented May 17, 2023

Description

When enabling non-privileged mode, the ownership of directories that calico-node uses should be changed to UID of calico-node.

In non-privileged mode, the UID of calico-node is 10001.

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

  • Milestone set according to targeted release.
  • Appropriate labels:
    • kind/bug if this is a bugfix.
    • kind/enhancement if this is a a new feature.
    • enterprise if this PR applies to Calico Enterprise only.

},
SecurityContext: securitycontext.NewRootContext(false),
SecurityContext: securitycontext.NewRootContext(true),
Copy link
Contributor

@hjiawei hjiawei May 17, 2023

Choose a reason for hiding this comment

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

Why is this changed to true (privileged)? If this is for chown permissions, maybe we can try to add the CHOWN cap first?

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 see issues when the init container does mkdirAll /var/log/calico/cni.

Copy link
Member

Choose a reason for hiding this comment

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

@sridhartigera do you know why this was working before then? Or was it never working? Or is this only needed for BPF mode? Or something about your setup?

@@ -896,7 +897,7 @@ var _ = Describe("Node rendering tests", func() {

// hostpath init container should have the correct env and security context.
hostPathContainer := rtest.GetContainer(ds.Spec.Template.Spec.InitContainers, "hostpath-init")
rtest.ExpectEnv(hostPathContainer.Env, "NODE_USER_ID", "999")
rtest.ExpectEnv(hostPathContainer.Env, "NODE_USER_ID", fmt.Sprintf("%d", securitycontext.GetNonRootUID()))
Copy link
Contributor

@hjiawei hjiawei May 17, 2023

Choose a reason for hiding this comment

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

You can use string "10001" directly here and remove the new GetNonRootUID() function in security_context.go.

@@ -76,3 +76,8 @@ func NewNonRootPodContext() *corev1.PodSecurityContext {
},
}
}

// GetNonRootUID returns the non-root UID
Copy link
Contributor

@hjiawei hjiawei May 17, 2023

Choose a reason for hiding this comment

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

runAsUserID is intentionally made private to hide UID/GID from the caller. You can use 10001 directly in your test. We do this in other places as well, for example,

Expect(*nodeContainer.SecurityContext.RunAsUser).To(BeEquivalentTo(10001))
.

Copy link
Contributor

@hjiawei hjiawei left a comment

Choose a reason for hiding this comment

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

See inline comments.

Copy link
Contributor

@hjiawei hjiawei left a comment

Choose a reason for hiding this comment

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

LGTM with the latest changes. Thanks.

@Brian-McM Brian-McM merged commit da42497 into tigera:master May 18, 2023
3 checks passed
@sridhartigera sridhartigera deleted the host-path-init branch May 18, 2023 20:17
sridhartigera pushed a commit to sridhartigera/operator that referenced this pull request May 18, 2023
Use correct calico-node UID when running in non-privileged mode.
sridhartigera pushed a commit to sridhartigera/operator that referenced this pull request May 18, 2023
Use correct calico-node UID when running in non-privileged mode.
Brian-McM added a commit that referenced this pull request May 18, 2023
Merge pull request #2645 from sridhartigera/host-path-init
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.

None yet

5 participants