[Fixes #195] Adding vendors support #275

mustela · 2025-09-15T07:53:36Z

No description provided.

joggrbot · 2025-09-15T07:53:50Z

📝 Documentation Analysis

All docs are up to date! 🎉

✅ Latest commit analyzed: 986c1ef | Powered by Joggr

scotwells

We should introduce a new API group for vendors since it's separate from generic resource management. Overall looking good, just a few comments / recommendations.

pkg/apis/resourcemanager/v1alpha1/corporationtypeconfig_types.go

pkg/apis/resourcemanager/v1alpha1/vendor_types.go

pkg/apis/resourcemanager/v1alpha1/corporationtypeconfig_types.go

mustela · 2025-09-25T09:05:52Z

@scotwells wanna give it another round?

mustela · 2025-09-29T06:46:33Z

@JoseSzycho wanna take a look?

## ✨ Autofixed 1 outdated doc(s) This pull was automatically generated by Joggr to fix 1 outdated doc(s) due to code changes in pull #275 ### Fixed docs The following docs were fixed: - `docs/api/resourcemanager.md`: Vendor-related CRDs and APIs have moved from resourcemanager.miloapis.com to a new vendors.miloapis.com API group, so the documentation here needed to make this explicitly clear and refer users to the new vendor API documentation for those types. ### How to fix To fix the docs, you can either: 1. Merge this pull request into your pull request to fix the docs 2. Wait for your pull request to be merged and then merge this pull request into your base branch --- Powered by [Joggr](https://joggr.ai?utm_source=gh&utm_medium=gh&utm_campaign=ghapr&utm_id=ghapr) - The documentation assistant for your codebase.

## ✨ Autofixed 1 outdated doc(s) This pull was automatically generated by Joggr to fix 1 outdated doc(s) due to code changes in pull #275 ### Fixed docs The following docs were fixed: - `docs/api/infrastructure.md`: The documentation was outdated because it documented 'Vendor' and 'CorporationTypeConfig' in the 'resourcemanager.miloapis.com/v1alpha1' API group, but these resources have been completely moved and restructured in the new 'vendors.miloapis.com/v1alpha1' group with a new design. All references to them in this doc must be deleted. ### How to fix To fix the docs, you can either: 1. Merge this pull request into your pull request to fix the docs 2. Wait for your pull request to be merged and then merge this pull request into your base branch --- Powered by [Joggr](https://joggr.ai?utm_source=gh&utm_medium=gh&utm_campaign=ghapr&utm_id=ghapr) - The documentation assistant for your codebase.

scotwells · 2025-10-01T13:59:55Z

docs/api/infrastructure.md

We should use the task generate command to generate documentation based on the API types instead of manually modifying these files.

scotwells · 2025-10-01T14:01:41Z

docs/api/resourcemanager.md

+**Important:**
+
+> As of this release, all vendor-related resources—including Vendor, VendorTypeDefinition, VendorVerification, and CorporationTypeConfig—have been moved from the `resourcemanager.miloapis.com/v1alpha1` API group to the new `vendors.miloapis.com/v1alpha1` API group.
+>
+> This document *only* covers resource types that remain in the `resourcemanager.miloapis.com/v1alpha1` API group (such as Organization, OrganizationMembership, and Project).
+>
+> For vendor management and vendor type APIs, see the [vendors.miloapis.com API documentation](./vendors-api-group.md).
+


I'm confused by this section of the documentation. Was this something that AI added after moving the vendors stuff to a new API type? This document is automatically generated by task generate.

scotwells · 2025-10-01T14:06:39Z

pkg/apis/vendors/v1alpha1/taxid_utils.go

+func GetTaxIdFromSecret(ctx context.Context, c client.Client, vendor *Vendor, taxIdRef TaxIdReference) (string, error) {
+	// Determine the namespace for the Secret
+	namespace := taxIdRef.Namespace
+	if namespace == "" {
+		namespace = vendor.Namespace
+		if namespace == "" {
+			namespace = "default" // fallback for cluster-scoped resources
+		}
+	}
+
+	// Get the Secret
+	secret := &corev1.Secret{}
+	secretKey := types.NamespacedName{
+		Name:      taxIdRef.SecretName,
+		Namespace: namespace,
+	}
+
+	if err := c.Get(ctx, secretKey, secret); err != nil {
+		return "", fmt.Errorf("failed to get secret %s/%s: %w", namespace, taxIdRef.SecretName, err)
+	}
+
+	// Extract the tax ID from the Secret
+	taxIdBytes, exists := secret.Data[taxIdRef.SecretKey]
+	if !exists {
+		return "", fmt.Errorf("key %s not found in secret %s/%s", taxIdRef.SecretKey, namespace, taxIdRef.SecretName)
+	}
+
+	return string(taxIdBytes), nil
+}


We should leave the pkg/apis/vendors package to API type definitions only. Any helper functions like this should be put in the controller package where they're used.

scotwells · 2025-10-01T14:13:07Z

docs/api/tax-id-secrets.md

Thoughts on moving this to a guide document (maybe under docs/guides/vendors/?) and not something in the api reference folder? Seems like this should be a guide on how to create secrets for tax ID numbers to use with vendors. I'd expect a general design document to talk about why we're using secrets for tax IDs.

We should also reduce the usage of the term "Kubernetes" in the document since it's really a concept in Milo we're using.

scotwells · 2025-10-01T14:13:42Z

docs/api/tax-id-secrets.md

+## Migration from Plain Text
+
+If you have existing vendors with plain text tax IDs:
+
+1. **Create secrets** for each vendor's tax ID
+2. **Update vendor resources** to use `TaxIdRef` instead of `TaxId`
+3. **Verify secrets** are accessible and contain correct data
+4. **Test validation** to ensure everything works
+
+### Migration Script Example
+
+```bash
+#!/bin/bash
+# Extract tax IDs and create secrets
+kubectl get vendors -o json | jq -r '.items[] | select(.spec.taxInfo.taxId) | "\(.metadata.name) \(.spec.taxInfo.taxId)"' | while read vendor_name tax_id; do
+  # Create secret
+  kubectl create secret generic "${vendor_name}-tax-id" \
+    --from-literal=tax-id="$tax_id" \
+    --namespace=default
+  
+  # Add labels
+  kubectl label secret "${vendor_name}-tax-id" \
+    vendor.miloapis.com/vendor="$vendor_name" \
+    vendor.miloapis.com/type=tax-id
+done


Not sure this is needed? There's no migrations needed since this functionality hasn't gone to production yet.

scotwells · 2025-10-01T14:14:32Z

docs/api/tax-id-secrets.md

+## Security Considerations
+
+### 1. **RBAC Configuration**
+Ensure proper RBAC for secret access:
+```yaml
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  name: vendor-secret-reader
+rules:
+- apiGroups: [""]
+  resources: ["secrets"]
+  verbs: ["get", "list"]
+  resourceNames: ["*-tax-id"]  # Restrict to tax ID secrets
+```
+
+### 2. **Network Policies**
+Consider network policies to restrict secret access:
+```yaml
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: restrict-secret-access
+spec:
+  podSelector: {}
+  policyTypes:
+  - Ingress
+  ingress:
+  - from:
+    - namespaceSelector:
+        matchLabels:
+          name: vendor-controllers
+```


This shouldn't be needed? Standard k8s RBAC / Network Policies aren't used with Milo. The network policy APIs aren't even available.

scotwells · 2025-10-01T14:18:59Z

pkg/apis/vendors/v1alpha1/vendorverification_types.go

+	// Whether this verification is required for vendor activation
+	// +kubebuilder:default=true
+	Required bool `json:"required"`


How is this field expected to be used?

scotwells · 2025-10-01T14:19:37Z

pkg/apis/vendors/v1alpha1/vendorverification_types.go

+	// Timestamp when verification was completed
+	// +optional
+	CompletedAt *metav1.Time `json:"completedAt,omitempty"`


Wouldn't this match the creation timestamp of the VendorVerification type?

Yes, fixed.

scotwells · 2025-10-01T14:20:12Z

pkg/apis/vendors/v1alpha1/vendorverification_types.go

+	// Last error message if verification failed
+	// +optional
+	LastError string `json:"lastError,omitempty"`


Wouldn't this be duplicated by data showing up in the status conditions?

scotwells · 2025-10-01T14:21:20Z

pkg/apis/vendors/v1alpha1/vendorverification_types.go

+	// Number of verification attempts
+	// +optional
+	AttemptCount int32 `json:"attemptCount,omitempty"`


Wouldn't the VendorVerification type being created indicate the vendor is verified? How does this value get incremented?

mustela · 2025-10-06T07:29:07Z

@scotwells would you mind to give it another try?

mustela requested a review from scotwells September 15, 2025 07:53

mustela requested a review from JoseSzycho September 15, 2025 07:53

joggrbot bot mentioned this pull request Sep 15, 2025

docs: for pull #275, fixed 1 outdated doc(s) #276

Closed

scotwells reviewed Sep 18, 2025

View reviewed changes

joggrbot bot mentioned this pull request Sep 29, 2025

docs: for pull #275, fixed 1 outdated doc(s) #298

Merged

joggrbot bot mentioned this pull request Sep 30, 2025

docs: for pull #275, fixed 1 outdated doc(s) #300

Merged

scotwells reviewed Oct 1, 2025

View reviewed changes

mustela added 2 commits October 5, 2025 22:57

[Fixes #195] Adding vendors support

aef0e1b

handling comments

d2df109

mustela force-pushed the adding_vendors branch from eaa5946 to 6016c57 Compare October 5, 2025 21:14

joggrbot bot mentioned this pull request Oct 5, 2025

docs: for pull #275, fixed 1 outdated doc(s) #317

Closed

fixing comments

6e3a5a8

mustela force-pushed the adding_vendors branch from 7280f05 to 6e3a5a8 Compare October 6, 2025 07:12

mustela added 2 commits October 7, 2025 17:47

Merge branch 'main' into adding_vendors

959a608

Merge branch 'main' into adding_vendors

986c1ef

[Fixes #195] Adding vendors support #275

Are you sure you want to change the base?

[Fixes #195] Adding vendors support #275

Uh oh!

Conversation

mustela commented Sep 15, 2025

Uh oh!

joggrbot bot commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Documentation Analysis

Uh oh!

scotwells left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mustela commented Sep 25, 2025

Uh oh!

mustela commented Sep 29, 2025

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!

mustela commented Oct 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joggrbot bot commented Sep 15, 2025 •

edited

Loading