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.

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

Multi-vo renew fts cronjob #126

Closed
wants to merge 4 commits into from

Conversation

gmatthews20
Copy link

Add variables required for multi-vo
Add additional secrets to mount extra voms info
Update readme with multi-vo info

@gmatthews20 gmatthews20 marked this pull request as ready for review January 23, 2023 13:28
@gmatthews20 gmatthews20 linked an issue Jan 23, 2023 that may be closed by this pull request
Copy link
Contributor

@rcarpa rcarpa left a comment

Choose a reason for hiding this comment

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

I don't like this PR at so many levels. This build on top of rucio/containers#207, which, IMHO, shouldn't have been merged as it is right now.

Why do we have a "vo" called multi_vo? isn't the whole point of multi_vo to have more than one VO?

We now have an implicit secret name <releasename>-rucio-x509up-<voms> which we will not be able to change without coordinate commits in two different repositories.

As per our own example in values.yaml in this repository, voms is not necessarily an alphanumeric string (example: voms: "cms:/cms/Role=production"), and here everything assumes that "vomses" will be a list of alphanumerics separated by commas. Why?

This, and rucio/containers#20, has to go back to the drawing board. @mlassnig , @bari12 ?

@gmatthews20
Copy link
Author

Thanks for the feedback.
I agree with the suggested changes and will have a look at fixes for them

@Thysk
Copy link

Thysk commented Jan 26, 2023

Thanks Radu, that was from our misunderstanding of the renewal process and how others may use it
George is working on re-working the rucio/containers#207 and this one.

The changes we are thinking of are checking in vo is a list to then enter a multi_vo set of code, that extracts vo and voms from the same index in the list, rather than what we have done now.

As for the implicit secret name, is this a problem? This is to be changed to <releasename>-rucio-x509up-<vo>
This method we thought was an ok alternative to having explicit and hard coded vo names. But if you have another idea on how to handle this please do let us know.

Add longproxy for multi-vo
Add additional secrets for voms server info
Add variables for multi-vo
Add multi-vo fts cronjob to documentation
@gmatthews20
Copy link
Author

I have made the changes so that vo and voms aren't the same. They are both set in a vomses section with the desired voms linked to the vo. I have also made changes reflecting this for the container (rucio/containers#226). I have also updated how the secrets are set so they can be set in the values.yaml.

@rcarpa
Copy link
Contributor

rcarpa commented Feb 13, 2023

The whole architecture of this is completely opaque to me. I don't understand how this is intended to be integrated with rucio. From a helm perspective: how the releasename-rucio-x509up-vo secrets will end be mounted in whatever pod needs them? For example: submitter. From the rucio perspective: how submitter will have to be configured (in rucio.cfg terms) to use these secrets ?

@rcarpa
Copy link
Contributor

rcarpa commented Feb 14, 2023

I went through rucio and my understanding is as follows: there is a configuration section vo_certs, which has the list of all vo-s to be used and the location of the associated certificates.

This will be configured in helm charts using the following values:

config:
  vo_certs:
    vo1: /path/to/cert1
    vo2: /path/to/cert2

Moreover, with your solution from the current PR, one will also have to manually create, inside the cluster, a bunch of
releasename-longproxy-vo1, releasename-longproxy-vo2 secrets. And add to the helm values the following config:

ftsRenewal:
  vomses:
    - vo: vo1
      voms: "a:/b/Role=production"
    - vo: vo2
      voms: "c:/d/Role=production"

This will generate the secrets releasename-x509up-vo1 and releasename-x509-vo2 in the cluster. So one will have now to, additionally, add to helm values the following configuration:

    additionalSecrets:
      - secretName: x509up-vo1
        mountPath: /path/to/cert1
        subPath: x509up
      - secretName: x509up-vo2
        mountPath: /path/to/cert2
        subPath: x509up

Did I understand it correctly ? If yes, I consider that this is quite a lot of repetition. Adding an additional vo is too complex and requires 3 different values to be set in the helm chart to make it work. All these values are linked implicitly by secret names which cannot be known without looking at the helm source code. Meaning that anybody who will want to try multi-vo rucio will have an extremely high barrier of entry.

We must find a way to implement the multi-vo workflow in helm charts in way that doesn't require repetition.

@rcarpa
Copy link
Contributor

rcarpa commented Feb 15, 2023

@Thysk , @gmatthews20 . I prepared a change to support multi-vo fts renewals. Obviously, an associated change will have to be done in the containers repository. I would appreciate you input. Does if fit your needs? Should I implement anything else ?

#139

@Thysk
Copy link

Thysk commented Feb 15, 2023

@rcarpa These changes look comprehensive and I believe should cover our usecase as well
So if I understand correctly, your changes in your commit
allows us to define which script to use , and then also what the VOs and voms roles are created. if I understand correctly the secretMounts then need to be defined, which is all fine, naming the secrets going in, mountPath and subPath

As I understand it, a Multi-VO user would then also need to create the:
'config:
vo_certs:'
section
So it has reduced the number of places that need configuration by 1
Is there a way to grab the mountPath and VO from the vos list to create that section in the config as well?

@rcarpa
Copy link
Contributor

rcarpa commented Feb 15, 2023

If I understood correctly what is required, the list of VOs will have to be configured only here, in the ftsRenewal section, with all the needed "longproxy" certificates as inputs.

The script in the containers repository can be made to create only one single secret, with all the x509 short proxies in the same k8s secret. As long as the list of VOs is not extremely long and all certificates fit into the 1MiB limit of the kubernetes secret, the certificate can then be mounted in one go inside the pods using something like

 secretMounts:
      - secretName: secret-with-all-certs
        mountPath: /opt/proxy/

Thanks to the change in rucio (rucio/rucio#6082), which I made on purpose recently for this use-case, only

[conveyor]
vo_certs_path =  /opt/proxy/

will have to be added to rucio.

As a consequence of that, only one place will have to be configured with the list of VOs.

@rcarpa
Copy link
Contributor

rcarpa commented Jun 12, 2023

I'm closing this one because the use-case should be handled by #139 .

@rcarpa rcarpa closed this Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fts-cron is not multi-VO capable
3 participants