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

fetch: Add support for providing HTTP headers #889

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cgwalters
Copy link
Member

I plan to use this in ignition-dracut to inject e.g. headers
from /usr/lib/os-release, so that a server (such as the
OpenShift 4 machine-config-operator server) can dispatch on them.

cgwalters added a commit to cgwalters/ignition-dracut that referenced this pull request Nov 22, 2019
Depends: coreos/ignition#889

So that the openshift/machine-config-operator server can dispatch
on them to aid with Ignition spec 3 transitions.
@cgwalters
Copy link
Member Author

Required by: coreos/ignition-dracut#136

I plan to use this in ignition-dracut to inject e.g. headers
from `/usr/lib/os-release`, so that a server (such as the
OpenShift 4 machine-config-operator server) can dispatch on them.
@lucab
Copy link
Contributor

lucab commented Nov 22, 2019

@cgwalters looking at the other dracut patches, I think your long term goal is to lift those from initramfs os-release via EnvironmentFile=.
If so, Instead of this broad --fetch-header flag, what about directly picking up ID and VERSION_ID from the env and, if populated, adding them to headers?

@@ -272,7 +276,17 @@ func (e *Engine) fetchReferencedConfig(cfgRef types.ConfigReference) (types.Conf
if err != nil {
return types.Config{}, err
}
rawCfg, err := e.Fetcher.FetchToBuffer(*u, resource.FetchOptions{})
headers := make(http.Header)
Copy link
Contributor

@ajeddeloh ajeddeloh Nov 22, 2019

Choose a reason for hiding this comment

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

This means the headers specified only happen to config fetches, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah.

@cgwalters
Copy link
Member Author

I think your long term goal is to lift those from initramfs os-release via EnvironmentFile=.

Long term? That's what the other patch already does, yes 😉

If so, Instead of this broad --fetch-header flag, what about directly picking up ID and VERSION_ID from the env and, if populated, adding them to headers?

I did think about adding this to Ignition by default, but passing things externally seemed more flexible (also, we avoid needing to have a "shell syntax parser" in Ignition). But, it does also make sense to me to have Ignition do some of this by default too.

I think we'd probably want the ability to specify external headers, even if we did some of this by default though?

But I don't have a really strong opinion on this - if others think it should be baked in, fine by me!

@bgilbert
Copy link
Contributor

I did think about adding this to Ignition by default, but passing things externally seemed more flexible (also, we avoid needing to have a "shell syntax parser" in Ignition). But, it does also make sense to me to have Ignition do some of this by default too.

I think we'd probably want the ability to specify external headers, even if we did some of this by default though?

Historically we didn't capture os-release into a header because on CL, the initramfs doesn't have a current copy of os-release (the initramfs is built too early for that). But if that's not the case on most other distros, I think it could make sense to build the functionality into Ignition directly.

IIUC this is behavior that doesn't need to be customized by users, but only by distros, right? In that case, we should consider making it a compile-time option via the distro mechanism, if we make it configurable at all.

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.

4 participants