openstack/gnocchi
Code Issues Proposed changes

rest: reject / as resource id and metric name

Browse Source 
This change reject with a 400 error any resource id or metric name
with a '/'.

Existing metric/resource are update to replace the '/' by a '_'.

Change-Id: I7fb97b5439119ad74035003c66c2d62272f7097f
This commit is contained in:
Mehdi Abaakouk 2017-01-11 15:53:10 +01:00
parent b0b8c1aeb2
commit fece429145
8 changed files with 275 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

184
gnocchi/indexer/alembic/versions/397987e38570_no_more_slash.py Normal file
View File

@ -0,0 +1,184 @@
# Copyright 2017 OpenStack Foundation
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
# a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.
#
"""no-more-slash
Revision ID: 397987e38570
Revises: aba5a217ca9b
Create Date: 2017-01-11 16:32:40.421758
"""
import uuid
from alembic import op
import six
import sqlalchemy as sa
import sqlalchemy_utils
from gnocchi import utils
# revision identifiers, used by Alembic.
revision = '397987e38570'
down_revision = 'aba5a217ca9b'
branch_labels = None
depends_on = None
resource_type_table = sa.Table(
'resource_type',
sa.MetaData(),
sa.Column('tablename', sa.String(35), nullable=False)
)
resource_table = sa.Table(
'resource',
sa.MetaData(),
sa.Column('id',
sqlalchemy_utils.types.uuid.UUIDType(),
nullable=False),
sa.Column('original_resource_id', sa.String(255)),
sa.Column('type', sa.String(255))
)
resourcehistory_table = sa.Table(
'resource_history',
sa.MetaData(),
sa.Column('id',
sqlalchemy_utils.types.uuid.UUIDType(),
nullable=False),
sa.Column('original_resource_id', sa.String(255))
)
metric_table = sa.Table(
'metric',
sa.MetaData(),
sa.Column('id',
sqlalchemy_utils.types.uuid.UUIDType(),
nullable=False),
sa.Column('name', sa.String(255)),
sa.Column('resource_id', sqlalchemy_utils.types.uuid.UUIDType())
)
uuidtype = sqlalchemy_utils.types.uuid.UUIDType()
def upgrade():
connection = op.get_bind()
resource_type_tables = {}
resource_type_tablenames = [
rt.tablename
for rt in connection.execute(resource_type_table.select())
if rt.tablename != "generic"
]
op.drop_constraint("fk_metric_resource_id_resource_id", "metric",
type_="foreignkey")
for table in resource_type_tablenames:
op.drop_constraint("fk_%s_id_resource_id" % table, table,
type_="foreignkey")
resource_type_tables[table] = sa.Table(
table,
sa.MetaData(),
sa.Column('id',
sqlalchemy_utils.types.uuid.UUIDType(),
nullable=False),
)
resource_type_tables["%s_history" % table] = sa.Table(
"%s_history" % table,
sa.MetaData(),
sa.Column('id',
sqlalchemy_utils.types.uuid.UUIDType(),
nullable=False),
)
for resource in connection.execute(resource_table.select().where(
resource_table.c.original_resource_id.like('%/%'))):
new_original_resource_id = resource.original_resource_id.replace(
'/', '_')
if six.PY2:
new_original_resource_id = new_original_resource_id.encode('utf-8')
new_id = sa.literal(uuidtype.process_bind_param(
str(uuid.uuid5(utils.RESOURCE_ID_NAMESPACE,
new_original_resource_id)),
connection.dialect))
# resource table
connection.execute(
resource_table.update().where(
resource_table.c.id == resource.id
).values(
id=new_id,
original_resource_id=new_original_resource_id
)
)
# resource history table
connection.execute(
resourcehistory_table.update().where(
resourcehistory_table.c.id == resource.id
).values(
id=new_id,
original_resource_id=new_original_resource_id
)
)
if resource.type != "generic":
rtable = resource_type_tables[resource.type]
htable = resource_type_tables["%s_history" % resource.type]
# resource table (type)
connection.execute(
rtable.update().where(
rtable.c.id == resource.id
).values(id=new_id)
)
# resource history table (type)
connection.execute(
htable.update().where(
htable.c.id == resource.id
).values(id=new_id)
)
# Metric
connection.execute(
metric_table.update().where(
metric_table.c.resource_id == resource.id
).values(
resource_id=new_id
)
)
for table in resource_type_tablenames:
op.create_foreign_key("fk_%s_id_resource_id" % table,
table, "resource",
("id",), ("id",),
ondelete="CASCADE")
op.create_foreign_key("fk_metric_resource_id_resource_id",
"metric", "resource",
("resource_id",), ("id",),
ondelete="SET NULL")
for metric in connection.execute(metric_table.select().where(
metric_table.c.name.like("%/%"))):
connection.execute(
metric_table.update().where(
metric_table.c.id == metric.id
).values(
name=metric.name.replace('/', '_'),
)
)

2
gnocchi/rest/__init__.py
View File

@ -488,6 +488,8 @@ class MetricsController(rest.RestController):
archive_policy_name = definition.get('archive_policy_name')
name = definition.get('name')
if name and '/' in name:
abort(400, "'/' is not supported in metric name")
if archive_policy_name is None:
try:
ap = pecan.request.indexer.get_archive_policy_for_metric(name)

11
gnocchi/tests/gabbi/gabbits/metric.yaml
View File

@ -78,6 +78,17 @@ tests:
$.name: disk.io.rate
$.unit: B/s
- name: create metric with invalid name
POST: /v1/metric
request_headers:
content-type: application/json
data:
name: "disk/io/rate"
unit: "B/s"
status: 400
response_strings:
- "'/' is not supported in metric name"
- name: create metric with name and over length unit
POST: /v1/metric
request_headers:

28
gnocchi/tests/gabbi/gabbits/resource.yaml
View File

@ -189,6 +189,20 @@ tests:
- "Invalid input: required key not provided @ data["
- "'display_name']"
- name: post instance with invalid metric name
POST: $LAST_URL
request_headers:
x-user-id: 0fbb231484614b1a80131fc22f6afc9c
x-project-id: f3d41b770cc14f0bb94a1d5be9c0e3ea
content-type: application/json
data:
metrics:
"disk/iops":
archive_policy_name: medium
status: 400
response_strings:
- "'/' is not supported in metric name"
- name: post instance resource
POST: $LAST_URL
request_headers:
@ -363,6 +377,20 @@ tests:
response_json_paths:
$.metrics.'disk.io.rate': $RESPONSE["$.metrics.'disk.io.rate'"]
- name: patch instance with invalid metric name
PATCH: $LAST_URL
request_headers:
x-user-id: 0fbb231484614b1a80131fc22f6afc9c
x-project-id: f3d41b770cc14f0bb94a1d5be9c0e3ea
content-type: application/json
data:
metrics:
"disk/iops":
archive_policy_name: medium
status: 400
response_strings:
- "'/' is not supported in metric name"
# Failure modes for history
- name: post instance history

11
gnocchi/tests/gabbi/gabbits/transformedids.yaml
View File

@ -58,6 +58,17 @@ tests:
archive_policy_name: medium
status: 409
- name: post new resource with invalid uuid
POST: /v1/resource/generic
data:
id: 'id-with-/'
user_id: 0fbb231484614b1a80131fc22f6afc9c
project_id: f3d41b770cc14f0bb94a1d5be9c0e3ea
status: 400
response_strings:
- "Invalid input: not a valid value for dictionary value @ data["
- "'id'] "
- name: post new resource non uuid
POST: /v1/resource/generic
data:

2
gnocchi/utils.py
View File

@ -40,6 +40,8 @@ RESOURCE_ID_NAMESPACE = uuid.UUID('0a7a15ff-aa13-4ac2-897c-9bdf30ce175b')
def ResourceUUID(value):
if '/' in value:
raise ValueError("'/' is not supported in resource id")
try:
try:
return uuid.UUID(value)

7
releasenotes/notes/forbid-slash-b3ec2bc77cc34b49.yaml Normal file
View File

@ -0,0 +1,7 @@
---
fixes:
- \'/\' in resource id and metric name have been accepted by mistake, because
they can be POSTed but not GETed/PATCHed/DELETEd. Now this char is forbidden
in resource id and metric name, REST api will return 400 if it presents.
Metric name and resource id already present with a \'/\' have their \'/\' replaced
by \'_\'.

37
run-upgrade-tests.sh
View File

@ -7,19 +7,25 @@ export GNOCCHI_USER_ID=99aae-4dc2-4fbc-b5b8-9688c470d9cc
export GNOCCHI_PROJECT_ID=c8d27445-48af-457c-8e0d-1de7103eae1f
export GNOCCHI_DATA=$(mktemp -d -t gnocchi.XXXX)
GDATE=$((which gdate >/dev/null && echo gdate) || echo date)
old_version=$(pip freeze | sed -n '/gnocchi==/s/.*==\(.*\)/\1/p')
[ "${old_version:0:1}" == "3" ] && have_resource_type_post=1
RESOURCE_IDS=(
"5a301761-aaaa-46e2-8900-8b4f6fe6675a"
"5a301761-bbbb-46e2-8900-8b4f6fe6675a"
"5a301761-cccc-46e2-8900-8b4f6fe6675a"
)
GDATE=$((which gdate >/dev/null && echo gdate) || echo date)
[ "$have_resource_type_post" ] && RESOURCE_ID_EXT="5a301761/dddd/46e2/8900/8b4f6fe6675a"
dump_data(){
dir="$1"
mkdir -p $dir
echo "* Dumping measures aggregations to $dir"
for resource_id in ${RESOURCE_IDS[@]}; do
gnocchi resource list > $dir/resources.list
for resource_id in ${RESOURCE_IDS[@]} $RESOURCE_ID_EXT; do
for agg in min max mean sum ; do
gnocchi measures show --aggregation $agg --resource-id $resource_id metric > $dir/${agg}.txt
done
@ -30,14 +36,18 @@ inject_data() {
echo "* Injecting measures in Gnocchi"
# TODO(sileht): Generate better data that ensure we have enought split that cover all
# situation
for resource_id in ${RESOURCE_IDS[@]}; do
gnocchi resource create generic --attribute id:$resource_id -n metric:high >/dev/null
gnocchi resource create generic --attribute id:$resource_id -n metric:high > /dev/null
done
gnocchi resource-type create ext > /dev/null
gnocchi resource create ext --attribute id:$RESOURCE_ID_EXT -n metric:high > /dev/null
{
echo -n '{'
resource_sep=""
for resource_id in ${RESOURCE_IDS[@]}; do
for resource_id in ${RESOURCE_IDS[@]} $RESOURCE_ID_EXT; do
echo -n "$resource_sep \"$resource_id\": { \"metric\": [ "
measures_sep=""
for i in $(seq 0 10 288000); do
@ -73,7 +83,6 @@ else
STORAGE_URL=file://$GNOCCHI_DATA
fi
old_version=$(pip freeze | sed -n '/gnocchi==/s/.*==\(.*\)/\1/p')
if [ "${old_version:0:5}" == "2.2.0" ]; then
# NOTE(sileht): temporary fix a gnocchi 2.2.0 bug
# https://review.openstack.org/#/c/369011/
@ -81,6 +90,7 @@ if [ "${old_version:0:5}" == "2.2.0" ]; then
fi
eval $(pifpaf run gnocchi --indexer-url $INDEXER_URL --storage-url $STORAGE_URL)
gnocchi resource delete $GNOCCHI_STATSD_RESOURCE_ID
inject_data $GNOCCHI_DATA
dump_data $GNOCCHI_DATA/old
pifpaf_stop
@ -89,12 +99,25 @@ new_version=$(python setup.py --version)
echo "* Upgrading Gnocchi from $old_version to $new_version"
pip install -q -U .[${GNOCCHI_VARIANT}]
eval $(pifpaf run gnocchi --indexer-url $INDEXER_URL --storage-url $STORAGE_URL)
eval $(pifpaf --debug run gnocchi --indexer-url $INDEXER_URL --storage-url $STORAGE_URL)
# Gnocchi 3.1 uses basic auth by default
export OS_AUTH_TYPE=gnocchi-basic
export GNOCCHI_USER=$GNOCCHI_USER_ID
gnocchi resource delete $GNOCCHI_STATSD_RESOURCE_ID
RESOURCE_IDS=(
"5a301761-aaaa-46e2-8900-8b4f6fe6675a"
"5a301761-bbbb-46e2-8900-8b4f6fe6675a"
"5a301761-cccc-46e2-8900-8b4f6fe6675a"
)
# NOTE(sileht): / are now _
[ "$have_resource_type_post" ] && RESOURCE_ID_EXT="5a301761_dddd_46e2_8900_8b4f6fe6675a"
dump_data $GNOCCHI_DATA/new
# NOTE(sileht): change the output of the old gnocchi to compare with the new without '/'
sed -i -e "s,5a301761/dddd/46e2/8900/8b4f6fe6675a,5a301761_dddd_46e2_8900_8b4f6fe6675a,g" \
-e "s,19235bb9-35ca-5f55-b7db-165cfb033c86,fe1bdabf-d94c-5b3a-af1e-06bdff53f228,g" $GNOCCHI_DATA/old/resources.list
echo "* Checking output difference between Gnocchi $old_version and $new_version"
diff -uNr $GNOCCHI_DATA/old $GNOCCHI_DATA/new