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

[CI:DOCS] podman machine: enforce a single search registry #11498

Merged
merged 2 commits into from
Sep 10, 2021

Conversation

vrothberg
Copy link
Member

Enforce "docker.io" to be the only search registry. Short-name
resolution for remote clients is not fully supported since there is no
means to prompt. Enforcing a single registry works around the problem
since prompting only fires with more than one search registry.

Fixes: #11489
Signed-off-by: Valentin Rothberg [email protected]

CI:DOCS since machine isn't yet tested in CI.

/hold
... to make sure everyone's on board.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2021
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2021
@Luap99
Copy link
Member

Luap99 commented Sep 9, 2021

I think this should be done via the ignition file, https://github.com/containers/podman/blob/main/pkg/machine/ignition.go

@rhatdan
Copy link
Member

rhatdan commented Sep 9, 2021

Also is it possible to only do this iff the machine is set in enforcing mode?

@vrothberg
Copy link
Member Author

I think this should be done via the ignition file, https://github.com/containers/podman/blob/main/pkg/machine/ignition.go

Good idea!

Also is it possible to only do this iff the machine is set in enforcing mode?

Via ignition, I am absolutely not sure. But even via SSH it'll be tough since the short-name mode is not exposed in podman info. Since there may be config files at various places, we shouldn't guess.

@vrothberg
Copy link
Member Author

Now done via ignition

@vrothberg
Copy link
Member Author

@containers/podman-maintainers @mtrmac PTAL

@rhatdan
Copy link
Member

rhatdan commented Sep 9, 2021

I know nothing about ingition, so can not comment.
@dustymabe PTAL

@rhatdan
Copy link
Member

rhatdan commented Sep 9, 2021

@miabbott PTAL

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

The code LGTM.

Why the CI:DOCS? (I don’t care very much at all what policy Podman chooses for testing incoming PRs; I find it very irritating to see it routinely bypassed. Should this be fully tested? If this is intentional and consistent with the project’s policy, it would be nice to update CONTRIBUTING.md to match.)

pkg/machine/ignition.go Show resolved Hide resolved
pkg/machine/ignition.go Show resolved Hide resolved
@mtrmac
Copy link
Collaborator

mtrmac commented Sep 9, 2021

Why the CI:DOCS?

I can’t be bothered to read, it seems:

CI:DOCS since machine isn't yet tested in CI.

I apologize.

@mtrmac
Copy link
Collaborator

mtrmac commented Sep 9, 2021

Should podman machine document this somewhere?

@miabbott
Copy link
Contributor

miabbott commented Sep 9, 2021

Also is it possible to only do this iff the machine is set in enforcing mode?

Via ignition, I am absolutely not sure. But even via SSH it'll be tough since the short-name mode is not exposed in podman info. Since there may be config files at various places, we shouldn't guess.

Ignition does not have a native way of reacting to a configured state or even a configuration included in an Ignition config.

https://coreos.github.io/ignition/rationale/#ignition-configs-are-declarative

Ignition configs do not allow users to provide arbitrary logic (including scripts for Ignition to run). Users describe which filesystems must exist, which files must be created, which users must exist, and etc. Any further customization must use systemd services, created by Ignition.

If you wanted a change to only be applied when SELinux is enforcing, you'd have to write a systemd unit that can detect the condition and apply the change.

@rhatdan
Copy link
Member

rhatdan commented Sep 9, 2021

Ok let's go with this solution for now, and then modify it once we have prompting capabilities.

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/machine/ignition.go Outdated Show resolved Hide resolved
pkg/machine/ignition.go Outdated Show resolved Hide resolved
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 10, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Enforce "docker.io" to be the only search registry.  Short-name
resolution for remote clients is not fully supported since there is no
means to prompt.  Enforcing a single registry  works around the problem
since prompting only fires with more than one search registry.

Fixes: containers#11489
Signed-off-by: Valentin Rothberg <[email protected]>
By popular request, turn decimals to octal.  Most eyes are trained to
parse file permissions in octal.

[NO TESTS NEEDED] since machine isn't tested yet.

Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg
Copy link
Member Author

Updated. PTanotherL.

I'll be on PTO next week. In case this PR is still open and more requests fly in, feel free to kick it over the finish line.

@rhatdan
Copy link
Member

rhatdan commented Sep 10, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2021
@rhatdan
Copy link
Member

rhatdan commented Sep 10, 2021

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2021
@openshift-merge-robot openshift-merge-robot merged commit e604622 into containers:main Sep 10, 2021
@vrothberg vrothberg deleted the fix-11489 branch September 10, 2021 15:03
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Machines created by podman machine must alter registries.conf
7 participants