combine xpkg auth ext anno during build #309

nullable-eth · 2023-03-17T15:40:42Z

Description of your changes

Provides options for defining an external auth config extension location and loads that file (defaults to auth.yaml). If the file exists after parsing the crossplane.yaml file it looks for the auth extension meta annotation and gets the group. Then iterates the crd's and annotates that object with the object annotation providing the auth config file in that crd. Lints as normal and then re-serializes the package for output in the image.

Fixes #520

I have:

Read and followed Upbound'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, as appropriate.

How has this code been tested

run up xpkg build with auth.yaml

version: '2023-01-30'
discriminant: spec.credentials.source
sources:
- name: None
  docs: |
    # None
    Ya don't need anything here!
- name: Secret
  docs: |
    # Secret
    You're going to need some static credentials.
  additionalResources:
  - type: Secret
    ref: spec.credentials.secretRef
  showFields:
  - spec.credentials.secretRef
- name: IRSA
  docs: |
    # IRSA
    If you're not running on EKS, don't even bother.
- name: WebIdentity
  docs: |
    # Web Identity
    Now we are getting fancy.
- name: Upbound
  docs: |
    # Upbound
    This is the stuff ya gotta pay for.

next to the crossplane.yaml

---
apiVersion: meta.pkg.crossplane.io/v1alpha1
kind: Provider
metadata:
  name: provider-aws
  annotations:
    meta.crossplane.io/maintainer: Upbound <support@upbound.io>
    meta.crossplane.io/source: github.com/upbound/provider-aws
    meta.crossplane.io/description: |
      Upbound's official Crossplane provider to manage Amazon Web Services (AWS)
      resources in Kubernetes.
    meta.crossplane.io/readme: |
      Provider AWS is a Crossplane provider for [Amazon Web Services
      (AWS)](https://aws.amazon.com/) developed and supported by Upbound.
      If you encounter an issue please reach out on our support@upbound.io
      email address.
    auth.upbound.io/group: aws.upbound.io
spec:
  controller:
    image: crossplane/provider-aws-controller:v0.20.0
---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  annotations:
    controller-gen.kubebuilder.io/version: v0.8.0
  creationTimestamp: null
  name: providerconfigs.aws.upbound.io
spec:
  group: aws.upbound.io
  names:
    categories:
    - crossplane
    - providerconfig
    - aws
    kind: ProviderConfig
    listKind: ProviderConfigList
    plural: providerconfigs
    singular: providerconfig

and found that it annotated the output package.yaml in the provider-aws.xpkg output layer.

---
apiVersion: meta.pkg.crossplane.io/v1alpha1
kind: Provider
metadata:
  annotations:
    auth.upbound.io/group: aws.upbound.io
    meta.crossplane.io/description: |
      Upbound's official Crossplane provider to manage Amazon Web Services (AWS)
      resources in Kubernetes.
    meta.crossplane.io/maintainer: Upbound <support@upbound.io>
    meta.crossplane.io/readme: |
      Provider AWS is a Crossplane provider for [Amazon Web Services
      (AWS)](https://aws.amazon.com/) developed and supported by Upbound.
      If you encounter an issue please reach out on our support@upbound.io
      email address.
    meta.crossplane.io/source: github.com/upbound/provider-aws
  creationTimestamp: null
  name: provider-aws
spec:
  controller:
    image: crossplane/provider-aws-controller:v0.20.0
---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  annotations:
    auth.upbound.io/config: |
      version: "2023-01-30"
      discriminant: spec.credentials.source
      sources:
      - name: None
        docs: |
          # None
          Ya don't need anything here!
        additionalResources: []
        showFields: []
      - name: Secret
        docs: |
          # Secret
          You're going to need some static credentials.
        additionalResources:
        - type: Secret
          ref: spec.credentials.secretRef
        showFields:
        - spec.credentials.secretRef
      - name: IRSA
        docs: |
          # IRSA
          If you're not running on EKS, don't even bother.
        additionalResources: []
        showFields: []
      - name: WebIdentity
        docs: |
          # Web Identity
          Now we are getting fancy.
        additionalResources: []
        showFields: []
      - name: Upbound
        docs: |-
          # Upbound
          This is the stuff ya gotta pay for.
        additionalResources: []
        showFields: []
    controller-gen.kubebuilder.io/version: v0.8.0
  creationTimestamp: null
  name: providerconfigs.aws.upbound.io
spec:
  group: aws.upbound.io
  names:
    categories:
    - crossplane
    - providerconfig
    - aws
    kind: ProviderConfig
    listKind: ProviderConfigList
    plural: providerconfigs
    singular: providerconfig
  scope: ""
  versions: null
status:
  acceptedNames:
    kind: ""
    plural: ""
  conditions: null
  storedVersions: null
---

Signed-off-by: nullable.eth <2248325+nullable-eth@users.noreply.github.com>

cmd/up/xpkg/build.go

internal/xpkg/build.go

hasheddan · 2023-03-21T15:47:27Z

internal/xpkg/build.go

+							return nil, nil, errors.Wrap(err, errParseAuth)
+						}
+						if err == nil {
+							for x, o := range pkg.GetObjects() {


Now that we have a separate encoding step, we could handle this there to avoid iterating through the objects multiple times.

Given this is a O(N) loop with O(1) lookup of the auth annotation the overhead does not seem like a lot to keep the logic separated. Also, this attempts to modify the object prior to Linting to ensure overall package consistency.

Fair enough! There is likely some optimization we can do, but I'm good with leaning into pre-processing actually being "pre" everything else 👍🏻

hasheddan · 2023-03-21T15:51:57Z

internal/xpkg/build.go

-	pkg, err := b.pp.Parse(ctx, annotatedTeeReadCloser(pkgReader, pkgBuf))
+	pkg, err := b.pp.Parse(ctx, pkgReader)


One option to avoid having to re-serialize all objects in the package would be to augment the annotatedTeeReadCloser to skip writing the ProviderConfig to the pkgBuf but store the contents so that they can be augmented and re-written to the end of the buffer prior to linting. That would also avoid any modification of content that was not going through the "pre-processor".

This could work but would require us to search a byte array rather than an already parsed object to check their Kind and then find the beginning and end of that object to extract and place at the end which seems like a much more problematic way to do it. Yes, the current method will cause a checksum change between identical packages post up release but only once going forward. The re-serialization of objects here gives us much greater flexibility in mutating objects here and in the future as needed.

Yes, the current method will cause a checksum change between identical packages post up release but only once going forward. The re-serialization of objects here gives us much greater flexibility in mutating objects here and in the future as needed.

This seems like a very reasonable outlook to me and I am comfortable with different versions of up producing varying package content. It is similar to any compiler -- the produced artifact from one version to the next should be valid in respect to how it "runs" and any contract established (such as FFI), but it doesn't need to be identical. We are producing valid xpkgs here so I think your point is a good one 👍🏻

Signed-off-by: nullable-eth <2248325+nullable-eth@users.noreply.github.com> Co-Authored-By: Daniel Mangum <31777345+hasheddan@users.noreply.github.com>

hasheddan · 2023-03-23T18:05:44Z

cmd/up/xpkg/build.go

+		if axf, err := c.fs.Open(ax); err == nil {
+			defer func() { _ = axf.Close() }()
+			if b, err := io.ReadAll(axf); err == nil {
+				authBE = parser.NewEchoBackend(string(b))
+			}
+		}


@nullable-eth I imagine we want to abort on errors here?

well the first two filepath.Abs(c.AuthExt) and c.fs.Open could simply be that the auth extension file was not found at the default location so I think this would not be an error, just that the auth extension option is not being used to annotate this package.

The io.ReadAll can be an abortable error.

Change incoming.

internal/xpkg/build.go

hasheddan · 2023-03-23T18:11:43Z

internal/xpkg/build.go

+		return nil, errors.New(errBuildObjectScheme)
+	}
+
+	do := json.NewSerializerWithOptions(json.DefaultMetaFactory, objScheme, objScheme, json.SerializerOptions{Yaml: true})


Hmm does this serializer work with the meta object? I was thinking we would just serializing without schemes altogether here.

It does, in fact in encoding it doesn't really use the object schemes at all https://github.com/kubernetes/apimachinery/blob/8d1258da8f386b809d312cdda316366d5612f54e/pkg/runtime/serializer/json/json.go#L223 but in the interest of using k8s machinery and dealing with runtime objects it seemed to more streamlined option than using a straight yaml serializer.

Nice! 👍🏻

internal/xpkg/build.go

Signed-off-by: nullable-eth <2248325+nullable-eth@users.noreply.github.com>

hasheddan

Thanks @nullable-eth!

nullable-eth added 2 commits March 17, 2023 11:30

combine xpkg auth ext anno during build

f3c9a5b

Signed-off-by: nullable.eth <2248325+nullable-eth@users.noreply.github.com>

fix struct annotations which only half work for yaml

bab91c3

Signed-off-by: nullable.eth <2248325+nullable-eth@users.noreply.github.com>

nullable-eth requested review from jastang and hasheddan March 19, 2023 14:41

check that Kind is ProviderConfig

ded5bc5

Signed-off-by: nullable.eth <2248325+nullable-eth@users.noreply.github.com>

nullable-eth force-pushed the auth-ext-build branch from a95fc7e to ded5bc5 Compare March 20, 2023 17:33

hasheddan reviewed Mar 21, 2023

View reviewed changes

check annotated and errors

37242de

Signed-off-by: nullable-eth <2248325+nullable-eth@users.noreply.github.com> Co-Authored-By: Daniel Mangum <31777345+hasheddan@users.noreply.github.com>

nullable-eth force-pushed the auth-ext-build branch from b6dfbfe to 37242de Compare March 22, 2023 13:26

hasheddan reviewed Mar 23, 2023

View reviewed changes

minor nits

40aa9c8

Signed-off-by: nullable-eth <2248325+nullable-eth@users.noreply.github.com>

hasheddan approved these changes Mar 23, 2023

View reviewed changes

nullable-eth merged commit 5e62eda into upbound:main Mar 23, 2023

nullable-eth deleted the auth-ext-build branch March 23, 2023 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

combine xpkg auth ext anno during build #309

combine xpkg auth ext anno during build #309

nullable-eth commented Mar 17, 2023 •

edited

Loading

hasheddan Mar 21, 2023

nullable-eth Mar 22, 2023

hasheddan Mar 23, 2023

hasheddan Mar 21, 2023

nullable-eth Mar 22, 2023

hasheddan Mar 23, 2023

hasheddan Mar 23, 2023

nullable-eth Mar 23, 2023 •

edited

Loading

hasheddan Mar 23, 2023

nullable-eth Mar 23, 2023

hasheddan Mar 23, 2023

hasheddan left a comment

		pkg, err := b.pp.Parse(ctx, annotatedTeeReadCloser(pkgReader, pkgBuf))
		pkg, err := b.pp.Parse(ctx, pkgReader)

combine xpkg auth ext anno during build #309

combine xpkg auth ext anno during build #309

Conversation

nullable-eth commented Mar 17, 2023 • edited Loading

Description of your changes

How has this code been tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nullable-eth Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasheddan left a comment

Choose a reason for hiding this comment

nullable-eth commented Mar 17, 2023 •

edited

Loading

nullable-eth Mar 23, 2023 •

edited

Loading