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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

providers/aliyun: Add aliyun #869

Merged
merged 1 commit into from Oct 9, 2019

Conversation

@ashcrow
Copy link
Member

commented Oct 2, 2019

Not 100% sure if this would be enough, but it's a start 馃槃

See:

@ashcrow ashcrow requested a review from arithx Oct 2, 2019
@coreosbot

This comment has been minimized.

Copy link

commented Oct 2, 2019

Can one of the admins verify this patch?

@ashcrow ashcrow requested a review from ajeddeloh Oct 2, 2019
@lucab

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

ok to test

Copy link
Member

left a comment

internal/providers/aliyun/aliyun.go Outdated Show resolved Hide resolved
@ashcrow ashcrow force-pushed the ashcrow:aliyun branch 2 times, most recently from 560717a to 73de5af Oct 2, 2019
@ajeddeloh

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

LGTM if it works, haven't tested

@lucab

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

As a side-note, I just did a manual check and the user-data endpoint directly returns a 404 if no config was provided when creating the instance. @ajeddeloh how does/should Ignition react to that?

@ashcrow

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2019

It looks like fetchToBuffer would end up passing back the error from the underlying http call.

Since it's valid to have no user-data at all we could catch the 404 response in the provider and pass an empty JSON struct into util.ParseConfig.

Thoughts?

@ashcrow

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2019

Something like:

diff --git a/internal/providers/aliyun/aliyun.go b/internal/providers/aliyun/aliyun.go
index 7b8a879..90858ff 100644
--- a/internal/providers/aliyun/aliyun.go
+++ b/internal/providers/aliyun/aliyun.go
@@ -39,6 +39,16 @@ func FetchConfig(f *resource.Fetcher) (types.Config, report.Report, error) {
        data, err := f.FetchToBuffer(userdataUrl, resource.FetchOptions{
                Headers: resource.ConfigHeaders,
        })
+       // Aliyun returns a 404 if no user-data is set. If this is the case, intercept
+       // the response, re-try the error, and set the data to be empty
+       // See: https://github.com/coreos/ignition/pull/869
+       if err == resource.ErrNotFound {
+               f.Logger.Debug("setting user-data empty as no user-data was set in or provided by the endpoint")
+               data.Reset()
+               // If we can not write the empty JSON to the buffer, capture the error
+               // and error out of FetchConfig
+               _, err = data.WriteString("{}")
+       }
        if err != nil {
                return types.Config{}, report.Report{}, err
        }
@ajeddeloh

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2019

If not providing a config gives a 404, we should treat it the same as if no config was provided, so catching the 404 sgtm.

@ashcrow We actually want to return an error here, with an empty Config{}. This will actually do that since {} is an invalid config and Parse() will choke, but I don't think that's what you intended and returning an explicit error would be more clear.

If a provider returns an error that's ok, Ignition will treat that the same as no config. Ignition actually tries the cmdline provider (ignition.config.url kcmdline), then the installed config (the one coreos-installer writes) then the platform specific one. If those encounter errors, that's fine, they just log it and fall back to the next one.

@ashcrow

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2019

@ajeddeloh Awesome, perfect!

@ashcrow ashcrow referenced this pull request Oct 8, 2019
6 of 6 tasks complete
@ashcrow ashcrow force-pushed the ashcrow:aliyun branch from 73de5af to 58b3f79 Oct 8, 2019
@ashcrow

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

This ended up passing very basic testing. Going to do a few manual tests in aliyun.

@ashcrow

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

Travis failure in 1/4 runs:

The following packages have unmet dependencies:

 libblkid-dev : Depends: libblkid1 (= 2.20.1-5.1ubuntu20) but 2.20.1-5.1ubuntu20.9 is to be installed

E: Unable to correct problems, you have held broken packages.
@arithx

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

@ashcrow: I re-ran the specific subjob, was just a travis flake.

@ajeddeloh

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

LGTM, this has been tested in cloud with an empty config, yes?

@ashcrow

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

Verifying now @ajeddeloh, almost done :-)

@ashcrow ashcrow changed the title WIP: providers/aliyun: Add aliyun providers/aliyun: Add aliyun Oct 9, 2019
@ashcrow

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

@ajeddeloh Verified ssh access and file/dir writing with this change (thanks to @arithx for providing an image to use!). It worked great!

@ashcrow ashcrow changed the title providers/aliyun: Add aliyun WIP: providers/aliyun: Add aliyun Oct 9, 2019
@ashcrow

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

Everything BUT an empty config passes. @arithx pointed out https://github.com/coreos/ignition/blob/master/internal/providers/aws/aws.go#L47 for 404 responses. I'll see about doing that as well here.

See:
- https://www.alibabacloud.com/help/doc-detail/49122.htm
- #589

Signed-off-by: Steve Milner <smilner@redhat.com>
@ashcrow ashcrow force-pushed the ashcrow:aliyun branch from 58b3f79 to 19d0d6c Oct 9, 2019
@ashcrow

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

Everything BUT an empty config passes. @arithx pointed out https://github.com/coreos/ignition/blob/master/internal/providers/aws/aws.go#L47 for 404 responses. I'll see about doing that as well here.

Updated to be inline with the above.

@ashcrow ashcrow changed the title WIP: providers/aliyun: Add aliyun providers/aliyun: Add aliyun Oct 9, 2019
@ashcrow

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

This now works with and without a config provided in Aliyuns metadata.

@ashcrow ashcrow merged commit 497ef9b into coreos:master Oct 9, 2019
2 checks passed
2 checks passed
Black Box Tests Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.