Skip to content

Conversation

@DahuK
Copy link
Contributor

@DahuK DahuK commented Nov 9, 2021

support alibaba cloud manual mode

cause we have not released the feature of fine-grained RAM roles for service account, in current we temp using one ram user accesskey id/secret to attach all of the components permission, and we will provide STS mode when RAM new feature release.

@DahuK DahuK changed the title support alibaba cloud manual mode Support alibaba cloud manual mode Nov 9, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 9, 2021

Hi @DahuK. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 9, 2021
@joelddiaz
Copy link
Contributor

It appears that this PR doesn't add to the previous PR #411 , as I don't see that PR's commit in this history.
Can we either close #411 or rebase this PR on top of #411 ?
Also, can the vendoring step be provided as a separate commit from the actual repo code changes?

@DahuK
Copy link
Contributor Author

DahuK commented Nov 10, 2021

It appears that this PR doesn't add to the previous PR #411 , as I don't see that PR's commit in this history. Can we either close #411 or rebase this PR on top of #411 ? Also, can the vendoring step be provided as a separate commit from the actual repo code changes?

Yes, this is a separate pr and #411 should be closed, cc @dongchen126
For vendor code, there is already a separate commit 1ff90d8 in this PR, should I move it out to a separate PR?

@DahuK
Copy link
Contributor Author

DahuK commented Nov 10, 2021

Have refined vendor commit and removed the interface in v1 api type_alibaba.go @joelddiaz

@DahuK
Copy link
Contributor Author

DahuK commented Nov 11, 2021

@joelddiaz Thanks a lot for your careful review!I have refined this based on your comments, pls review them when you free

@kwoodson
Copy link

@DahuK Please rebase PR. Thanks!

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2021
@DahuK
Copy link
Contributor Author

DahuK commented Nov 12, 2021

@DahuK Please rebase PR. Thanks!

done, thx

@joelddiaz
Copy link
Contributor

@DahuK I reviewed the recent changes, and things are looking good. Still a couple of bits of feedback around the required CLI parameters, and the naming of some constants.

@DahuK
Copy link
Contributor Author

DahuK commented Nov 14, 2021

@DahuK I reviewed the recent changes, and things are looking good. Still a couple of bits of feedback around the required CLI parameters, and the naming of some constants.

@joelddiaz sorry for some missing comments, I have submit a new commit for removing the component user's input parms such as ak/sk and username, please check it again when you free, thanks!

@DahuK DahuK requested a review from joelddiaz November 14, 2021 06:33
Copy link
Contributor

@joelddiaz joelddiaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me
@akhil-rane you want to take a look at this?

Comment on lines 60 to 69
```bash
$ ccoctl alibabacloud attach-ram-policy --name <name> --region=<region> --credentials-requests-dir=<path_to_directory_with_list_of_credentials_requests>/credrequests --root-access-key=xxxxx --root-access-key-secret=xxxxx --user-name=testuser --component-access-key=xxxxxx --component-access-secret=xxxxxx --output-dir=xxxxxx
```

where:

- `name` is the name used to tag any cloud resources that are created for tracking.
- `region` is the Alibaba Cloud region in which cloud resources will be created.
- `credentials-requests-dir` is the directory containing files of component CredentialsRequests.
- `output-dir`/manifests is the directory containing files of component credentials secret.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following parameter definitions are missing:

  • root-access-key
  • root-access-key-secret
  • user-name
  • component-access-key
  • component-access-secret

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have removed these deprecated parms

d. Copy the generated credential files to the target manifests directory:

```bash
$ cp <output_dir>/manifests/*credentials.yaml ./path/to/installation/dir/manifests/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also maintain a separate doc for ccoctl where we include all the command info for different cloud providers. It will help if we can add alibaba cloud there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved the doc to ccoctl.md

//call sts GetCallerIdentity for the username of given component ak/sk
stsClient, err := sts.NewClientWithAccessKey(AttachRAMPolicyOpts.Region, componentAk, componentSk)
if err != nil {
log.Fatalf("Failed to init sts fro ram username: %++v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Fatalf("Failed to init sts fro ram username: %++v", err)
log.Fatalf("Failed to init sts client from ram username: %++v", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix this typo

@kwoodson
Copy link

@DahuK
When I test this command I get a nil pointer:

CCOCTL_ALIBABA_COMPONENT_ACCESS_KEY_ID=xxxxx CCOCTL_ALIBABA_COMPONENT_ACCESS_KEY_SECRET=xxxxxx ~/go/src/github.com/openshift/cloud-credential-operator/ccoctl alibabacloud attach-ram-policy-to-existing-user --credentials-requests-dir ./cred-requests
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x18d5960]

goroutine 1 [running]:
github.com/openshift/cloud-credential-operator/pkg/cmd/provisioning/alibabacloud.initEnvForAttachRAMPolicyCmd(0xc0002d4840, {0x1d816c7, 0x2, 0x2})
	github.com/openshift/cloud-credential-operator/pkg/cmd/provisioning/alibabacloud/attach-ram-policy.go:237 +0x180
github.com/spf13/cobra.(*Command).execute(0xc0002d4840, {0xc000650340, 0x2, 0x2})
	github.com/spf13/[email protected]/command.go:834 +0x50a
github.com/spf13/cobra.(*Command).ExecuteC(0xc0004ff340)
	github.com/spf13/[email protected]/command.go:958 +0x3ad
github.com/spf13/cobra.(*Command).Execute(...)
	github.com/spf13/[email protected]/command.go:895
main.main()
	github.com/openshift/cloud-credential-operator/cmd/ccoctl/main.go:25 +0x105

Have you had a chance to test this? Did I miss a parameter or an option? I created a blank user and attempted to assign the policies to this user.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2021
@DahuK
Copy link
Contributor Author

DahuK commented Nov 16, 2021

@DahuK When I test this command I get a nil pointer:

CCOCTL_ALIBABA_COMPONENT_ACCESS_KEY_ID=xxxxx CCOCTL_ALIBABA_COMPONENT_ACCESS_KEY_SECRET=xxxxxx ~/go/src/github.com/openshift/cloud-credential-operator/ccoctl alibabacloud attach-ram-policy-to-existing-user --credentials-requests-dir ./cred-requests
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x18d5960]

goroutine 1 [running]:
github.com/openshift/cloud-credential-operator/pkg/cmd/provisioning/alibabacloud.initEnvForAttachRAMPolicyCmd(0xc0002d4840, {0x1d816c7, 0x2, 0x2})
	github.com/openshift/cloud-credential-operator/pkg/cmd/provisioning/alibabacloud/attach-ram-policy.go:237 +0x180
github.com/spf13/cobra.(*Command).execute(0xc0002d4840, {0xc000650340, 0x2, 0x2})
	github.com/spf13/[email protected]/command.go:834 +0x50a
github.com/spf13/cobra.(*Command).ExecuteC(0xc0004ff340)
	github.com/spf13/[email protected]/command.go:958 +0x3ad
github.com/spf13/cobra.(*Command).Execute(...)
	github.com/spf13/[email protected]/command.go:895
main.main()
	github.com/openshift/cloud-credential-operator/cmd/ccoctl/main.go:25 +0x105

Have you had a chance to test this? Did I miss a parameter or an option? I created a blank user and attempted to assign the policies to this user.

sorry for this panic, I have fixed the issue cause by init the ram request, actually I have no env and I cloud only run the mock test in local, so please help to try it again, thanks so much!

@DahuK
Copy link
Contributor Author

DahuK commented Nov 16, 2021

@akhil-rane thanks for your comments! I have fixed them, please review it again, thx!

@DahuK DahuK requested a review from akhil-rane November 16, 2021 10:21
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2021
@lwan-wanglin
Copy link

/assign @jianping-shu
@jianping-shu will help to take this one, thanks @jianping-shu.

Copy link

@jeana-redhat jeana-redhat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, docs look good. I just had a few questions I would want to know the answer to so we can commit to OCP product docs.


1. Extract and prepare the ccoctl binary from the release image.

2. Choose an existing RAM user who has the below permissions, and get this user's accesskey id/secret for creating the RAM users and policies for each in-cluster component.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to tell the user where/how to get the accesskey id/secret for the user account they will use?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally these are given to a user when they are assigned an account. Users can manage their keys with their RAM (resource access management console). This document explains a littl emore about getting AccessKeys https://www.alibabacloud.com/help/doc-detail/53045.htm

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent - that documentation is very helpful.

ram:DeleteAccessKey
```

3. Use the selected RAM user’s accesskey id/secret to configure the Alibaba Cloud SDK client's credential provider chain with [Envionment Creadentials](https://github.com/aliyun/alibaba-cloud-sdk-go/blob/master/docs/2-Client-EN.md#1-environment-credentials) mode or through [Credentials File](https://github.com/aliyun/alibaba-cloud-sdk-go/blob/master/docs/2-Client-EN.md#2-credentials-file) mode
Copy link

@jeana-redhat jeana-redhat Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these links official Alibaba Cloud documentation intended for end user consumption?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this sdk is our official supported, and these docs are also for end user consumption.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 👍

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DahuK is there something more official? When we downstream these docs; we don't want to link to github; if at all possible.

- `name` is the name used to tag any cloud resources that are created for tracking.
- `region` is the Alibaba Cloud region in which cloud resources will be created.
- `credentials-requests-dir` is the directory containing files of component CredentialsRequests.
- `output-dir`/manifests is the directory containing files of component credentials secret.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does /manifests on this line mean?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This /manifests means that results from the ccoctl binary will be placed in a directory on the system located at /manifests.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm interpreting that to mean <output-dir>/manifests then - let me know if that's incorrect

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeana-redhat your interpretation is correct, the manifests will be placed inside <output-dir>/manifests. Let me share some examples:
If I run:

ccoctl alibabacloud create-ram-users \
  --region ${ALIBABA_REGION_ID} \
  --name <cluster_name> \
  --credentials-requests-dir ${PWD}/cco-credrequests \
  --output-dir ${PWD}/cco-manifests

I will get that structure:

$ tree cco-manifests/
cco-manifests/
└── manifests
    ├── openshift-cluster-csi-drivers-alibaba-disk-credentials-credentials.yaml
    ├── openshift-image-registry-installer-cloud-credentials-credentials.yaml
    ├── openshift-ingress-operator-cloud-credentials-credentials.yaml
    └── openshift-machine-api-alibabacloud-credentials-credentials.yaml

```bash
$ cp <output_dir>/manifests/*credentials.yaml ./path/to/installation/dir/manifests/
```
6. To delete resources created by ccoctl, run

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the user always delete after installing? Or is this a cleanup step for uninstalling (I am asking based on other provider docs)?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These keys will most likely live until the cluster is uninstalled. The keys are being used by the cluster's internal components that require API access. They MUST not be deleted after installation or the cluster will not continue to function properly.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so a cleanup after uninstall step similar to other platforms then. Thanks!

@jianping-shu
Copy link

jianping-shu commented Dec 8, 2021

@joelddiaz @akhil-rane @lwan-wanglin @DahuK
Here is the test case OCP-46768 for the PR, PTAL!

My comments after testing:

  1. If run ccoctl alibabacloud create-ram-users for the same user > 2 times, then its accessKey will be changed. If this happened on a live site, then the previous generated manifests secret will become stale and customer shall apply the new generated secrets again. Do you think we shall add this as note in the ccoctl manual?
    2021/12/08 02:00:54 RAM User jshu-alicloud-openshift-ingress-operator-cloud-credentials's accesskey number limit exceeded, will delete original accesskeys
    2021/12/08 02:00:55 Ready to delete user jshu-alicloud-openshift-ingress-operator-cloud-credentials accesskey LTAI5tH4ujv6FsHsWrXUxndV
    2021/12/08 02:00:56 Created access keys for RAM User: jshu-alicloud-openshift-ingress-operator-cloud-credentials

  2. I checked ccoctl.md, the ###procedure and its step 1 are in wrong format as below, pls. check

Screen Shot 2021-12-08 at 3 59 15 PM

@jeana-redhat
Copy link

Thanks @DahuK and @kwoodson for the clarification on my questions!
/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Dec 8, 2021
Copy link

@sferich888 sferich888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/label px-approved

Looks good just a few comments/nits about the docs. All things I think @jeana-redhat can take care of when we move to downstream this work.


## Alibaba Cloud

This is a guide for using manual mode on alibaba cloud, for more info about manual mode, please refer to [cco-mode-manual](https://github.com/openshift/cloud-credential-operator/blob/master/docs/mode-manual-creds.md).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeana-redhat while this is fine for 'upstream docs' when we downstream these docs we need to remove / redirect this to docs we have that don't reside on github.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, would def be pointing to the product docs version of this content 👍

ram:DeleteAccessKey
```

3. Use the selected RAM user’s accesskey id/secret to configure the Alibaba Cloud SDK client's credential provider chain with [Envionment Creadentials](https://github.com/aliyun/alibaba-cloud-sdk-go/blob/master/docs/2-Client-EN.md#1-environment-credentials) mode or through [Credentials File](https://github.com/aliyun/alibaba-cloud-sdk-go/blob/master/docs/2-Client-EN.md#2-credentials-file) mode

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DahuK is there something more official? When we downstream these docs; we don't want to link to github; if at all possible.


### Prerequisite

1. Extract and prepare the ccoctl binary from the release image.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Show a command for how to do this; don't assume you users know how to do this.

Copy link

@jeana-redhat jeana-redhat Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, we show this for the AWS version and I will be modelling the structure for new platform support on what we have for AWS

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sferich888 thanks for the review! I could only found a Chinese doc for these on https://help.aliyun.com/document_detail/311677.html, and I will check if there is an English version with my colleague

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Dec 8, 2021
@joelddiaz
Copy link
Contributor

1. If run ccoctl alibabacloud create-ram-users for the same user > 2 times, then its accessKey will be changed. If this happened on a live site, then the previous generated manifests secret will become stale and customer shall apply the new generated secrets again. Do you think we shall add this as note in the ccoctl manual?

IMO, this should be fixed before GA.

@kwoodson
Copy link

kwoodson commented Dec 8, 2021

@joelddiaz @jianping-shu Would you like it completed in this PR or in a follow up PR? Please let @DahuK know so that we can address this feedback.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2021
@DahuK
Copy link
Contributor Author

DahuK commented Dec 8, 2021

@jianping-shu @joelddiaz @kwoodson thanks for the review! I have added a new commit for refine the doc, pls review again

@joelddiaz
Copy link
Contributor

@joelddiaz @jianping-shu Would you like it completed in this PR or in a follow up PR? Please let @DahuK know so that we can address this feedback.

I'm okay with a followup PR.

@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #412 (de9cc73) into master (b3496af) will decrease coverage by 0.25%.
The diff coverage is 42.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #412      +/-   ##
==========================================
- Coverage   46.64%   46.39%   -0.26%     
==========================================
  Files          88       94       +6     
  Lines        9160     9688     +528     
==========================================
+ Hits         4273     4495     +222     
- Misses       4337     4612     +275     
- Partials      550      581      +31     
Impacted Files Coverage Δ
cmd/ccoctl/main.go 0.00% <0.00%> (ø)
pkg/alibabacloud/client.go 0.00% <0.00%> (ø)
pkg/cmd/provisioning/alibabacloud/alibaba.go 0.00% <0.00%> (ø)
pkg/cmd/provisioning/alibabacloud/utils.go 12.72% <12.72%> (ø)
.../cmd/provisioning/alibabacloud/delete-ram-users.go 37.11% <37.11%> (ø)
.../cmd/provisioning/alibabacloud/create-ram-users.go 38.67% <38.67%> (ø)
pkg/alibabacloud/mock/client_generated.go 76.59% <76.59%> (ø)
pkg/apis/cloudcredential/v1/register.go 87.50% <100.00%> (+0.83%) ⬆️

@kwoodson
Copy link

kwoodson commented Dec 8, 2021

@DahuK It appears the verify test is failing. Please run make update-gofmt and resubmit with the updates.

@jianping-shu Would it be acceptable to apply the approval label on this PR and address your feedback in a follow up PR?

Also, please open another PR to address the feedback found by @jianping-shu in this comment #412 (comment). We would like to merge this current PR and address the comments in a follow up.

This is now blocking our CI and I would like to get this moving forward so that we can test. Thanks

Error: unknown command "alibabacloud" for "ccoctl"
Run 'ccoctl --help' for usage.
2021/12/08 22:32:49 unknown command "alibabacloud" for "ccoctl"
{"component":"entrypoint","error":"wrapped process failed: exit status 1","file":"prow/entrypoint/run.go:80","func":"k8s.io/test-infra/prow/entrypoint.Options.Run","level":"error","msg":"Error executing test process","severity":"error","time":"2021-12-08T22:32:49Z"}

@jianping-shu
Copy link

I checked 5f45a79, my two comments were already addressed. The ccoctl.md looks good to me now.

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Dec 9, 2021
@DahuK
Copy link
Contributor Author

DahuK commented Dec 9, 2021

@kwoodson CI testing is pass now, and I have addressed the comment from @jianping-shu , thanks!

Copy link
Contributor

@joelddiaz joelddiaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2021
@openshift-merge-robot openshift-merge-robot merged commit 64bab70 into openshift:master Dec 9, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 9, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DahuK, joelddiaz, sferich888

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:

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

ming1013 pushed a commit to ming1013/cloud-credential-operator that referenced this pull request Dec 15, 2025
Support alibaba cloud manual mode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants