move openshift/cloud-credential-operator API into openshift/api #683

joelddiaz · 2020-07-09T19:51:54Z

Move the api currently stored in github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential into openshift/api/cloudcredential.
Add Makefile hooks for generating deepcopy/CRD.

This is primarily a direct copy over from cloud-credential-operator except for:

a brand new cloudcredential/register.go
a cloudcredential/v1/doc.go with extra annotations
updated path for the k8s:conversion-gen annotation in cloudcredential/v1/register.go
cleaned up cloudcredential/v1/doc.go

The deepcopy has been regenerated, the swagger newly generated, and the CRD generated as well (matches current CRD in openshift/cloud-credential-operator/manifests/00-crd.yaml.

openshift-ci-robot · 2020-07-09T19:52:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: joelddiaz
To complete the pull request process, please assign derekwaynecarr
You can assign the PR to them by writing /assign @derekwaynecarr in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

specifically the `assert` package

Move the api currently stored in github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential into openshift/api/cloudcredential. Add Makefile hooks for generating deepcopy/CRD. This is primarily a direct copy over from cloud-credential-operator except for: 1) a brand new `cloudcredential/register.go` 2) a `cloudcredential/v1/doc.go` with extra annotations 3) updated path for the k8s:conversion-gen annotation in `cloudcredential/v1/register.go` 4) cleaned up `cloudcredential/v1/doc.go` The deepcopy has been regenerated, the swagger newly generated, and the CRD generated as well (matches current CRD in `openshift/cloud-credential-operator/manifests/00-crd.yaml`.

sttts · 2020-07-13T08:53:05Z

cloudcredential/v1/aws_manual.deepcopy.go

+// DeepCopyInto will perform a DeepCopy into the provided AWSProviderSpec
+func (in *AWSProviderSpec) DeepCopyInto(out *AWSProviderSpec) {
+	*out = *in
+	out.TypeMeta = in.TypeMeta


isn't this implicit from *out = *in?

sttts · 2020-07-13T08:54:17Z

cloudcredential/v1/types_aws.go

+// TODO: these types should eventually be broken out, along with the actuator, to a separate repo.
+
+// AWSProviderSpec contains the required information to create a user policy in AWS.
+// +k8s:deepcopy-gen=false


The field in AWSProviderSpec.StatementEntries[].PolicyCondition being of type map[string]map[string]interface{}. This means we cannot generate DeepCopy. The interface{} unfortunately snuck into our API while DeepCopy generation was mistakenly turned off (restored in openshift/cloud-credential-operator#204 ).

Just create custom deepcopy funcs for IAMPolicyCondition or even IAMPolicyConditionKeyValue. Doesn't that work? No need to disable generation for these normal structs.

sttts · 2020-07-13T08:55:36Z

cloudcredential/v1/types_aws.go

+	// Action describes the particular AWS service actions that should be allowed or denied. (i.e. ec2:StartInstances, iam:ChangePassword)
+	Action []string `json:"action"`
+	// Resource specifies the object(s) this statement should apply to. (or "*" for all)
+	Resource string `json:"resource"`


should any of these be required?

I suppose we could default to "*" if Resource is not set, but I'm not thrilled with the idea that by default we would be granting permissions with the least restrictions possible. If there were a workable minimum-permissions-by-default, I could definitely see making this not required.

I don't think we need to require any of these, they seem optional, similarly how we do in audit. But defaulting to * is not a good idea. Default should be empty which means none.

@soltysh sorry, getting back to this after some other unrelated work. If we mark this optional, and the empty string is treated as None, is there any value in defining a list of permissions and then saying that you can perform all those actions against nothing? What have we gained by making the field optional?

Question: since this is an embedded type hidden behind a RawExtension, what effect does annotating these fields have?

sttts · 2020-07-13T08:56:05Z

cloudcredential/v1/types_aws.go

+// +k8s:deepcopy-gen=false
+type StatementEntry struct {
+	// Effect indicates if this policy statement is to Allow or Deny.
+	Effect string `json:"effect"`


this is an enum? Add kubebuilder markers.

I believe today the only allowed values are "Allow" or "Deny" so I supposes we can make this an enum.

Same question here, for an embedded type behind a RawExtension, what effect does annotating these fields have? (I annotated it as an enum, and there was no difference in the generated CRD which confused me until I realized this was behind the spec.providerSpec RawExtension.)

That's a kubebuilder question. It might have special code to generate a RawExtensins schema.

sttts · 2020-07-13T08:56:20Z

cloudcredential/v1/types_aws.go

+	User string `json:"user"`
+	// Policy is the name of the policy attached to the user in AWS.
+	Policy string `json:"policy"`
+}


anything required?

None of the status fields are required. When cloud-cred-operator is running in "passthrough" mode, no users are created, and no policies are created. The credentials are simply copied as-is from the secret kube-system/aws-creds.

sttts · 2020-07-13T08:57:17Z

cloudcredential/v1/types_aws.go

+
+// IAMPolicyConditionKeyValue - mapping of values for the chosen type
+// +k8s:deepcopy-gen=false
+type IAMPolicyConditionKeyValue map[string]interface{}


what are the values? Arbitrary JSON? It needs kubebuilder markers for that (x-kubernetes-preserve-unknown-fields).

Yes, to the cloud-cred-operator, this is basically arbitrary (and IMO quite complex https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html ) JSON that just gets passed through to the AWS API calls as-is. I'll work on adding the markers.

Compare the JSON type here: https://github.com/kubernetes/apiextensions-apiserver/blob/master/pkg/apis/apiextensions/v1/types_jsonschema.go#L59

You migth want to replicate what it does with a custom deepcopy func.

sttts · 2020-07-13T08:57:25Z

cloudcredential/v1/types_aws.go

+
+// IAMPolicyCondition - map of condition types, with associated key - value mapping
+// +k8s:deepcopy-gen=false
+type IAMPolicyCondition map[string]IAMPolicyConditionKeyValue


why no deepcopy?

Again, the interface{} messes up the auto DeepCopy generation here.

soltysh · 2020-07-15T13:12:42Z

cloudcredential/v1/aws_manual.deepcopy_test.go

+
+	"k8s.io/apimachinery/pkg/runtime"
+
+	"github.com/stretchr/testify/assert"


The goal of this repo is to have minimal set of dependencies, I'd prefer to use go built-in mechanism over fancy assert libraries here.

soltysh · 2020-07-15T13:13:50Z

cloudcredential/v1/register.go

+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/


No need for this copyright note, we have the license file at the root of this repo and that's sufficient.

soltysh · 2020-07-15T13:16:43Z

cloudcredential/v1/types_aws.go

+	// Action describes the particular AWS service actions that should be allowed or denied. (i.e. ec2:StartInstances, iam:ChangePassword)
+	Action []string `json:"action"`
+	// Resource specifies the object(s) this statement should apply to. (or "*" for all)
+	Resource string `json:"resource"`


I don't think we need to require any of these, they seem optional, similarly how we do in audit. But defaulting to * is not a good idea. Default should be empty which means none.

soltysh · 2020-07-15T13:21:47Z

cloudcredential/v1/codec.go

+)
+
+// NewScheme creates a new Scheme
+func NewScheme() (*runtime.Scheme, error) {


This file is quite unexpected here. Eventual codecs should be created where this is being used, not in API.

Sounds like we should move this back to the CCO repo @joelddiaz

soltysh · 2020-07-15T13:23:17Z

cloudcredential/v1/register.go

+		&AWSProviderStatus{}, &AWSProviderSpec{},
+		&AzureProviderStatus{}, &AzureProviderSpec{},
+		&GCPProviderStatus{}, &GCPProviderSpec{},
+		&VSphereProviderStatus{}, &VSphereProviderSpec{},


This seems off. The usual k8s api is to have:

spec - what user specified

status - what the controller achieved

but in your case spec and status are separate, why? I believe this can't be changed at the moment but I must admit this is unexpected.

This looks suspicious, all of these cloud specific types are embedded in the CredentialsRequest RawExtensions. I'm wondering if we need to add these as known types at all. It might be related to serialization as we do have to decode them in the cloud specific actuators.

If these types are embedded I don't think they should be part of openshift/api at all.

I actually considered removing this in the CCO repo (openshift/cloud-credential-operator#219) as prep-work before opening this PR, but it was never merged. I can certainly resurrect that PR and stop registering these types.

openshift-ci-robot · 2020-07-22T23:32:30Z

@joelddiaz: PR needs rebase.

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-bot · 2020-11-15T14:30:06Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-bot · 2020-12-15T16:27:50Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

openshift-bot · 2021-01-14T18:16:51Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

openshift-ci-robot · 2021-01-14T18:17:11Z

@openshift-bot: Closed this PR.

Details

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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-robot requested review from deads2k and sttts July 9, 2020 19:52

joelddiaz mentioned this pull request Jul 9, 2020

WIP: migrate to using openshift/api/cloudcredential API openshift/cloud-credential-operator#224

Closed

Joel Diaz added 2 commits July 10, 2020 09:06

vendor stretchr/testify

7810204

specifically the `assert` package

joelddiaz force-pushed the cloud-cred-op-types branch from b131f88 to 84068c0 Compare July 10, 2020 13:13

sttts reviewed Jul 13, 2020

View reviewed changes

soltysh suggested changes Jul 15, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2020

joelddiaz mentioned this pull request Aug 11, 2020

don't register non-core types into scheme openshift/cloud-credential-operator#219

Closed

openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 15, 2020

openshift-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 15, 2020

openshift-ci-robot closed this Jan 14, 2021

joelddiaz mentioned this pull request Nov 3, 2021

Alibaba: add Alibaba Cloud manual mode openshift/cloud-credential-operator#411

Closed


		"k8s.io/apimachinery/pkg/runtime"

		"github.com/stretchr/testify/assert"

move openshift/cloud-credential-operator API into openshift/api #683

move openshift/cloud-credential-operator API into openshift/api #683

Uh oh!

Conversation

joelddiaz commented Jul 9, 2020

Uh oh!

openshift-ci-robot commented Jul 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jul 22, 2020

Uh oh!

openshift-bot commented Nov 15, 2020

Uh oh!

openshift-bot commented Dec 15, 2020

Uh oh!

openshift-bot commented Jan 14, 2021

Uh oh!

openshift-ci-robot commented Jan 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects