Bug 1769879: allow CA Cert bundles to be trusted #78

iamemilio · 2020-01-08T21:16:35Z

What this PR does / why we need it: Adds ability to trust CA bundle to CAPO

openshift-ci-robot · 2020-01-08T21:16:38Z

@iamemilio: This pull request references Bugzilla bug 1788072, which is invalid:

expected the bug to target the "4.4.0" release, but it targets "4.3.z" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

[WIP] Bug 1788072: allow CA Cert bundles to be trusted

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-01-08T21:21:22Z

@iamemilio: This pull request references Bugzilla bug 1769879, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

[WIP] Bug 1769879: allow CA Cert bundles to be trusted

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

iamemilio · 2020-01-08T21:33:08Z

/retest

iamemilio · 2020-01-09T19:37:18Z

Was able to deploy workers on a test cluster with self signed certificates, and was able to scale the workers as well. Ready to review and merge.

mandre · 2020-01-10T07:15:23Z

pkg/cloud/openstack/clients/machineservice.go

 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("Create new provider client failed (clients/machienservice.go 197): %v", err)


We don't need the extra debugging messages with the line numbers - leftover from testing ?

mandre · 2020-01-10T07:18:58Z

pkg/apis/openstackproviderconfig/v1alpha1/types.go

@@ -209,6 +212,9 @@ type OpenstackClusterProviderSpec struct {

 	// Default: True. In case of server tag errors, set to False
 	DisableServerTags bool `json:"disableServerTags,omitempty"`
+
+	// CertBundle gets populated from the value in Machines
+	//	CertBundle string `json:"certBundle,omitempty"`


If this is commented out, does it mean it's not needed? In that case can we remove it from the patch?

mandre · 2020-01-10T07:23:46Z

pkg/apis/openstackproviderconfig/v1alpha1/types.go

@@ -82,6 +82,9 @@ type OpenstackProviderSpec struct {

 	// The volume metadata to boot from
 	RootVolume *RootVolume `json:"rootVolume,omitempty"`
+
+	// A plaintext string of PEM(s)
+	CertBundle string `json:"caCert,omitempty"`


Could you possibly move this up to the top of the struct, together with the other cloud parameters CloudsSecret and CloudName?

mandre · 2020-01-10T07:33:28Z

pkg/cloud/openstack/clients/machineservice.go

 	}

-	opts.AllowReauth = true


Any reason to drop this option?

According to the documentation:

// AllowReauth should be set to true if you grant permission for Gophercloud to // cache your credentials in memory, and to allow Gophercloud to attempt to // re-authenticate automatically if/when your token expires. If you set it to // false, it will not cache these settings, but re-authentication will not be // possible. This setting defaults to false. // // NOTE: The reauth function will try to re-authenticate endlessly if left // unchecked. The way to limit the number of attempts is to provide a custom // HTTP client to the provider client and provide a transport that implements // the RoundTripper interface and stores the number of failed retries. For an // example of this, see here: // https://github.com/rackspace/rack/blob/1.0.0/auth/clients.go#L311

oops, that was an oversight

pkg/cloud/openstack/clients/machineservice.go

pierreprinetti · 2020-01-10T17:26:47Z

LGTM

@mandre it's all yours

iamemilio · 2020-01-10T18:58:26Z

/retest

iamemilio · 2020-01-10T20:26:45Z

/retest

pierreprinetti · 2020-01-11T10:19:17Z

/retest

mandre · 2020-01-13T07:12:59Z

/test e2e-openstack

tomassedovic · 2020-01-13T07:56:31Z

/test e2e-openstack

Infra flakyness.

mandre · 2020-01-13T08:47:58Z

CIRO won't start because the spec isn't valid after openshift/api#560:

E0111 10:54:45.224224      13 controller.go:257] unable to sync: Config.imageregistry.operator.openshift.io "cluster" is invalid: spec.rolloutStrategy: Invalid value: "": spec.rolloutStrategy in body should match '^(RollingUpdate|Recreate)$', requeuing

https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-api-provider-openstack/78/pull-ci-openshift-cluster-api-provider-openstack-master-e2e-openstack/95/artifacts/e2e-openstack/pods/openshift-image-registry_cluster-image-registry-operator-5498f86dc7-vzlcm_cluster-image-registry-operator.log

mandre · 2020-01-13T08:51:05Z

Should be fixed with openshift/api#563

mandre · 2020-01-13T14:11:21Z

/lgtm

openshift-ci-robot · 2020-01-13T14:11:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamemilio, mandre

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~~ [iamemilio,mandre]

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

* Implemented bootstrap tokens via cluster-bootstrap Therefore, I added new dependency to k8s.io/cluster-bootstrap. Shelling out to kubeadm from token generation is now removed from machineactuator.go. I used the tools of cluster-bootstrap to generate a kubeadm compliant token name and create a secret out of it. This implementation does not use the ways kubeadm currently uses, because it would pull a hole bunch of other dependencies in, which are IMHO not needed at that point. Fixes openshift#78 * Renamed TokenExpiration to TokenTTL * Increased TokenTTL to 60 minutes * Added link to inspiration of bootstrap/token.go * Changed error handling to panic and removed needless lines

…t#95) * Added Role to kube-system to allow CAP-OS to create secrets When CAP-OS is deployed to the resulted cluster, it runs in the namespace openstack-provider-system, and gets the default ServiceAccount mounted in. Because of openshift#78, CAP-OS needs to create secrets (bootstrap tokens essentially are secrets) in namespace kube-system. Created a Role and RoleBinding to allow this. Using yaml files in config/rbac in favor of kubebuilder auto generation, because this only works for ClusterRoles so far. See kubernetes-sigs/kubebuilder#401 How to test: ``` generate-yaml.sh -c ... -p ubuntu configure machines.yaml and provider-components.yaml clusterctl create cluster \ --minikube kubernetes-version=v1.12.2 \ --vm-driver hyperkit \ --provider openstack \ -c examples/openstack/ubuntu/out/cluster.yaml \ -m examples/openstack/ubuntu/out/machines.yaml \ -p examples/openstack/ubuntu/out/provider-components.yaml wait until master and the one node appear. create a new machine manually by hand. It should be created, come up and join the cluster. e.g. apiVersion: cluster.k8s.io/v1alpha1 kind: Machine metadata: generateName: openstack-node- labels: set: node name: openstack-node-manual namespace: default spec: providerConfig: value: apiVersion: openstackproviderconfig/v1alpha1 availabilityZone: es1 flavor: m1.medium image: Ubuntu 16.04 Xenial Xerus - Latest kind: OpenstackProviderConfig networks: - uuid: e21aeb04-f98a-4c05-bc84-69441dbb304c securityGroups: - default - secgrp_docs sshUserName: ubuntu versions: kubelet: 1.12.1 ``` Fixes openshift#93 * Removed redundant role and rolebinding

iamemilio requested a review from Fedosin January 8, 2020 21:16

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jan 8, 2020

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 8, 2020

openshift-ci-robot requested review from mandre and tomassedovic January 8, 2020 21:17

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 8, 2020

iamemilio changed the title ~~[WIP] Bug 1788072: allow CA Cert bundles to be trusted~~ [WIP] Bug 1769879: allow CA Cert bundles to be trusted Jan 8, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 8, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jan 8, 2020

iamemilio changed the title ~~[WIP] Bug 1769879: allow CA Cert bundles to be trusted~~ Bug 1769879: allow CA Cert bundles to be trusted Jan 9, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 9, 2020

mandre reviewed Jan 10, 2020

View reviewed changes

pierreprinetti reviewed Jan 10, 2020

View reviewed changes

pkg/cloud/openstack/clients/machineservice.go Outdated Show resolved Hide resolved

pierreprinetti reviewed Jan 10, 2020

View reviewed changes

pkg/cloud/openstack/clients/machineservice.go Outdated Show resolved Hide resolved

allow CA Cert bundles to be trusted

17be769

iamemilio force-pushed the machine-api-CAcerts branch from eda87fd to 17be769 Compare January 10, 2020 16:40

openshift-ci-robot assigned mandre Jan 13, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2020

openshift-merge-robot merged commit ffbc0f0 into openshift:master Jan 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1769879: allow CA Cert bundles to be trusted #78

Bug 1769879: allow CA Cert bundles to be trusted #78

iamemilio commented Jan 8, 2020

openshift-ci-robot commented Jan 8, 2020

openshift-ci-robot commented Jan 8, 2020

iamemilio commented Jan 8, 2020

iamemilio commented Jan 9, 2020 •

edited

mandre Jan 10, 2020

mandre Jan 10, 2020

mandre Jan 10, 2020

mandre Jan 10, 2020

iamemilio Jan 10, 2020

pierreprinetti commented Jan 10, 2020

iamemilio commented Jan 10, 2020

iamemilio commented Jan 10, 2020

pierreprinetti commented Jan 11, 2020

mandre commented Jan 13, 2020

tomassedovic commented Jan 13, 2020

mandre commented Jan 13, 2020

mandre commented Jan 13, 2020

mandre commented Jan 13, 2020

openshift-ci-robot commented Jan 13, 2020

Bug 1769879: allow CA Cert bundles to be trusted #78

Bug 1769879: allow CA Cert bundles to be trusted #78

Conversation

iamemilio commented Jan 8, 2020

openshift-ci-robot commented Jan 8, 2020

openshift-ci-robot commented Jan 8, 2020

iamemilio commented Jan 8, 2020

iamemilio commented Jan 9, 2020 • edited

mandre Jan 10, 2020

Choose a reason for hiding this comment

mandre Jan 10, 2020

Choose a reason for hiding this comment

mandre Jan 10, 2020

Choose a reason for hiding this comment

mandre Jan 10, 2020

Choose a reason for hiding this comment

iamemilio Jan 10, 2020

Choose a reason for hiding this comment

pierreprinetti commented Jan 10, 2020

iamemilio commented Jan 10, 2020

iamemilio commented Jan 10, 2020

pierreprinetti commented Jan 11, 2020

mandre commented Jan 13, 2020

tomassedovic commented Jan 13, 2020

mandre commented Jan 13, 2020

mandre commented Jan 13, 2020

mandre commented Jan 13, 2020

openshift-ci-robot commented Jan 13, 2020

iamemilio commented Jan 9, 2020 •

edited