Skip to content
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

Add support for alpha provider identity support #69

Merged
merged 1 commit into from
Feb 10, 2023

Conversation

hasheddan
Copy link
Contributor

@hasheddan hasheddan commented Feb 8, 2023

Description of your changes

Adds support for enabling provider identity. This feature is unlikely to graduate from alpha, and will likely be replaced by an implementation of the runtime interface. It is disabled by default and is only functional when running on Upbound.

Signed-off-by: hasheddan georgedanielmangum@gmail.com

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Verified that we are mounting successfully when the --enable-provider-identity flag is set and that we are not when it is not.

apiVersion: v1
kind: Pod
metadata:
  creationTimestamp: "2023-02-10T20:28:56Z"
  generateName: upbound-provider-gcp-5145d928dcf9-67889956cf-
  labels:
    pkg.crossplane.io/provider: provider-gcp
    pkg.crossplane.io/revision: upbound-provider-gcp-5145d928dcf9
    pod-template-hash: 67889956cf
  name: upbound-provider-gcp-5145d928dcf9-67889956cf-sshrt
  namespace: crossplane-system
  ownerReferences:
  - apiVersion: apps/v1
    blockOwnerDeletion: true
    controller: true
    kind: ReplicaSet
    name: upbound-provider-gcp-5145d928dcf9-67889956cf
    uid: 01b8ce14-bca2-4e57-8879-19daa6296467
  resourceVersion: "2136"
  uid: d5d06a60-1495-4bb2-ac97-2117ec32b208
spec:
  containers:
  - env:
    - name: POD_NAMESPACE
      valueFrom:
        fieldRef:
          apiVersion: v1
          fieldPath: metadata.namespace
    - name: UPBOUND_CONTEXT
      value: uxp
    image: xpkg.upbound.io/upbound/provider-gcp:v0.27.0
    imagePullPolicy: IfNotPresent
    name: provider-gcp
    ports:
    - containerPort: 8080
      name: metrics
      protocol: TCP
    resources: {}
    securityContext:
      allowPrivilegeEscalation: false
      privileged: false
      runAsGroup: 2000
      runAsNonRoot: true
      runAsUser: 2000
    terminationMessagePath: /dev/termination-log
    terminationMessagePolicy: File
    volumeMounts:
    - mountPath: /var/run/secrets/upbound.io/provider
      name: proidc
      readOnly: true
    - mountPath: /var/run/secrets/kubernetes.io/serviceaccount
      name: kube-api-access-dnbk4
      readOnly: true
  dnsPolicy: ClusterFirst
  enableServiceLinks: true
  nodeName: build-2f418432-inttests-control-plane
  preemptionPolicy: PreemptLowerPriority
  priority: 0
  restartPolicy: Always
  schedulerName: default-scheduler
  securityContext:
    runAsGroup: 2000
    runAsNonRoot: true
    runAsUser: 2000
  serviceAccount: upbound-provider-gcp-5145d928dcf9
  serviceAccountName: upbound-provider-gcp-5145d928dcf9
  terminationGracePeriodSeconds: 30
  tolerations:
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    operator: Exists
    tolerationSeconds: 300
  - effect: NoExecute
    key: node.kubernetes.io/unreachable
    operator: Exists
    tolerationSeconds: 300
  volumes:
  - csi:
      driver: proidc.csi.upbound.io
      readOnly: true
    name: proidc
  - name: kube-api-access-dnbk4
    projected:
      defaultMode: 420
      sources:
      - serviceAccountToken:
          expirationSeconds: 3607
          path: token
      - configMap:
          items:
          - key: ca.crt
            path: ca.crt
          name: kube-root-ca.crt
      - downwardAPI:
          items:
          - fieldRef:
              apiVersion: v1
              fieldPath: metadata.namespace
            path: namespace

Copy link
Member

@tnthornton tnthornton left a comment

Choose a reason for hiding this comment

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

Thanks @hasheddan! Left a couple of comments/questions below 👍

// and should be removed when a runtime interface is introduced upstream.
// See https://github.com/crossplane/crossplane/issues/2671 for more
// information.
EnableProviderIdentity bool `group:"Alpha Features" help:"Enable support for Provider identity."`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
EnableProviderIdentity bool `group:"Alpha Features" help:"Enable support for Provider identity."`
EnableProviderIdentity bool `group:"Alpha Features:" help:"Enable support for Provider identity."`

Based on formatting of the above flags 👍.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah nice catch!

Comment on lines 62 to 64
func buildProviderDeployment(provider *pkgmetav1.Provider, revision v1.PackageRevision, cc *v1alpha1.ControllerConfig, namespace string, pullSecrets []corev1.LocalObjectReference, providerIdentity bool) (*corev1.ServiceAccount, *appsv1.Deployment, *corev1.Service) { // nolint:gocyclo
s := &corev1.ServiceAccount{
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth expanding the signature with functional options here? I realize this is "temporary", however it seems like we could see other non-default options being passed in the future.

Also, maybe this doesn't matter as much if we go down the PRI path, I just see the super long function signature and wonder if that's necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this weren't the structure upstream I would definitely agree. However, introducing functional arguments for this "temporary" functionality feels like it isn't worth the increased diff. Agreed it is gross and I look forward to removing :)

@phisco
Copy link
Contributor

phisco commented Feb 10, 2023

@hasheddan we merged #68, so we'll need to rebase this one

Copy link
Contributor Author

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

Thanks @phisco! I wanted that one to go in first 👍🏻

@hasheddan hasheddan force-pushed the proidc-mount branch 2 times, most recently from 8a3e9c0 to 11433ae Compare February 10, 2023 20:03
Adds support for enabling provider identity. This feature is unlikely to
graduate from alpha, and will likely be replaced by an implementation of
the runtime interface. It is disabled by default and is only functional
when running on Upbound.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
@hasheddan hasheddan marked this pull request as ready for review February 10, 2023 20:34
@hasheddan hasheddan merged commit 188b77e into upbound:master Feb 10, 2023
phisco pushed a commit to phisco/upbound-crossplane that referenced this pull request Feb 21, 2023
Add support for alpha provider identity support
@phisco phisco mentioned this pull request Feb 21, 2023
3 tasks
phisco pushed a commit to phisco/upbound-crossplane that referenced this pull request Feb 21, 2023
Add support for alpha provider identity support
@turkenh turkenh added the upbound label Nov 1, 2023
turkenh added a commit to turkenh/crossplane that referenced this pull request Nov 1, 2023
- Port original PR upbound/crossplane#69
to refactored new structure of the relevant codebase

Signed-off-by: Hasan Turken <turkenh@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants