Skip to content

Conversation

@aegal
Copy link
Contributor

@aegal aegal commented Apr 1, 2020

What this PR does / why we need it:

This PR consumes endpoint policies targeting APIPA endpoints. In the future, we are extending policies to requested by the orchestrator in a extensible way from the Create Network Container Request.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #533

Special notes for your reviewer:

Tested the successful creation of the HostNCApipaEndpoint with the additional policies requested.

Release note:

@aegal aegal requested a review from ashvindeodhar April 1, 2020 03:15

var requestedAclPolicy hcn.AclPolicySetting

if err := json.Unmarshal(requestedPolicy.Settings, &requestedAclPolicy); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I think you don't need to create new err here. ( err := )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@ashvindeodhar ashvindeodhar requested a review from ninzavivek April 1, 2020 19:49
@aegal aegal requested a review from ashvindeodhar April 1, 2020 20:03

// Validate - Validates network container request policies
func (networkContainerRequestPolicy *NetworkContainerRequestPolicies) Validate() error {
// validate ACL policy
Copy link
Member

Choose a reason for hiding this comment

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

As one of my comments said can we enforce the validation on apipa endpoint? Check if the provided acl is on apipa endpoint type, if not fail the validation because we don;t have support for acls on cust subnet endpoint. When we add that we should remove the enforced apipa endpoint validation you'll add here. Let me know if it makes sense / you want to discuss this further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, restricted this API only to APIPA in latest commit.

@ashvindeodhar
Copy link
Member

@tamilmani1989 can you please check the struct to see if this has everything we will need when we extend this to linux as well. If there is anything missing we should add those fields now.
type ValidAclPolicySetting struct {

@ashvindeodhar
Copy link
Member

ashvindeodhar commented Apr 3, 2020

@aegal how did you e2e test this?

Tested invoking CNI via containerd which triggerd the CreateHostNCApipaEndpointRequest which reported success. I can send you a log file offline.

@aegal aegal requested a review from ashvindeodhar April 8, 2020 05:33
@aegal aegal merged commit 5319878 into master Apr 9, 2020
@aegal aegal deleted the consumeFromNCContainerRequest branch April 13, 2020 18:10
tamilmani1989 pushed a commit to tamilmani1989/azure-container-networking that referenced this pull request Jun 20, 2021
…pdateNetworkContainerRequest

adding DNC changes here for early review, **Requires CNS changes to be checked in first**

Azure#535

-Includes a Endpoint policy in NetworkContainerRequest API
-Includes Endpoint policy in CNS CreateOrUpdateNetworkContainerRequest
-Includes Endpoint policy in CreateOrUpdateNetworkContainerRequest for Publish

PR URL: https://msazure.visualstudio.com/DefaultCollection/One/_git/Networking-Aquarius/pullrequest/2750702

Related work items: #6501471
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.

3 participants