HOSTEDCP-2035: Use Client Cert Auth for ARO HCP deployments by bryan-cox · Pull Request #156 · openshift/cloud-network-config-controller

bryan-cox · 2024-10-07T19:42:04Z

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.

openshift-ci · 2024-10-07T19:42:08Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2024-10-09T18:36:12Z

@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.

Details

In response to this:

Refactor to use the Azure SDK for Go's default credential chain function, NewDefaultAzureCredential.

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 · 2024-10-09T22:16:37Z

/retest

openshift-ci-robot · 2024-10-19T17:22:02Z

@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.

Details

In response to this:

Refactor to use the Azure SDK for Go's default credential chain function, NewDefaultAzureCredential.

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.

openshift-ci-robot · 2024-10-19T19:44:12Z

@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.

Details

In response to this:

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.

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 · 2024-10-19T19:45:04Z

I tested this PR with the following PRs and was able to deploy an Azure Hosted Cluster from an AKS management cluster:

OCPBUGS-42434: Add Managed Identity Support in Azure HC API behind AROHCPManagedIdentities Feature gate hypershift#4811
OCPBUGS-42434: Update Azure CLI to Handle Authenticating with Service Principal Backed by Certificates hypershift#4877
OCPBUGS-44967: Reconcile SecretProvider for CNCC on ARO HCP hypershift#4903
HOSTEDCP-2035: Set up Azure authentication with cert for ARO HCP cluster-network-operator#2522

bryan-cox · 2024-10-20T00:05:37Z

/retest

bryan-cox · 2024-10-20T09:36:01Z

/retest

bryan-cox · 2024-10-20T19:02:08Z

/retest

bryan-cox · 2024-10-21T11:38:06Z

/retest

bryan-cox · 2024-10-29T12:26:30Z

/test unit

bryan-cox · 2024-11-02T14:32:26Z

/retest

kyrtapz

I am not confident in the new approach, It causes a hard stop and has a potentially significant delay.

kyrtapz · 2024-11-04T13:57:02Z

+			return
+		}
+
+		initialFileHash = hashSimple(fileContents)


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.

We can certainly poll sooner than 30m for the file check. How quick would you like it to poll?

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

kyrtapz · 2024-11-04T13:57:56Z

+
+	done := make(chan bool)
+
+	go func() {


you run checkForFileChanges as a go routine but it starts one inside with the done channel waiting for it, why?

Yeah, I didn't need the second goroutine so I removed it.

Apparently this is needed. The pod does not restart when I removed this.

kyrtapz · 2024-11-04T14:06:01Z

+				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)


When a file read fails we will just stop watching it and if it changes this will never be caught.

The return should just return from the sync.Once function and the error would be returned at the end of WatchFileForChanges

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 <brcox@redhat.com>

kyrtapz · 2024-11-18T13:50:36Z

/lgtm

openshift-ci · 2024-11-18T13:53:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bryan-cox, kyrtapz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [kyrtapz]

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

openshift-ci-robot · 2024-11-18T16:35:10Z

/retest-required

Remaining retests: 0 against base HEAD a6bc591 and 2 for PR HEAD ba58106 in total

bryan-cox · 2024-11-18T19:40:30Z

/test e2e-azure-ovn

openshift-ci · 2024-11-18T22:09:39Z

@bryan-cox: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/security	`ba58106`	link	false	`/test security`

Full PR test history. Your PR dashboard.

Details

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.

openshift-bot · 2024-11-19T03:08:07Z

[ART PR BUILD NOTIFIER]

Distgit: ose-cloud-network-config-controller
This PR has been included in build ose-cloud-network-config-controller-container-v4.19.0-202411190035.p0.gf648c78.assembly.stream.el9.
All builds following this will include this PR.

bryan-cox · 2024-11-25T17:41:44Z

/cherry-pick release-4.17

openshift-cherrypick-robot · 2024-11-25T17:42:27Z

@bryan-cox: #156 failed to apply on top of branch "release-4.17":

Applying: Use Client Cert Auth for ARO HCP deployments
Using index info to reconstruct a base tree...
M	go.mod
Falling back to patching base and 3-way merge...
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Use Client Cert Auth for ARO HCP deployments

Details

In response to this:

/cherry-pick release-4.17

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.

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 7, 2024

bryan-cox mentioned this pull request Oct 7, 2024

Enable Authenticating with Azure with Certificates using Azure SDK for Go's Generic NewDefaultAzureCredential openshift/enhancements#1694

Closed

