HOSTEDCP-2035: Use Client Cert Auth for ARO HCP deployments #156
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
bryan-cox
force-pushed
the
HOSTEDCP-1994
branch
2 times, most recently
from
c8f4099
to
dbc3e29
Compare
bryan-cox
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request references HOSTEDCP-1994 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target only the "4.18.0" version, but multiple target versions were set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
bryan-cox
force-pushed
the
HOSTEDCP-1994
branch
from
dbc3e29
to
454d72b
Compare
openshift-ci
bot
added
the
do-not-merge/work-in-progress
bryan-cox
force-pushed
the
HOSTEDCP-1994
branch
from
454d72b
to
0125b54
Compare
bryan-cox
changed the title
|
@bryan-cox: This pull request references HOSTEDCP-2035 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target only the "4.18.0" version, but multiple target versions were set. 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 openshift-eng/jira-lifecycle-plugin repository. |
bryan-cox
changed the title
|
@bryan-cox: This pull request references HOSTEDCP-2035 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target only the "4.18.0" version, but multiple target versions were set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
I tested this PR with the following PRs and was able to deploy an Azure Hosted Cluster from an AKS management cluster:
|
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/test e2e-azure-ovn |
bryan-cox
force-pushed
the
HOSTEDCP-1994
branch
2 times, most recently
from
63df6d1
to
bb903ac
Compare
|
/test e2e-gcp-ovn |
bryan-cox
force-pushed
the
HOSTEDCP-1994
branch
from
bb903ac
to
4ec12ab
Compare
|
/test unit |
bryan-cox
force-pushed
the
HOSTEDCP-1994
branch
4 times, most recently
from
1eb3c6b
to
63e9561
Compare
|
/retest |
| var fileContents []byte | ||
| var err error | ||
|
|
||
| watchCertificateFileOnce.Do(func() { |
There was a problem hiding this comment.
what is the purpose of watchCertificateFileOnce? Is it just to keep the initialFileHash intact?
If so it should certainly be documented and an error returned if the function was called more than once.
There was a problem hiding this comment.
I can add documentation.
I'm not sure how we can return on a second run of watchCertificateFileOnce.Do. This shouldn't be possible since that is the point of sync.Once.
pkg/filewatcher/filewatcher.go
Outdated
| return | ||
| } | ||
|
|
||
| initialFileHash = hashSimple(fileContents) |
There was a problem hiding this comment.
What was the reason behind going with periodic hash check instead of a file watch?
With the current implementation there is a chance we are going to be using the old cert file for ~30min which seems unnecessary.
There was a problem hiding this comment.
We can certainly poll sooner than 30m for the file check. How quick would you like it to poll?
There was a problem hiding this comment.
The reason for the periodic hash check - the previous file watcher code was not catching the changes when I was testing this function against the CPO. I could exec into the pod and see the file changed but the pod was not restarted nor did I see any messages about the file changing.
This current change seemed just as simple and required less code. This change was working when I was testing the same function against the CPO in openshift/hypershift#4997
|
|
||
| done := make(chan bool) | ||
|
|
||
| go func() { |
There was a problem hiding this comment.
you run checkForFileChanges as a go routine but it starts one inside with the done channel waiting for it, why?
There was a problem hiding this comment.
Yeah, I didn't need the second goroutine so I removed it.
There was a problem hiding this comment.
Apparently this is needed. The pod does not restart when I removed this.
pkg/filewatcher/filewatcher.go
Outdated
| klog.Infof("Checking file for changes, %s", fileToWatch) | ||
| fileContents, err := os.ReadFile(fileToWatch) | ||
| if err != nil { | ||
| klog.Error("failed to read the file: %v", err) |
There was a problem hiding this comment.
When a file read fails we will just stop watching it and if it changes this will never be caught.
There was a problem hiding this comment.
The return should just return from the sync.Once function and the error would be returned at the end of WatchFileForChanges
bryan-cox
force-pushed
the
HOSTEDCP-1994
branch
2 times, most recently
from
0c47480
to
a637648
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bryan-cox The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
bryan-cox
force-pushed
the
HOSTEDCP-1994
branch
3 times, most recently
from
5be01e8
to
ca29e0b
Compare
Use Client Certificate Authentication for ARO HCP deployments. HyperShift will pass the needed environment variables for this authentication method: ARO_HCP_MI_CLIENT_ID, ARO_HCP_TENANT_ID, and ARO_HCP_CLIENT_CERTIFICATE_PATH. Signed-off-by: Bryan Cox <[email protected]>
bryan-cox
force-pushed
the
HOSTEDCP-1994
branch
from
ca29e0b
to
6ba6f9b
Compare
|
@bryan-cox: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
Use Client Certificate Authentication for ARO HCP deployments. HyperShift will pass the needed environment variables for this authentication method: ARO_HCP_MI_CLIENT_ID, ARO_HCP_TENANT_ID, and ARO_HCP_CLIENT_CERTIFICATE_PATH.