fixed re-trigger job issue when public key deleted

[rchikatw]

• fixed re-trigger job issue when the public key deleted
• formatted the logger in a proper format

[jarrpa]

I made a few comments mostly asking for clarity on a few things. I'd also like to see anything that's not directly related to the problem solution (like the util function and the csv-merger changes) in a separate commit, but if that's not common practice in this part of the project I'm all right with leaving it as one commit.

[nb-ohad]

@rchikatw why aren't we setting a controller reference on the secrets and using an Own watch?
If we don't do it we need to make sure to delete the secrets ourselves!

[nb-ohad]

I made a few comments mostly asking for clarity on a few things. I'd also like to see anything that's not directly related to the problem solution (like the util function and the csv-merger changes) in a separate commit, but if that's not common practice in this part of the project I'm all right with leaving it as one commit.

@jarrpa Regarding the separation into separate PRs. From my point of view, a PR needs to be a complete contribution not to be dependent on some code that needs to be reviewed somewhere else.

Because of that, I would claim that a separate PR is an overkill but I do agree that it makes sense to separate these parts into different commits under the same PR, similar to what I ask when we have vendor changes.

[jarrpa]

Just to note, I poked at the whole pointer-to-value thing and effectively came to a similar conclusion to yours... with one catch: there's already a k8s library for this: https://pkg.go.dev/k8s.io/utils/ptr I find its implementation more elegant (and more complete), so I'd like to move to that.

I submitted a draft PR to showcase its usage. I based it on this PR, so as soon as this one merges I'll mark mine as no longer a draft.

[rchikatw]

/retest

[nb-ohad]

@rchikatw What is the reason you choose to use different formats for marshaling of private and public keys?

You use PKCS1 for the private key:
privteKeyBytes := x509.MarshalPKCS1PrivateKey(privateKey)

And PKIX for the public key:
publicKeyBytes, err := x509.MarshalPKIXPublicKey(publicKey)

[rchikatw]

@rchikatw What is the reason you choose to use different formats for marshaling of private and public keys?

You use PKCS1 for the private key: privteKeyBytes := x509.MarshalPKCS1PrivateKey(privateKey)

And PKIX for the public key: publicKeyBytes, err := x509.MarshalPKIXPublicKey(publicKey)

A good catch actually but I never faced any issues while onboarding the application cluster. I did generate the token several times and onboarded the application cluster. But let me try once changing x509.MarshalPKIXPublicKey -> x509.MarshalPKCS1PublicKey

I found the reason why I am using PKIX for marshaling the public key, when a new OnboardConsumer RPC call to onboard a new OCS application cluster happens it parses the public key using the ParsePKIXPublicKey so i have used the same for the public key. refer here and there is no function marshell private key using PKIX

[nb-ohad]

@rchikatw What is the reason you choose to use different formats for marshaling of private and public keys?
You use PKCS1 for the private key: privteKeyBytes := x509.MarshalPKCS1PrivateKey(privateKey)
And PKIX for the public key: publicKeyBytes, err := x509.MarshalPKIXPublicKey(publicKey)

A good catch actually but I never faced any issues while onboarding the application cluster. I did generate the token several times and onboarded the application cluster. But let me try once changing x509.MarshalPKIXPublicKey -> x509.MarshalPKCS1PublicKey

I found the reason why I am using PKIX for marshaling the public key, when a new OnboardConsumer RPC call to onboard a new OCS application cluster happens it parses the public key using the ParsePKIXPublicKey so i have used the same for the public key. refer here and there is no function marshell private key using PKIX

But in that case shouldn't we just align the server code to use PKCS1 as well?

P.S
I just found this
https://stackoverflow.com/questions/49876521/golang-x509-marshalpkixpublickey-vs-x509-marshalpkcs1publickey#:~:text=Add%20a%20comment-,1%20Answer,-Sorted%20by%3A

According to this PCIX was designed to be used for key certificates. It encodes an identity as well as encryption. I think we should align all of our code to use PKCS1. That means both private and public keys

[rchikatw]

@rchikatw What is the reason you choose to use different formats for marshaling of private and public keys?
You use PKCS1 for the private key: privteKeyBytes := x509.MarshalPKCS1PrivateKey(privateKey)
And PKIX for the public key: publicKeyBytes, err := x509.MarshalPKIXPublicKey(publicKey)

A good catch actually but I never faced any issues while onboarding the application cluster. I did generate the token several times and onboarded the application cluster. But let me try once changing x509.MarshalPKIXPublicKey -> x509.MarshalPKCS1PublicKey
I found the reason why I am using PKIX for marshaling the public key, when a new OnboardConsumer RPC call to onboard a new OCS application cluster happens it parses the public key using the ParsePKIXPublicKey so i have used the same for the public key. refer here and there is no function marshell private key using PKIX

But in that case shouldn't we just align the server code to use PKCS1 as well?

P.S I just found this https://stackoverflow.com/questions/49876521/golang-x509-marshalpkixpublickey-vs-x509-marshalpkcs1publickey#:~:text=Add%20a%20comment-,1%20Answer,-Sorted%20by%3A

According to this PCIX was designed to be used for key certificates. It encodes an identity as well as encryption. I think we should align all of our code to use PKCS1. That means both private and public keys

Yes that solves the problem if we used PKCS1 at both places (Client & provider) and for both the keys, Let me do that and verify

[rchikatw]

/retest

[nb-ohad]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leelavg, nb-ohad, rchikatw

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 [nb-ohad]

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

[leelavg]

/cherrypick release-4.15

[leelavg]

/cherrypick fusion-hci-4.14

[openshift-cherrypick-robot]

@leelavg: new pull request created: #2438

In response to this:

/cherrypick release-4.15

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-cherrypick-robot]

@leelavg: #2385 failed to apply on top of branch "fusion-hci-4.14":

Applying: fixed re trigger job issue when public key deleted
Using index info to reconstruct a base tree...
M	controllers/storagecluster/provider_server.go
M	controllers/storagecluster/storagecluster_controller.go
M	services/provider/server/server.go
Falling back to patching base and 3-way merge...
Auto-merging services/provider/server/server.go
Auto-merging controllers/storagecluster/storagecluster_controller.go
CONFLICT (content): Merge conflict in controllers/storagecluster/storagecluster_controller.go
Auto-merging controllers/storagecluster/provider_server.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fixed re trigger job issue when public key deleted
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick fusion-hci-4.14

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.