restore DeepCopy generation #204

joelddiaz · 2020-06-08T20:34:18Z

Bring back hooks to update/verify our generated DeepCopy files.

The DeepCopy generator does not work with the current AWSProviderSpec.StatementEntries[].PolicyCondition field being a map[string]map[string]interface{}. Annotate those fields so that there is no attempt to generate the DeepCopy methods, and provide custom DeepCopy methods where needed.

joelddiaz · 2020-06-08T23:39:51Z

/test e2e-aws

dgoodwin

Approach looks good to me.

pkg/apis/cloudcredential/v1/aws_types.go

dgoodwin · 2020-06-09T11:24:22Z

pkg/aws/actuator/actuator.go

+			}
+			statement.Condition = out
+		}
+		policyDoc.Statement = append(policyDoc.Statement, statement)


Were you able to manually test with this PolicyCondition actually in use?

Yes, with and without it. That's how I figured that I needed to annotate the StatementEntry struct to put the 'omitempty' (otherwise the JSON sent to AWS when there was no policy always included "Condition": null which would cause the AWS API calls to fail).

FWIW, this is a simple example:

spec: secretRef: name: my-cred-request-secret namespace: openshift-cloud-credential-operator providerSpec: apiVersion: cloudcredential.openshift.io/v1 kind: AWSProviderSpec statementEntries: - effect: Allow action: - s3:CreateBucket - s3:DeleteBucket resource: "*" policyCondition: StringEquals: aws:userid: "testuser"

joelddiaz · 2020-06-09T15:02:40Z

Here's a summary of where we stand with runtime.RawExtension being a pointer or not-a-pointer and comparing that to what the object definition is today.

Today, the object definition has the PolicyCondition field as a map[string]map[string]interface{}. I can't show an example of a CRD with that for two reasons. 1) The PolicyCondition field is buried under a *runtime.RawExtension 2) If I bring the PolicyCondition out of the RawExtension (for demonstration purposes), the CRD cannot be generated because of the interface{} (the primary motivator for this PR!).

Given that, I have created a new field (TestField) under CredentialsRequestSpec with type map[string]int. Generating the CRD for that results in:

testField:
  type: object
  additionalProperties:
    type: integer

Changing the TestField to *runtime.RawExtension:

testField:
  type: object

Changing it to a non-pointer runtime.RawExtension has no change in the generated CRD.

It is not obvious to me that there is a benefit (or even a change) in choosing between runtime.RawExtension and *runtime.RawExtension. I'd lean to the pointer-version simply because CredentialsRequestSpec.ProviderSpec is a pointer today.

Do we have any API experts who can review/comment on this proposed change?

csrwng · 2020-06-09T15:08:59Z

@joelddiaz fwiw the machine-api also uses a pointer to runtime.RawExtension (so I was wrong thinking it wasn't):
https://github.com/openshift/machine-api-operator/blob/d5a738f9925fbf9ce64b5c20cf0a2058b09f9243/pkg/apis/machine/v1beta1/common_types.go#L35

joelddiaz · 2020-06-09T15:28:25Z

/test e2e-aws-upgrade

joelddiaz · 2020-06-15T19:35:57Z

@deads2k as one our resident "API people", can you PTAL and see if there are any concerns with this migration from a field which is presently listed as type interface{} (living under a *runtime.RawExtension) to having it also be a *runtime.RawExtension (also living under the "parent" RawExtension as the json-location of the field hasn't changed)?

Our primary concern here is that we don't make any breaking API changes here (specifically the change to pkg/apis/cloudcredential/v1/aws_types.go).

dgoodwin · 2020-06-25T17:26:13Z

Good solve. Looks good to me, but will let @csrwng do the honors once he's had a chance to look.

csrwng

lgtm, just a question, and you also may want to remove the WIP from the title

csrwng · 2020-06-25T17:31:33Z

hack/update-codegen.sh

+verify="${VERIFY:-}"
+
+valid_gopath=$(realpath $REPO_FULL_PATH/../../../..)
+if [[ "$(realpath ${valid_gopath}/src/github.com/openshift/cloud-credential-operator)" == "${REPO_FULL_PATH}" ]]; then


is there a reason you don't just always do the fake go path? It seems to me that there's less chance to have issues with other things in the real gopath if you do have one.

It was an optimization suggested by @staebler so that we wouldn't have to wait for the various pieces to be downloaded to the GOPATH if your usable GOPATH already had them in there. Works out that the TMP_DIR holds around 174MB of data on disk.

I'll un-WIP and squash some things up and reduce the verbosity of the deepcopy generation in a moment.

Turns out that converting to rawExtension can be considered an API-breaking change if we were to really use the full flexibility of the rawExtension. So preserve the map[string]map[string]interface{} fields. New Makefile targets for generating deepcopy. Scripts for generating/verifying deepcopy. Mark fields so that we don't try to generate deepcopy methods for those that cannot be generated. Create manual deepcopy methods for the fields that need it. Test cases for the manual deep copy implementations. Didn't previously have test coverage for CredentialsRequests that defined the PolicyConditions to be applied. Write up some test cases to validate the behavior. As far as the deepcopy scripts: Rather than always setting up a temp dir so that deepcopy-gen can run in there, see if the repo's directory tree looks like: src/github.com/openshift/cloud-credential-operator If the tree looks that way, then just use that as the GOPATH when generating the deepcopy files. If the tree doesn't look that way, then set up a temp GOPATH directory setup and symlink back to the current repo w/o copying it to the temp dir.

csrwng · 2020-06-25T22:33:02Z

/lgtm

openshift-ci-robot · 2020-06-25T22:33:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csrwng, joelddiaz

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~~ [csrwng,joelddiaz]

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 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 8, 2020

openshift-ci-robot requested review from csrwng and staebler June 8, 2020 20:34

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2020

joelddiaz force-pushed the rawextension-aws-conditions branch from 78a3ce7 to 32b61a8 Compare June 8, 2020 20:39

dgoodwin reviewed Jun 9, 2020

View reviewed changes

joelddiaz force-pushed the rawextension-aws-conditions branch 3 times, most recently from 7251f45 to 41f208c Compare June 24, 2020 13:54

joelddiaz changed the title ~~WIP: migrate AWSProviderSpec from interface{} to rawExtension~~ WIP: restore DeepCopy generation Jun 24, 2020

joelddiaz force-pushed the rawextension-aws-conditions branch 2 times, most recently from 89d95b9 to 13ff01c Compare June 24, 2020 14:22

dgoodwin assigned csrwng Jun 25, 2020

csrwng reviewed Jun 25, 2020

View reviewed changes

Joel Diaz added 2 commits June 25, 2020 15:27

vendor k8s.io/code-generator and stretchr/testify/require

581427e

joelddiaz force-pushed the rawextension-aws-conditions branch from 13ff01c to 88b8dc7 Compare June 25, 2020 19:34

joelddiaz changed the title ~~WIP: restore DeepCopy generation~~ restore DeepCopy generation Jun 25, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 25, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2020

openshift-merge-robot merged commit d58ad49 into openshift:master Jun 25, 2020

joelddiaz mentioned this pull request Jul 14, 2020

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

restore DeepCopy generation #204

restore DeepCopy generation #204

joelddiaz commented Jun 8, 2020 •

edited

joelddiaz commented Jun 8, 2020

dgoodwin left a comment

dgoodwin Jun 9, 2020

joelddiaz Jun 9, 2020

joelddiaz commented Jun 9, 2020

csrwng commented Jun 9, 2020

joelddiaz commented Jun 9, 2020

joelddiaz commented Jun 15, 2020

dgoodwin commented Jun 25, 2020

csrwng left a comment

csrwng Jun 25, 2020

joelddiaz Jun 25, 2020

csrwng commented Jun 25, 2020

openshift-ci-robot commented Jun 25, 2020

restore DeepCopy generation #204

restore DeepCopy generation #204

Conversation

joelddiaz commented Jun 8, 2020 • edited

joelddiaz commented Jun 8, 2020

dgoodwin left a comment

Choose a reason for hiding this comment

dgoodwin Jun 9, 2020

Choose a reason for hiding this comment

joelddiaz Jun 9, 2020

Choose a reason for hiding this comment

joelddiaz commented Jun 9, 2020

csrwng commented Jun 9, 2020

joelddiaz commented Jun 9, 2020

joelddiaz commented Jun 15, 2020

dgoodwin commented Jun 25, 2020

csrwng left a comment

Choose a reason for hiding this comment

csrwng Jun 25, 2020

Choose a reason for hiding this comment

joelddiaz Jun 25, 2020

Choose a reason for hiding this comment

csrwng commented Jun 25, 2020

openshift-ci-robot commented Jun 25, 2020

joelddiaz commented Jun 8, 2020 •

edited