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.

Sign up for GitHub

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

Jump to bottom

Add kubeadm configs, load balancer, public IP, refactor actuators, types #88

Merged
merged 8 commits into from
Feb 26, 2019
Merged

Add kubeadm configs, load balancer, public IP, refactor actuators, types #88

merged 8 commits into from
Feb 26, 2019

Conversation

justaugustus
Copy link
Member

@justaugustus justaugustus commented Feb 5, 2019 •
edited
Loading

What this PR does / why we need it:

  • Refactor actuator methods
  • Update certificates package
  • Add kubeadm config package
  • Add loadbalancer package
  • Add public IP package
  • Bump docker image version
  • Update Makefile, generate-yaml.sh, machine templates, types, CRDs, RBAC
  • Bump vendor

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

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Add kubeadm configs, load balancer, public IP, refactor actuators, types

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 5, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justaugustus

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 5, 2019
@justaugustus
Copy link
Member Author

justaugustus commented Feb 5, 2019 •
edited
Loading

This is NOT ready for review; just putting it up for visibility.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2019
@justaugustus
Fix dependencies
88e4209 
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
@justaugustus
Add annotation to machine Create to satisfy waitForMachineReady
5235009 
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2019
@justaugustus
Refactor actuator methods, update types, regenerate manifests
08a77ae 
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 19, 2019
tariq1890
tariq1890 reviewed Feb 26, 2019
View reviewed changes
pkg/cloud/azure/services/resources/template/deployment-params.json Outdated
@@ -33,7 +33,7 @@
"value": "Standard_B2ms"
},
"location": {
"value": "eastus"
"value": "eastus2"
Copy link
Contributor

Choose a reason for hiding this comment

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

I am really a westus2 person :P

Suggested change
"value": "eastus2"
"value": "westus2"

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh. This file is actually from the older implementations of capz.
We never copy this into the cluster-api-azure-controller, so I'll delete it.

tariq1890
tariq1890 reviewed Feb 26, 2019
View reviewed changes
pkg/deployer/deployer.go

pip, err := pipsvc.GetPublicIPAddress(scope.ClusterConfig.ResourceGroup, resources.GetPublicIPName(machine))
pip, err := pipSvc.CreateOrUpdatePublicIPAddress(scope.ClusterConfig.ResourceGroup, resourcesSvc.GetPublicIPName(machine))
Copy link
Contributor

Choose a reason for hiding this comment

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

The more that I think about this, I have a preference towards naming this EnsurePublicIPAddress. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Still sticking to my comment above 😛 :#88 (comment)

I initially had this as CreateOrGetPublicAddress with separate logic for each, but we actually always want to reconcile it (update DNS name label, etc.), so I changed the method name.

I thought about EnsurePublicIPAddress, because it's similar to the usage in cloud-provider-azure, but decided on CreateOrUpdate, because it's more inline with the underlying Azure call (s.scope.PublicIPAddresses.CreateOrUpdate).

tariq1890
tariq1890 reviewed Feb 26, 2019
View reviewed changes
pkg/cloud/azure/services/resources/groups.go
)

// ReconcileResourceGroup reconciles the resource group of the given cluster.
func (s *Service) ReconcileResourceGroup() (err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if there is a ResourceGroup equivalent in AWS. But I personally feel we shouldn't be using the Resource Group abstractions at all. (This is just my opinion though and not representative of Azure). As we are experimenting with the possibilities of VNet peering (using inter-resource-group Vnets), this could prove to be problematic. Definitely open to discussing this in detail in the future.

Copy link
Contributor

@tariq1890 tariq1890 Feb 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@awesomenix FYI. Let me know If I may have expressed anything incorrectly here.

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK, AWS doesn't have an equivalent abstraction like resource group.
In the pre-refactor capz (v0.1.0-alpha.1), we're still creating resource groups; it's just happening in the deployment template as opposed to handled in the code.

I can see a future where we may allow multiple resource groups, say, to separate network from compute resources, but in the near-term, and definitely for this PR, we need to be able to handle them.

A huge benefit of having everything scoped to a single RG, especially at this stage of maturity for capz, is that cleanup is as simple as destroying the RG.

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 26, 2019
This was referenced Feb 26, 2019
Closed
Closed
Closed
@justaugustus
Update method comments, copyrights, remove deployment-params.json
6436ff4 
Co-Authored-By: justaugustus <justaugustus@users.noreply.github.com>
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
@justaugustus
Copy link
Member Author

@tariq1890 / @soggiest -- this is ready for a final sweep.
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 26, 2019
@tariq1890
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2019
@k8s-ci-robot k8s-ci-robot merged commit e22395e into kubernetes-sigs:master Feb 26, 2019
detiber
detiber reviewed Feb 26, 2019
View reviewed changes
Makefile Show resolved Hide resolved
cmd/clusterctl/examples/azure/generate-yaml.sh Show resolved Hide resolved
docs/getting-started.md Show resolved Hide resolved
pkg/apis/azureprovider/v1alpha1/azureclusterproviderconfig_types.go
FrontProxyCAKeyPair KeyPair `json:"frontProxyCAKeyPair,omitempty"`

// SAKeyPair is the service account key pair.
SAKeyPair KeyPair `json:"saKeyPair,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it would make sense to publish these external to the AWS/Azure providers and import them into both?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add them to cluster-api itself, rather than in the provider? Makes sense, since certs are provider agnostic as far as I'm aware.

Copy link
Member

Choose a reason for hiding this comment

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

Long term, yes. Shorter term, we can stage in a separate repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed that we should eventually pull the reusable pieces into capi proper.

pkg/tokens/tokens.go Show resolved Hide resolved
@justaugustus
Copy link
Member Author

@detiber -- the version reversion was intentional. I retagged and released the existing versions (v0.1.0-alpha.1..3) to fall closer in line with the versioning scheme in capa.

Once we hit a place where master consistently builds Azure clusters and work out a few bugs, we'll cut a v0.1.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soggiest soggiest soggiest approved these changes

@vincepri vincepri vincepri left review comments

@detiber detiber detiber left review comments

@roberthbailey roberthbailey Awaiting requested review from roberthbailey

@tariq1890 tariq1890 Awaiting requested review from tariq1890

Assignees

@detiber detiber

@randomvariable randomvariable

@tariq1890 tariq1890

@vincepri vincepri

@soggiest soggiest

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@justaugustus @k8s-ci-robot @soggiest @randomvariable @tariq1890 @detiber @vincepri