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.

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

MCO-1100: enable RHEL entitlements in on-cluster layering #4312

Closed
wants to merge 1 commit into from
Closed

MCO-1100: enable RHEL entitlements in on-cluster layering #4312

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
62 changes: 56 additions & 6 deletions pkg/controller/build/assets/buildah-build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
# custom build pod.
set -xeuo

ETC_PKI_ENTITLEMENT_MOUNTPOINT="${ETC_PKI_ENTITLEMENT_MOUNTPOINT:-}"
ETC_PKI_RPM_GPG_MOUNTPOINT="${ETC_PKI_RPM_GPG_MOUNTPOINT:-}"
ETC_YUM_REPOS_D_MOUNTPOINT="${ETC_YUM_REPOS_D_MOUNTPOINT:-}"

build_context="$HOME/context"

# Create a directory to hold our build context.
Expand All @@ -14,12 +18,58 @@ mkdir -p "$build_context/machineconfig"
cp /tmp/dockerfile/Dockerfile "$build_context"
cp /tmp/machineconfig/machineconfig.json.gz "$build_context/machineconfig/"

# Build our image using Buildah.
buildah bud \
--storage-driver vfs \
--authfile="$BASE_IMAGE_PULL_CREDS" \
--tag "$TAG" \
--file="$build_context/Dockerfile" "$build_context"
build_args=(
--log-level=DEBUG
--storage-driver vfs
--authfile="$BASE_IMAGE_PULL_CREDS"
--tag "$TAG"
--file="$build_context/Dockerfile"
)

mount_opts="z,rw"

# If we have RHSM certs, copy them into a tempdir to avoid SELinux issues, and
# tell Buildah about them.
rhsm_path="/var/run/secrets/rhsm"
if [[ -d "$rhsm_path" ]]; then
rhsm_certs="$(mktemp -d)"
cp -r -v "$rhsm_path/." "$rhsm_certs"
chmod -R 0755 "$rhsm_certs"
build_args+=("--volume=$rhsm_certs:/run/secrets/rhsm:$mount_opts")
fi

# If we have /etc/pki/entitlement certificates, commonly used with RHEL
# entitlements, copy them into a tempdir to avoid SELinux issues, and tell
# Buildah about them.
if [[ -n "$ETC_PKI_ENTITLEMENT_MOUNTPOINT" ]] && [[ -d "$ETC_PKI_ENTITLEMENT_MOUNTPOINT" ]]; then
configs="$(mktemp -d)"
cp -r -v "$ETC_PKI_ENTITLEMENT_MOUNTPOINT/." "$configs"
chmod -R 0755 "$configs"
build_args+=("--volume=$configs:$ETC_PKI_ENTITLEMENT_MOUNTPOINT:$mount_opts")
fi

# If we have /etc/yum.repos.d configs, commonly used with Red Hat Satellite
# subscriptions, copy them into a tempdir to avoid SELinux issues, and tell
# Buildah about them.
if [[ -n "$ETC_YUM_REPOS_D_MOUNTPOINT" ]] && [[ -d "$ETC_YUM_REPOS_D_MOUNTPOINT" ]]; then
configs="$(mktemp -d)"
cp -r -v "$ETC_YUM_REPOS_D_MOUNTPOINT/." "$configs"
chmod -R 0755 "$configs"
build_args+=("--volume=$configs:$ETC_YUM_REPOS_D_MOUNTPOINT:$mount_opts")
fi

# If we have /etc/pki/rpm-gpg configs, commonly used with Red Hat Satellite
# subscriptions, copy them into a tempdir to avoid SELinux issues, and tell
# Buildah about them.
if [[ -n "$ETC_PKI_RPM_GPG_MOUNTPOINT" ]] && [[ -d "$ETC_PKI_RPM_GPG_MOUNTPOINT" ]]; then
configs="$(mktemp -d)"
cp -r -v "$ETC_PKI_RPM_GPG_MOUNTPOINT/." "$configs"
chmod -R 0755 "$configs"
build_args+=("--volume=$configs:$ETC_PKI_RPM_GPG_MOUNTPOINT:$mount_opts")
fi
Comment on lines +41 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

