-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Allow users to select the cloud metadata providers #13812
Conversation
// specific language governing permissions and limitations | ||
// under the License. | ||
|
||
package add_cloud_metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't use an underscore in package name
// specific language governing permissions and limitations | ||
// under the License. | ||
|
||
package add_cloud_metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't use an underscore in package name
// specific language governing permissions and limitations | ||
// under the License. | ||
|
||
package add_cloud_metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't use an underscore in package name
// specific language governing permissions and limitations | ||
// under the License. | ||
|
||
package add_cloud_metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't use an underscore in package name
Jenkins is under maintenance right now. Let's rely on Travis for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for addressing this issue!
|
||
func (c *config) Validate() error { | ||
// XXX: remove this check. A bug in go-ucfg prevents the correct validation | ||
// on providerList |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a bug we can reference so that we know when it's ok to remove the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet. The bug is quite bad I'd say, so I will adress this tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go-ucfg issue: elastic/go-ucfg#133
We introduce a new setting 'providers' to the add_cloud_metadata processor. By now all the implementation for metadata providers requires developers to mark a provider as 'local'. The alibaba and tencent providers are not marked as local by now. If the 'providers' setting is not used, then no all providers marked as 'local' are applied. This is a breaking change, because alibaba and tencent providers will not be enabled anymore by default. If the providers setting is used, only the selected providers will be used.
cd8d811
to
45d9ce2
Compare
The libbeat testsuite fails in the elasticsearch output. But required unit tests succeed. There are no system tests in libbeat that test add_cloud_metadata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, a much nicer design this way as well. A few nits but nothing concerning.
visited := map[string]bool{} | ||
|
||
for name, ff := range providers { | ||
if visited[ff.Name] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "visited" to account for the redundant short names like "alibaba" and "ecs"? A clarifying comment would be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did add a comment to the loop.
CI is as green as it currently gets. All related tests passed. |
* Allow users to select the cloud metadata providers We introduce a new setting 'providers' to the add_cloud_metadata processor. By now all the implementation for metadata providers requires developers to mark a provider as 'local'. The alibaba and tencent providers are not marked as local by now. If the 'providers' setting is not used, then no all providers marked as 'local' are applied. This is a breaking change, because alibaba and tencent providers will not be enabled anymore by default. If the providers setting is used, only the selected providers will be used. (cherry picked from commit dc99773)
* Allow users to select the cloud metadata providers (#13812) * Allow users to select the cloud metadata providers We introduce a new setting 'providers' to the add_cloud_metadata processor. By now all the implementation for metadata providers requires developers to mark a provider as 'local'. The alibaba and tencent providers are not marked as local by now. If the 'providers' setting is not used, then no all providers marked as 'local' are applied. This is a breaking change, because alibaba and tencent providers will not be enabled anymore by default. If the providers setting is used, only the selected providers will be used. (cherry picked from commit dc99773)
Thanks so much @urso! 🚀 |
Something has gone wrong the list of providers in the docs, I think the list item symbol is https://www.elastic.co/guide/en/beats/filebeat/master/add-cloud-metadata.html: |
elastic#13815) * Allow users to select the cloud metadata providers (elastic#13812) * Allow users to select the cloud metadata providers We introduce a new setting 'providers' to the add_cloud_metadata processor. By now all the implementation for metadata providers requires developers to mark a provider as 'local'. The alibaba and tencent providers are not marked as local by now. If the 'providers' setting is not used, then no all providers marked as 'local' are applied. This is a breaking change, because alibaba and tencent providers will not be enabled anymore by default. If the providers setting is used, only the selected providers will be used. (cherry picked from commit fa6d344)
Resolves: #11145
We introduce a new setting 'providers' to the add_cloud_metadata
processor.
By now all the implementation for metadata providers requires developers
to mark a provider as 'local'. The alibaba and tencent providers are not
marked as local by now.
If the 'providers' setting is not used, then no all providers marked as
'local' are applied. This is a breaking change, because alibaba and
tencent providers will not be enabled anymore by default.
Although it's a breaking change, I prefered to disable non-local providers by default. (I epxect the fix to go into 7.4.0).
If the providers setting is used, only the selected providers will be
used.
The change supports alises for different providers. But the setup will only initialized a provider twice if the alias is used.