Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
prefetch-task-rhsm-integration #1205
prefetch-task-rhsm-integration #1205
Changes from all commits
d9579c2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Commit message:
s/THe/The
How is the input modified? Yes, we copy stuff to a shared volume, most importantly the certs that get passed to cachi2 to use for TLS auth, but other than that, how is the user input modified?
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.
added example of how the input is modified.
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.
Noob question - how is the
ACTIVATION_KEY
parameter even used, it doesn't seem to since you're hardcoding the volume path here? What if someone changes the value, how do we probe the right file system location? I'm clearly missing something here making me confused.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.
The path is hardcoded but the secret that is populated to that path has a default value of 'activation-key' in the task parameter declarations and can be overridden in the params passed to task invocation. It is the secret name the user needs to be concerned with, the path is an internal implementation detail.
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.
Why do we need to copy the data? Why not reading it directly from the volume
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.
It could be possible to read it directly from the volume but I tried to keep this code in sync with the buildah task which does a copy.
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 is IMO not needed at all, IIUC you only added it to report the RHSM_ID in the logs, but I wonder what value that brings compared to reporting the TLS auth credentials as part of cachi2's own debug logging system.
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.
208, 210 and 211 for visibility and we could remove them but it would make any necessary debugging harder. 207 and 209 are crucial functionality.
the "RHSM_ID" is not known before
subsription-manager register
is run.That is why the code has to pase this value from the file names. Those files also don't exist before
subscrip-manager-register
is run. What is happening (and there is no alternative) is this[rhsm_id.pem]
and `[rhsm_id].keyrpm
.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.
We can extract the file names on demand right before executing cachi2. The intermediate RHSM_ID handling is just opaque and makes it visibly
hardharder to read, why complicating stuff? :)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 cannot extract the names only once, modify the input in a separate task and omit passing the names between steps.
Passing data between steps by writing to an emptyDir is a common pattern to save Tekton results space. It is not unexpected in this context or abnormally complicated. If you want to avoid it for some reason then we have to extract the names from the files twice. In any case, I don't think the difference between the two approaches is worth the time spent discussing it and I find parsing the name once to be the simpler way (hence the way I went).
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.
Your code already assumes there's only ever going to be a single set of entitlement key-cert pair of files which is a reasonable assumption ATM, so you don't really need this RHSM_ID filename construction logic, one file is going to be named
xyz-key.pem
the otherxyz.pem
no RHSM involvement needed at all.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.
The construction is necessary. Even though the code limits the user to one set of certificates, the certificate names are not known until subscription-manager register is run.
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.
It's true that you don't know the actual names, but I don't believe the construction really is necessary, regardless of whether this is Bash or Python, you can extract the right names dynamically without knowing RHSM_ID at the time of use, because you have the most important bit of information - the files are named the same sans the
-key
suffix.What I'm trying to say is, we should only be using and passing around data what we absolutely have to and need instead of generating/passing around data we don't.
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.
...but the names of the keys need to be input into the python step so it can be injected into input JSON.
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.
Why? IIUC the shared volume
entitlement
directory contains only 2 files from which you can derive the intent of each of those.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.
if the user input is simply rpm and an activation key is presented, the input string has to be transformed before being passed to cachi2 to:
[{"type": "rpm", "options": {"ssl": {"client_key": null, "client_cert": null, "ca_bundle": null, "verify": 1}}}]
And then the client_key and client_cert values need to be populated with the name of the pem file generated by subscription-manager register