Add OpenStack credential propagation support #697
Conversation
Signed-off-by: Bharath Nallapeta <[email protected]>
bnallapeta
requested review from
Kshatrix,
eromanova and
a13x5
as code owners
| return nil | ||
| } | ||
|
|
||
| func generateOpenStackCCMSecret(openstackSecret *corev1.Secret) *corev1.Secret { |
There was a problem hiding this comment.
You should create the secret with OpenStack cloud configuration following the specific format. It's not the same as the secret with credentials in the management cluster (the format: https://github.com/kubernetes/cloud-provider-openstack/blob/master/manifests/controller-manager/cloud-config).
Related thing: credentials should not be exposed, so instead of passing the credentials as chart values you should pass the name of this secret to the CCM and CSI values: https://github.com/Mirantis/hmc/pull/696/files#diff-c5afee6dd916ada2ea759da67744b5845149af9a6532b3891574865168c2c44e
See: https://github.com/kubernetes/cloud-provider-openstack/blob/master/charts/cinder-csi-plugin/values.yaml#L162
Similarly, it's done for the azure and the vsphere providers. You can use it as a reference.
There was a problem hiding this comment.
I believe it's done because the secret format is exactly the same. In this case this function makes no sense and must be removed.
@bnallapeta you can just makeSecret("openstack-cloud-config", openstackSecret.Data)
There was a problem hiding this comment.
@eromanova Yes, it already follows the same format. And based on the other comments, I have removed passing values to ccm and csi and am passing just the secret. So, this should ideally work.
@a13x5 Yes, I defined a function here with the intention of doing the INI format thing. But later realized I don't have to do that. I agree that we don't need this function. Modified.
I will anyways test this out and make sure everything works.
There was a problem hiding this comment.
Several issues with overall logic.
Also I believe the scope of this PR could be broaden a little bit by adding credential passing to the cluster (setIdentityHelmValues function). Currently we're expecting ClusterIdentity object only, this isn't the case with OpenStack provider, so logic should be adjusted/tested. If you prepared another PR with these changes it should be merged first.
| @@ -53,11 +53,11 @@ func applyCCMConfigs(ctx context.Context, kubeconfSecret *corev1.Secret, objects | |||
| return nil | |||
| } | |||
|
|
|||
| func makeSecret(name, namespace string, data map[string][]byte) *corev1.Secret { | |||
| func makeSecret(name string, data map[string][]byte) *corev1.Secret { | |||
There was a problem hiding this comment.
Honestly don't see a point in hardcoding parameters here. I can't be sure that at some point we won't need to set namespace to something else.
There was a problem hiding this comment.
https://github.com/Mirantis/hmc/actions/runs/12067350380/job/33650115071
This is the reason for this change. I am not sure how it passed lint for other functions which I have updated too. Basically it would always get metav1.NamespaceSystem which would always evaluate to kube-system.
I think when we get to that point of changing the namespace, we can parameterize this.
There was a problem hiding this comment.
Well, shit...
Got it, then please adjust makeConfigMap as well, so both functions have the same signature.
It would be good also to rename them to reflect that it will make secret in kube ns, but it's up to you really.
| return nil | ||
| } | ||
|
|
||
| func generateOpenStackCCMSecret(openstackSecret *corev1.Secret) *corev1.Secret { |
There was a problem hiding this comment.
I believe it's done because the secret format is exactly the same. In this case this function makes no sense and must be removed.
@bnallapeta you can just makeSecret("openstack-cloud-config", openstackSecret.Data)
| openstackCredential := &hmc.Credential{} | ||
| if err := cfg.Client.Get(ctx, client.ObjectKey{ | ||
| Name: openstackManagedCluster.Spec.Credential, | ||
| Namespace: openstackManagedCluster.Namespace, | ||
| }, openstackCredential); err != nil { | ||
| return fmt.Errorf("failed to get OpenStackCredential %s: %w", cfg.ManagedCluster.Spec.Credential, err) | ||
| } |
There was a problem hiding this comment.
This call could be avoided. We already requested this object before in the managedcluster_controller.
There was a problem hiding this comment.
Not that a single api call would impact api server anyways. Especially since we would have 1 Credential resource. But sure, have removed this and passed params from managedcluster_controller.go
There was a problem hiding this comment.
It's not just single API call here and it's not depends on the amount of Credential resource, but rather on amount of ManagedCluster resources.
So it's more like one more call for each reconcile for each ManagedCluster.
| openstackSecretName := openstackCredential.Spec.IdentityRef.Name | ||
| openstackSecretNamespace := openstackCredential.Spec.IdentityRef.Namespace |
There was a problem hiding this comment.
These aren't needed - you use this variables just once, consider passing values directly into struct below.
There was a problem hiding this comment.
Why? Its generally a good practice to use variables. What if its used just once?
There was a problem hiding this comment.
Generally yes.
But in this particular example you already have variable openstackCredential. You copied part of it in other vars without any processing to pass them as a parameters and discard.
It would make sense to use variables here in case of:
- modifying data from
openstackCredential - using it in multiple places to improve readability (doubtful, but ok)
TL;DR here it just looks weird.
bnallapeta
force-pushed
the
openstack-add-creds
branch
from
ff9acc5
to
e6b634f
Compare
Signed-off-by: Bharath Nallapeta <[email protected]>
bnallapeta
force-pushed
the
openstack-add-creds
branch
from
e6b634f
to
de4cd66
Compare
No description provided.