-
Notifications
You must be signed in to change notification settings - Fork 143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
API changes for ServiceAccount credential linking #187
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sjenning The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test e2e-aws-upgrade |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if I'm missing some context here.
|
||
// StorageDestination specifies the type and location of a resource to which the credentials | ||
// information can be stored or linked | ||
StorageDestination StorageDestination `json:"storage,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies I may have misunderstood your other proposal (3), but I expected StorageDestination here to be more like StorageType, which is a const string just indicating which field is set on the CredentialsRequest. I don't think we should introduce a new way to set the secret target when we have to keep the old.
i.e.
credRequest.Spec.Type or StorageType
credRequest.Spec.SecretRef
credRequest.Spec.ServiceAccountRef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could flatten the storage
field down into the spec
. The reason I didn't is that now we have to deal with the existing SecretRef
at that level and if we continue to use that we can't deprecate it in favor of the better typed secret
field in the storage
union.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm +1 for collapsing, I really don't want two secretRef fields with one deprecated on our v1 API, immediately making all our cred requests today in various repos technically using deprecated fields.
I'm +1 for ServiceAccountRef being ObjectReference, keeping it consistent with SecretRef, even though this is not recommended use now. +0 for ServiceAccountRef being typed instead of ObjectRef, it's weird to have Secret and ServiceAccount out of sync but I can live with it.
I'm +1 for making SecretRef typed as well if that's ok, from an API caller perspective I think it's valid as we're still just using name+namespace but I have no idea if this is an acceptable thing to do. I'm going to ask in forum-api-review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend against collapsing the discriminated union in your top level spec. It's easier to reason about what an API does if the "pick one" isn't co-located with "applies in to anything" (in this case your providerSpec
for instance).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one thing I hoped to avoid in this is two places to set the secret ref, deprecating all our existing usage, and carrying that confusion forever.
type SecretStorageDestination struct { | ||
Namespace string `json:"namespace"` | ||
Name string `json:"name"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should just stick with ObjectReference here, even if we don't use or honor all the fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k how strong is your opinion that ObjectReference
should not be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is best practice I'd be curious to know.
However we should stay consistent with secretRef which is already ObjectReference.
Perhaps we could switch it to a type though, as we only would be respecting the Name and Namespace, and API callers would not be impacted?
In any case, would like to see them the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just heard in another review that ObjectReference use is discouraged. So can we safely flip secretRef? Should we make them different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not saying we should change the existing secretRef
but if we are going to create a new storage
union as part of this work, I'm saying we shouldn't make the new fields ObjectReference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k how strong is your opinion that
ObjectReference
should not be used?
It's pretty strong. We should not use ObjectReference.
// 1. Ignored fields. It includes many fields which are not generally honored. For instance, ResourceVersion and FieldPath are both very rarely valid in actual usage.
// 2. Invalid usage help. It is impossible to add specific help for individual usage. In most embedded usages, there are particular
// restrictions like, "must refer only to types A and B" or "UID not honored" or "name must be restricted".
// Those cannot be well described when embedded.
// 3. Inconsistent validation. Because the usages are different, the validation rules are different by usage, which makes it hard for users to predict what will happen.
// 4. The fields are both imprecise and overly precise. Kind is not a precise mapping to a URL. This can produce ambiguity
// during interpretation and require a REST mapping. In most cases, the dependency is on the group,resource tuple
// and the version of the actual struct is irrelevant.
// 5. We cannot easily change it. Because this type is embedded in many locations, updates to this type
// will affect numerous schemas. Don't make new APIs embed an underspecified API type they do not control.
// +union | ||
type StorageDestination struct { | ||
// StorageType contains the type of storage that should be used for the credentials | ||
// +unionDiscriminator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoah, what does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/20190325-unions.md
However, afaict, it doesn't have an impact on the OpenAPIv3 schema that is output by the generator 😕 I figured I'd keep it in there though.
https://github.com/openshift/api/search?q=unionDiscriminator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's best to list this
/test e2e-aws CI release image got pruned |
type StorageDestination struct { | ||
// StorageType contains the type of storage that should be used for the credentials | ||
// +unionDiscriminator | ||
Type string `json:"type"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explicitly required if you would.
} | ||
|
||
type SecretStorageDestination struct { | ||
Namespace string `json:"namespace"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
help with an example. If I create the credential request in namespace foo, I can reference a secret in namespace bar? Does this work today?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this works today. All component cred requests land in our openshift-cloud-credential-operator namespace by convention, but I don't think we explicitly filter on this. If you have permissions to create CredRequests, you can do what you describe.
|
||
type ServiceAccountStorageDestination struct { | ||
Namespace string `json:"namespace"` | ||
Name string `json:"name"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add help, indicate that this is for a serviceaccount and these are required.
pkg/operator/credentialsrequest/credentialsrequest_controller.go
Outdated
Show resolved
Hide resolved
pkg/operator/credentialsrequest/credentialsrequest_controller.go
Outdated
Show resolved
Hide resolved
pkg/operator/credentialsrequest/credentialsrequest_controller.go
Outdated
Show resolved
Hide resolved
|
/hold |
/cc @dgoodwin @joelddiaz @derekwaynecarr @marun @deads2k
Implementing option #3 from openshift/enhancements#260 (comment)
Carrying everything from #185
This just makes the API change and needed wiring changes in the core credentailsrequest controller to minimize reviewer cognitive load. There is no functional change.
This should make the next PR localized to the AWS actuator (creating Roles and linking ARNs to
ServiceAccount
s)Reminder to review only the most recent commit since I am carrying commits from previous PRs to make progress during the freeze.
Note to myself. Need to address @joelddiaz comments in #185 (review)