Skip to content

Bug 1940488: move entitlement related secrets back to mounts.conf #238

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

Merged
openshift-merge-robot merged 1 commit into from
Mar 31, 2021
Merged

Bug 1940488: move entitlement related secrets back to mounts.conf #238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions imagecontent/etc/containers/mounts.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
/run/secrets/rhsm:/run/secrets/rhsm
/run/secrets/etc-pki-entitlement:/run/secrets/etc-pki-entitlement
Copy link
Copy Markdown
Contributor Author

@gabemontero gabemontero Mar 31, 2021

Choose a reason for hiding this comment

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

so @nalind if we are running on RHCOS, with no entitlements, whatever flavor of error log we get from buildah if these files are missing are the only possible cause for concern perhaps

I'm going to try and bring up a IPI / rhcos cluster today, use a builder image from this PR, and get a sense of what if anything that looks like. Based on the results, we'll see if that steers us back to just changing the ordering in buildah

but of course let me know what you think

@wewang58 adding a regression test case with any build on a normal IPI / RHCOS / clusterbot cluster of this PR to see what the logs look like makes sense for you ... if the e2e's pass, the builds work, but we are just worried about scary but non-fatal messages in the build log

Copy link
Copy Markdown
Member

@nalind nalind Mar 31, 2021

Choose a reason for hiding this comment

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

IIRC it's not a fatal error message, but it's not something we have an API for suppressing.

Copy link
Copy Markdown
Contributor Author

@gabemontero gabemontero Mar 31, 2021

Choose a reason for hiding this comment

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

OK I'm launching a RHCOS cluster now. I'll get an example of what the message looks like and we can decide if it is a show stopper for this approach.

Copy link
Copy Markdown
Member

@nalind nalind Mar 31, 2021

Choose a reason for hiding this comment

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

I expect something along the lines of time="2021-03-30T21:08:24Z" level=warning msg="Path \"/run/secrets/redhat.repo\" from \"/etc/containers/mounts.conf\" doesn't exist, skipping" for each RUN instruction.

Copy link
Copy Markdown
Contributor Author

@gabemontero gabemontero Mar 31, 2021

Choose a reason for hiding this comment

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

hmmmm ... not terrible, if that is all ... though perhaps if there are a lot of RUN's and they pile up

my RHCOS cluster is no up, hope to confirm soon

Copy link
Copy Markdown
Contributor Author

@gabemontero gabemontero Mar 31, 2021

Choose a reason for hiding this comment

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

yep confirmed @nalind ... the following occurs with each RUN:

time="2021-03-31T17:16:09Z" level=warning msg="Path \"/run/secrets/etc-pki-entitlement\" from \"/etc/containers/mounts.conf\" doesn't exist, skipping"
time="2021-03-31T17:16:09Z" level=warning msg="Path \"/run/secrets/redhat.repo\" from \"/etc/containers/mounts.conf\" doesn't exist, skipping"

wdyt @bparees .... too noisy?

Copy link
Copy Markdown
Contributor

@bparees bparees Mar 31, 2021

Choose a reason for hiding this comment

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

given the apparent urgency to get a resolution here, i'd say we can merge this for now if we can clean it up after.

Copy link
Copy Markdown
Contributor Author

@gabemontero gabemontero Mar 31, 2021

Choose a reason for hiding this comment

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

ok thanks @bparees

since it will still be a day or two before we can pick to the 4.7 z stream, and I have test clusters up, I'll spend some time today developing locally the "change buildah ordering" alternative mentioned in the description that @nalind and I have discussed, just to explicitly asses its viability while I still have access to UPI test envs and the like

in the interim we can see if the escalation warnings come to fruition, etc.

Copy link
Copy Markdown
Member

@nalind nalind Mar 31, 2021

Choose a reason for hiding this comment

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

@rhatdan is taking a run at it in containers/buildah#3117

Copy link
Copy Markdown
Contributor Author

@gabemontero gabemontero Mar 31, 2021

Choose a reason for hiding this comment

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

awesome thanks @nalind

If my POC works with the buildah change I'll post a WIP/do not merge PR up for us to compare

/run/secrets/redhat.repo:/run/secrets/redhat.repo
83 changes: 0 additions & 83 deletions pkg/build/builder/daemonless.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,6 @@ import (

"github.com/openshift/builder/pkg/build/builder/cmd/dockercfg"
builderutil "github.com/openshift/builder/pkg/build/builder/util"
s2ifs "github.com/openshift/source-to-image/pkg/util/fs"
)

const (
defaultMountStart = "/run/secrets"
repoFile = "redhat.repo"
subMgrCertDir = "rhsm"
etcPkiEntitle = "etc-pki-entitlement"
)

var (
Expand Down Expand Up @@ -317,85 +309,10 @@ func buildDaemonlessImage(sc types.SystemContext, store storage.Store, isolation

func generateTransientMounts() []string {
mounts := []string{}
mounts = appendRHSMMount(defaultMountStart, mounts)
mounts = appendETCPKIMount(defaultMountStart, mounts)
mounts = appendRHRepoMount(defaultMountStart, mounts)
mounts = appendCATrustMount(mounts)
return mounts
}

func appendRHRepoMount(pathStart string, mounts []string) []string {
path := filepath.Join(pathStart, repoFile)
st, err := os.Stat(path)
if err != nil {
// since the presence of the repo file is not a given, we won't log this a V(0)
log.V(4).Infof("Failed to stat %s, falling back to the Red Hat yum repository configuration in the build's base image. Error: %v", path, err)
return mounts
}
if !st.Mode().IsRegular() {
// if the file is there, but an unexpected type, then always have log show up via V(0)
log.V(0).Infof("Falling back to the Red Hat yum repository configuration in the build's base image: %s secrets location %s is a directory.", repoFile, path)
return mounts
}

// Add a bind of repo file, to pass along anything that the runtime mounted from the node
log.V(0).Infof("Adding transient rw bind mount for %s", path)
tmpDir, err := ioutil.TempDir("/tmp", repoFile+"-copy")
if err != nil {
log.V(0).Infof("Falling back to the Red Hat yum repository configuration in the base image: failed to create tmpdir for %s secret: %v", repoFile, err)
return mounts
}
fs := s2ifs.NewFileSystem()
err = fs.Copy(path, filepath.Join(tmpDir, repoFile), map[string]string{})
if err != nil {
log.V(0).Infof("Falling back to the Red Hat yum repository configuration in the base image: failed to copy %s secret: %v", repoFile, err)
return mounts
}
mounts = append(mounts, fmt.Sprintf("%s:/run/secrets/%s:rw,nodev,noexec,nosuid", filepath.Join(tmpDir, repoFile), repoFile))
return mounts
}

func coreAppendSecretLinksToDirs(pathStart, pathEnd string, mounts []string) []string {
path := filepath.Join(pathStart, pathEnd)
st, err := os.Stat(path)
if err != nil {
// since the presence of dir secret is not a given, we won't log this a V(0)
log.V(0).Infof("Red Hat subscription content will not be available in this build: failed to stat directory %s: %v", path, err)
return mounts
}
if !st.IsDir() && st.Mode()&os.ModeSymlink == 0 {
// if the file is there, but an unexpected type, then always have log show up via V(0)
log.V(0).Infof("Red Hat subscription content will not be available in this build: %s is not a directory", path)
return mounts
}

// Add a bind of dir secret, to pass along anything that the runtime mounted from the node
log.V(0).Infof("Adding transient rw bind mount for %s", path)
tmpDir, err := ioutil.TempDir("/tmp", pathEnd+"-copy")
if err != nil {
log.V(0).Infof("Red Hat subscription content will not be available in this build: failed to create tmpdir for %s secrets: %v", pathEnd, err)
return mounts
}
fs := s2ifs.NewFileSystem()
err = fs.CopyContents(path, tmpDir, map[string]string{})
if err != nil {
log.V(0).Infof("Red Hat subscription content will not be available in this build: failed to copy %s secrets: %v", pathEnd, err)
return mounts
}
mounts = append(mounts, fmt.Sprintf("%s:/run/secrets/%s:rw,nodev,noexec,nosuid", tmpDir, pathEnd))
return mounts
return mounts
}

func appendETCPKIMount(pathStart string, mounts []string) []string {
return coreAppendSecretLinksToDirs(pathStart, etcPkiEntitle, mounts)

}

func appendRHSMMount(pathStart string, mounts []string) []string {
return coreAppendSecretLinksToDirs(pathStart, subMgrCertDir, mounts)
}

func appendCATrustMount(mounts []string) []string {
mountCAEnv, exists := os.LookupEnv("BUILD_MOUNT_ETC_PKI_CATRUST")
if !exists {
Expand Down
243 changes: 0 additions & 243 deletions pkg/build/builder/daemonless_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
package builder

import (
"io/ioutil"
"os"
"path/filepath"
"reflect"
"strings"
"testing"
Expand Down Expand Up @@ -226,247 +224,6 @@ func TestParseDropCapabilities(t *testing.T) {
}
}

type appendFunc func(string, []string) []string

func coreTestSubscriptionDirMounts(t *testing.T, path string, fn appendFunc) {
cases := []struct {
name string
mode os.FileMode
exists bool
badLink bool
}{
{
name: "dir",
mode: os.ModeDir,
exists: true,
},
{
name: "good-link",
mode: os.ModeSymlink,
exists: true,
},
{
name: "bad-link",
mode: os.ModeSymlink,
exists: true,
badLink: true,
},
{
name: "normal-file",
exists: true,
},
{
name: "non-existent-file",
exists: false,
},
}
for _, tc := range cases {
tmpDir, err := ioutil.TempDir(os.TempDir(), tc.name)
if err != nil {
t.Fatalf(err.Error())
}
defer os.RemoveAll(tmpDir)
if tc.exists {
switch {
case tc.mode&os.ModeType == 0:
// regular file
_, err = os.Create(filepath.Join(tmpDir, path))
if err != nil {
t.Fatalf(err.Error())
}

case tc.mode&os.ModeDir != 0:
// dir
err = os.Mkdir(filepath.Join(tmpDir, path), 0777)
if err != nil {
t.Fatalf(err.Error())
}
_, err = os.Create(filepath.Join(tmpDir, path, "ca"))
if err != nil {
t.Fatalf(err.Error())
}
case tc.mode&os.ModeSymlink != 0:
// symlink
if tc.badLink {
_, err = os.Create(filepath.Join(tmpDir, path+"-link-destination"))
if err != nil {
t.Fatalf(err.Error())
}
} else {
err = os.Mkdir(filepath.Join(tmpDir, path+"-link-destination"), 0777)
if err != nil {
t.Fatalf(err.Error())
}
_, err = os.Create(filepath.Join(tmpDir, path+"-link-destination", "ca"))
if err != nil {
t.Fatalf(err.Error())
}
}
err = os.Symlink(filepath.Join(tmpDir, path+"-link-destination"), filepath.Join(tmpDir, path))
if err != nil {
t.Fatalf(err.Error())
}
}
mounts := fn(tmpDir, []string{})
if tc.mode.IsRegular() && len(mounts) != 0 {
t.Fatalf("bad mounts len %d for %s", len(mounts), tc.name)
}
if tc.badLink && len(mounts) != 0 {
t.Fatalf("bad mounts len %d for %s", len(mounts), tc.name)
}
if tc.badLink {
continue
}
if tc.mode.IsRegular() {
continue
}
if len(mounts) != 1 {
t.Fatalf("bad mounts len %d for %s", len(mounts), tc.name)
}
splitMount := strings.Split(mounts[0], ":")
if len(splitMount) > 0 {
copyDir := splitMount[0]
files, err := ioutil.ReadDir(copyDir)
if err != nil {
t.Fatalf(err.Error())
}
found := false
for _, file := range files {
if file.Name() == "ca" {
found = true
break
}
}
os.RemoveAll(splitMount[0])
if !found {
t.Fatalf("did not find ca in copy")
}
} else {
t.Fatalf("bad mount string for %s: %s", tc.name, mounts[0])
}
} else {
mounts := fn(filepath.Join(tmpDir, "does-not-exist"), []string{})
if len(mounts) != 0 {
t.Fatalf("returned mount when it did not exist")
}
}

}

}

func TestETCPKIMount(t *testing.T) {
coreTestSubscriptionDirMounts(t, "etc-pki-entitlement", appendETCPKIMount)
}

func TestRHSMMount(t *testing.T) {
coreTestSubscriptionDirMounts(t, "rhsm", appendRHSMMount)
}

func TestRHRepoMount(t *testing.T) {
path := "redhat.repo"
cases := []struct {
name string
mode os.FileMode
exists bool
badLink bool
}{
{
name: "dir",
mode: os.ModeDir,
exists: true,
},
{
name: "good-link",
mode: os.ModeSymlink,
exists: true,
},
{
name: "bad-link",
mode: os.ModeSymlink,
exists: true,
badLink: true,
},
{
name: "normal-file",
exists: true,
},
{
name: "non-existent-file",
exists: false,
},
}
for _, tc := range cases {
tmpDir, err := ioutil.TempDir(os.TempDir(), tc.name)
if err != nil {
t.Fatalf(err.Error())
}
defer os.RemoveAll(tmpDir)
if tc.exists {
switch {
case tc.mode&os.ModeType == 0:
// regular file
_, err = os.Create(filepath.Join(tmpDir, path))
if err != nil {
t.Fatalf(err.Error())
}

case tc.mode&os.ModeDir != 0:
// dir
err = os.Mkdir(filepath.Join(tmpDir, path), 0777)
if err != nil {
t.Fatalf(err.Error())
}
case tc.mode&os.ModeSymlink != 0:
// symlink
if tc.badLink {
err = os.Mkdir(filepath.Join(tmpDir, path+"-link-destination"), 0777)
if err != nil {
t.Fatalf(err.Error())
}
} else {
_, err = os.Create(filepath.Join(tmpDir, path+"-link-destination"))
if err != nil {
t.Fatalf(err.Error())
}
}
err = os.Symlink(filepath.Join(tmpDir, path+"-link-destination"), filepath.Join(tmpDir, path))
if err != nil {
t.Fatalf(err.Error())
}
}
mounts := appendRHRepoMount(tmpDir, []string{})
if tc.mode.IsDir() && len(mounts) != 0 {
t.Fatalf("bad mounts len %d for %s", len(mounts), tc.name)
}
if tc.badLink && len(mounts) != 0 {
t.Fatalf("bad mounts len %d for %s", len(mounts), tc.name)
}
if tc.badLink {
continue
}
if tc.mode.IsDir() {
continue
}
if len(mounts) != 1 {
t.Fatalf("bad mounts len %d for %s", len(mounts), tc.name)
}
splitMount := strings.Split(mounts[0], ":")
if len(splitMount) > 0 {
os.RemoveAll(splitMount[0])
} else {
t.Fatalf("bad mount string for %s: %s", tc.name, mounts[0])
}
} else {
mounts := appendRHRepoMount(filepath.Join(tmpDir, "does-not-exist"), []string{})
if len(mounts) != 0 {
t.Fatalf("returned mount when it did not exist")
}
}

}
}

func TestAppendCATrustMount(t *testing.T) {
cases := []struct {
name string
Expand Down