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

WIP Stop using the manifest-tool secrets in jobs #4404

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 1 addition & 3 deletions cmd/ci-operator/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,6 @@ type options struct {
dependencyOverrides stringSlice

targetAdditionalSuffix string
manifestToolDockerCfg string
localRegistryDNS string

restrictNetworkAccess bool
Expand Down Expand Up @@ -504,7 +503,6 @@ func bindOptions(flag *flag.FlagSet) *options {

flag.StringVar(&opt.targetAdditionalSuffix, "target-additional-suffix", "", "Inject an additional suffix onto the targeted test's 'as' name. Used for adding an aggregate index")

flag.StringVar(&opt.manifestToolDockerCfg, "manifest-tool-dockercfg", "/secrets/manifest-tool/.dockerconfigjson", "The dockercfg file path to be used to push the manifest listed image after build. This is being used by the manifest-tool binary.")
flag.StringVar(&opt.localRegistryDNS, "local-registry-dns", "image-registry.openshift-image-registry.svc:5000", "Defines the target image registry.")

opt.resultsOptions.Bind(flag)
Expand Down Expand Up @@ -902,7 +900,7 @@ func (o *options) Run() []error {
// load the graph from the configuration
buildSteps, promotionSteps, err := defaults.FromConfig(ctx, o.configSpec, &o.graphConfig, o.jobSpec, o.templates, o.writeParams, o.promote, o.clusterConfig,
o.podPendingTimeout, leaseClient, o.targets.values, o.cloneAuthConfig, o.pullSecret, o.pushSecret, o.censor, o.hiveKubeconfig,
o.consoleHost, o.nodeName, nodeArchitectures, o.targetAdditionalSuffix, o.manifestToolDockerCfg, o.localRegistryDNS, streams, injectedTest)
o.consoleHost, o.nodeName, nodeArchitectures, o.targetAdditionalSuffix, o.localRegistryDNS, streams, injectedTest)
if err != nil {
return []error{results.ForReason("defaulting_config").WithError(err).Errorf("failed to generate steps from config: %v", err)}
}
Expand Down
1 change: 0 additions & 1 deletion images/ci-operator/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,5 @@ FROM registry.access.redhat.com/ubi9/ubi-minimal:latest

RUN microdnf install -y git python3 findutils tar jq

ADD manifest-tool /usr/bin/manifest-tool
ADD ci-operator /usr/bin/ci-operator
ENTRYPOINT ["/usr/bin/ci-operator"]
3 changes: 0 additions & 3 deletions pkg/api/constant.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@ const (
GCSUploadCredentialsSecret = "gce-sa-credentials-gcs-publisher"
GCSUploadCredentialsSecretMountPath = "/secrets/gcs"

ManifestToolLocalPusherSecret = "manifest-tool-local-pusher"
ManifestToolLocalPusherSecretMountPath = "/secrets/manifest-tool"

ReleaseAnnotationSoftDelete = "release.openshift.io/soft-delete"

// DPTPRequesterLabel is the label on a Kubernates CR whose value indicates the automated tool that requests the CR
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func AddToManager(mgr manager.Manager, architectures []string, dockerCfgPath str
logger: logger,
client: mgr.GetClient(),
architectures: architectures,
manifestPusher: manifestpusher.NewManifestPusher(logger, registryURL, dockerCfgPath),
manifestPusher: manifestpusher.NewManifestPusher(logger, registryURL),
imageMirrorer: &ocImage{log: logger, registryConfig: dockerCfgPath},
scheme: mgr.GetScheme(),
}); err != nil {
Expand Down
3 changes: 1 addition & 2 deletions pkg/defaults/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ func FromConfig(
nodeName string,
nodeArchitectures []string,
targetAdditionalSuffix string,
manifestToolDockerCfg string,
localRegistryDNS string,
integratedStreams map[string]*configresolver.IntegratedStream,
injectedTest bool,
Expand All @@ -94,7 +93,7 @@ func FromConfig(
if err != nil {
return nil, nil, fmt.Errorf("could not get build client for cluster config: %w", err)
}
buildClient := steps.NewBuildClient(client, buildGetter.RESTClient(), nodeArchitectures, manifestToolDockerCfg, localRegistryDNS)
buildClient := steps.NewBuildClient(client, buildGetter.RESTClient(), nodeArchitectures, localRegistryDNS)

templateGetter, err := templateclientset.NewForConfig(clusterConfig)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/defaults/defaults_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1285,7 +1285,7 @@ func TestFromConfig(t *testing.T) {
t.Fatal(err)
}
}
buildClient := steps.NewBuildClient(client, nil, nil, "", "")
buildClient := steps.NewBuildClient(client, nil, nil, "")
var templateClient steps.TemplateClient
podClient := kubernetes.NewPodClient(client, nil, nil, 0)

Expand Down
36 changes: 23 additions & 13 deletions pkg/manifestpusher/manifestpusher.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package manifestpusher

import (
"fmt"
"os"

"github.com/estesp/manifest-tool/v2/pkg/registry"
"github.com/estesp/manifest-tool/v2/pkg/types"
Expand All @@ -12,30 +13,31 @@ import (
)

const (
nodeArchitectureLabel = "kubernetes.io/arch"
nodeArchitectureLabel = "kubernetes.io/arch"
serviceAccountTokenPath = "/var/run/secrets/kubernetes.io/serviceaccount/token"
)

type ManifestPusher interface {
PushImageWithManifest(builds []buildv1.Build, targetImageRef string) error
}

func NewManifestPusher(logger *logrus.Entry, registryURL string, dockercfgPath string) ManifestPusher {
return &manifestPusher{
logger: logger,
registryURL: registryURL,
dockercfgPath: dockercfgPath,
}
func NewManifestPusher(logger *logrus.Entry, registryURL string) ManifestPusher {
return &manifestPusher{logger: logger, registryURL: registryURL}
}

type manifestPusher struct {
logger *logrus.Entry
registryURL string
dockercfgPath string
logger *logrus.Entry
registryURL string
}

func (m manifestPusher) PushImageWithManifest(builds []buildv1.Build, targetImageRef string) error {
srcImages := []types.ManifestEntry{}

serviceAccountToken, err := resolveServiceAccountToken()
if err != nil {
return fmt.Errorf("couldn't get the service account token: %w", err)
}

for _, build := range builds {
srcImages = append(srcImages, types.ManifestEntry{
Image: fmt.Sprintf("%s/%s/%s", m.registryURL, build.Spec.Output.To.Namespace, build.Spec.Output.To.Name),
Expand All @@ -47,14 +49,14 @@ func (m manifestPusher) PushImageWithManifest(builds []buildv1.Build, targetImag
}

digest, _, err := registry.PushManifestList(
"", // username: we don't we use basic auth
"", // password: "
"ci-operator",
serviceAccountToken,
types.YAMLInput{Image: fmt.Sprintf("%s/%s", m.registryURL, targetImageRef), Manifests: srcImages},
false, // --ignore-missing. We don't want to ignore missing images.
true, // --insecure to allow pushing to the local registry.
false, // --plain-http is false by default in manifest-tool. False for the OpenShift registry.
types.Docker, // we only need docker type manifest.
m.dockercfgPath,
"",
)
if err != nil {
return err
Expand All @@ -63,3 +65,11 @@ func (m manifestPusher) PushImageWithManifest(builds []buildv1.Build, targetImag

return nil
}

func resolveServiceAccountToken() (string, error) {
data, err := os.ReadFile(serviceAccountTokenPath)
if err != nil {
return "", fmt.Errorf("failed to read token file: %w", err)
}
return string(data), nil
}
11 changes: 0 additions & 11 deletions pkg/prowgen/podspec.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,6 @@ var defaultPodSpec = corev1.PodSpec{
MountPath: cioperatorapi.GCSUploadCredentialsSecretMountPath,
ReadOnly: true,
},
{
Name: "manifest-tool-local-pusher",
MountPath: cioperatorapi.ManifestToolLocalPusherSecretMountPath,
ReadOnly: true,
},
},
},
},
Expand All @@ -67,12 +62,6 @@ var defaultPodSpec = corev1.PodSpec{
Secret: &corev1.SecretVolumeSource{SecretName: "result-aggregator"},
},
},
{
Name: "manifest-tool-local-pusher",
VolumeSource: corev1.VolumeSource{
Secret: &corev1.SecretVolumeSource{SecretName: cioperatorapi.ManifestToolLocalPusherSecret},
},
},
},
}

Expand Down
23 changes: 8 additions & 15 deletions pkg/steps/build_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,22 @@ type BuildClient interface {
loggingclient.LoggingClient
Logs(namespace, name string, options *buildapi.BuildLogOptions) (io.ReadCloser, error)
NodeArchitectures() []string
ManifestToolDockerCfg() string
LocalRegistryDNS() string
}

type buildClient struct {
loggingclient.LoggingClient
client rest.Interface
nodeArchitectures []string
manifestToolDockerCfg string
localRegistryDNS string
client rest.Interface
nodeArchitectures []string
localRegistryDNS string
}

func NewBuildClient(client loggingclient.LoggingClient, restClient rest.Interface, nodeArchitectures []string, manifestToolDockerCfg, localRegistryDNS string) BuildClient {
func NewBuildClient(client loggingclient.LoggingClient, restClient rest.Interface, nodeArchitectures []string, localRegistryDNS string) BuildClient {
return &buildClient{
LoggingClient: client,
client: restClient,
nodeArchitectures: nodeArchitectures,
manifestToolDockerCfg: manifestToolDockerCfg,
localRegistryDNS: localRegistryDNS,
LoggingClient: client,
client: restClient,
nodeArchitectures: nodeArchitectures,
localRegistryDNS: localRegistryDNS,
}
}

Expand All @@ -52,10 +49,6 @@ func (c *buildClient) NodeArchitectures() []string {
return c.nodeArchitectures
}

func (c *buildClient) ManifestToolDockerCfg() string {
return c.manifestToolDockerCfg
}

func (c *buildClient) LocalRegistryDNS() string {
return c.localRegistryDNS
}
2 changes: 1 addition & 1 deletion pkg/steps/index_generator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ func TestDatabaseIndex(t *testing.T) {
if err := yaml.Unmarshal(rawImageStreamTag, ist); err != nil {
t.Fatalf("failed to unmarshal imagestreamTag: %v", err)
}
actual, actualErr := databaseIndex(NewBuildClient(loggingclient.New(fakectrlruntimeclient.NewClientBuilder().WithObjects(ist, image).Build()), nil, nil, "", ""),
actual, actualErr := databaseIndex(NewBuildClient(loggingclient.New(fakectrlruntimeclient.NewClientBuilder().WithObjects(ist, image).Build()), nil, nil, ""),
testCase.isTagName, "ns")
if diff := cmp.Diff(testCase.expectedErr, actualErr, testhelper.EquateErrorMessage); diff != "" {
t.Fatalf("actual did not match expected, diff: %s", diff)
Expand Down
2 changes: 1 addition & 1 deletion pkg/steps/source.go
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ func handleBuilds(ctx context.Context, buildClient BuildClient, podClient kubern
}

if len(errs) == 0 {
manifestPusher := manifestpusher.NewManifestPusher(logrus.WithField("for-build", build.Name), buildClient.LocalRegistryDNS(), buildClient.ManifestToolDockerCfg())
manifestPusher := manifestpusher.NewManifestPusher(logrus.WithField("for-build", build.Name), buildClient.LocalRegistryDNS())
if err := manifestPusher.PushImageWithManifest(builds, fmt.Sprintf("%s/%s", build.Spec.Output.To.Namespace, build.Spec.Output.To.Name)); err != nil {
errs = append(errs, err)
}
Expand Down
10 changes: 5 additions & 5 deletions pkg/steps/source_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ func TestWaitForBuild(t *testing.T) {
CompletionTimestamp: &end,
},
},
).Build()), nil, nil, "", ""),
).Build()), nil, nil, ""),
expected: fmt.Errorf("build didn't start running within 0s (phase: Pending)"),
},
{
Expand Down Expand Up @@ -484,7 +484,7 @@ func TestWaitForBuild(t *testing.T) {
Namespace: ns,
},
},
).Build()), nil, nil, "", ""),
).Build()), nil, nil, ""),
expected: fmt.Errorf("build didn't start running within 0s (phase: Pending):\nFound 0 events for Pod some-build-build:"),
},
{
Expand Down Expand Up @@ -525,7 +525,7 @@ func TestWaitForBuild(t *testing.T) {
}},
},
},
).Build()), nil, nil, "", ""),
).Build()), nil, nil, ""),
expected: fmt.Errorf(`build didn't start running within 0s (phase: Pending):
* Container the-container is not ready with reason the_reason and message the_message
Found 0 events for Pod some-build-build:`),
Expand All @@ -544,7 +544,7 @@ Found 0 events for Pod some-build-build:`),
StartTimestamp: &start,
CompletionTimestamp: &end,
},
}).Build()), nil, nil, "", ""),
}).Build()), nil, nil, ""),
timeout: 30 * time.Minute,
},
{
Expand Down Expand Up @@ -588,7 +588,7 @@ Found 0 events for Pod some-build-build:`),
Time: now.Add(-59 * time.Minute),
},
},
}).Build()), nil, nil, "", ""),
}).Build()), nil, nil, ""),
timeout: 30 * time.Minute,
},
{
Expand Down
1 change: 0 additions & 1 deletion test/e2e/framework/ci-operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ func newCiOperatorCommand(t *T) CiOperatorCommand {
cmd := exec.CommandContext(ctx, "ci-operator",
"--input-hash="+strconv.Itoa(rand.Int()), // we need unique namespaces
GCSPushCredentialsFlag(t),
ManifestToolCredentialsFlag(t),
LocalRegistryDNSFlag(t),
)
cmd.Env = append(cmd.Env, KubernetesClientEnv(t)...)
Expand Down
10 changes: 0 additions & 10 deletions test/e2e/framework/framework.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,16 +114,6 @@ func GCSPushCredentialsFlag(t *T) string {
return flag("gcs-upload-secret", value)
}

// ManifestToolCredentialsFlag formats a flag to provide access to push the manifest listed image
// to the target registry for ci-operator, failing if the required env is not present to supply it.
func ManifestToolCredentialsFlag(t *T) string {
value, set := os.LookupEnv("MANIFEST_TOOL_SECRET")
if !set {
t.Fatal("required environment MANIFEST_TOOL_SECRET is not set")
}
return flag("manifest-tool-dockercfg", value)
}

// LocalRegistryDNSFlag formats a flag for the targeted image registry DNS for ci-operator,
// failing if the required env is not present to supply it.
func LocalRegistryDNSFlag(t *T) string {
Expand Down