bryan-cox force-pushed the HOSTEDCP-1994 branch 2 times, most recently from c8f4099 to dbc3e29 Compare October 9, 2024 18:35

bryan-cox marked this pull request as ready for review October 9, 2024 18:35

bryan-cox changed the title ~~Refactor to use Azure SDK default cred chain~~ HOSTEDCP-1994: Refactor to use Azure SDK default cred chain Oct 9, 2024

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 9, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 9, 2024

openshift-ci Bot requested review from andreaskaris and trozet October 9, 2024 18:40

bryan-cox force-pushed the HOSTEDCP-1994 branch from dbc3e29 to 454d72b Compare October 10, 2024 16:41

bryan-cox marked this pull request as draft October 18, 2024 20:23

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2024

bryan-cox force-pushed the HOSTEDCP-1994 branch from 454d72b to 0125b54 Compare October 19, 2024 17:07

bryan-cox changed the title ~~HOSTEDCP-1994: Refactor to use Azure SDK default cred chain~~ HOSTEDCP-2035: Refactor to use Azure SDK default cred chain Oct 19, 2024

bryan-cox changed the title ~~HOSTEDCP-2035: Refactor to use Azure SDK default cred chain~~ HOSTEDCP-2035: Use Client Cert Auth for ARO HCP deployments Oct 19, 2024

bryan-cox mentioned this pull request Oct 19, 2024

HOSTEDCP-2035: Set up Azure authentication with cert for ARO HCP openshift/cluster-network-operator#2522

Merged

bryan-cox marked this pull request as ready for review October 19, 2024 19:45

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 19, 2024

openshift-ci Bot requested a review from danwinship October 19, 2024 19:46

bryan-cox force-pushed the HOSTEDCP-1994 branch from bb903ac to 4ec12ab Compare October 28, 2024 16:03

bryan-cox force-pushed the HOSTEDCP-1994 branch 4 times, most recently from 1eb3c6b to 63e9561 Compare November 1, 2024 23:45

kyrtapz suggested changes Nov 4, 2024

View reviewed changes

openshift-ci Bot assigned kyrtapz Nov 4, 2024

bryan-cox force-pushed the HOSTEDCP-1994 branch 6 times, most recently from ca29e0b to 6ba6f9b Compare November 6, 2024 16:22

bryan-cox force-pushed the HOSTEDCP-1994 branch from 6ba6f9b to ba58106 Compare November 18, 2024 13:17

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 18, 2024

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2024

openshift-merge-bot Bot merged commit f648c78 into openshift:master Nov 18, 2024

bryan-cox deleted the HOSTEDCP-1994 branch November 19, 2024 02:16

bryan-cox mentioned this pull request Dec 9, 2024

HOSTEDCP-2207: Add filewatcher to library-go openshift/library-go#1904

Closed

Conversation

bryan-cox commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Oct 7, 2024

Uh oh!

openshift-ci-robot commented Oct 9, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryan-cox commented Oct 9, 2024

Uh oh!

openshift-ci-robot commented Oct 19, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Oct 19, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryan-cox commented Oct 19, 2024

Uh oh!

bryan-cox commented Oct 20, 2024

Uh oh!

bryan-cox commented Oct 20, 2024

Uh oh!

bryan-cox commented Oct 20, 2024

Uh oh!

bryan-cox commented Oct 21, 2024

Uh oh!

bryan-cox commented Oct 29, 2024

Uh oh!

bryan-cox commented Nov 2, 2024

Uh oh!

kyrtapz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kyrtapz Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

bryan-cox Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

bryan-cox Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyrtapz Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryan-cox Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

bryan-cox Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

kyrtapz Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

bryan-cox Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

kyrtapz commented Nov 18, 2024

Uh oh!

openshift-ci Bot commented Nov 18, 2024

Uh oh!

openshift-ci-robot commented Nov 18, 2024

Uh oh!

bryan-cox commented Nov 18, 2024

Uh oh!

openshift-ci Bot commented Nov 18, 2024

Uh oh!

openshift-bot commented Nov 19, 2024

Uh oh!

bryan-cox commented Nov 25, 2024

Uh oh!

openshift-cherrypick-robot commented Nov 25, 2024

Uh oh!

Reviewers

Assignees

bryan-cox commented Oct 7, 2024 •

edited

Loading

openshift-ci-robot commented Oct 9, 2024 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Oct 19, 2024 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Oct 19, 2024 •

edited by openshift-ci Bot

Loading

bryan-cox Nov 4, 2024 •

edited

Loading

kyrtapz Nov 4, 2024 •

edited

Loading