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

Using example AccessLogPolicy in readme results in nil pointer exception #626

Open
shulin-sq opened this issue Apr 26, 2024 · 1 comment
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@shulin-sq
Copy link
Contributor

using this sample accesslogpolicy

```yaml
apiVersion: application-networking.k8s.aws/v1alpha1
kind: AccessLogPolicy
metadata:
name: my-access-log-policy
spec:
destinationArn: "arn:aws:logs:us-west-2:123456789012:log-group:myloggroup:*"
targetRef:
group: gateway.networking.k8s.io
kind: HTTPRoute
name: inventory

results in an NPE

this is because in the controller:

targetRefNamespace := k8s.NamespaceOrDefault(alp.Spec.TargetRef.Namespace)
if targetRefNamespace != alp.Namespace {
message := fmt.Sprintf("The targetRef's namespace, \"%s\", does not match the Access Log Policy's"+
" namespace, \"%s\"", string(*alp.Spec.TargetRef.Namespace), alp.Namespace)
r.eventRecorder.Event(alp, corev1.EventTypeWarning, k8s.FailedReconcileEvent, message)
return r.updateAccessLogPolicyStatus(ctx, alp, gwv1alpha2.PolicyReasonInvalid, message)
}

namespace is nullchecked, but then the nil value is reused in string(*alp.Spec.TargetRef.Namespace)

I looked at all other objects and it seems that they do not require the operator to specify target ref namespace. I'm wondering why in this accesslogpolicy we require target ref namespace instead of just defaulting it to the access log policy's namespace if it's missing. This is also the only place that uses k8s.NamespaceOrDefault for now.

I would make a contribution to fix this but I'm not sure if the maintainers would prefer

  • for this NPE to be fixed by not referencing the nil in the log
  • or for the default namespace to be assumed
@erikfuller
Copy link
Contributor

When the namespace isn't present, I think it would be better to use the namespace of the access log policy rather than "default" as the code implies above. I think the AccessLogPolicy CRD even documents this behaviour.

@erikfuller erikfuller added bug Something isn't working good first issue Good for newcomers labels May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants