NE-1244: Use permissions instead of the "Contributor" role in Azure CredentialsRequest #929

Miciah · 2023-05-08T23:08:55Z

Instead of requesting the "Contributor" role, request the individual permissions that the operator requires in order to manage DNS records on Azure.

manifests/00-ingress-credentials-request.yaml: Replace the "Contributor" role with a list of permissions.

Notes for reviewers: Per discussions on NE-1244, this PR restricts the CredentialsRequest for Azure to a specific set of permissions. Because of the way the e2e-azure-operator job is configured, this job doesn't actually use the precise rules or permissions that are listed in the CredentialsRequest, but the e2e-azure-manual-oidc job that was added by openshift/release#41502 does.

openshift-ci-robot · 2023-05-08T23:08:59Z

@Miciah: This pull request references NE-1244 which is a valid jira issue.

In response to this:

Instead of requesting the "Contributor" role, request the less privileged "DNS Zone Contributor" and "Private DNS Zone Contributor" roles.

The Azure built-in roles are documented here: https://learn.microsoft.com/en-us/azure/role-based-access-control/built-in-roles.

The "Contributor" role grants access to all resources whereas the "DNS Zone Contributor" and "Private DNS Zone Contributor" roles grant access only to DNS zones and record sets.

manifests/00-ingress-credentials-request.yaml: Replace the "Contributor" role with the "DNS Zone Contributor" and "Private DNS Zone Contributor" roles.

pkg/manifests/bindata.go: Regenerate.

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.

Miciah · 2023-05-08T23:08:59Z

/test e2e-azure-operator

openshift-ci · 2023-05-08T23:08:59Z

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

Miciah · 2023-05-09T06:59:11Z

e2e-azure-operator failed on TestManagedDNSToUnmanagedDNSIngressController. As that was the only failure, it seems that the operator is working properly in general with the finer-grained roles in the CredentialsRequest.

Miciah · 2023-05-09T07:01:10Z

@abutcher, is this sufficiently fine-grained to satisfy the requirements of the epic?

cluster-ingress-operator/manifests/00-ingress-credentials-request.yaml

Lines 46 to 51 in c468b1d

    
           providerSpec: 
        
             apiVersion: cloudcredential.openshift.io/v1 
        
             kind: AzureProviderSpec 
        
             roleBindings: 
        
             - role: DNS Zone Contributor 
        
             - role: Private DNS Zone Contributor

abutcher · 2023-05-09T12:53:26Z

@Miciah Yeah, I think this will be sufficiently fine grained. Azure e2e is currently credentialsMode: passthrough only and we don't yet have e2e utilizing the CredentialsRequests roleBinding field; that will require openshift/cloud-credential-operator#523 (CCO-232) and CCO-234 to follow which will process the CredentialsRequests for Azure Workload Identity purposes.

openshift-ci-robot · 2023-07-19T16:10:15Z

@Miciah: This pull request references NE-1244 which is a valid jira issue.

In response to this:

Instead of requesting the "Contributor" role, request the individual permissions that the operator requires in order to manage DNS records on Azure.

manifests/00-ingress-credentials-request.yaml: Replace the "Contributor" role with a list of permissions.

pkg/manifests/bindata.go: Regenerate.

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.

Miciah · 2023-07-19T16:10:20Z

/test e2e-azure-operator

Miciah · 2023-07-19T22:18:36Z

e2e-azure-operator failed because TestHeaderNameCaseAdjustment failed. TestHeaderNameCaseAdjustment is checking for the "X-Forwarded-For" and "Cache-Control" headers, but the logs indicate it only got the "X-Forwarded-For" header and not "Cache-Control":

        GET / HTTP/1.1
        user-agent: curl/7.61.1
        accept: */*
        host: header-name-case-adjustment-echo-openshift-ingress.apps.ci-op-q50r7bib-04a70.ci.azure.devcluster.openshift.com
        x-forwarded-host: header-name-case-adjustment-echo-openshift-ingress.apps.ci-op-q50r7bib-04a70.ci.azure.devcluster.openshift.com
        x-forwarded-port: 80
        x-forwarded-proto: http
        forwarded: for=10.128.2.33;host=header-name-case-adjustment-echo-openshift-ingress.apps.ci-op-q50r7bib-04a70.ci.azure.devcluster.openshift.com;proto=http
        X-Forwarded-For: 10.128.2.33

I don't think I've seen this failure before. At the same time, I don't see how the changes in this PR could cause it.
/test e2e-azure-operator

Miciah · 2023-07-20T01:02:00Z

I think the bot missed my previous comment.
/test e2e-azure-operator

Miciah · 2023-07-20T21:09:10Z

Rebased for #900.

Instead of requesting the "Contributor" role, request the individual permissions that the operator requires in order to manage DNS records on Azure. This commit resolves NE-1244. https://issues.redhat.com/browse/NE-1244 * manifests/00-ingress-credentials-request.yaml: Replace the "Contributor" role with a list of permissions.

Miciah · 2023-07-24T23:36:48Z

Rebased for #905.

/test e2e-azure-operator
since openshift/release#41502 merged.

openshift-ci-robot · 2023-07-24T23:37:02Z

@Miciah: This pull request references NE-1244 which is a valid jira issue.

In response to this:

Instead of requesting the "Contributor" role, request the individual permissions that the operator requires in order to manage DNS records on Azure.

manifests/00-ingress-credentials-request.yaml: Replace the "Contributor" role with a list of permissions.

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.

abutcher · 2023-07-25T12:52:35Z

/test e2e-azure-manual-oidc

openshift-ci-robot · 2023-07-25T16:03:40Z

@Miciah: This pull request references NE-1244 which is a valid jira issue.

In response to this:

Instead of requesting the "Contributor" role, request the individual permissions that the operator requires in order to manage DNS records on Azure.

manifests/00-ingress-credentials-request.yaml: Replace the "Contributor" role with a list of permissions.

Notes for reviewers: Per discussions on NE-1244, this PR restricts the CredentialsRequest for Azure to a specific set of permissions. Because of the way the e2e-azure-operator job is configured, this job doesn't actually use the precise rules or permissions that are listed in the CredentialsRequest, but the e2e-azure-manual-oidc job that was added by openshift/release#41502 does.

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.

abutcher · 2023-07-25T21:02:29Z

A custom role was created during credentials provisioning:

2023/07/25 13:06:16 Created customRole ci-op-p522x59d-281b0-openshift-ingress-operator-cloud-credentials ...

Ingress operator pod logs from the test are here.

I noticed we kicked off an e2e-azure-operator test. Will it be necessary to integrate e2e-azure-operator with e2e-azure-manual-oidc into a workflow that combines steps from e2e-azure-operator to fully test the operator on Azure?

Miciah · 2023-07-25T21:20:36Z

A custom role was created during credentials provisioning:
2023/07/25 13:06:16 Created customRole ci-op-p522x59d-281b0-openshift-ingress-operator-cloud-credentials ...

Excellent!

Ingress operator pod logs from the test are here.

I noticed we kicked off an e2e-azure-operator test. Will it be necessary to integrate e2e-azure-operator with e2e-azure-manual-oidc into a workflow that combines steps from e2e-azure-operator to fully test the operator on Azure?

The e2e-azure-manual-oidc job suffices in my opinion. The operator logs show that the operator succeeded in both creating and deleting DNS records, in both the public and private zones. We only use one record type on Azure (namely type A), so there isn't much variation in what the operator does on Azure as far as DNS is concerned.

candita · 2023-07-26T15:42:14Z

/assign

newtonheath · 2023-08-03T15:25:57Z

@candita - can you wrap this up today?

candita · 2023-08-03T19:42:10Z

/lgtm
/approve

openshift-ci · 2023-08-03T19:42:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: candita

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~~ [candita]

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 · 2023-08-03T19:50:13Z

/retest-required

Remaining retests: 0 against base HEAD ddba423 and 2 for PR HEAD 162f14c in total

candita · 2023-08-03T20:01:37Z

@Miciah followup question asked in slack.

Miciah · 2023-08-04T00:29:59Z

e2e-aws-operator failed with a lot of test failures: TestAWSELBConnectionIdleTimeout, TestHTTPHeaderBufferSize, TestHeaderNameCaseAdjustment, TestUniqueIdHeader, TestForwardedHeaderPolicyReplace, TestForwardedHeaderPolicyNever, TestForwardedHeaderPolicyAppend, TestUnmanagedDNSToManagedDNSInternalIngressController, and TestHstsPolicyWorks.

It appears that AWS did not enforce the 3-second idle timeout that TestAWSELBConnectionIdleTimeout configures. This might be a new failure to watch for.

The TestUnmanagedDNSToManagedDNSInternalIngressController failure doesn't look like OCPBUGS-10983, so this too might be a novel failure.

The other tests appear to have failed because they were pulling the "openshift/origin-node" image from Docker Hub. I filed OCPBUGS-17359 and posted #970 to address that issue.

openshift-ci-robot · 2023-08-04T00:55:38Z

/retest-required

Remaining retests: 0 against base HEAD 4e7b2da and 1 for PR HEAD 162f14c in total

newtonheath · 2023-08-04T15:21:41Z

@Miciah @candita does this good and affected by items outside of this PR?

candita · 2023-08-04T16:25:52Z

@Miciah @candita does this good and affected by items outside of this PR?

Hi @newtonheath - the hold is on CI failures right now.

candita · 2023-08-04T17:41:15Z

/test e2e-hypershift

openshift-ci · 2023-08-04T19:22:23Z

@Miciah: The following tests 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/e2e-gcp-ovn	`162f14c`	link	false	`/test e2e-gcp-ovn`
ci/prow/e2e-aws-ovn-single-node	`162f14c`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/e2e-azure-ovn	`162f14c`	link	false	`/test e2e-azure-ovn`

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/test-infra repository. I understand the commands that are listed here.

candita · 2023-08-04T20:01:41Z

--- FAIL: TestNodePool/NodePool_Tests_Group/TestNodepoolMachineconfigGetsRolledout/EnsureNoCrashingPods (0.08s) --- FAIL: TestNodePool/NodePool_Tests_Group/TestNTOMachineConfigGetsRolledOut/EnsureNoCrashingPods (0.08s)

/test e2e-hypershift

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

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 May 8, 2023

Miciah force-pushed the NE-1244-use-fine-grained-roles-in-Azure-CredentialsRequest branch from c468b1d to 6330929 Compare July 19, 2023 16:08

Miciah changed the title ~~NE-1244: Use fine-grained roles in Azure CredentialsRequest~~ NE-1244: Use permissions instead of the "Contributor" role in Azure CredentialsRequest Jul 19, 2023

dlom mentioned this pull request Jul 20, 2023

NE-1244: Add azure-manual-oidc workload to cluster ingress operator openshift/release#41502

Merged

Miciah force-pushed the NE-1244-use-fine-grained-roles-in-Azure-CredentialsRequest branch from 6330929 to 2a198b1 Compare July 20, 2023 21:09

Miciah force-pushed the NE-1244-use-fine-grained-roles-in-Azure-CredentialsRequest branch from 2a198b1 to 162f14c Compare July 24, 2023 23:36

Miciah marked this pull request as ready for review July 25, 2023 17:47

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 Jul 25, 2023

openshift-ci bot requested review from frobware and knobunc July 25, 2023 17:48

openshift-ci bot assigned candita Jul 26, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2023

openshift-merge-robot merged commit 833bc28 into openshift:master Aug 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NE-1244: Use permissions instead of the "Contributor" role in Azure CredentialsRequest #929

NE-1244: Use permissions instead of the "Contributor" role in Azure CredentialsRequest #929

Miciah commented May 8, 2023 •

edited

Loading

openshift-ci-robot commented May 8, 2023 •

edited by openshift-ci bot

Loading

Miciah commented May 8, 2023

openshift-ci bot commented May 8, 2023

Miciah commented May 9, 2023

Miciah commented May 9, 2023

abutcher commented May 9, 2023 •

edited

Loading

openshift-ci-robot commented Jul 19, 2023 •

edited by openshift-ci bot

Loading

Miciah commented Jul 19, 2023

Miciah commented Jul 19, 2023 •

edited

Loading

Miciah commented Jul 20, 2023

Miciah commented Jul 20, 2023

Miciah commented Jul 24, 2023

openshift-ci-robot commented Jul 24, 2023 •

edited by openshift-ci bot

Loading

abutcher commented Jul 25, 2023

openshift-ci-robot commented Jul 25, 2023 •

edited by openshift-ci bot

Loading

abutcher commented Jul 25, 2023 •

edited

Loading

Miciah commented Jul 25, 2023

candita commented Jul 26, 2023

newtonheath commented Aug 3, 2023

candita commented Aug 3, 2023

openshift-ci bot commented Aug 3, 2023

openshift-ci-robot commented Aug 3, 2023

candita commented Aug 3, 2023

Miciah commented Aug 4, 2023

openshift-ci-robot commented Aug 4, 2023

newtonheath commented Aug 4, 2023

candita commented Aug 4, 2023

candita commented Aug 4, 2023

openshift-ci bot commented Aug 4, 2023 •

edited

Loading

candita commented Aug 4, 2023

NE-1244: Use permissions instead of the "Contributor" role in Azure CredentialsRequest #929

NE-1244: Use permissions instead of the "Contributor" role in Azure CredentialsRequest #929

Conversation

Miciah commented May 8, 2023 • edited Loading

openshift-ci-robot commented May 8, 2023 • edited by openshift-ci bot Loading

Miciah commented May 8, 2023

openshift-ci bot commented May 8, 2023

Miciah commented May 9, 2023

Miciah commented May 9, 2023

abutcher commented May 9, 2023 • edited Loading

openshift-ci-robot commented Jul 19, 2023 • edited by openshift-ci bot Loading

Miciah commented Jul 19, 2023

Miciah commented Jul 19, 2023 • edited Loading

Miciah commented Jul 20, 2023

Miciah commented Jul 20, 2023

Miciah commented Jul 24, 2023

openshift-ci-robot commented Jul 24, 2023 • edited by openshift-ci bot Loading

abutcher commented Jul 25, 2023

openshift-ci-robot commented Jul 25, 2023 • edited by openshift-ci bot Loading

abutcher commented Jul 25, 2023 • edited Loading

Miciah commented Jul 25, 2023

candita commented Jul 26, 2023

newtonheath commented Aug 3, 2023

candita commented Aug 3, 2023

openshift-ci bot commented Aug 3, 2023

openshift-ci-robot commented Aug 3, 2023

candita commented Aug 3, 2023

Miciah commented Aug 4, 2023

openshift-ci-robot commented Aug 4, 2023

newtonheath commented Aug 4, 2023

candita commented Aug 4, 2023

candita commented Aug 4, 2023

openshift-ci bot commented Aug 4, 2023 • edited Loading

candita commented Aug 4, 2023

Miciah commented May 8, 2023 •

edited

Loading

openshift-ci-robot commented May 8, 2023 •

edited by openshift-ci bot

Loading

abutcher commented May 9, 2023 •

edited

Loading

openshift-ci-robot commented Jul 19, 2023 •

edited by openshift-ci bot

Loading

Miciah commented Jul 19, 2023 •

edited

Loading

openshift-ci-robot commented Jul 24, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jul 25, 2023 •

edited by openshift-ci bot

Loading

abutcher commented Jul 25, 2023 •

edited

Loading

openshift-ci bot commented Aug 4, 2023 •

edited

Loading