)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"2fea57a73d6e011585dca59976ca63aebf9937e3","unresolved":false,"context_lines":[{"line_number":33,"context_line":"Method B (POST /v2/images/{image_id}/import)."},{"line_number":34,"context_line":"Also, moved the policy check for both Method A and Method B"},{"line_number":35,"context_line":"at the entry point of the respective APIs in the controller itself."},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"Closes-Bug: #1732141"},{"line_number":38,"context_line":"Change-Id: Icc62add5f8d48549aac94c8058d66d6b77b56d41"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"ff82abbf_14586ff7","line":36,"updated":"2017-11-30 04:41:52.000000000","message":"You should add DocImpact tag as well","commit_id":"3c4edd528ad6f842323785f7d7ffb4501ec562c4"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"d8b055634407e99edc0c69091db9d0d31b05989f","unresolved":false,"context_lines":[{"line_number":33,"context_line":"Method B (POST /v2/images/{image_id}/import)."},{"line_number":34,"context_line":"Also, moved the policy check for both Method A and Method B"},{"line_number":35,"context_line":"at the entry point of the respective APIs in the controller itself."},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"Closes-Bug: #1732141"},{"line_number":38,"context_line":"Change-Id: Icc62add5f8d48549aac94c8058d66d6b77b56d41"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"ff82abbf_829e7ab6","line":36,"in_reply_to":"ff82abbf_14586ff7","updated":"2017-11-30 08:59:06.000000000","message":"I have checked other patches and come to know that for new policy no need to add DocImpact flag though. So please ignore this comment.","commit_id":"3c4edd528ad6f842323785f7d7ffb4501ec562c4"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"47abaf4a35e0f092cf67e068754302842027ebc2","unresolved":false,"context_lines":[{"line_number":34,"context_line":"for import API call (POST /v2/images/{image_id}/import)"},{"line_number":35,"context_line":"on the theory that an operator can stop import by restricting the"},{"line_number":36,"context_line":"\u0027image_create\u0027 policy"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"Partial-Bug: #1732141"},{"line_number":39,"context_line":"Change-Id: Icc62add5f8d48549aac94c8058d66d6b77b56d41"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"df87a7cf_f7fe5b69","line":37,"updated":"2017-12-11 06:54:43.000000000","message":"I guess now you should change commit message as per discussion yesterday happened on openstack-glance irc channel. For example glance is planning to remove policy layer by moving policy checks to controller layer.","commit_id":"8a928a48720b359516dcdeb45568a16a1385bc1d"}],"glance/api/v2/image_data.py":[{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"2fea57a73d6e011585dca59976ca63aebf9937e3","unresolved":false,"context_lines":[{"line_number":41,"context_line":"class ImageDataController(object):"},{"line_number":42,"context_line":"    def __init__(self, db_api\u003dNone, store_api\u003dNone,"},{"line_number":43,"context_line":"                 policy_enforcer\u003dNone, notifier\u003dNone,"},{"line_number":44,"context_line":"                 gateway\u003dNone):"},{"line_number":45,"context_line":"        if gateway is None:"},{"line_number":46,"context_line":"            db_api \u003d db_api or glance.db.get_api()"},{"line_number":47,"context_line":"            store_api \u003d store_api or glance_store"}],"source_content_type":"text/x-python","patch_set":1,"id":"ff82abbf_14b4eff0","side":"PARENT","line":44,"range":{"start_line":44,"start_character":17,"end_line":44,"end_character":29},"updated":"2017-11-30 04:41:52.000000000","message":"any particular reason to remove this? IMO this change is not related to your patch and as a result you have modified https://review.openstack.org/#/c/524060/1/glance/tests/unit/v2/test_image_data_resource.py@117 which also doesn\u0027t looks good.","commit_id":"4a2626c262fe2acfeb32ca46683014b7f45448ba"},{"author":{"_account_id":20182,"name":"Bhagyashri Shewale","email":"bshewale@redhat.com","username":"bhagyashris"},"change_message_id":"e9095d7488a1be0e4fe914fc6209c0cad9ec2dee","unresolved":false,"context_lines":[{"line_number":41,"context_line":"class ImageDataController(object):"},{"line_number":42,"context_line":"    def __init__(self, db_api\u003dNone, store_api\u003dNone,"},{"line_number":43,"context_line":"                 policy_enforcer\u003dNone, notifier\u003dNone,"},{"line_number":44,"context_line":"                 gateway\u003dNone):"},{"line_number":45,"context_line":"        if gateway is None:"},{"line_number":46,"context_line":"            db_api \u003d db_api or glance.db.get_api()"},{"line_number":47,"context_line":"            store_api \u003d store_api or glance_store"}],"source_content_type":"text/x-python","patch_set":1,"id":"ff82abbf_02cb4a72","side":"PARENT","line":44,"range":{"start_line":44,"start_character":17,"end_line":44,"end_character":29},"in_reply_to":"ff82abbf_14b4eff0","updated":"2017-11-30 09:24:42.000000000","message":"Hi Abhishek, Thanks for review,\n\nThe reason behind removing gateway parameter is:\n\nAt the time of starting glance service \u0027gateway\u0027 is not pass to ImageDataController constructor [1] and it is just used in unit test cases The issue which I am trying to fix in this patch needs policy object in \u0027upload()\u0027 method so If I try to access policy object like \u0027self.gateway.policy\u0027 then it\u0027s not a good practice according to the OOPS concept and also the newly defining policy object makes code duplication. \n\n[1]: https://github.com/openstack/glance/blob/master/glance/api/v2/image_data.py#L42","commit_id":"4a2626c262fe2acfeb32ca46683014b7f45448ba"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"2c7e67d8b9203fee42aef2894209abc8716043bc","unresolved":false,"context_lines":[{"line_number":105,"context_line":"        refresher \u003d None"},{"line_number":106,"context_line":"        cxt \u003d req.context"},{"line_number":107,"context_line":"        try:"},{"line_number":108,"context_line":"            self.policy.enforce(req.context, \u0027upload_image\u0027, {})"},{"line_number":109,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":110,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":111,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff82abbf_cbba44e0","line":108,"range":{"start_line":108,"start_character":61,"end_line":108,"end_character":63},"updated":"2017-12-01 04:14:35.000000000","message":"previously it was taking self.target which was object of ImageTarget(image) class. If some deployers want to enforce some restrictions on upload image based on image properties (something like \"upload_image\": \"not \u0027test_key\u0027:(test_key)s\" in policy.json) then it will not work.\n\nI would like to know other cores opinion though as I am not sure at this point whether it is required or not.","commit_id":"b4fdbe7f5cfe88adf4032a24e3acd58159d452a4"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"2c7e67d8b9203fee42aef2894209abc8716043bc","unresolved":false,"context_lines":[{"line_number":105,"context_line":"        refresher \u003d None"},{"line_number":106,"context_line":"        cxt \u003d req.context"},{"line_number":107,"context_line":"        try:"},{"line_number":108,"context_line":"            self.policy.enforce(req.context, \u0027upload_image\u0027, {})"},{"line_number":109,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":110,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":111,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff82abbf_8b58bcfa","line":108,"range":{"start_line":108,"start_character":32,"end_line":108,"end_character":43},"updated":"2017-12-01 04:14:35.000000000","message":"you can pass here cxt which is defined at line #106","commit_id":"b4fdbe7f5cfe88adf4032a24e3acd58159d452a4"},{"author":{"_account_id":20182,"name":"Bhagyashri Shewale","email":"bshewale@redhat.com","username":"bhagyashris"},"change_message_id":"b7d2c63581fa6587873340ec359f0bdc447c79d0","unresolved":false,"context_lines":[{"line_number":105,"context_line":"        refresher \u003d None"},{"line_number":106,"context_line":"        cxt \u003d req.context"},{"line_number":107,"context_line":"        try:"},{"line_number":108,"context_line":"            self.policy.enforce(req.context, \u0027upload_image\u0027, {})"},{"line_number":109,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":110,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":111,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"df87a7cf_b38f1022","line":108,"range":{"start_line":108,"start_character":61,"end_line":108,"end_character":63},"in_reply_to":"df87a7cf_bc65a547","updated":"2017-12-05 08:41:30.000000000","message":"Hi Brain, Thanks for your opinion.","commit_id":"b4fdbe7f5cfe88adf4032a24e3acd58159d452a4"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"f0611f0457662abba5c40f0dc914bd3fc66981c0","unresolved":false,"context_lines":[{"line_number":105,"context_line":"        refresher \u003d None"},{"line_number":106,"context_line":"        cxt \u003d req.context"},{"line_number":107,"context_line":"        try:"},{"line_number":108,"context_line":"            self.policy.enforce(req.context, \u0027upload_image\u0027, {})"},{"line_number":109,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":110,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":111,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"df87a7cf_bc65a547","line":108,"range":{"start_line":108,"start_character":61,"end_line":108,"end_character":63},"in_reply_to":"ff82abbf_cbba44e0","updated":"2017-12-05 03:28:12.000000000","message":"That\u0027s a good question.  I think in this case, though, where you\u0027re in the process of creating the image, it would be weird to have an image property (that might or might not exist yet) govern whether you can upload the data for an image you are creating.  (It definitely makes sense for download, but when image export is implemented, I think the current download_image policy, which can be configured with test_key as you outline above, will prevent the export from succeeding.)","commit_id":"b4fdbe7f5cfe88adf4032a24e3acd58159d452a4"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"6b260f6cd8466066ec95f335d9ee6725cc39d759","unresolved":false,"context_lines":[{"line_number":105,"context_line":"        refresher \u003d None"},{"line_number":106,"context_line":"        cxt \u003d req.context"},{"line_number":107,"context_line":"        try:"},{"line_number":108,"context_line":"            self.policy.enforce(cxt, \u0027upload_image\u0027, {})"},{"line_number":109,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":110,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":111,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":3,"id":"df87a7cf_185ff40e","line":108,"updated":"2017-12-05 10:04:05.000000000","message":"Afaiu it contradicts to architecture of v2 api. policy checks must be in policy layer,it allows not to mess functionality.\nis there any specific reason to put policy check  here?","commit_id":"8ea6dc4a025a0bae5301346b2152d3883891635e"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"201280a6b54372bac0372a8fab9fc680e3fed131","unresolved":false,"context_lines":[{"line_number":105,"context_line":"        refresher \u003d None"},{"line_number":106,"context_line":"        cxt \u003d req.context"},{"line_number":107,"context_line":"        try:"},{"line_number":108,"context_line":"            self.policy.enforce(cxt, \u0027upload_image\u0027, {})"},{"line_number":109,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":110,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":111,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":3,"id":"df87a7cf_7bace333","line":108,"in_reply_to":"df87a7cf_185ff40e","updated":"2017-12-06 04:51:52.000000000","message":"Kairat makes a good point.  But the problem we\u0027ve got is that the upload_image policy is hard-coded into image.set_data() in the policy layer, and image.set_data() is used in two different contexts now: direct upload and image import.  We could pass in the policy to be checked as a parameter to set_data(), but I think that violates the spirit behind the policy layer, which is to make it so that you don\u0027t need to worry about policies when you\u0027re working in other layers.\n\nThe real problem we\u0027ve got is that for most of the other services, policies are tied very closely to what API calls you can make.  In Glance, the policies are tied closely to the objects that Glance manipulates, and only indirectly to the API calls.  (This also causes a lot of problems with image locations.)\n\nI think it makes sense to handle upload_image, stage_image, and import_image at the controller layer, but as Kairat points out, this is a code smell given the Glance architecture.  We need some more eyes on this.","commit_id":"8ea6dc4a025a0bae5301346b2152d3883891635e"}],"glance/api/v2/images.py":[{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"2c7e67d8b9203fee42aef2894209abc8716043bc","unresolved":false,"context_lines":[{"line_number":97,"context_line":"        task_input \u003d {\u0027image_id\u0027: image_id}"},{"line_number":98,"context_line":""},{"line_number":99,"context_line":"        try:"},{"line_number":100,"context_line":"            self.policy.enforce(req.context, \u0027import_image\u0027, {})"},{"line_number":101,"context_line":"            import_task \u003d task_factory.new_task(task_type\u003d\u0027api_image_import\u0027,"},{"line_number":102,"context_line":"                                                owner\u003dreq.context.owner,"},{"line_number":103,"context_line":"                                                task_input\u003dtask_input)"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff82abbf_0bc82c72","line":100,"range":{"start_line":100,"start_character":61,"end_line":100,"end_character":63},"updated":"2017-12-01 04:14:35.000000000","message":"same thing will be applicable here as well, but lets see opinions of other core members.","commit_id":"b4fdbe7f5cfe88adf4032a24e3acd58159d452a4"}],"glance/tests/unit/v2/test_image_data_resource.py":[{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"9989b1b4a043ad42ff9ff4e565d51400e4d05e61","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        self.gateway \u003d FakeGateway(self.db, self.store, self.notifier,"},{"line_number":115,"context_line":"                                   self.policy, self.image_repo)"},{"line_number":116,"context_line":"        self.controller \u003d glance.api.v2.image_data.ImageDataController()"},{"line_number":117,"context_line":"        self.controller.gateway \u003d self.gateway"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"    def test_download(self):"},{"line_number":120,"context_line":"        request \u003d unit_test_utils.get_fake_request()"}],"source_content_type":"text/x-python","patch_set":1,"id":"ff82abbf_65b6e0df","line":117,"range":{"start_line":117,"start_character":34,"end_line":117,"end_character":46},"updated":"2017-11-30 10:15:34.000000000","message":"instead of this can you initialize FakeGateWay here only, as self.gateway is not used anywhere else","commit_id":"3c4edd528ad6f842323785f7d7ffb4501ec562c4"}]}