Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

feat: reconcile private hosts for AKS BYO DNS #3556

Merged
merged 1 commit into from
Jul 2, 2020
Merged

feat: reconcile private hosts for AKS BYO DNS #3556

merged 1 commit into from
Jul 2, 2020

Conversation

levimm
Copy link
Contributor

@levimm levimm commented Jun 28, 2020 •
edited by devigned

Reason for Change:

This pr is required by AKS private cluster BYO DNS scenario. Since this pr also changes the api model, the pr need to be merged into master.
For AKS BYO DNS, we'll config the /etc/hosts file on all agent nodes so we can resolve the private dns without azure private dns zone.

Issue Fixed:

Requirements:

Notes:

@welcome
Copy link

welcome bot commented Jun 28, 2020

💖 Thanks for opening your first pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix. Examples of commit messages with semantic prefixes: - fix: change azure disk cachingMode to ReadOnly - feat: make maximumLoadBalancerRuleCount configurable - docs: add note on AKS Engine and AKS relationship
Make sure to check out the developer guide for guidance on testing your change.

@levimm levimm changed the title reconcile private hosts for AKS BYO DNS feat: reconcile private hosts for AKS BYO DNS Jun 28, 2020
@feiskyer
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@levimm levimm force-pushed the lima2/0628-reconcileHosts branch from b87d46d to 82b4bb9 Compare June 29, 2020 05:32
@levimm
Copy link
Contributor Author

levimm commented Jun 29, 2020

/azp run pr-e2e

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 3556 in repo Azure/aks-engine

@levimm levimm marked this pull request as ready for review June 29, 2020 05:40
@feiskyer
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

feiskyer
feiskyer previously approved these changes Jun 30, 2020
View reviewed changes
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

looks good. @jackfrancis could you help to have a look? Windows part would be added after this PR.

@feiskyer
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov
Copy link

codecov bot commented Jun 30, 2020 •
edited

Codecov Report

Merging #3556 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3556   +/-   ##
=======================================
  Coverage   73.19%   73.20%           
=======================================
  Files         147      147           
  Lines       25078    25087    +9     
=======================================
+ Hits        18355    18364    +9     
  Misses       5589     5589           
  Partials     1134     1134
Impacted Files Coverage Δ
pkg/api/types.go 94.21% <ø> (ø)
pkg/api/vlabs/types.go 71.83% <ø> (ø)
pkg/engine/templates_generated.go 54.08% <ø> (ø)
pkg/api/converterfromapi.go 94.02% <100.00%> (+0.01%) ⬆️
pkg/api/convertertoapi.go 93.53% <100.00%> (+0.01%) ⬆️
pkg/api/defaults.go 92.75% <100.00%> (+0.02%) ⬆️
pkg/engine/template_generator.go 82.08% <100.00%> (+0.11%) ⬆️
pkg/api/common/versions.go 96.84% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9f0ce6...2b6e2d1. Read the comment docs.

@chewong chewong requested a review from jackfrancis June 30, 2020 16:00
devigned
devigned suggested changes Jul 2, 2020
View reviewed changes
Copy link
Member

@devigned devigned left a comment •
edited

Choose a reason for hiding this comment

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

Code looks good, but I don't have much context on the feature itself.

Please rebase the commits and format the commit message as https://www.conventionalcommits.org/.

Also, please document the API model entry with what this new field does: https://github.com/Azure/aks-engine/blob/master/docs/topics/clusterdefinitions.md#privatecluster

@levimm levimm force-pushed the lima2/0628-reconcileHosts branch from a4eb9ff to 2b6e2d1 Compare July 2, 2020 03:13
@levimm levimm requested a review from devigned July 2, 2020 03:14
@levimm
Copy link
Contributor Author

levimm commented Jul 2, 2020

Thanks @devigned for the comment. Rebased the commit and fixed API doc. Please help do a re-review and trigger the test.

@levimm levimm requested a review from feiskyer July 2, 2020 03:17
@feiskyer
Copy link
Member

feiskyer commented Jul 2, 2020

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@feiskyer feiskyer mentioned this pull request Jul 2, 2020
Merged
4 tasks
devigned
devigned approved these changes Jul 2, 2020
View reviewed changes
Copy link
Member

@devigned devigned left a comment

Choose a reason for hiding this comment

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

/lgtm

Thank you for addressing the comments, @levimm!

@acs-bot acs-bot added the lgtm label Jul 2, 2020
@devigned
Copy link
Member

devigned commented Jul 2, 2020

/approve

@acs-bot
Copy link

acs-bot commented Jul 2, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devigned, feiskyer, levimm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@devigned devigned merged commit 2486c70 into Azure:master Jul 2, 2020
@welcome
Copy link

welcome bot commented Jul 2, 2020

Congrats on merging your first pull request! 🎉🎉🎉

levimm added a commit to levimm/aks-engine that referenced this pull request Jul 9, 2020
@levimm
feat: enable /etc/hosts config agent for AKS private cluster BYO DNS (A…
84d1228 
…zure#3556)

Co-authored-by: Li Ma <lima2@microsoft.com>
@levimm levimm mentioned this pull request Jul 9, 2020
Merged
4 tasks
xuto2 pushed a commit that referenced this pull request Jul 9, 2020
@levimm
feat: enable /etc/hosts config agent for AKS private cluster BYO DNS (#…
a0d03f2 
…3556) (#3585)

Co-authored-by: Li Ma <lima2@microsoft.com>

Co-authored-by: Li Ma <lima2@microsoft.com>
@fmotrifork fmotrifork mentioned this pull request Aug 3, 2020
Merged
penggu pushed a commit to penggu/aks-engine that referenced this pull request Oct 28, 2020
@levimm @penggu
feat: enable /etc/hosts config agent for AKS private cluster BYO DNS (A…
691f776 
…zure#3556)

Co-authored-by: Li Ma <lima2@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@devigned devigned devigned approved these changes

@feiskyer feiskyer feiskyer approved these changes

@jackfrancis jackfrancis Awaiting requested review from jackfrancis

Assignees

@devigned devigned

Labels
approved lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@levimm @feiskyer @devigned @acs-bot