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

fix Firewall policy param cannot be updated #1074

Merged
merged 1 commit into from
Sep 18, 2017

Conversation

scsunchen
Copy link
Contributor

fix Firewall policy param cannot be updated #1068

@auhlig
Copy link
Member

auhlig commented Aug 11, 2017

Hi @scsunchen,
I'm very sorry this PR didn't get attention so far.
I was looking at https://developer.openstack.org/api-ref/networking/v2/index.html?expanded=create-firewall-group-detail,id284-detail#id284 and didn't see the firewall_policy_id attribute. Looks like that was a thing in v1.
In v2 this was replaced by egress_firewall_policy_id and ingress_firewall_policy_id.
AFAIK, OpenStack4j currently supports only networking v1 and v2 only if there was no change, but I'm not to familiar with this part. Maybe you are?

@scsunchen
Copy link
Contributor Author

@auhlig that is in v1. It's a bug .

@auhlig
Copy link
Member

auhlig commented Aug 11, 2017

Both v1 and v2 point to /v2/-paths? Probably the docs are incorrect on the path here.
In v1 I can still seefirewall_policy_id as a valid parameter in both request and response of the update method https://developer.openstack.org/api-ref/networking/v2/index.html?expanded=create-firewall-group-detail,id284-detail,update-firewall-rule-detail#update-firewall-rule. Which version of the networking version are you using @scsunchen?

@scsunchen
Copy link
Contributor Author

scsunchen commented Aug 14, 2017

@auhlig
when I use os.networking().firewalls().firewall() .update()ClientResponseException{message=Unrecognized attribute(s) 'policy', status=400, status-code=BAD_REQUEST}
firewall_policy_id is a valid. but policy is a invalid
please look at the NeutronFirewallUpdate.java . 79 line.

{
"firewall" : {
"shared" : false,
"policy" : "036ab1f1-271e-4db1-8d04-e4ed226924c8",
"admin_state_up" : true,
"firewall_policy_id" : "036ab1f1-271e-4db1-8d04-e4ed226924c8"
}
}
It has not been validated

https://developer.openstack.org/api-ref/networking/v2/?expanded=update-firewall-detail#extensions

So. I use @JsonIgnore ignore it

@auhlig auhlig mentioned this pull request Sep 6, 2017
@auhlig
Copy link
Member

auhlig commented Sep 6, 2017

What do you think on this @vinodborole?

@vinodborole
Copy link
Contributor

@auhlig @scsunchen After referring to the documentation for v2 i believe the parameter "policy" does not exists. So it makes sense to ignore it.

@auhlig
Copy link
Member

auhlig commented Sep 7, 2017

We're currently not distinguishing between v1 and v2. There just one networking service for both we're lucky they don't seem to differ much. I was concerned, as I thought merging this could break things for v1, where the policy attribute exists. Makes sense @vinodborole @scsunchen ?

@scsunchen
Copy link
Contributor Author

如果还不能证明我是对的。那么我放弃提交这个更新!

@auhlig
Copy link
Member

auhlig commented Sep 14, 2017

As mentioned previously, I'm not sure about this. Don't have a strong opinion on this anymore. What do you think @vinodborole?

@vinodborole
Copy link
Contributor

@auhlig I think it makes perfect sense to ignore the "policy" property in neutronFirewall Update as the get firewall has ignored it.

@auhlig auhlig merged commit b8ede13 into ContainX:master Sep 18, 2017
@auhlig
Copy link
Member

auhlig commented Sep 18, 2017

Thanks @scsunchen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants