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 the user-data on Alibaba Cloud correctly #1766

Closed
wants to merge 9 commits into from
Closed

Conversation

ETiV
Copy link
Contributor

@ETiV ETiV commented Aug 11, 2022

The response of user-data API should not be fetched line by line.

Description

On Alibaba Cloud, the response of user-data metadata API is not processed properly.

Related Issue

#1765

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

This is my first time to coding in Ruby, I've did my best 😂.
If there's any error or the coding style is not meet the repo, please just fix it, thank you.

@ETiV ETiV requested review from a team as code owners August 11, 2022 03:40
Copy link
Member

@marcparadise marcparadise left a comment

Choose a reason for hiding this comment

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

I was able to find docs here: https://www.alibabacloud.com/help/en/elastic-compute-service/latest/view-instance-metadata

But they don't include example output. I don't have this environment available - would you be able to include example output being parsed for user-data and other newline-delimited data in comments, or a reference to where the behavior is defined?

lib/ohai/mixin/alibaba_metadata.rb Outdated Show resolved Hide resolved
@ETiV
Copy link
Contributor Author

ETiV commented Aug 17, 2022

And, in my opinion, if the purpose of this module is to ONLY retrieve the [meta-data/...](the meta-data and its sub-directory's contents), the root URL should be http://100.100.100.200/2016-01-01/meta-data/, not http://100.100.100.200/2016-01-01/.

If the starting point is http://100.100.100.200/2016-01-01/(as the code here), it will also get [user-data], [dynamic/...], [global-config], [maintenance/...], these things are not [meta-data/...] related I think.

@ETiV
Copy link
Contributor Author

ETiV commented Aug 17, 2022

Currently the meta-data API's structure is like this, I've pasted some of the file's content after =>

http://100.100.100.200/2016-01-01/meta-data/
├── dns-conf/
│   └── nameservers        => `100.100.2.136\n100.100.2.138`
├── eipv4                  => ``
├── hibernation/
│   └── configured         => `false`
├── hostname               => `iZuf64XXXXXXXXXXXX`
├── image-id               => `debian_10_9_x64_20G_alibase_20210521.vhd`
├── instance/
│   ├── instance-name      => `vm-instance-name`
│   ├── instance-type      => `ecs.c5.2xlarge`
│   ├── last-host-landing-time
│   ├── max-netbw-egress
│   ├── max-netbw-ingress
│   ├── virtualization-solution
│   └── virtualization-solution-version
├── instance-id
├── mac
├── network/
│   └── interfaces/
│       └── macs/
│           └── 00:11:22:xx:yy:zz/
│               ├── gateway
│               ├── netmask
│               ├── network-interface-id
│               ├── primary-ip-address
│               ├── private-ipv4s
│               ├── vpc-cidr-block
│               ├── vpc-id
│               ├── vswitch-cidr-block
│               └── vswitch-id
├── network-type
├── ntp-conf/
│   └── ntp-servers
├── owner-account-id
├── private-ipv4
├── public-keys/
│   └── 0/
│       └── openssh-key
├── ram/
│   └── security-credentials/
│       └── AnRAMRoleName
├── region-id
├── serial-number
├── source-address
├── sub-private-ipv4-list
├── vpc-cidr-block
├── vpc-id
├── vswitch-cidr-block
├── vswitch-id
└── zone-id

@marcparadise
Copy link
Member

Thanks for the additional info @ETiV ; these changes look good.

I agree that it would make more sense to include meta-data at the URL; that would also let us get rid of the special case for user-data; could you make those updates?

* No more /user-data access
* No more need to access the configs through alibaba["meta_data"]

Signed-off-by: ETiV Wang <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@ETiV
Copy link
Contributor Author

ETiV commented Aug 25, 2022

hi @marcparadise , I just pushed a commit to apply what we've talked earlier.

Copy link
Collaborator

@jaymzh jaymzh left a comment

Choose a reason for hiding this comment

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

Assuming tests pass, I'm 👍

Copy link
Collaborator

@jaymzh jaymzh left a comment

Choose a reason for hiding this comment

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

Code is fine, but the tests need updating...

@jaymzh jaymzh added the Status: Waiting on Contributor A pull request that has unresolved requested actions from the author. label Sep 20, 2022
@jaymzh
Copy link
Collaborator

jaymzh commented Jan 17, 2023

@ETiV - ping?

Copy link
Collaborator

@jaymzh jaymzh left a comment

Choose a reason for hiding this comment

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

You have 10 unit tests failing for this plugin specifically.

spec/unit/mixin/alibaba_metadata_spec.rb Outdated Show resolved Hide resolved
ETiV added a commit to ETiV/ohai that referenced this pull request Mar 5, 2023
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@ETiV ETiV requested review from jaymzh and marcparadise and removed request for jaymzh and marcparadise March 6, 2023 07:38
@tpowell-progress tpowell-progress self-assigned this Mar 7, 2023
@marcparadise
Copy link
Member

@ETiV these changes look good to me, but we're still seeing test failures in the new specs.

Copy link
Collaborator

@jaymzh jaymzh left a comment

Choose a reason for hiding this comment

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

All the tests around the alibaba test are failing... please fix them and we'll merge.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Collaborator

@jaymzh jaymzh left a comment

Choose a reason for hiding this comment

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

Your tests still need updating, please.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@ETiV ETiV deleted the branch chef:main July 30, 2023 20:48
@ETiV ETiV closed this Jul 30, 2023
@ETiV ETiV deleted the main branch July 30, 2023 20:48
@tpowell-progress tpowell-progress removed their assignment Aug 1, 2023
@tpowell-progress tpowell-progress removed the Status: Waiting on Contributor A pull request that has unresolved requested actions from the author. label Aug 1, 2023
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