For modularity, you could make a function that encapsulates this, only if you wanted to. This isnt really a mandatory, as your code looks good! just a suggestion:

function prepare_and_mount_dir {
	if [[ -n "$mount_point" ]] && [[ -d "$mount_point" ]]; then
		configs=$(mktemp -d)
		cp -r -v "$mount_point/." "$configs"
		chmod -R 0755 "$configs"
		build_args+=("--volume=$configs:$mount_point:$mount_opts")
	fi
}

prepare_and_mount_dir "RHSM Certs" "$rhsm_path" "rhsm_certs"
prepare_and_mount_dir "RPM-GPG Configs" "$ETC_PKI_RPM_GPG_MOUNTPOINT" "rpm_gpg_configs"

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to do that, but I couldn't get it to work quite the way that I wanted it to. Admittedly, my Bash is a little rusty. But I did think of two interesting paths forward for the future:

  1. There is a Python3 interpreter available in the official Buildah image, the MCO image, and the RHCOS image. So if I were so inclined, I could re-write this in Python. That would open the door to writing unit tests around the script. Although, one doesn't strictly need Python to do unit tests since Bats exists.
  2. Use Go instead of Bash to orchestrate things. Instead of this Bash script, we would add another binary to the MCO container which would get called instead. As a starting point, this binary could do what this Bash script does, but it could eventually do so much more.


# Build our image.
buildah bud "${build_args[@]}" "$build_context"

# Push our built image.
buildah push \
Expand Down
103 changes: 98 additions & 5 deletions pkg/controller/build/build_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,17 @@ import (
"github.com/openshift/machine-config-operator/internal/clients"
)

const (
// Name of the etc-pki-entitlement secret from the openshift-config-managed namespace.
etcPkiEntitlementSecretName = "etc-pki-entitlement"

// Name of the etc-pki-rpm-gpg secret.
etcPkiRpmGpgSecretName = "etc-pki-rpm-gpg"

// Name of the etc-yum-repos-d ConfigMap.
etcYumReposDConfigMapName = "etc-yum-repos-d"
)

const (
targetMachineConfigPoolLabel = "machineconfiguration.openshift.io/targetMachineConfigPool"
// TODO(zzlotnik): Is there a constant for this someplace else?
Expand Down Expand Up @@ -472,6 +483,20 @@ func (ctrl *Controller) customBuildPodUpdater(pod *corev1.Pod) error {

ps := newPoolState(pool)

// We cannot solely rely upon the pod phase to determine whether the build
// pod is in an error state. This is because it is possible for the build
// container to enter an error state while the wait-for-done container is
// still running. The pod phase in this state will still be "Running" as
// opposed to error.
if isBuildPodError(pod) {
if err := ctrl.markBuildFailed(ps); err != nil {
return err
}

ctrl.enqueueMachineConfigPool(pool)
return nil
}

switch pod.Status.Phase {
case corev1.PodPending:
if !ps.IsBuildPending() {
Expand Down Expand Up @@ -503,6 +528,22 @@ func (ctrl *Controller) customBuildPodUpdater(pod *corev1.Pod) error {
return nil
}

// Determines if the build pod is in an error state by examining the individual
// container statuses. Returns true if a single container is in an error state.
func isBuildPodError(pod *corev1.Pod) bool {
for _, container := range pod.Status.ContainerStatuses {
if container.State.Waiting != nil && container.State.Waiting.Reason == "ErrImagePull" {
return true
}

if container.State.Terminated != nil && container.State.Terminated.ExitCode != 0 {
return true
}
}

return false
}

func (ctrl *Controller) handleConfigMapError(pools []*mcfgv1.MachineConfigPool, err error, key interface{}) {
klog.V(2).Infof("Error syncing configmap %v: %v", key, err)
utilruntime.HandleError(err)
Expand Down Expand Up @@ -950,17 +991,69 @@ func (ctrl *Controller) getBuildInputs(ps *poolState) (*buildInputs, error) {
return nil, fmt.Errorf("could not get MachineConfig %s: %w", currentMC, err)
}

etcPkiEntitlements, err := ctrl.getOptionalSecret(etcPkiEntitlementSecretName)
if err != nil {
return nil, err
}

etcPkiRpmGpgKeys, err := ctrl.getOptionalSecret(etcPkiRpmGpgSecretName)
if err != nil {
return nil, err
}

etcYumReposDConfigs, err := ctrl.getOptionalConfigMap(etcYumReposDConfigMapName)
if err != nil {
return nil, err
}

inputs := &buildInputs{
onClusterBuildConfig: onClusterBuildConfig,
osImageURL: osImageURL,
customDockerfiles: customDockerfiles,
pool: ps.MachineConfigPool(),
machineConfig: mc,
onClusterBuildConfig: onClusterBuildConfig,
osImageURL: osImageURL,
customDockerfiles: customDockerfiles,
pool: ps.MachineConfigPool(),
machineConfig: mc,
etcPkiEntitlementKeys: etcPkiEntitlements,
etcYumReposDConfigs: etcYumReposDConfigs,
etcPkiRpmGpgKeys: etcPkiRpmGpgKeys,
}

return inputs, nil
}

// Fetches an optional secret to inject into the build. Returns a nil error if
// the secret is not found.
func (ctrl *Controller) getOptionalSecret(secretName string) (*corev1.Secret, error) {
optionalSecret, err := ctrl.kubeclient.CoreV1().Secrets(ctrlcommon.MCONamespace).Get(context.TODO(), secretName, metav1.GetOptions{})
if err == nil {
klog.Infof("Optional build secret %q found, will include in build", secretName)
return optionalSecret, nil
}

if k8serrors.IsNotFound(err) {
klog.Infof("Could not find optional secret %q, will not include in build", secretName)
return nil, nil
}

return nil, fmt.Errorf("could not retrieve optional secret: %s: %w", secretName, err)
}

// Fetches an optional ConfigMap to inject into the build. Returns a nil error if
// the ConfigMap is not found.
func (ctrl *Controller) getOptionalConfigMap(configmapName string) (*corev1.ConfigMap, error) {
optionalConfigMap, err := ctrl.kubeclient.CoreV1().ConfigMaps(ctrlcommon.MCONamespace).Get(context.TODO(), configmapName, metav1.GetOptions{})
if err == nil {
klog.Infof("Optional build ConfigMap %q found, will include in build", configmapName)
return optionalConfigMap, nil
}

if k8serrors.IsNotFound(err) {
klog.Infof("Could not find ConfigMap %q, will not include in build", configmapName)
return nil, nil
}

return nil, fmt.Errorf("could not retrieve optional ConfigMap: %s: %w", configmapName, err)
}
Comment on lines 1022 to +1055
Copy link
Contributor

Choose a reason for hiding this comment

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

another non mandatory suggestion. these can be combined into a "getOptionalResource" and just pass in a resource type, do something like, then combine the rest

if resourceType == "Secret" {
        resource, err = ctrl.kubeclient.CoreV1().Secrets(ctrlcommon.MCONamespace).Get(context.TODO(), resourceName, metav1.GetOptions{})
    } else if resourceType == "ConfigMap" {
        resource, err = ctrl.kubeclient.CoreV1().ConfigMaps(ctrlcommon.MCONamespace).Get(context.TODO(), resourceName, metav1.GetOptions{})
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good idea for the future, especially since both of these helpers only really concern themselves about the existence of the resource. That would blend nicely with some future refactoring ideas that I have.


// Prepares all of the objects needed to perform an image build.
func (ctrl *Controller) prepareForBuild(inputs *buildInputs) (ImageBuildRequest, error) {
ibr := newImageBuildRequestFromBuildInputs(inputs)
Expand Down
Loading