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

Store auth in keyring and transfer it to nydusd via environment variable #512

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sctb512
Copy link
Member

@sctb512 sctb512 commented Jul 21, 2023

  1. Store auth for nydusd in the keyring instead of the configuration file.
  2. Set auth via environment variable to avoid displaying it in command parameter.

Related to dragonflyoss/nydus#1381 and dragonflyoss/nydus#1382

@sctb512 sctb512 changed the title Store auth in keyring and set to nydusd via environment variable Store auth in keyring and transfer it to nydusd via environment variable Jul 21, 2023
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #512 (c317c16) into main (0e3857e) will increase coverage by 0.53%.
Report is 2 commits behind head on main.
The diff coverage is 50.76%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #512      +/-   ##
==========================================
+ Coverage   37.83%   38.37%   +0.53%     
==========================================
  Files          60       62       +2     
  Lines        7092     7339     +247     
==========================================
+ Hits         2683     2816     +133     
- Misses       4097     4188      +91     
- Partials      312      335      +23     
Files Changed Coverage Δ
config/config.go 31.38% <ø> (ø)
config/daemonconfig/daemonconfig.go 0.00% <0.00%> (ø)
config/global.go 30.86% <0.00%> (-0.79%) ⬇️
pkg/daemon/daemon.go 0.00% <0.00%> (ø)
pkg/manager/daemon_adaptor.go 0.00% <0.00%> (ø)
pkg/manager/manager.go 14.84% <0.00%> (-0.05%) ⬇️
pkg/auth/keyring.go 60.11% <60.11%> (ø)
pkg/auth/cache.go 64.58% <64.58%> (ø)

... and 2 files with indirect coverage changes

@sctb512 sctb512 force-pushed the get-auth-from-keyring branch 6 times, most recently from af671f5 to d03befe Compare July 27, 2023 08:32
@@ -192,6 +195,19 @@ func (m *Manager) BuildDaemonCommand(d *daemon.Daemon, bin string, upgrade bool)

cmd := exec.Command(nydusdPath, args...)

if config.IsKeyringEnabled() && !config.IsFusedevSharedModeEnabled() {
Copy link
Member

Choose a reason for hiding this comment

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

Daemon has its own working mode which means nydusd daemons can work in the different modes in a single node.
Better to judge by daemon, we already have some helper functions doing this

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

err = daemonconfig.SupplementDaemonConfig(cfg, imageID, snapshotID, false, labels, params)
var updateErr error
err = daemonconfig.SupplementDaemonConfig(cfg, imageID, snapshotID, false, labels, params, func(imageHost string, keyChain *auth.PassKeyChain) {
logrus.Infof("add key for %s", imageHost)
Copy link
Member

Choose a reason for hiding this comment

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

It's better to lower the log level to Debug

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@@ -192,6 +195,19 @@ func (m *Manager) BuildDaemonCommand(d *daemon.Daemon, bin string, upgrade bool)

cmd := exec.Command(nydusdPath, args...)

if config.IsKeyringEnabled() && !config.IsFusedevSharedModeEnabled() {
c, err := daemonconfig.NewDaemonConfig(d.States.FsDriver, configPath)
Copy link
Member

@changweige changweige Jul 31, 2023

Choose a reason for hiding this comment

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

I am afraid the current way to identify a container image reference's host is not agile. For the dedicated mode nydusd daemon, it only has a rafs instance in its Instances where we can calculate what host is

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@@ -0,0 +1,42 @@
package keyring
Copy link
Member

Choose a reason for hiding this comment

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

Please add license claim

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@@ -66,6 +71,7 @@ type Daemon struct {
// fusedev shared mode: zero, one or more RAFS instances
// fscache shared mode: zero, one or more RAFS instances
Instances rafsSet
authCache *lru.Cache
Copy link
Member

Choose a reason for hiding this comment

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

Auth should be shared among different nydusd daemons. So why to bind the cache to a single nydusd daemon?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Move it to the manager.

@@ -151,6 +158,31 @@ func (d *Daemon) RemoveInstance(snapshotID string) {
d.DecRef()
}

func (d *Daemon) UpdateAuth(imageHost, auth string) error {
key, err := keyring.Add(imageHost, auth)
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to consider the atomicity when updating?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

}

data, err := keyring.Search(imageHost)
if err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Checking if err is nil looks strange.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@imeoer
Copy link
Collaborator

imeoer commented Jul 31, 2023

This approach seems to only solve the nydusd registry credential persistence problem, but when the credential is changed (e.g. registry user/pass dynamically generated by a credential helper), nydusd may not be able to timely detect the update. I think #504 is a better implementation.

@sctb512 sctb512 force-pushed the get-auth-from-keyring branch 4 times, most recently from 30ee1e2 to 25b9d09 Compare August 1, 2023 06:08
@sctb512
Copy link
Member Author

sctb512 commented Aug 1, 2023

This approach seems to only solve the nydusd registry credential persistence problem, but when the credential is changed (e.g. registry user/pass dynamically generated by a credential helper), nydusd may not be able to timely detect the update. I think #504 is a better implementation.

I believe this PR does not conflict with #504. It is better to get authentication from the credential server if snapshotter does not send it through env.

@sctb512 sctb512 force-pushed the get-auth-from-keyring branch 6 times, most recently from 92d4b96 to e79e344 Compare August 1, 2023 08:11
@sctb512 sctb512 requested a review from changweige August 1, 2023 11:15

import (
"github.com/pkg/errors"
"k8s.io/utils/lru"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems we have used github.com/golang/groupcache/lru.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

lock *sync.RWMutex
}

func NewKeyRing() *KeyRing {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe give some unit test cases for this?

Copy link
Member

Choose a reason for hiding this comment

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

Agree, please try to add some unit test, especially for keying garbage collection and if the boundary is correctly handled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -192,6 +199,25 @@ func (m *Manager) BuildDaemonCommand(d *daemon.Daemon, bin string, upgrade bool)

cmd := exec.Command(nydusdPath, args...)

if config.IsKeyringEnabled() && !d.IsSharedDaemon() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we support shared daemon?

Copy link
Member Author

@sctb512 sctb512 Aug 2, 2023

Choose a reason for hiding this comment

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

Yes, it calls c.FillAuth(keyChain) to fill auth before sending mount request to nydus daemon.

@sctb512 sctb512 force-pushed the get-auth-from-keyring branch 27 times, most recently from ee9c16d to a9c1dd0 Compare August 4, 2023 01:57
Copy link
Member

@changweige changweige left a comment

Choose a reason for hiding this comment

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

Could we also have an e2e test case defined in integration/entrypoint.sh

c.FillAuth(keyChain)
if config.IsKeyringEnabled() && fn != nil {
if err := fn(registryHost, keyChain); err != nil {
if errors.Is(err, unix.EINVAL) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the error type should be transparent for the caller

Signed-off-by: Bin Tang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants