-
Notifications
You must be signed in to change notification settings - Fork 253
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.
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
Support keystone with certificate #224
Support keystone with certificate #224
Conversation
Hi @hidekazuna. Thanks for your PR. I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test |
transport := &http.Transport{TLSClientConfig: tlsConfig} | ||
client.HTTPClient = http.Client{Transport: transport} | ||
|
||
err = openstack.Authenticate(client, *ao) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgive me if I misunderstand something here.. I believe https need ca and http doesn't need it
from context we are requesting that to be mandatory https ,maybe I missed something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. I should have update code https is not mandatory. I will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in the following commit: 192e70c
f172002
to
1a44f4e
Compare
1a44f4e
to
4ce9537
Compare
f4901b3
to
41b7b6c
Compare
41b7b6c
to
3b2ecfe
Compare
cmd/clusterctl/examples/openstack/ubuntu/provider-components.yaml.template
Outdated
Show resolved
Hide resolved
@@ -143,7 +144,7 @@ PASSWORD=$(echo "$OPENSTACK_CLOUD_CONFIG_PLAIN" | yq r - clouds.$CLOUD.auth.pass | |||
REGION=$(echo "$OPENSTACK_CLOUD_CONFIG_PLAIN" | yq r - clouds.$CLOUD.region_name) | |||
PROJECT_ID=$(echo "$OPENSTACK_CLOUD_CONFIG_PLAIN" | yq r - clouds.$CLOUD.auth.project_id) | |||
DOMAIN_NAME=$(echo "$OPENSTACK_CLOUD_CONFIG_PLAIN" | yq r - clouds.$CLOUD.auth.user_domain_name) | |||
|
|||
CACERT_ORIGINAL=$(echo "$OPENSTACK_CLOUD_CONFIG_PLAIN" | yq r - clouds.$CLOUD.cacert) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be problem if cacert doesn't exist...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in the following commit: 192e70c
|
||
cloudFromYaml, err := clientconfig.GetCloudFromYAML(clientOpts) | ||
if cloudFromYaml.CACertFile != "" { | ||
roots, err := certutil.NewPool(caFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking about the mounted caFile here. In theory, the entire code allows us to use a separate clouds.yaml
per machine. But it is only possible to pass in a caFile for all machines.
How about extending the cloud-config
secret with another field for the contents of the caFile.... and create a separate x509.CertPool
from this secret?
I think it would be ok to extend the cloud-config
secret, because it relates directly to the clouds.yaml
, rather than having a separate secret.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hidekazuna @gyliu513 provided this to me
master...cdc1807:cacert-auth
I think we can try to refer to this one, at least it works for me now on a https openstack and I think we can use secret to replace the CACERT in the commit diff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jichenjc Thanks advice.
I updated like this: 672c5af#diff-6f1595bb05179a80513b3baba49fbaf1R58
I noticed I need to update getNetworkClient function in pkg/cloud/openstack/cluster/actuator.go.
But I wonder how to fix this function as well. Could you advice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might need more time to figure out but FYI https://github.com/kubernetes/cloud-provider-openstack has a set of https handling including network, maybe we can refer to their implementation @hidekazuna
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrigl @jichenjc getNetworkClient is the method of Actuator in cluster package and it's augment is *clusterv1.Cluster. We can not secrets from Actuator nor clusterv1.Cluster. I thinks this is from design issue.
Since we have to create a CA file for using OpenStack Cloud provider anyway, can't we make a compromise to use CA file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not aware clusterv1 can't use secrets from Actuator, ... but conceptually yes, if you want to sync cluster to create network, apparently we need CA in order to connect to openstack, so I think it's mandatory to use CA in actuator, can we define in openstackclusterspec for this ? @hidekazuna
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jichenjc Thanks, now I am trying to update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc |
@morvencao: GitHub didn't allow me to request PR reviews from the following users: morvencao. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
@@ -79,6 +79,8 @@ CLUSTER_DNS_SERVER=$(prips ${SERVICE_CIDR} | head -n 11 | tail -n 1) | |||
|
|||
# Write the cloud.conf so that the kubelet can use it. | |||
echo $OPENSTACK_CLOUD_PROVIDER_CONF | base64 -d > /etc/kubernetes/cloud.conf | |||
mkdir /etc/certs | |||
echo $OPENSTACK_CLOUD_CACERT_CONFIG | base64 -d > /etc/certs/cacert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious
this is only for ubuntu, do we have a centos version? I didn't see it ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks added this for centos.
@@ -186,14 +199,17 @@ if [[ "$PROVIDER_OS" == "coreos" ]]; then | |||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you updated centos and ubuntu
but how about coreos ? I didn't see the change ,maybe a follow up??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I Updated files for coreos.
thanks @hchenxa and @hidekazuna in addition, I had some comments above, can you help to take a look @hidekazuna ? thank you |
0bf8a1b
to
b06fd90
Compare
@hidekazuna
|
b06fd90
to
7cff3fa
Compare
/test pull-cluster-api-provider-openstack-test |
copy the link I added in the slack channel :) @hidekazuna the #224 failed with a new test I added recently #322, at least it works when I submit the PR with my local stuffs and gate, please let me know whether some edge case triggered by your PR and any info needed by me ,thanks |
7cff3fa
to
ab62a6c
Compare
/lgtm |
I think we are ready for this one, as the @hchenxa already tested it |
LGTM Leave this to @chrigl for approve. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gyliu513, hidekazuna 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 |
What this PR does / why we need it:
This PR supports keystone endpoint with certificate by the following clouds.yaml with cacert key.
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 #209
Special notes for your reviewer:
Release note: