Skip to content

Conversation

@saiyan86
Copy link
Contributor

@saiyan86 saiyan86 commented May 1, 2018

What this PR does / why we need it:

This PR adds kubernetes DNS & endpoint policy support to Azure CNI.
It also fixes IP leaking in Windows.

Special notes for your reviewer:

This PR needs PR Azure/acs-engine#2822 from acs-engine.

@saiyan86 saiyan86 changed the title Fix dns Support k8s DNS & endpoint policy May 1, 2018
"github.com/Azure/azure-container-networking/network"
"github.com/Azure/azure-container-networking/platform"
"github.com/Azure/azure-container-networking/telemetry"
"github.com/Microsoft/hcsshim"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file is common to both linux and windows..we should move this to separate file

* We can delete this if statement once they fix it.
* Issue link: https://github.com/kubernetes/kubernetes/issues/57253
*/
epInfo, _ = plugin.nm.GetEndpointInfo(networkId, endpointId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be common to linux and windows. can you please move this to separate file(windows)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Address string `json:"ipAddress,omitempty"`
QueryInterval string `json:"queryInterval,omitempty"`
}
DNS cniTypes.DNS `json:"dns"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space damage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alignment is correct.

* We can delete this if statement once they fix it.
* Issue link: https://github.com/kubernetes/kubernetes/issues/57253
*/
func HandleConsecutiveAdd(containerId, endpointId string, dnsServers []string, nwInfo *network.NetworkInfo, nwCfg *cni.NetworkConfig) (*cniTypesCurr.Result, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove dnsservers parameter


// Parse Pod arguments.
podCfg, err := cni.ParseCniArgs(args.Args)
k8sNamespace := "default"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cni will be always invoked with namespace. if not we throw error. don't want to assume default if not specified

cni/policy.go Outdated
}

// GetPoliciesFromNwCfg returns network policies from network config.
func GetPoliciesFromNwCfg(kvp []KVPair) []Policy {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls define this in netconfig.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

}

// HotAttachEndpoint is a wrapper of hcsshim's HotAttachEndpoint.
func (endpoint *EndpointInfo) HotAttachEndpoint(containerID string) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this inside _windows.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

VirtualNetwork: nw.HnsId,
DNSSuffix: epInfo.DNS.Suffix,
DNSServerList: strings.Join(epInfo.DNS.Servers, ","),
Policies: cni.SerializePolicies(cni.EndpointPolicy, epInfo.Policies),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cannot call cni specific functions from common code..put this inside policy_windows.go

"net"
"strings"

"github.com/Azure/azure-container-networking/cni"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be removed

NetworkAdapterName: extIf.Name,
DNSSuffix: nwInfo.DNS.Suffix,
DNSServerList: strings.Join(nwInfo.DNS.Servers, ","),
Policies: cni.SerializePolicies(cni.NetworkPolicy, nwInfo.Policies),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cant call cni methods here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

// Initialize endpoint info.
epInfo := &network.EndpointInfo{
var dns network.DNSInfo
if len(nwCfg.DNS.Search) > 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check for nameserver also

// Populate DNS info.
epInfo.DNS.Suffix = result.DNS.Domain
epInfo.DNS.Servers = result.DNS.Nameservers
setPolicies(epInfo, policies)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to separate this for Linux and windows

Mode string
Subnets []SubnetInfo
DNS DNSInfo
Policies []policy.Policy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

define this structure for linux also

@tamilmani1989 tamilmani1989 merged commit 0bbce8d into Azure:master May 3, 2018
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

Successfully merging this pull request may close these issues.

2 participants