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

Jump to bottom

providers/azure: support Azure IMDS #39

Closed

tormath1 wants to merge 3 commits into main from tormath1/azuredata

Contributor

tormath1 commented Apr 12, 2022 •

edited

Loading

In this PR, we add support for Azure IMDS on top of existing CustomData. If it fails to fetch base64 encoded userData Ignition will fallback on CustomData.

provider/azure: try to fetch user-data from IMDS could be upstreamed once the tests / validation are OK.

Note for reviewers

the last two commits: config: update headers_test to stop using strings.Title() and ci: add testing with Go 1.18 are from the upstream - it makes the CI happy.

tormath1 self-assigned this

tormath1 force-pushed the tormath1/azuredata branch from e0a88fb to d0ea0b2 Compare

April 12, 2022 17:03

tormath1 mentioned this pull request

sys-apps/ignition: pull Azure IMDS flatcar-archive/coreos-overlay#1814

Closed

2 tasks

tormath1 force-pushed the tormath1/azuredata branch 3 times, most recently from 1ea879a to 63b2cfc Compare

April 13, 2022 13:35

tormath1 mentioned this pull request

provider/azure: support userData on top of customData coreos/ignition#1347

Closed

bgilbert reviewed

View reviewed changes

Contributor

bgilbert left a comment

👋 Thanks for working on this! A few comments from upstream.

internal/resource/url.go Outdated

               		return ioutil.NopCloser(r), nil
               	case "gzip":
               		return gzip.NewReader(r)
+              	case "base64":

Contributor

bgilbert Apr 13, 2022

I'd prefer that we not do this here, and handle base64-decoding in the Azure provider instead. Otherwise this function will be a mix of cases that are supported by the Resource.Compression spec field and cases that are supported only for internal convenience.

internal/providers/azurestack/azurestack.go Outdated

@@ @@ -34,5 +34,5 @@ const ( @@
               // FetchConfig implements the platform.NewFetcher interface.
               func FetchConfig(f *resource.Fetcher) (types.Config, report.Report, error) {
-              	return azure.FetchFromOvfDevice(f, []string{CDS_FSTYPE_UDF, CDS_FSTYPE_ISO9660})
+              	return azure.FetchFromAzureMetadata(f, []string{CDS_FSTYPE_UDF, CDS_FSTYPE_ISO9660})

Contributor

bgilbert Apr 13, 2022

Have you confirmed that Azure Stack always supports the metadata service? If not, we should probably keep FetchFromOvfDevice here, and make FetchFromAzureMetadata a wrapper around it.

Contributor Author

tormath1 Apr 14, 2022

Right - thanks for the heads-up ! According to the documentation¹:

The Azure Instance Metadata Service isn't supported on Azure Stack Hub.

https://docs.microsoft.com/en-us/azure-stack/user/azure-stack-vm-considerations ↩

internal/providers/azure/azure.go Outdated

@@ @@ -56,15 +57,38 @@ const ( @@
               	CDS_FSTYPE_UDF = "udf"
               )
-              // FetchConfig wraps FetchOvfDevice to implement the platform.NewFetcher interface.
+              var (
+              	imdsURL = url.URL{

Contributor

bgilbert Apr 13, 2022

Nit: maybe imdsUserdataURL?

internal/providers/azure/azure.go Outdated

+              	// we first try to config from IMDS, in case of failure we fallback on the custom data.
+              	data, err := f.FetchToBuffer(imdsURL, resource.FetchOptions{Compression: "base64"})
+              	if err == nil {
+              		return util.ParseConfig(logger, data)

Contributor

bgilbert Apr 13, 2022

For user debugging purposes, let's log at Info level if we've read the config from userdata and if we've read it from custom data.

internal/providers/azure/azure.go Outdated

+              	// we first try to config from IMDS, in case of failure we fallback on the custom data.
+              	data, err := f.FetchToBuffer(imdsURL, resource.FetchOptions{Compression: "base64"})
+              	if err == nil {

Contributor

bgilbert Apr 13, 2022

If there's any error, we're falling through and failing to report it. I assume the metadata service is always available, and returns 404 if userdata is missing? If so, we should probably fall through on resource.ErrNotFound, and error out otherwise.

Contributor Author

tormath1 Apr 14, 2022 •

edited

Loading

Just checked and if userdata is missing it seems to return 200 with an empty string:

curl -H Metadata:true --noproxy "*" "http://169.254.169.254/metadata/instance/compute/userData?api-version=2021-01-01&format=text" -v
*   Trying 169.254.169.254:80...
* Connected to 169.254.169.254 (169.254.169.254) port 80 (#0)
> GET /metadata/instance/compute/userData?api-version=2021-01-01&format=text HTTP/1.1
> Host: 169.254.169.254
> User-Agent: curl/7.79.1
> Accept: */*
> Metadata:true
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Type: text/plain; charset=utf-8
< Server: IMDS/150.870.65.597
< Date: Thu, 14 Apr 2022 09:07:49 GMT
< Content-Length: 0
< 
* Connection #0 to host 169.254.169.254 left intact

EDIT: added a check for data length

Contributor

bgilbert Apr 14, 2022

Ah, okay. We're still falling through, though. Presumably we should be doing something like:

	} else if err != errors.ErrEmpty {
		return types.Config{}, report.Report{}, err
	}

Otherwise, an error accessing the IMDS server could cause Ignition to conclude that no userdata was provided and allow the node to boot unconfigured.

tormath1 force-pushed the tormath1/azuredata branch 2 times, most recently from 6f0f27a to d419951 Compare

April 14, 2022 13:20

bgilbert reviewed

View reviewed changes

internal/providers/azure/azure.go Outdated

+              // fetchFromAzureMetadata first tries to fetch userData from IMDS then fallback on customData in case
+              // of failure or empty config.
+              func fetchFromAzureMetadata(f *resource.Fetcher) (types.Config, report.Report, error) {

Contributor

bgilbert Apr 14, 2022

Nit: let's move this above the previous function so the file reads consistently from top to bottom.

internal/providers/azure/azure.go Outdated

+              	n := len(data)
+              	if n == 0 {
+              		return nil, fmt.Errorf("userdata is empty")

Contributor

bgilbert Apr 14, 2022

Suggested change

      
            		return nil, fmt.Errorf("userdata is empty")
          
            		return nil, errors.ErrEmpty

internal/providers/azure/azure.go

+              		return nil, fmt.Errorf("fetching to buffer: %w", err)
+              	}
+              	n := len(data)

Contributor

bgilbert Apr 14, 2022

nit: unnecessary variable

Contributor Author

tormath1 Apr 15, 2022

declared because because used twice:

to check if length is 0
later to decode base64

Contributor

bgilbert Apr 15, 2022

Argh, yup, I had missed that.

internal/providers/azure/azure.go Outdated

+              	// we first try to config from IMDS, in case of failure we fallback on the custom data.
+              	data, err := f.FetchToBuffer(imdsURL, resource.FetchOptions{Compression: "base64"})
+              	if err == nil {

Contributor

bgilbert Apr 14, 2022

Ah, okay. We're still falling through, though. Presumably we should be doing something like:

	} else if err != errors.ErrEmpty {
		return types.Config{}, report.Report{}, err
	}

Otherwise, an error accessing the IMDS server could cause Ignition to conclude that no userdata was provided and allow the node to boot unconfigured.

tormath1 force-pushed the tormath1/azuredata branch from d419951 to 87639d8 Compare

April 19, 2022 08:18

Contributor Author

tormath1 commented Apr 19, 2022

@bgilbert thanks for the review - comments have been addressed and a CI test is running.
One additional change has been made in order to use RawQuery in the URL definition. Otherwise string URL was rendered like: http//169.254.169.254/metadata/instance/compute/userData%3Fapi-version=2021-01-01&format=text which was leading to an error 400 / Bad Request.

tormath1 and others added 3 commits

April 19, 2022 20:12


          provider/azure: try to fetch userdata from IMDS

7158e25

Azure userdata fetching is now divided into two parts:
* try to fetch userdata from IMDS
* in case of failure: try to fetch custom data from CDROM

Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>


          ci: add testing with Go 1.18

362fce6

Update `golangci-lint` to version v1.45.0 which has support for go 1.18

Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>


          config: update headers_test to stop using strings.Title()

3fa7f8b

`strings.Title()` is deprecated in go 1.18, so change the affected
tests to use different constants

Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>

tormath1 force-pushed the tormath1/azuredata branch from 1d3355c to 3fa7f8b Compare

April 19, 2022 19:15

tormath1 marked this pull request as ready for review

April 19, 2022 19:15

tormath1 requested a review from a team

April 19, 2022 19:15

Contributor Author

tormath1 commented Apr 19, 2022

CI passed: flatcar-archive/coreos-overlay#1814 (comment)

bgilbert mentioned this pull request

kola: Test Ignition with Azure userdata coreos/coreos-assembler#2823

Open

bgilbert approved these changes

View reviewed changes

Contributor

bgilbert left a comment

LGTM!

tormath1 mentioned this pull request

provider/azure: try to fetch userdata from IMDS coreos/ignition#1351

Merged

Member

pothos commented Apr 22, 2022

With the upstream PR merged I would say that we can either cherry pick directly or better update to a newer version since we have to do this anyway sooner or later

tormath1 closed this

tormath1 deleted the tormath1/azuredata branch

April 28, 2022 16:04

Contributor Author

tormath1 commented Apr 28, 2022

it should come with the next release of ignition in Flatcar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet