All the fixes for cluster api v0.1.4 #62

chuckha · 2019-07-03T15:36:27Z

Signed-off-by: Chuck Ha chuckh@vmware.com

What this PR does / why we need it:

This PR removes the need to copy and paste RBAC and CRD definitions. Instead we download them from a provided version of CAPI.

This PR also adds many fixes to support cluster-api v0.1.4.

This PR adds a very annoying bug that I've yet to find a good solution for regarding kubeconfig secrets and the cluster endpoint.

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 #32

Removes `capd` and `crds` command since those were only useful for setting up a management cluster

Signed-off-by: Chuck Ha <chuckh@vmware.com>

k8s-ci-robot · 2019-07-03T15:36:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha

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:

~~OWNERS~~ [chuckha]

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

Signed-off-by: Chuck Ha <chuckh@vmware.com>

chuckha · 2019-07-03T18:41:35Z

/hold

this is not right, rbac changed a fair bit here

Signed-off-by: Chuck Ha <chuckh@vmware.com>

chuckha · 2019-07-03T21:15:04Z

/hold cancel

yay this is finally done

actuators/machine.go

cmd/capd-manager/main.go

chuckha · 2019-07-03T21:16:20Z

kind/actions/cluster_actions.go

@@ -379,3 +346,8 @@ func KubeadmReset(clusterName, nodeName string) error {

 	return nil
 }
+
+// ProviderID formats the provider id needed to set on the node
+func ProviderID(name string) string {


Put this logic in ONE place instead of having it split in two places with the wrong provider ID

chuckha · 2019-07-03T21:17:47Z

kind/actions/kind.go

-// getLoadBalancerPort returns the port on the host on which the APIServer is exposed
-func getLoadBalancerPort(allNodes []nodes.Node) (int32, error) {
+// GetLoadBalancerHostAndPort returns the port on the host on which the APIServer is exposed
+func GetLoadBalancerHostAndPort(allNodes []nodes.Node) (string, int32, error) {


This gets the docker IP of the load balancer container. We use this to write the Docker IP into the kubeadm config for the node-ref controller

chuckha · 2019-07-03T21:19:47Z

cmd/capdctl/main.go

+				}
+
+				// This is a poor person's kustomize
+				if strings.HasSuffix(header.Name, "manager.yaml") {


This makes me sad but it is better than copy and pasting

actuators/actuators.go

vincepri

minor nits, otherwise LGTM

actuators/actuators.go

cmd/capd-manager/main.go

vincepri · 2019-07-03T21:29:35Z

cmd/capd-manager/main.go

@@ -35,6 +36,9 @@ import (
 )

 func main() {
+	flag.Set("v", "0")
+	flag.Parse()
+
 	cfg, err := config.GetConfig()
 	if err != nil {
 		panic(err)


klog.Fatal?

Yep, This is covered in #8

I'll time that out soon depending on if the contributor contributes or not

vincepri · 2019-07-03T21:31:02Z

kind/actions/cluster_actions.go

@@ -179,7 +179,7 @@ func KubeadmInit(clusterName, version string) error {
 	}

 	// save the kubeconfig on the host with the loadbalancer endpoint
-	hostPort, err := getLoadBalancerPort(allNodes)
+	_, hostPort, err := GetLoadBalancerHostAndPort(allNodes)


hostPort -> port?

maybe? I don't really see a huge benefit in this rename. I recognize host is a bit inaccurate technically but it may help to think about each container as a host.

ah i refer to host as docker host in other places, i will clarify, good point

akutz

Hi @chuckha,

This isn't the review I want to give, but it's the review that time permits at the moment. This is awesome stuff!

akutz · 2019-07-04T00:40:52Z

actuators/actuators.go

+	lines := bytes.Split(data, []byte("\n"))
+	for i, line := range lines {
+		if bytes.Contains(line, []byte("https://")) {
+			lines[i] = []byte(fmt.Sprintf("    server: https://%s:%d", lbip, 6443))


Hi @chuckha,

Should the port always be assumed?

yes, the port should always be assumed as 6443. The apiservers will always be listening on their own IP at port 6443 until we allow users to customize support. This will change when support for that feature is added.

akutz · 2019-07-04T00:44:37Z

actuators/actuators.go

+	// They share the same bridged network and the load balancer does respond on 6443 at its docker IP
+	// however, the *HOST* is listening on some random port (the one returned from the GetLoadBalancerHostAndPort).
+	lbip, _, err := actions.GetLoadBalancerHostAndPort(allNodes)
+	lines := bytes.Split(data, []byte("\n"))


Hi @chuckha,

Why not use ScanLines? Even if not, maybe prefer the Golang regex \r?\n?

ScanLines would also work, this was just in my head at the time. If this becomes an issue in the future we can address it later.

Fair, but I also don't think there's a good reason for not switching to stdlib.

I don't understand what you mean, bytes is in the stdlib

Yeah, sorry, my response was poorly worded. I was referring to how ScanLines uses a more comprehensive matching pattern. If this won't be an issue here because the line endings are guaranteed, then this is fine.

ah i see what you mean. Yes, that would definitely be more robust. It will be worth fixing if we need to fix it later as I'm not expecting line endings other than \n at this time. The file we're reading is generated by kubeadm on a unix based system.

FWIW, I know Golang treats \n as a special character. If you use it in a fmt.Sprintf (or a variant), Go will always translate \n to the OS-specific line-ending character(s). I'm not sure if this is the case on scanning though. Either way, as you said, it's a non-issue here.

actuators/machine.go

akutz · 2019-07-04T00:46:56Z

cmd/capdctl/main.go

-	capdImage := capd.String("capd-image", "gcr.io/kubernetes1-226021/capd-manager:latest", "The capd manager image to run")
-	capiImage := capd.String("capi-image", "gcr.io/k8s-cluster-api/cluster-api-controller:0.1.3", "The capi manager image to run")
+	version := setup.String("capi-version", "v0.1.4", "The CRD versions to pull from CAPI")
+	capdImage := setup.String("capd-image", "gcr.io/kubernetes1-226021/capd-manager:latest", "The capd manager image to run")


Hi @chuckha,

I'm not personally a fan of latest as it has unpredictable results. Will using it here result in possible breakage?

Totally agree. I've opened an issue to figure out a better strategy here: #67

kind/actions/kind.go

vincepri

/lgtm

chuckha added 2 commits July 3, 2019 11:33

Download CAPI CRDs from release tarball

ac0eeba

Signed-off-by: Chuck Ha <chuckh@vmware.com>

Removes unnecessary instructions in README

414d24e

Signed-off-by: Chuck Ha <chuckh@vmware.com>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 3, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 3, 2019

k8s-ci-robot requested review from detiber and justinsb July 3, 2019 15:36

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 3, 2019

fix verify

1448394

Signed-off-by: Chuck Ha <chuckh@vmware.com>

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2019

Fixes for cluster api v0.1.4

c29fe4e

Signed-off-by: Chuck Ha <chuckh@vmware.com>

chuckha changed the title ~~Download CAPI CRDs from release tarball~~ All the fixes for cluster api v0.1.4 Jul 3, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2019

chuckha commented Jul 3, 2019

View reviewed changes

vincepri reviewed Jul 3, 2019

View reviewed changes

akutz reviewed Jul 4, 2019

View reviewed changes

vincepri approved these changes Jul 5, 2019

View reviewed changes

k8s-ci-robot assigned vincepri Jul 5, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2019

k8s-ci-robot merged commit 7a0f729 into kubernetes-retired:master Jul 5, 2019

chuckha mentioned this pull request Jul 10, 2019

Get rid of the code that sets the node ref #43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

All the fixes for cluster api v0.1.4 #62

All the fixes for cluster api v0.1.4 #62

chuckha commented Jul 3, 2019 •

edited

Loading

k8s-ci-robot commented Jul 3, 2019

chuckha commented Jul 3, 2019

chuckha commented Jul 3, 2019

chuckha Jul 3, 2019

chuckha Jul 3, 2019

chuckha Jul 3, 2019

vincepri left a comment

vincepri Jul 3, 2019

chuckha Jul 5, 2019

vincepri Jul 3, 2019

chuckha Jul 5, 2019

chuckha Jul 5, 2019

akutz left a comment

akutz Jul 4, 2019

chuckha Jul 5, 2019

akutz Jul 4, 2019

chuckha Jul 5, 2019

akutz Jul 5, 2019

chuckha Jul 5, 2019

akutz Jul 5, 2019

chuckha Jul 5, 2019

akutz Jul 5, 2019

akutz Jul 4, 2019

chuckha Jul 5, 2019

vincepri left a comment

All the fixes for cluster api v0.1.4 #62

All the fixes for cluster api v0.1.4 #62

Conversation

chuckha commented Jul 3, 2019 • edited Loading

k8s-ci-robot commented Jul 3, 2019

chuckha commented Jul 3, 2019

chuckha commented Jul 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akutz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

chuckha commented Jul 3, 2019 •

edited

Loading