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

CRP and NRP Swagger are not accurate for NIC/IPConfiguration's primary fields #305

Closed
colemickens opened this issue May 18, 2016 · 18 comments

Comments

@colemickens
Copy link

colemickens commented May 18, 2016

The core details are here: Azure/azure-sdk-for-go#259 (comment)

The Swagger spec for Compute Instance -> NetworkProfile NIC -> primary is not correct -- in that [someone] is not populating the "primary" field for an Interface in my VM's NetworkProfile.

The Swagger spec for Network's Interface -> IPConfiguration -> primary field does not exist.

The net result of this is that I can NOT accurately write code to say... place a VM in a load balancer backend pool because I have no way of determining which NIC is primary, or which ip configuration on the NIC is primary.

I'm currently "working around" this by assuming that there is only one NIC and only one IP Configuration... which is probably fine for my use case, but we should still fix this.

@colemickens colemickens changed the title NRP Swagger is not accurate for NIC/IPConfiguration's primary fields CRP and NRP Swagger are not accurate for NIC/IPConfiguration's primary fields May 18, 2016
@devigned
Copy link
Member

@deepakswifty and @dihan0604 please respond with a fix or an explanation which could be used to make the correction to the spec.

@colemickens
Copy link
Author

Ping?

@devigned
Copy link
Member

@markjbrown can you or someone else from networking comment on this?

@dihan0604
Copy link
Contributor

@deepakswifty, Hi Deepak, could you take a look? This is not express route cmdlets,I am not familiar with it.

@colemickens
Copy link
Author

Ping?

@DeepakRajendranMsft
Copy link
Contributor

Hi @colemickens, sorry for the late reply.

we have just added the primary field for ipconfguration. It will be under a feature flag.

I would say the algo should be that (for nic and ipconfiguration), if the primary flag is not set, treat it as primary. If there are multiple nics and ipconfiguration then the primary flags for all nics and ipconfigurations will be set

@colemickens
Copy link
Author

I would say the algo should be that (for nic and ipconfiguration), if the primary flag is not set, treat it as primary. If there are multiple nics and ipconfiguration then the primary flags for all nics and ipconfigurations will be set

Makes sense. Will this /always/ be behind a feature flag? How are feature-flagged portions of the API handled when it comes to the Swagger spec?

Is there anyone from the CRP side we can talk to so that they will add the same flag is supposed to be there for the NIC in the VM Instance response? Or is that added now as well?

@DeepakRajendranMsft
Copy link
Contributor

Makes sense. Will this /always/ be behind a feature flag?

  • Multi-NIC feature is already public. Multi-CA/ Multi-Ipconfigurations per NIC is still behind a feature flag. Feature flags are not something we would like to keep forever, they should be removed once the feature goes public. Feature flags are subscription based meaning that we have to manually enable each subscription based on the request.

How are feature-flagged portions of the API handled when it comes to the Swagger spec?

  • we dont have an explicit way of handling this right now, (@david, is there something we can do here?)

Is there anyone from the CRP side we can talk to so that they will add the same flag is supposed to be there for the NIC in the VM Instance response? Or is that added now as well?

  • The flag from CRP side should be the users input. So if the user had specified the primary flags you should be seeing them. If it is a single nuc scenario then the primary flag is optional, so in this case you may or may not see the primary flag

@colemickens
Copy link
Author

Thank you for the explanation @DeepakRajendranMsft.

This definitely gives me enough information to write a conformant implementation. That having been said, I would suggest that always returning the primary field in the single-nic or single-ipconfig scenario is preferred.

As is, I have to write more heuristic code-- (is there more than one nic, okay, then just take that, if not loop through the nics and look at the primary flag. then fetch that nic and check if there are more than one ipconfig, if not, just use the first one, if so, look through all of them and examine primary flag).

I can write the code much more generically if I just loop through them regardless of how many there are, looking for the primary flag. More importantly, I'd argue that that is what the Swagger spec and MSDN REST documentation encourages people to do. I don't think the behavior you've described is documented anywhere... even for the GA feature of multi-NIC.

@DeepakRajendranMsft
Copy link
Contributor

DeepakRajendranMsft commented Jun 8, 2016

@colemickens thanks for feedback.

we always want to maintain the customers input (even on the output). thats why its optional.

@markjbrown for documenation

@devigned
Copy link
Member

devigned commented Jun 8, 2016

@DeepakRajendranMsft wouldn't a feature flag be part of a preview api-version? I wouldn't imagine they would be part of a non-preview api-version.

Beyond that, I think the only other thing we can do is to include a comment in the description of the operations or on the model calling out the fact that the feature is there, but not available in all subscriptions. Perhaps, also including some information on how to be an early adopter of the feature.

@tjprescott
Copy link
Member

Is there an estimate for when the ip-config primary flag will no longer be behind a feature flag?

@colemickens
Copy link
Author

@DeepakRajendranMsft @devigned Can we re-sync on the new field that was exposed from NRP?

In particular, has it been added to any published Swagger specs? If not, when will it be? And when will it no longer be behind a "feature flag"?

@colemickens
Copy link
Author

Ping @DeepakRajendranMsft , @devigned for the above questions. Is this stuff actually consistent between the services are returning, documented to return, and what is actually in the Swagger spec? Both the CRP field and the NRP field?

@colemickens
Copy link
Author

Summary:

Thanks to @DeepakRajendranMsft for providing this information.

@devigned
Copy link
Member

@colemickens thank you for adding the summary of the discussion!

@pearcec
Copy link

pearcec commented Jan 20, 2017

So I am looking to fix hashicorp/terraform#6514 Reading through the code I justed realize there are two Primary flags. It is not clear to me. Can I choose to set either? Give the choice I rather put it on the interface and not the ip config.

@pearcec
Copy link

pearcec commented Jan 20, 2017

Looks like I am reading this wrong. We need to set the primary doing the CRP.

blueww pushed a commit to blueww/azure-rest-api-specs that referenced this issue Dec 8, 2017
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

No branches or pull requests

6 participants