Expose default primary PrivateEndpoint privateIpAddress #4107
Conversation
super-harsh
requested review from
davefellows,
theunrepentantgeek,
matthchr and
babbageclunk
as code owners
super-harsh
changed the title
| return configmaps.SliceToClientObjectSlice(configs), nil | ||
| } | ||
|
|
||
| func configByName(resp armnetwork.InterfacesClientGetResponse) map[string]string { |
There was a problem hiding this comment.
minor: Could you combine the configByName and configMapToWrite sections here?
I think in some secrets handlers we do this pattern because the actual data comes from a map[string]string to start, but here it's well-typed so it feels like just passing armnetwork.InterfacesClientGetResponse to configMapToWrite is OK?
Though actually, you shouldn't pass the GetResponse, if you look inside it there should be a resource type embedded and that's what you wanna pass. You don't need the HTTP junk that's with it on the GetResponse.
There was a problem hiding this comment.
I followed this approach to make the code easily extendable in the future if we plan on adding more config exports. That way it's cleaner to just have all the fetching logic in one method and writing logic in another.
There was a problem hiding this comment.
The thing I think could be improved about the way it is now is that you fetch from the typed structure into a map[string]string, with a somewhat magical key of primaryNICPrivateIPAddress, and then you write by reading that somewhat magical key again.
My argument is that it's no clearer to read it from a map[string]string than it is to read it from a network.PrivateEndpoint, and if in the future we need to read other things from the same network.PrivateEndpoint there's no advantage. Only if we were reading from multiple places might there be a slight advantage, but even then it's only slight because while you avoid needing N parameters for the configMapToWrite function, you still need N parameters for the configByName function so it's not really all that much cleaner/clearer?
It feels like the map[string]string is adding a step that doesn't really have a ton of value now. I'd argue rather than position us for a possible future where things are more complicated we just keep it simple now and could split into 2 functions in the future if/when the need arises. YAGNI and all that, I guess.
Alternatively, if you want the structure of the split maybe you could accomplish it by still having 1 function, but it constructs the collector and passes it to a function that extracts the values and writes it - that way there's no intermediate map it's just straight from the struct to the collector? Then if there were other sets to collect you'd have another function for those and pass the same collector down to it too?
| return nil, nil | ||
| } | ||
|
|
||
| if endpoint.Status.NetworkInterfaces[0].Id == nil { |
There was a problem hiding this comment.
Is the default network interface always [0]? Or do you need to scan them for an .IsDefault property or something?
There was a problem hiding this comment.
I added a scan for this at the IPConfiguration level but left the assumption that there's only 1 NIC because as far as I can tell there's no dual-NIC'ed PEs.
matthchr
force-pushed
the
feature/expose-pe-primaryipaddress
branch
from
e9cfde5 to
1179659
Compare
Closes #3901
What this PR does / why we need it:
This PR adds a PrivateEndpoint extension to expose the privateIpAddress generated by Azure for PrivateEndpoint. This private IP Address is found on the NIC attached to the PrivateEndpoint.
Special notes for your reviewer:
Only exporting this one since this is the only address generated by Azure.
How does this PR make you feel:
If applicable: